Re: [nf PATCH] netfilter: nf_tables: Fix netdev hook sanity checks

Hi Fernando,

On Tue, Sep 01, 2026 at 01:15:05PM +0200, Fernando Fernandez Mancera wrote:
> On 8/27/26 10:48 PM, Phil Sutter wrote:
> > Wildcard interface names were confused with non-wildcard ones or those
> > matching the wildcard's prefix. Fix for odd behaviour when updating
> > flowtable or netdev chain hook devices.
> > 
> > Unify behaviour between flowtable and netdev chain parsers by having
> > both call the same parsing/validating code, also with old NFTA_HOOK_DEV
> > attribute (which is still used for single hook updates by user space).
> > 
> > The validating code must distinguish between hooks being added or
> > deleted (duplicates are ignored in the former case). Transport this info
> > in a 'bool add' parameter, but rename the existing one with same name
> > first since that actually reflects whether the code is parsing hooks for
> > a new object or an existing one.
> > 
> > Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> > Signed-off-by: Phil Sutter <[email protected]>
> > ---
> > This is a follow-up to Fernando's "netfilter: nf_tables: fix device name
> > and prefix match in hook lookup", sorting odd behaviour in similar,
> > other cases, too. With his v4 and this patch in place, the new nftables
> > shell tests pass for me.
> > ---
> >   net/netfilter/nf_tables_api.c | 102 +++++++++++++++++++++++-----------
> >   1 file changed, 71 insertions(+), 31 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 500ef483e485..8fac88f07d74 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -2450,10 +2450,44 @@ static struct nft_hook *nft_hook_list_find(struct list_head *hook_list,
> >   	return NULL;
> >   }
> >   
> > +static struct nft_hook *
> > +nf_tables_parse_netdev_hook(struct net *net,
> > +			    const struct nlattr *tmp,
> > +			    struct list_head *hook_list,
> > +			    struct list_head *old_hook_list,
> > +			    bool prefix, bool add)
> > +{
> > +	struct nft_hook *hook;
> > +
> > +	hook = nft_netdev_hook_alloc(net, tmp, prefix);
> > +	if (IS_ERR(hook))
> > +		return hook;
> > +
> > +	/* Reject new hooks which overlap */
> > +	if (nft_hook_list_find(hook_list, hook, false)) {
> > +		nft_netdev_hook_free(hook);
> > +		return ERR_PTR(-EEXIST);
> > +	}
> > +	/* Skip this exact duplicate when adding */
> > +	if (nft_hook_list_find(old_hook_list, hook, true)) {
> > +		if (add) {
> > +			nft_netdev_hook_free(hook);
> > +			return NULL;
> > +		}
> > +	/* Reject partial overlaps in either case */
> > +	} else if (nft_hook_list_find(old_hook_list, hook, false)) {
> > +		nft_netdev_hook_free(hook);
> > +		return ERR_PTR(add ? -EEXIST : -ENOENT);
> > +	}
> > +	return hook;
> > +}
> > +
> >   static int nf_tables_parse_netdev_hooks(struct net *net,
> >   					const struct nlattr *attr,
> >   					struct list_head *hook_list,
> > -					struct netlink_ext_ack *extack)
> > +					struct list_head *old_hook_list,
> > +					struct netlink_ext_ack *extack,
> > +					bool add)
> >   {
> >   	struct nft_hook *hook, *next;
> >   	const struct nlattr *tmp;
> > @@ -2473,18 +2507,17 @@ static int nf_tables_parse_netdev_hooks(struct net *net,
> >   			goto err_hook;
> >   		}
> >   
> > -		hook = nft_netdev_hook_alloc(net, tmp, prefix);
> > +		hook = nf_tables_parse_netdev_hook(net, tmp, hook_list,
> > +						   old_hook_list, prefix, add);
> > +		if (!hook)
> > +			continue;
> > +
> >   		if (IS_ERR(hook)) {
> >   			NL_SET_BAD_ATTR(extack, tmp);
> >   			err = PTR_ERR(hook);
> >   			goto err_hook;
> >   		}
> > -		if (nft_hook_list_find(hook_list, hook, false)) {
> > -			NL_SET_BAD_ATTR(extack, tmp);
> > -			nft_netdev_hook_free(hook);
> > -			err = -EEXIST;
> > -			goto err_hook;
> > -		}
> > +
> >   		list_add_tail(&hook->list, hook_list);
> >   		n++;
> >   
> > @@ -2513,13 +2546,17 @@ struct nft_chain_hook {
> >   
> >   static int nft_chain_parse_netdev(struct net *net, struct nlattr *tb[],
> >   				  struct list_head *hook_list,
> > -				  struct netlink_ext_ack *extack, u32 flags)
> > +				  struct list_head *old_hook_list,
> > +				  struct netlink_ext_ack *extack, u32 flags,
> > +				  bool add)
> >   {
> >   	struct nft_hook *hook;
> >   	int err;
> >   
> >   	if (tb[NFTA_HOOK_DEV]) {
> > -		hook = nft_netdev_hook_alloc(net, tb[NFTA_HOOK_DEV], false);
> > +		hook = nf_tables_parse_netdev_hook(net, tb[NFTA_HOOK_DEV],
> > +						   hook_list, old_hook_list,
> > +						   false, false);
> >   		if (IS_ERR(hook)) {
> >   			NL_SET_BAD_ATTR(extack, tb[NFTA_HOOK_DEV]);
> >   			return PTR_ERR(hook);
> > @@ -2528,7 +2565,8 @@ static int nft_chain_parse_netdev(struct net *net, struct nlattr *tb[],
> >   		list_add_tail(&hook->list, hook_list);
> >   	} else if (tb[NFTA_HOOK_DEVS]) {
> >   		err = nf_tables_parse_netdev_hooks(net, tb[NFTA_HOOK_DEVS],
> > -						   hook_list, extack);
> > +						   hook_list, old_hook_list,
> > +						   extack, add);
> >   		if (err < 0)
> >   			return err;
> >   
> > @@ -2544,8 +2582,10 @@ static int nft_chain_parse_netdev(struct net *net, struct nlattr *tb[],
> >   static int nft_chain_parse_hook(struct net *net,
> >   				struct nft_base_chain *basechain,
> >   				const struct nlattr * const nla[],
> > -				struct nft_chain_hook *hook, u8 family,
> > -				u32 flags, struct netlink_ext_ack *extack)
> > +				struct nft_chain_hook *hook,
> > +				struct list_head *old_hook_list, u8 family,
> > +				u32 flags, struct netlink_ext_ack *extack,
> > +				bool add)
> >   {
> >   	struct nftables_pernet *nft_net = nft_pernet(net);
> >   	struct nlattr *ha[NFTA_HOOK_MAX + 1];
> > @@ -2623,7 +2663,8 @@ static int nft_chain_parse_hook(struct net *net,
> >   
> >   	INIT_LIST_HEAD(&hook->list);
> >   	if (nft_base_chain_netdev(family, hook->num)) {
> > -		err = nft_chain_parse_netdev(net, ha, &hook->list, extack, flags);
> > +		err = nft_chain_parse_netdev(net, ha, &hook->list,
> > +					     old_hook_list, extack, flags, add);
> >   		if (err < 0) {
> >   			module_put(type->owner);
> >   			return err;
> > @@ -2756,6 +2797,7 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 policy,
> >   	if (nla[NFTA_CHAIN_HOOK]) {
> >   		struct nft_stats __percpu *stats = NULL;
> >   		struct nft_chain_hook hook = {};
> > +		LIST_HEAD(empty);
> >   
> >   		if (table->flags & __NFT_TABLE_F_UPDATE)
> >   			return -EINVAL;
> > @@ -2763,8 +2805,8 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 policy,
> >   		if (flags & NFT_CHAIN_BINDING)
> >   			return -EOPNOTSUPP;
> >   
> > -		err = nft_chain_parse_hook(net, NULL, nla, &hook, family, flags,
> > -					   extack);
> > +		err = nft_chain_parse_hook(net, NULL, nla, &hook, &empty,
> > +					   family, flags, extack, true);
> >   		if (err < 0)
> >   			return err;
> >   
> > @@ -2916,7 +2958,8 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
> >   
> >   		basechain = nft_base_chain(chain);
> >   		err = nft_chain_parse_hook(ctx->net, basechain, nla, &hook,
> > -					   ctx->family, flags, extack);
> > +					   &basechain->hook_list,
> > +					   ctx->family, flags, extack, true);
> >   		if (err < 0)
> >   			return err;
> >   
> > @@ -2936,7 +2979,7 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
> >   					ops->hook	= basechain->ops.hook;
> >   				}
> >   
> > -				if (nft_hook_list_find(&basechain->hook_list, h, false)) {
> > +				if (nft_hook_list_find(&basechain->hook_list, h, true)) {
> >   					list_del(&h->list);
> >   					nft_netdev_hook_free(h);
> >   					continue;
> > @@ -3246,7 +3289,8 @@ static int nft_delchain_hook(struct nft_ctx *ctx,
> >   		return -EOPNOTSUPP;
> >   
> >   	err = nft_chain_parse_hook(ctx->net, basechain, nla, &chain_hook,
> > -				   ctx->family, chain->flags, extack);
> > +				   &basechain->hook_list, ctx->family,
> > +				   chain->flags, extack, false);
> >   	if (err < 0)
> >   		return err;
> >   
> > @@ -8866,7 +8910,8 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx,
> >   				    const struct nlattr * const nla[],
> >   				    struct nft_flowtable_hook *flowtable_hook,
> >   				    struct nft_flowtable *flowtable,
> > -				    struct netlink_ext_ack *extack, bool add)
> > +				    struct netlink_ext_ack *extack,
> > +				    bool new, bool add)
> >   {
> >   	struct nlattr *tb[NFTA_FLOWTABLE_HOOK_MAX + 1];
> >   	struct nf_hook_ops *ops;
> > @@ -8882,7 +8927,7 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx,
> >   	if (err < 0)
> >   		return err;
> >   
> > -	if (add) {
> > +	if (new) {
> >   		if (!tb[NFTA_FLOWTABLE_HOOK_NUM] ||
> >   		    !tb[NFTA_FLOWTABLE_HOOK_PRIORITY]) {
> >   			NL_SET_BAD_ATTR(extack, nla[NFTA_FLOWTABLE_NAME]);
> > @@ -8918,7 +8963,8 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx,
> >   		err = nf_tables_parse_netdev_hooks(ctx->net,
> >   						   tb[NFTA_FLOWTABLE_HOOK_DEVS],
> >   						   &flowtable_hook->list,
> > -						   extack);
> > +						   &flowtable->hook_list,
> > +						   extack, add);
> >   		if (err < 0)
> >   			return err;
> >   	}
> > @@ -9112,17 +9158,11 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
> >   	int err;
> >   
> >   	err = nft_flowtable_parse_hook(ctx, nla, &flowtable_hook, flowtable,
> > -				       extack, false);
> > +				       extack, false, true);
> >   	if (err < 0)
> >   		return err;
> >   
> >   	list_for_each_entry_safe(hook, next, &flowtable_hook.list, list) {
> > -		if (nft_hook_list_find(&flowtable->hook_list, hook, false)) {
> > -			list_del(&hook->list);
> > -			nft_netdev_hook_free(hook);
> > -			continue;
> > -		}
> > -
> 
> Hi Phil,
> 
> Hm.. we should be able to also get rid of this check in 
> nf_tables_updchain() no? Or am I missing something? AFAICS, this is 
> already validated in the parse function with your patch.
> 
>                                  if 
> (nft_hook_list_find(&basechain->hook_list, h, true)) {
>                                          list_del(&h->list);
>                                          nft_netdev_hook_free(h);
>                                          continue;
>                                  }

Good point! It is complicated, though:

With netdev chains, we get either NFTA_HOOK_DEV or _DEVS attribute. I
didn't want to mess with the code in nft_chain_parse_netdev too much, so
I decided to always set 'add' parameter to false when calling
nf_tables_parse_netdev_hook for _DEV attribute.

Looking at it again, it seems possible to pass the real 'add' value and
call list_add_tail conditionally in there. AIUI, IS_ERR(NULL) should
evaluate to false, so that part does not need to be touched.

I'll try that and see what LLM and test suites say about it. :)

> Other than that, the patch looks good to me.

Thanks, Phil