| Message ID | 20230904130201.14632-1-00107082@163.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | uapi/netfilter: Change netfilter hook verdict code definition from macro to enum | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Hi David, On Mon, Sep 04, 2023 at 09:02:02PM +0800, David Wang wrote: > As BPF_PROG_TYPE_NETFILTER was added in 6.4, a netfilter > bpf program can attach to netfilter hooks, process package > and return verdict back to netfilter. But those verdict > codes are defined as macro, which could not be compiled > into BTF with btf.c. libbpf, and maybe other bpf tools, > would extract information from BTF and generate a > common header "vmlinux.h". With macro definition, netfilter > bpf program would have to redefine those macro again, > besides including "vmlinux.h". > > This code change netfilter hook verdict code definition to > enum, this way, make it into BTF. > > Signed-off-by: David Wang <00107082@163.com> > --- > include/uapi/linux/netfilter.h | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/include/uapi/linux/netfilter.h b/include/uapi/linux/netfilter.h > index 5a79ccb76701..d2f5dfab20dc 100644 > --- a/include/uapi/linux/netfilter.h > +++ b/include/uapi/linux/netfilter.h > @@ -8,13 +8,15 @@ > #include <linux/in6.h> > > /* Responses from hook functions. */ > -#define NF_DROP 0 > -#define NF_ACCEPT 1 > -#define NF_STOLEN 2 > -#define NF_QUEUE 3 > -#define NF_REPEAT 4 > -#define NF_STOP 5 /* Deprecated, for userspace nf_queue compatibility. */ > -#define NF_MAX_VERDICT NF_STOP > +enum { > + NF_DROP = 0, > + NF_ACCEPT = 1, > + NF_STOLEN = 2, > + NF_QUEUE = 3, > + NF_REPEAT = 4, > + NF_STOP = 5, /* Deprecated, for userspace nf_queue compatibility. */ > + NF_MAX_VERDICT = NF_STOP, > +}; Switching from macro to enum works for almost all use cases, but not all. If someone if #ifdefing the symbols (which is plausible) this change would break them. I think I've seen some other networking code define both enums and macros. But it was a little ugly. Not sure if that is acceptable here or not. [...] Thanks, Daniel
At 2023-09-06 00:38:02, "Daniel Xu" <dxu@dxuuu.xyz> wrote: >Hi David, > >On Mon, Sep 04, 2023 at 09:02:02PM +0800, David Wang wrote: >> #include <linux/in6.h> >> >> /* Responses from hook functions. */ >> -#define NF_DROP 0 >> -#define NF_ACCEPT 1 >> -#define NF_STOLEN 2 >> -#define NF_QUEUE 3 >> -#define NF_REPEAT 4 >> -#define NF_STOP 5 /* Deprecated, for userspace nf_queue compatibility. */ >> -#define NF_MAX_VERDICT NF_STOP >> +enum { >> + NF_DROP = 0, >> + NF_ACCEPT = 1, >> + NF_STOLEN = 2, >> + NF_QUEUE = 3, >> + NF_REPEAT = 4, >> + NF_STOP = 5, /* Deprecated, for userspace nf_queue compatibility. */ >> + NF_MAX_VERDICT = NF_STOP, >> +}; > >Switching from macro to enum works for almost all use cases, but not >all. If someone if #ifdefing the symbols (which is plausible) this >change would break them. > >I think I've seen some other networking code define both enums and >macros. But it was a little ugly. Not sure if that is acceptable here or >not. > >[...] > >Thanks, >Daniel Thanks for the review~ I do not have a strong reasoning to deny the possibility of breaking unexpected usage of this macros, but I also agree that it is ugly to use both enum and macro at the same time. Kind of don't know how to proceed from here now...
David Wang <00107082@163.com> wrote: > At 2023-09-06 00:38:02, "Daniel Xu" <dxu@dxuuu.xyz> wrote: > >Hi David, > > > >On Mon, Sep 04, 2023 at 09:02:02PM +0800, David Wang wrote: > > >> #include <linux/in6.h> > >> > >> /* Responses from hook functions. */ > >> -#define NF_DROP 0 > >> -#define NF_ACCEPT 1 > >> -#define NF_STOLEN 2 > >> -#define NF_QUEUE 3 > >> -#define NF_REPEAT 4 > >> -#define NF_STOP 5 /* Deprecated, for userspace nf_queue compatibility. */ > >> -#define NF_MAX_VERDICT NF_STOP > >> +enum { > >> + NF_DROP = 0, > >> + NF_ACCEPT = 1, > >> + NF_STOLEN = 2, > >> + NF_QUEUE = 3, > >> + NF_REPEAT = 4, > >> + NF_STOP = 5, /* Deprecated, for userspace nf_queue compatibility. */ > >> + NF_MAX_VERDICT = NF_STOP, > >> +}; > > > >Switching from macro to enum works for almost all use cases, but not > >all. If someone if #ifdefing the symbols (which is plausible) this > >change would break them. > > > >I think I've seen some other networking code define both enums and > >macros. But it was a little ugly. Not sure if that is acceptable here or > >not. > > > >[...] > > > >Thanks, > >Daniel > > > Thanks for the review~ > I do not have a strong reasoning to deny the possibility of breaking unexpected usage of this macros, > > but I also agree that it is ugly to use both enum and macro at the same time. > > Kind of don't know how to proceed from here now... I don't see anyone doing #ifdef tests on these, so I suggest we give your patch a try and see if anything breaks. Technically only ACCEPT and DROP can be used by bpf programs but splitting it in enum-for-accept-drop-and-define-for-the-rest looks even more silly.
David Wang <00107082@163.com> wrote: Hello, > At 2023-09-06 00:38:02, "Daniel Xu" <dxu@dxuuu.xyz> wrote: > >Hi David, > > > >On Mon, Sep 04, 2023 at 09:02:02PM +0800, David Wang wrote: > > >> #include <linux/in6.h> > >> > >> /* Responses from hook functions. */ > >> -#define NF_DROP 0 > >> -#define NF_ACCEPT 1 > >> -#define NF_STOLEN 2 > >> -#define NF_QUEUE 3 > >> -#define NF_REPEAT 4 > >> -#define NF_STOP 5 /* Deprecated, for userspace nf_queue compatibility. */ > >> -#define NF_MAX_VERDICT NF_STOP > >> +enum { > >> + NF_DROP = 0, > >> + NF_ACCEPT = 1, > >> + NF_STOLEN = 2, > >> + NF_QUEUE = 3, > >> + NF_REPEAT = 4, > >> + NF_STOP = 5, /* Deprecated, for userspace nf_queue compatibility. */ > >> + NF_MAX_VERDICT = NF_STOP, > >> +}; > > > >Switching from macro to enum works for almost all use cases, but not > >all. If someone if #ifdefing the symbols (which is plausible) this > >change would break them. > > > >I think I've seen some other networking code define both enums and > >macros. But it was a little ugly. Not sure if that is acceptable here or > >not. > > > >[...] > > > >Thanks, > >Daniel > > > Thanks for the review~ > I do not have a strong reasoning to deny the possibility of breaking unexpected usage of this macros, > > but I also agree that it is ugly to use both enum and macro at the same time. > > Kind of don't know how to proceed from here now... I was about to apply this as-is, but Pablo Neira would prefer to keep the defines as well. So, as a compromise, I would suggest to just *add* /* verdicts available to BPF are exported via vmlinux.h */ enum { NF_DROP = 0, NF_ACCEPT = 1, }; #define NF_DROP 0 ... This way BTF won't have the other verdicts, but ATM those cannot be used in BPF programs anyway. Would you mind making a new version of the patch? Otherwise I can mangle it locally here as needed.
At 2023-09-28 19:53:59, "Florian Westphal" <fw@strlen.de> wrote: > >I was about to apply this as-is, but Pablo Neira would prefer to >keep the defines as well. > >So, as a compromise, I would suggest to just *add* > >/* verdicts available to BPF are exported via vmlinux.h */ >enum { > NF_DROP = 0, > NF_ACCEPT = 1, >}; > >#define NF_DROP 0 >... > >This way BTF won't have the other verdicts, but ATM those >cannot be used in BPF programs anyway. > >Would you mind making a new version of the patch? >Otherwise I can mangle it locally here as needed. Sorry for this late response, I got caught up by an unexpected personal "crisis" for quite a long while.. Hope you have already made the change, and it is OK. David
diff --git a/include/uapi/linux/netfilter.h b/include/uapi/linux/netfilter.h index 5a79ccb76701..d2f5dfab20dc 100644 --- a/include/uapi/linux/netfilter.h +++ b/include/uapi/linux/netfilter.h @@ -8,13 +8,15 @@ #include <linux/in6.h> /* Responses from hook functions. */ -#define NF_DROP 0 -#define NF_ACCEPT 1 -#define NF_STOLEN 2 -#define NF_QUEUE 3 -#define NF_REPEAT 4 -#define NF_STOP 5 /* Deprecated, for userspace nf_queue compatibility. */ -#define NF_MAX_VERDICT NF_STOP +enum { + NF_DROP = 0, + NF_ACCEPT = 1, + NF_STOLEN = 2, + NF_QUEUE = 3, + NF_REPEAT = 4, + NF_STOP = 5, /* Deprecated, for userspace nf_queue compatibility. */ + NF_MAX_VERDICT = NF_STOP, +}; /* we overload the higher bits for encoding auxiliary data such as the queue * number or errno values. Not nice, but better than additional function
As BPF_PROG_TYPE_NETFILTER was added in 6.4, a netfilter bpf program can attach to netfilter hooks, process package and return verdict back to netfilter. But those verdict codes are defined as macro, which could not be compiled into BTF with btf.c. libbpf, and maybe other bpf tools, would extract information from BTF and generate a common header "vmlinux.h". With macro definition, netfilter bpf program would have to redefine those macro again, besides including "vmlinux.h". This code change netfilter hook verdict code definition to enum, this way, make it into BTF. Signed-off-by: David Wang <00107082@163.com> --- include/uapi/linux/netfilter.h | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)