[PATCH bpf-next v2] libbpf: fix compilation errors on ubuntu 16.04

[PATCH bpf-next v2] libbpf: fix compilation errors on ubuntu 16.04

[Yonghong Song]

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;
 
 	libbpf_nla_parse_nested(tbb, TCA_BPF_MAX, tb[TCA_OPTIONS], NULL);
 	if (!tbb[TCA_BPF_ID])
-- 
2.30.2

RE: [PATCH bpf-next v2] libbpf: fix compilation errors on ubuntu 16.04

[John Fastabend]

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
> 

Re: [PATCH bpf-next v2] libbpf: fix compilation errors on ubuntu 16.04

[Yonghong Song]

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

Re: [PATCH bpf-next v2] libbpf: fix compilation errors on ubuntu 16.04

[Daniel Borkmann]

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