From: lihaofeng <lihaofeng@kylinos.cn>

The original implementation of netfilter_link_compar() used subtraction
to compare the netfilter.flags field, which is  an unsigned type.
This could result in incorrect comparison results when the unsigned
value wrapped around due to underflow.

Changed the comparison logic for flags to use explicit conditional
checks (similar to how priority is handled) instead of subtraction,
ensuring correct negative/zero/positive return values regardless of
the underlying data type.

This fixes potential sorting issues when using this comparison function
with algorithms like qsort() or bsearch().

Signed-off-by: lihaofeng <lihaofeng@kylinos.cn>
---
 tools/bpf/bpftool/net.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index cfc6f944f7c3..9f840821beda 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -816,7 +816,11 @@ static int netfilter_link_compar(const void *a, const void *b)
 	if (nfa->netfilter.priority > nfb->netfilter.priority)
 		return 1;
 
-	return nfa->netfilter.flags - nfb->netfilter.flags;
+	if (nfa->netfilter.flags < nfb->netfilter.flags)
+		return -1;
+	if (nfa->netfilter.flags > nfb->netfilter.flags)
+		return 1;
+	return 0;
 }
 
 static void show_link_netfilter(void)
-- 
2.25.1

2025-09-19 17:04 UTC+0800 ~ Haofeng Li <920484857@qq.com>
> From: lihaofeng <lihaofeng@kylinos.cn>
> 
> The original implementation of netfilter_link_compar() used subtraction
> to compare the netfilter.flags field, which is  an unsigned type.
> This could result in incorrect comparison results when the unsigned
> value wrapped around due to underflow.
> 
> Changed the comparison logic for flags to use explicit conditional
> checks (similar to how priority is handled) instead of subtraction,
> ensuring correct negative/zero/positive return values regardless of
> the underlying data type.
> 
> This fixes potential sorting issues when using this comparison function
> with algorithms like qsort() or bsearch().
> 
> Signed-off-by: lihaofeng <lihaofeng@kylinos.cn>
> ---
>  tools/bpf/bpftool/net.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index cfc6f944f7c3..9f840821beda 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c
> @@ -816,7 +816,11 @@ static int netfilter_link_compar(const void *a, const void *b)
>  	if (nfa->netfilter.priority > nfb->netfilter.priority)
>  		return 1;
>  
> -	return nfa->netfilter.flags - nfb->netfilter.flags;
> +	if (nfa->netfilter.flags < nfb->netfilter.flags)
> +		return -1;
> +	if (nfa->netfilter.flags > nfb->netfilter.flags)
> +		return 1;
> +	return 0;
>  }
>  

Thanks! Did you actually observe an overflow producing an error when
sorting, here? Or did you run into some compiler warning? If I'm not
mistaken you'd need the difference between flags for nfa and nfb to be
bigger than 1 << 31 for the sign to reverse. As far as I can tell, the
netfilter.flags can be 0 or 1, so this is unlikely to happen. I note
that fields netfilter.pf and netfilter.hooknum are processed in a
similar way earlier in the function, with the types allowing overflows
but possible values preventing the comparison to reverse in practice.

This being said, I don't mind making the code cleaner for these
comparisons, but we should probably treat all three attributes the same,
and update the rest of the function as well?

Quentin

Thank you for your review and feedback.

>Did you actually observe an overflow producing an error when sorting, 
>here? Or did you run into some compiler warning?

I did not encounter a runtime error or a compiler warning caused by this potential overflow. 
The issue was identified during code review as a potential risk, 
considering the theoretical possibility of wrap-around with unsigned subtraction, 
which prompted me to submit this patch for code robustness.

>This being said, I don't mind making the code cleaner for these
>comparisons, but we should probably treat all three attributes the same,
>and update the rest of the function as well?

Thank you for pointing this out. 
I will prepare a v2 patch that thoroughly reviews and updates the comparison 
logic for all three fields (netfilter.pf, netfilter.hooknum, and netfilter.flags), 
replacing all subtraction-based comparisons with explicit conditional checks. 
This will ensure the entire comparison function is robust and consistent in its behavior.

Best regards,
	Haofeng Li

On Sun, Sep 21, 2025 at 6:52 PM Haofeng Li <920484857@qq.com> wrote:
>
> Thank you for your review and feedback.
>
> >Did you actually observe an overflow producing an error when sorting,
> >here? Or did you run into some compiler warning?
>
> I did not encounter a runtime error or a compiler warning caused by this potential overflow.
> The issue was identified during code review as a potential risk,
> considering the theoretical possibility of wrap-around with unsigned subtraction,
> which prompted me to submit this patch for code robustness.
>
> >This being said, I don't mind making the code cleaner for these
> >comparisons, but we should probably treat all three attributes the same,
> >and update the rest of the function as well?
>
> Thank you for pointing this out.
> I will prepare a v2 patch that thoroughly reviews and updates the comparison
> logic for all three fields (netfilter.pf, netfilter.hooknum, and netfilter.flags),
> replacing all subtraction-based comparisons with explicit conditional checks.

Don't. We don't fix theoretical issues.