| Message ID | 20210712165832.1833460-1-yhs@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] libbpf: fix compilation errors on ubuntu 16.04 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: toke@redhat.com; 6 maintainers not CCed: netdev@vger.kernel.org kpsingh@kernel.org kafai@fb.com john.fastabend@gmail.com songliubraving@fb.com toke@redhat.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Yonghong Song wrote: > libbpf is used as a submodule in bcc. > When importing latest libbpf repo in bcc, I observed the > following compilation errors when compiling on ubuntu 16.04. > .../netlink.c:416:23: error: ‘TC_H_CLSACT’ undeclared (first use in this function) > *parent = TC_H_MAKE(TC_H_CLSACT, > ^ > .../netlink.c:418:9: error: ‘TC_H_MIN_INGRESS’ undeclared (first use in this function) > TC_H_MIN_INGRESS : TC_H_MIN_EGRESS); > ^ > .../netlink.c:418:28: error: ‘TC_H_MIN_EGRESS’ undeclared (first use in this function) > TC_H_MIN_INGRESS : TC_H_MIN_EGRESS); > ^ > .../netlink.c: In function ‘__get_tc_info’: > .../netlink.c:522:11: error: ‘TCA_BPF_ID’ undeclared (first use in this function) > if (!tbb[TCA_BPF_ID]) > ^ > > In ubuntu 16.04, TCA_BPF_* enumerator looks like below > enum { > TCA_BPF_UNSPEC, > TCA_BPF_ACT, > ... > TCA_BPF_NAME, > TCA_BPF_FLAGS, > __TCA_BPF_MAX, > }; > #define TCA_BPF_MAX (__TCA_BPF_MAX - 1) > while in latest bpf-next, the enumerator looks like > enum { > TCA_BPF_UNSPEC, > ... > TCA_BPF_FLAGS, > TCA_BPF_FLAGS_GEN, > TCA_BPF_TAG, > TCA_BPF_ID, > __TCA_BPF_MAX, > }; > > In this patch, TCA_BPF_ID is defined as a macro with proper value and this > works regardless of whether TCA_BPF_ID is defined in uapi header or not. > > I also added a comparison "TCA_BPF_MAX < TCA_BPF_ID" in function __get_tc_info() > such that if the compare result if true, returns -EOPNOTSUPP. This is used to > prevent otherwise array overflows: > .../netlink.c:538:10: warning: array subscript is above array bounds [-Warray-bounds] > if (!tbb[TCA_BPF_ID]) > ^ > > Fixes: 715c5ce454a6 ("libbpf: Add low level TC-BPF management API") > Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > tools/lib/bpf/netlink.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > Changelog: > v1 -> v2: > - gcc 8.3 doesn't like macro condition > (__TCA_BPF_MAX - 1) <= 10 > where __TCA_BPF_MAX is an enumerator value. > So define TCA_BPF_ID macro without macro condition. > > diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c > index 39f25e09b51e..e00660e0b87a 100644 > --- a/tools/lib/bpf/netlink.c > +++ b/tools/lib/bpf/netlink.c > @@ -22,6 +22,24 @@ > #define SOL_NETLINK 270 > #endif > > +#ifndef TC_H_CLSACT > +#define TC_H_CLSACT TC_H_INGRESS > +#endif > + > +#ifndef TC_H_MIN_INGRESS > +#define TC_H_MIN_INGRESS 0xFFF2U > +#endif > + > +#ifndef TC_H_MIN_EGRESS > +#define TC_H_MIN_EGRESS 0xFFF3U > +#endif > + > +/* TCA_BPF_ID is an enumerate value in uapi/linux/pkt_cls.h. > + * Declare it as a macro here so old system can still work > + * without TCA_BPF_ID defined in pkt_cls.h. > + */ > +#define TCA_BPF_ID 11 > + > typedef int (*libbpf_dump_nlmsg_t)(void *cookie, void *msg, struct nlattr **tb); > > typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, libbpf_dump_nlmsg_t, > @@ -504,6 +522,8 @@ static int __get_tc_info(void *cookie, struct tcmsg *tc, struct nlattr **tb, > return -EINVAL; > if (!tb[TCA_OPTIONS]) > return NL_CONT; > + if (TCA_BPF_MAX < TCA_BPF_ID) > + return -EOPNOTSUPP; I'm a bit confused here. Generally what I want to have happen is compilation to work always and then runtime to detect the errors. So when I compile my libs on machine A and run it on machine B it does what I expect. This seems like a bit of an ugly workaround to me. I would expect the user should update the uapi? Or should we (maybe just libbpf git repo?) include the defines needed? The change here seems likely to cause issues where someone compiles on old kernel then tries to run it later on newer kernel and is confused when they get EOPNOTSUPP. Did I miss something? What if we just include the enum directly and wrap in ifndef? This is how I've dealt with these dependencies on other libs/apps. > > libbpf_nla_parse_nested(tbb, TCA_BPF_MAX, tb[TCA_OPTIONS], NULL); > if (!tbb[TCA_BPF_ID]) > -- > 2.30.2 >
On 7/12/21 10:28 AM, John Fastabend wrote: > Yonghong Song wrote: >> libbpf is used as a submodule in bcc. >> When importing latest libbpf repo in bcc, I observed the >> following compilation errors when compiling on ubuntu 16.04. >> .../netlink.c:416:23: error: ‘TC_H_CLSACT’ undeclared (first use in this function) >> *parent = TC_H_MAKE(TC_H_CLSACT, >> ^ >> .../netlink.c:418:9: error: ‘TC_H_MIN_INGRESS’ undeclared (first use in this function) >> TC_H_MIN_INGRESS : TC_H_MIN_EGRESS); >> ^ >> .../netlink.c:418:28: error: ‘TC_H_MIN_EGRESS’ undeclared (first use in this function) >> TC_H_MIN_INGRESS : TC_H_MIN_EGRESS); >> ^ >> .../netlink.c: In function ‘__get_tc_info’: >> .../netlink.c:522:11: error: ‘TCA_BPF_ID’ undeclared (first use in this function) >> if (!tbb[TCA_BPF_ID]) >> ^ >> >> In ubuntu 16.04, TCA_BPF_* enumerator looks like below >> enum { >> TCA_BPF_UNSPEC, >> TCA_BPF_ACT, >> ... >> TCA_BPF_NAME, >> TCA_BPF_FLAGS, >> __TCA_BPF_MAX, >> }; >> #define TCA_BPF_MAX (__TCA_BPF_MAX - 1) >> while in latest bpf-next, the enumerator looks like >> enum { >> TCA_BPF_UNSPEC, >> ... >> TCA_BPF_FLAGS, >> TCA_BPF_FLAGS_GEN, >> TCA_BPF_TAG, >> TCA_BPF_ID, >> __TCA_BPF_MAX, >> }; >> >> In this patch, TCA_BPF_ID is defined as a macro with proper value and this >> works regardless of whether TCA_BPF_ID is defined in uapi header or not. >> >> I also added a comparison "TCA_BPF_MAX < TCA_BPF_ID" in function __get_tc_info() >> such that if the compare result if true, returns -EOPNOTSUPP. This is used to >> prevent otherwise array overflows: >> .../netlink.c:538:10: warning: array subscript is above array bounds [-Warray-bounds] >> if (!tbb[TCA_BPF_ID]) >> ^ >> >> Fixes: 715c5ce454a6 ("libbpf: Add low level TC-BPF management API") >> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> tools/lib/bpf/netlink.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> Changelog: >> v1 -> v2: >> - gcc 8.3 doesn't like macro condition >> (__TCA_BPF_MAX - 1) <= 10 >> where __TCA_BPF_MAX is an enumerator value. >> So define TCA_BPF_ID macro without macro condition. >> >> diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c >> index 39f25e09b51e..e00660e0b87a 100644 >> --- a/tools/lib/bpf/netlink.c >> +++ b/tools/lib/bpf/netlink.c >> @@ -22,6 +22,24 @@ >> #define SOL_NETLINK 270 >> #endif >> >> +#ifndef TC_H_CLSACT >> +#define TC_H_CLSACT TC_H_INGRESS >> +#endif >> + >> +#ifndef TC_H_MIN_INGRESS >> +#define TC_H_MIN_INGRESS 0xFFF2U >> +#endif >> + >> +#ifndef TC_H_MIN_EGRESS >> +#define TC_H_MIN_EGRESS 0xFFF3U >> +#endif >> + >> +/* TCA_BPF_ID is an enumerate value in uapi/linux/pkt_cls.h. >> + * Declare it as a macro here so old system can still work >> + * without TCA_BPF_ID defined in pkt_cls.h. >> + */ >> +#define TCA_BPF_ID 11 >> + >> typedef int (*libbpf_dump_nlmsg_t)(void *cookie, void *msg, struct nlattr **tb); >> >> typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, libbpf_dump_nlmsg_t, >> @@ -504,6 +522,8 @@ static int __get_tc_info(void *cookie, struct tcmsg *tc, struct nlattr **tb, >> return -EINVAL; >> if (!tb[TCA_OPTIONS]) >> return NL_CONT; >> + if (TCA_BPF_MAX < TCA_BPF_ID) >> + return -EOPNOTSUPP; > > I'm a bit confused here. Generally what I want to have happen is compilation > to work always and then runtime to detect the errors. So when I compile my > libs on machine A and run it on machine B it does what I expect. This seems > like a bit of an ugly workaround to me. I would expect the user should > update the uapi? The reason is due to the declaration struct nlattr *tbb[TCA_BPF_MAX + 1]; so I have to have the above to check to ensure we don't have out-of-bound access. Alternative, I can redefine macro TCA_BPF_MAX to be the value based on the *current* repo, we should be fine, I think. > > Or should we (maybe just libbpf git repo?) include the defines needed? The > change here seems likely to cause issues where someone compiles on old > kernel then tries to run it later on newer kernel and is confused when they > get EOPNOTSUPP. That is true. Compiling in old system and then using in new system might have issues. > > Did I miss something? What if we just include the enum directly and > wrap in ifndef? This is how I've dealt with these dependencies on > other libs/apps. As I am mentioned in the commit message, we cannot use a simple ifndef to control including the enum since header file in the old system contains *some* enumerators. Testing whether a particular enumerator should be included requires some arithmetic (minus) in the macro condition and some compiler (e.g., gcc 8.3) does not like it. But I think by defining TCA_BPF_MAX explicitly should solve the problem. Will send a new patch soon. > >> >> libbpf_nla_parse_nested(tbb, TCA_BPF_MAX, tb[TCA_OPTIONS], NULL); >> if (!tbb[TCA_BPF_ID]) >> -- >> 2.30.2
On 7/12/21 9:10 PM, Yonghong Song wrote: > On 7/12/21 10:28 AM, John Fastabend wrote: >> Yonghong Song wrote: >>> libbpf is used as a submodule in bcc. >>> When importing latest libbpf repo in bcc, I observed the >>> following compilation errors when compiling on ubuntu 16.04. >>> .../netlink.c:416:23: error: ‘TC_H_CLSACT’ undeclared (first use in this function) >>> *parent = TC_H_MAKE(TC_H_CLSACT, >>> ^ >>> .../netlink.c:418:9: error: ‘TC_H_MIN_INGRESS’ undeclared (first use in this function) >>> TC_H_MIN_INGRESS : TC_H_MIN_EGRESS); >>> ^ >>> .../netlink.c:418:28: error: ‘TC_H_MIN_EGRESS’ undeclared (first use in this function) >>> TC_H_MIN_INGRESS : TC_H_MIN_EGRESS); >>> ^ >>> .../netlink.c: In function ‘__get_tc_info’: >>> .../netlink.c:522:11: error: ‘TCA_BPF_ID’ undeclared (first use in this function) >>> if (!tbb[TCA_BPF_ID]) >>> ^ >>> >>> In ubuntu 16.04, TCA_BPF_* enumerator looks like below >>> enum { >>> TCA_BPF_UNSPEC, >>> TCA_BPF_ACT, >>> ... >>> TCA_BPF_NAME, >>> TCA_BPF_FLAGS, >>> __TCA_BPF_MAX, >>> }; >>> #define TCA_BPF_MAX (__TCA_BPF_MAX - 1) >>> while in latest bpf-next, the enumerator looks like >>> enum { >>> TCA_BPF_UNSPEC, >>> ... >>> TCA_BPF_FLAGS, >>> TCA_BPF_FLAGS_GEN, >>> TCA_BPF_TAG, >>> TCA_BPF_ID, >>> __TCA_BPF_MAX, >>> }; >>> >>> In this patch, TCA_BPF_ID is defined as a macro with proper value and this >>> works regardless of whether TCA_BPF_ID is defined in uapi header or not. >>> >>> I also added a comparison "TCA_BPF_MAX < TCA_BPF_ID" in function __get_tc_info() >>> such that if the compare result if true, returns -EOPNOTSUPP. This is used to >>> prevent otherwise array overflows: >>> .../netlink.c:538:10: warning: array subscript is above array bounds [-Warray-bounds] >>> if (!tbb[TCA_BPF_ID]) >>> ^ >>> >>> Fixes: 715c5ce454a6 ("libbpf: Add low level TC-BPF management API") >>> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> >>> Signed-off-by: Yonghong Song <yhs@fb.com> >>> --- >>> tools/lib/bpf/netlink.c | 20 ++++++++++++++++++++ >>> 1 file changed, 20 insertions(+) >>> >>> Changelog: >>> v1 -> v2: >>> - gcc 8.3 doesn't like macro condition >>> (__TCA_BPF_MAX - 1) <= 10 >>> where __TCA_BPF_MAX is an enumerator value. >>> So define TCA_BPF_ID macro without macro condition. >>> >>> diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c >>> index 39f25e09b51e..e00660e0b87a 100644 >>> --- a/tools/lib/bpf/netlink.c >>> +++ b/tools/lib/bpf/netlink.c >>> @@ -22,6 +22,24 @@ >>> #define SOL_NETLINK 270 >>> #endif >>> +#ifndef TC_H_CLSACT >>> +#define TC_H_CLSACT TC_H_INGRESS >>> +#endif >>> + >>> +#ifndef TC_H_MIN_INGRESS >>> +#define TC_H_MIN_INGRESS 0xFFF2U >>> +#endif >>> + >>> +#ifndef TC_H_MIN_EGRESS >>> +#define TC_H_MIN_EGRESS 0xFFF3U >>> +#endif >>> + >>> +/* TCA_BPF_ID is an enumerate value in uapi/linux/pkt_cls.h. >>> + * Declare it as a macro here so old system can still work >>> + * without TCA_BPF_ID defined in pkt_cls.h. >>> + */ >>> +#define TCA_BPF_ID 11 >>> + >>> typedef int (*libbpf_dump_nlmsg_t)(void *cookie, void *msg, struct nlattr **tb); >>> typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, libbpf_dump_nlmsg_t, >>> @@ -504,6 +522,8 @@ static int __get_tc_info(void *cookie, struct tcmsg *tc, struct nlattr **tb, >>> return -EINVAL; >>> if (!tb[TCA_OPTIONS]) >>> return NL_CONT; >>> + if (TCA_BPF_MAX < TCA_BPF_ID) >>> + return -EOPNOTSUPP; >> >> I'm a bit confused here. Generally what I want to have happen is compilation >> to work always and then runtime to detect the errors. So when I compile my >> libs on machine A and run it on machine B it does what I expect. This seems >> like a bit of an ugly workaround to me. I would expect the user should >> update the uapi? > > The reason is due to the declaration > struct nlattr *tbb[TCA_BPF_MAX + 1]; > so I have to have the above to check to ensure we > don't have out-of-bound access. > > Alternative, I can redefine macro TCA_BPF_MAX to be the value > based on the *current* repo, we should be fine, I think. I'm confused why upstream libbpf compilation is failing for you. There is already a copy of pkt_sched.h with all the defines you need under: tools/include/uapi/linux/pkt_sched.h Are you saying this is not being included from tools/lib/bpf/ ? Afaik, it should be (so the patch should not be needed). Thanks, Daniel
diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c index 39f25e09b51e..e00660e0b87a 100644 --- a/tools/lib/bpf/netlink.c +++ b/tools/lib/bpf/netlink.c @@ -22,6 +22,24 @@ #define SOL_NETLINK 270 #endif +#ifndef TC_H_CLSACT +#define TC_H_CLSACT TC_H_INGRESS +#endif + +#ifndef TC_H_MIN_INGRESS +#define TC_H_MIN_INGRESS 0xFFF2U +#endif + +#ifndef TC_H_MIN_EGRESS +#define TC_H_MIN_EGRESS 0xFFF3U +#endif + +/* TCA_BPF_ID is an enumerate value in uapi/linux/pkt_cls.h. + * Declare it as a macro here so old system can still work + * without TCA_BPF_ID defined in pkt_cls.h. + */ +#define TCA_BPF_ID 11 + typedef int (*libbpf_dump_nlmsg_t)(void *cookie, void *msg, struct nlattr **tb); typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, libbpf_dump_nlmsg_t, @@ -504,6 +522,8 @@ static int __get_tc_info(void *cookie, struct tcmsg *tc, struct nlattr **tb, return -EINVAL; if (!tb[TCA_OPTIONS]) return NL_CONT; + if (TCA_BPF_MAX < TCA_BPF_ID) + return -EOPNOTSUPP; libbpf_nla_parse_nested(tbb, TCA_BPF_MAX, tb[TCA_OPTIONS], NULL); if (!tbb[TCA_BPF_ID])
libbpf is used as a submodule in bcc. When importing latest libbpf repo in bcc, I observed the following compilation errors when compiling on ubuntu 16.04. .../netlink.c:416:23: error: ‘TC_H_CLSACT’ undeclared (first use in this function) *parent = TC_H_MAKE(TC_H_CLSACT, ^ .../netlink.c:418:9: error: ‘TC_H_MIN_INGRESS’ undeclared (first use in this function) TC_H_MIN_INGRESS : TC_H_MIN_EGRESS); ^ .../netlink.c:418:28: error: ‘TC_H_MIN_EGRESS’ undeclared (first use in this function) TC_H_MIN_INGRESS : TC_H_MIN_EGRESS); ^ .../netlink.c: In function ‘__get_tc_info’: .../netlink.c:522:11: error: ‘TCA_BPF_ID’ undeclared (first use in this function) if (!tbb[TCA_BPF_ID]) ^ In ubuntu 16.04, TCA_BPF_* enumerator looks like below enum { TCA_BPF_UNSPEC, TCA_BPF_ACT, ... TCA_BPF_NAME, TCA_BPF_FLAGS, __TCA_BPF_MAX, }; #define TCA_BPF_MAX (__TCA_BPF_MAX - 1) while in latest bpf-next, the enumerator looks like enum { TCA_BPF_UNSPEC, ... TCA_BPF_FLAGS, TCA_BPF_FLAGS_GEN, TCA_BPF_TAG, TCA_BPF_ID, __TCA_BPF_MAX, }; In this patch, TCA_BPF_ID is defined as a macro with proper value and this works regardless of whether TCA_BPF_ID is defined in uapi header or not. I also added a comparison "TCA_BPF_MAX < TCA_BPF_ID" in function __get_tc_info() such that if the compare result if true, returns -EOPNOTSUPP. This is used to prevent otherwise array overflows: .../netlink.c:538:10: warning: array subscript is above array bounds [-Warray-bounds] if (!tbb[TCA_BPF_ID]) ^ Fixes: 715c5ce454a6 ("libbpf: Add low level TC-BPF management API") Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/lib/bpf/netlink.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) Changelog: v1 -> v2: - gcc 8.3 doesn't like macro condition (__TCA_BPF_MAX - 1) <= 10 where __TCA_BPF_MAX is an enumerator value. So define TCA_BPF_ID macro without macro condition.