| Message ID | 20210410164951.770920-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | support build selftests/bpf with clang | expand |
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. 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
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 >
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;
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(-)