Re: [PATCH] netfilter: Consistently use NFPROTO_, not AF_

[Florian Westphal <fw@strlen.de>]

Keno Fischer <keno@juliahub.com> wrote:
> The uapi headers document `nfgen_family` as `AF_*`. However,
> this hasn't been technically true since 7e9c6eeb, which switched
> the interpretation of this field to `NFPROTO_*`.
> This is value-compatible on AF_INET (though note that this is
> NFPROTO_IPV4, *not* NFPROTO_INET), AF_INET6, AF_IPV6 and AF_DECnet,
> and AF_BRIDGE, but has since grown additional values.
> Now, because of the value compatibility between AF_ and
> NFPROTO_, it doesn't matter too much, but to the extent that
> the uapi headers constitute interface documentation, it can be
> misleading. For example, some userspace tooling, such as wireshark
> will print AF_UNIX for netlink packets that have an NFPROTO_INET
> family set. I will submit a patch for this downstream, but I wanted
> to cleanup the kernel side also. To that end, change the comment in
> the UAPI header and audit uses of AF_* in the netfilter code and
> switch them to NFPROTO_ unless calling non-netfilter APIs.

I'm not sold on this patch, lots of code churn for little gain.

> diff --git a/include/uapi/linux/netfilter/nfnetlink.h b/include/uapi/linux/netfilter/nfnetlink.h
> index 6cd58cd2a6f0..9d7fe3daf327 100644
> --- a/include/uapi/linux/netfilter/nfnetlink.h
> +++ b/include/uapi/linux/netfilter/nfnetlink.h
> @@ -32,7 +32,7 @@ enum nfnetlink_groups {
>  /* General form of address family dependent message.
>   */
>  struct nfgenmsg {
> -	__u8  nfgen_family;		/* AF_xxx */
> +	__u8  nfgen_family;		/* NFPROTO_xxx */
>  	__u8  version;		/* nfnetlink version */
>  	__be16    res_id;		/* resource id */
>  };

Maybe just chase down comments like this and leave the rest alone?

> @@ -690,7 +690,7 @@ module_param_call(hashsize, nf_conntrack_set_hashsize, param_get_uint,
>  		  &nf_conntrack_htable_size, 0600);
>  
>  MODULE_ALIAS("ip_conntrack");
> -MODULE_ALIAS("nf_conntrack-" __stringify(AF_INET));
> -MODULE_ALIAS("nf_conntrack-" __stringify(AF_INET6));
> +MODULE_ALIAS("nf_conntrack-" __stringify(NFPROTO_IPV4));
> +MODULE_ALIAS("nf_conntrack-" __stringify(NFPROTO_IPV6));

This breaks module autloading aliases, __stringify needs a define not enum:

alias:          nf_conntrack-NFPROTO_IPV6
alias:          nf_conntrack-NFPROTO_IPV4

... this NFPROTO_IPV6 should not be there, this needs to be kept as

alias:          nf_conntrack-10
alias:          nf_conntrack-2