uapi/netfilter: Change netfilter hook verdict code definition from macro to enum

[PATCH] uapi/netfilter: Change netfilter hook verdict code definition from macro to enum

Posted by David Wang 2 years, 3 months ago

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,
+};
 
 /* we overload the higher bits for encoding auxiliary data such as the queue
  * number or errno values. Not nice, but better than additional function
-- 
2.20.1

Re: [PATCH] uapi/netfilter: Change netfilter hook verdict code definition from macro to enum

Posted by Daniel Xu 2 years, 3 months ago

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

Re: [PATCH] uapi/netfilter: Change netfilter hook verdict code definition from macro to enum

Posted by David Wang 2 years, 3 months ago


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...

Re: [PATCH] uapi/netfilter: Change netfilter hook verdict code definition from macro to enum

Posted by Florian Westphal 2 years, 2 months ago

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.

Re:Re: [PATCH] uapi/netfilter: Change netfilter hook verdict code definition from macro to enum

Posted by David Wang 2 years, 2 months ago

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

Re: [PATCH] uapi/netfilter: Change netfilter hook verdict code definition from macro to enum

Posted by Florian Westphal 2 years, 3 months ago

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.

Re: [PATCH] uapi/netfilter: Change netfilter hook verdict code definition from macro to enum

Posted by Duncan Roe 2 years, 3 months ago

On Wed, Sep 06, 2023 at 12:57:56AM +0800, David Wang 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 did see code like that somewhere and wondered what was going on. The #define
lines were interspersed with the enum members which indeed looked ugly to me.

I'd suggest a block of #defines after the enum close e.g.

> #define NF_DROP NF_DROP
>...

perhaps with a comment preceding to advise that the defines were there for
the benefit of anyone using #ifdef.

Cheers ... Duncan.