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@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?
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
