Re: [v3,net-next,08/15] ipmr: Move unregister_netdevice_many() out of ipmr_free_table().

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ipmr: Move unregister_netdevice_many() out of ipmr_free_table().

This commit refactors network device cleanup in the IP multicast routing
code by moving unregister_netdevice_many() from ipmr_free_table() to its
callers, enabling batched cleanup across multiple tables per netns.

> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index d15e05662b09..7e2aa2026f01 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c

[ ... ]

> @@ -349,8 +355,13 @@ static int __net_init ipmr_rules_init(struct net *net)
>
>  static void __net_exit ipmr_rules_exit(struct net *net)
>  {
> +	LIST_HEAD(dev_kill_list);
> +
>  	ASSERT_RTNL();
> -	ipmr_free_table(net->ipv4.mrt);
> +
> +	ipmr_free_table(net->ipv4.mrt, &dev_kill_list);
> +	unregister_netdevice_many(&dev_kill_list);
> +
>  	net->ipv4.mrt = NULL;
>  }

In the !CONFIG_IP_MROUTE_MULTIPLE_TABLES version of ipmr_rules_exit(),
does this sequence lead to a use-after-free?

ipmr_free_table() calls kfree(mrt) to free the table:

net/ipv4/ipmr.c:ipmr_free_table() {
    ...
    rhltable_destroy(&mrt->mfc_hash);
    kfree(mrt);
}

Then unregister_netdevice_many() is called, which triggers NETDEV_UNREGISTER
notifications. The global notifier ipmr_device_event() handles this event
and calls ipmr_for_each_table().

In the non-MULTIPLE_TABLES case, ipmr_for_each_table() expands to:

    for (mrt = net->ipv4.mrt; mrt; mrt = NULL)

Since net->ipv4.mrt still points to the freed memory at this point (it's
only set to NULL after unregister_netdevice_many() completes), won't
ipmr_device_event() access mrt->vif_table and mrt->maxvif from a freed
slab object?

The CONFIG_IP_MROUTE_MULTIPLE_TABLES version appears safe because
list_del(&mrt->list) removes the table from the iteration list before
kfree().

Should net->ipv4.mrt = NULL be moved before calling
unregister_netdevice_many(), or before calling ipmr_free_table()?

> @@ -425,18 +436,16 @@ static struct mr_table *ipmr_new_table(struct net *net, u32 id)
>  			      ipmr_expire_process, ipmr_new_table_set);
>  }
>
> -static void ipmr_free_table(struct mr_table *mrt)
> +static void ipmr_free_table(struct mr_table *mrt, struct list_head *dev_kill_list)
>  {
>  	struct net *net = read_pnet(&mrt->net);
> -	LIST_HEAD(dev_kill_list);
>
>  	WARN_ON_ONCE(!mr_can_free_table(net));
>
>  	timer_shutdown_sync(&mrt->ipmr_expire_timer);
>  	mroute_clean_tables(mrt, MRT_FLUSH_VIFS | MRT_FLUSH_VIFS_STATIC |
>  			    MRT_FLUSH_MFC | MRT_FLUSH_MFC_STATIC,
> -			    &dev_kill_list);
> -	unregister_netdevice_many(&dev_kill_list);
> +			    dev_kill_list);
>  	rhltable_destroy(&mrt->mfc_hash);
>  	kfree(mrt);
>  }