Re: [PATCH bpf-next 5/5] bpftool: fix a clang compilation warning

[Yonghong Song <yhs@fb.com>]

On 4/11/21 4:05 AM, Sedat Dilek wrote:
> On Sat, Apr 10, 2021 at 6:49 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> With clang compiler:
>>    make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
>>    # build selftests/bpf or bpftool
>>    make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
>>    make -j60 -C tools/bpf/bpftool LLVM=1 LLVM_IAS=1
>> the following compilation warning showed up,
>>    net.c:160:37: warning: comparison of integers of different signs: '__u32' (aka 'unsigned int') and 'int' [-Wsign-compare]
>>                  for (nh = (struct nlmsghdr *)buf; NLMSG_OK(nh, len);
>>                                                    ^~~~~~~~~~~~~~~~~
>>    .../tools/include/uapi/linux/netlink.h:99:24: note: expanded from macro 'NLMSG_OK'
>>                             (nlh)->nlmsg_len <= (len))
>>                             ~~~~~~~~~~~~~~~~ ^   ~~~
>>
>> In this particular case, "len" is defined as "int" and (nlh)->nlmsg_len is "unsigned int".
>> The macro NLMSG_OK is defined as below in uapi/linux/netlink.h.
>>    #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
>>                               (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
>>                               (nlh)->nlmsg_len <= (len))
>>
>> The clang compiler complains the comparision "(nlh)->nlmsg_len <= (len))",
>> but in bpftool/net.c, it is already ensured that "len > 0" must be true.
>> So let us add an explicit type conversion (from "int" to "unsigned int")
>> for "len" in NLMSG_OK to silence this warning.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/bpf/bpftool/net.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
>> index ff3aa0cf3997..f836d115d7d6 100644
>> --- a/tools/bpf/bpftool/net.c
>> +++ b/tools/bpf/bpftool/net.c
>> @@ -157,7 +157,7 @@ static int netlink_recv(int sock, __u32 nl_pid, __u32 seq,
>>                  if (len == 0)
>>                          break;
>>
>> -               for (nh = (struct nlmsghdr *)buf; NLMSG_OK(nh, len);
>> +               for (nh = (struct nlmsghdr *)buf; NLMSG_OK(nh, (unsigned int)len);
>>                       nh = NLMSG_NEXT(nh, len)) {
>>                          if (nh->nlmsg_pid != nl_pid) {
>>                                  ret = -LIBBPF_ERRNO__WRNGPID;
>> --
>> 2.30.2
>>
> 
> Thanks for the patch.
> 
> I remember darkly I have seen this, too.

In this particular case, through analysis, the compiler COULD decide
the comparison is okay as the range of "int" value for "len" is > 0.
But it really depends on when and how much analysis the compiler
did before issuing this particular warning. So working around at the 
source code is a better choice than silencing all similar warnings. Some
of such warnings may actually reveal a real issue.

> 
> The only warning I see remaining *here* is fixed by this patch from bpf-next:
> 
> commit 7519c387e69d367075bf493de8a9ea427c9d2a1b
> "selftests: xsk: Remove unused function"
> 
> - Sedat -
> 
> [1] https://git.kernel.org/bpf/bpf-next/c/7519c387e69d367075bf493de8a9ea427c9d2a1b
>