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@xxxxxxxxx>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
> 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
>
