Re: [PATCH net] af_packet: move notifier's packet_dev_mc out of rcu critical section

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Stanislav Fomichev wrote:
> Calling `PACKET_ADD_MEMBERSHIP` on an ops-locked device can trigger
> the `NETDEV_UNREGISTER` notifier,

This made it sound to me as if the notifier is called as a result of
the setsockopt.

If respinning, please rewrite to make clear that the two are
independent events. The stack trace in the bug also makes clear
that the notifier trigger is a device going away.

> which may require disabling promiscuous
> and/or allmulti mode. Both of these operations require acquiring the netdev
> instance lock. Move the call to `packet_dev_mc` outside of the RCU critical
> section.
> 
> Closes: https://syzkaller.appspot.com/bug?extid=b191b5ccad8d7a986286
> Reported-by: syzbot+b191b5ccad8d7a986286@syzkaller.appspotmail.com
> Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs operations")
> Signed-off-by: Stanislav Fomichev <stfomichev@gmail.com>
> ---
>  net/packet/af_packet.c | 20 +++++++++++++++-----
>  net/packet/internal.h  |  1 +
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index d4dba06297c3..5a6132816b2e 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3713,15 +3713,15 @@ static int packet_dev_mc(struct net_device *dev, struct packet_mclist *i,
>  }
>  
>  static void packet_dev_mclist_delete(struct net_device *dev,
> -				     struct packet_mclist **mlp)
> +				     struct packet_mclist **mlp,
> +				     struct list_head *list)
>  {
>  	struct packet_mclist *ml;
>  
>  	while ((ml = *mlp) != NULL) {
>  		if (ml->ifindex == dev->ifindex) {
> -			packet_dev_mc(dev, ml, -1);
> +			list_add(&ml->remove_list, list);
>  			*mlp = ml->next;
> -			kfree(ml);
>  		} else
>  			mlp = &ml->next;
>  	}
> @@ -4233,9 +4233,11 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,
>  static int packet_notifier(struct notifier_block *this,
>  			   unsigned long msg, void *ptr)
>  {
> -	struct sock *sk;
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>  	struct net *net = dev_net(dev);
> +	struct packet_mclist *ml, *tmp;
> +	LIST_HEAD(mclist);
> +	struct sock *sk;
>  
>  	rcu_read_lock();
>  	sk_for_each_rcu(sk, &net->packet.sklist) {
> @@ -4244,7 +4246,8 @@ static int packet_notifier(struct notifier_block *this,
>  		switch (msg) {
>  		case NETDEV_UNREGISTER:
>  			if (po->mclist)
> -				packet_dev_mclist_delete(dev, &po->mclist);
> +				packet_dev_mclist_delete(dev, &po->mclist,
> +							 &mclist);
>  			fallthrough;
>  
>  		case NETDEV_DOWN:
> @@ -4277,6 +4280,13 @@ static int packet_notifier(struct notifier_block *this,
>  		}
>  	}
>  	rcu_read_unlock();
> +
> +	/* packet_dev_mc might grab instance locks so can't run under rcu */
> +	list_for_each_entry_safe(ml, tmp, &mclist, remove_list) {
> +		packet_dev_mc(dev, ml, -1);
> +		kfree(ml);
> +	}
> +

Just verifying my understanding of the not entirely obvious locking:

po->mclist modifications (add, del, flush, unregister) are all
protected by the RTNL, not the RCU. The RCU only protects the sklist
and by extension the sks on it. So moving the mclist operations out of
the RCU is fine.

The delayed operation on the mclist entry is still within the RTNL
from unregister_netdevice_notifier. Which matter as it protects not
only the list, but also the actual operations in packet_dev_mc, such
as inc/dec on dev->promiscuity and associated dev_change_rx_flags.
And new packet_mclist.remove_list too.

>  	return NOTIFY_DONE;
>  }
>  
> diff --git a/net/packet/internal.h b/net/packet/internal.h
> index d5d70712007a..1e743d0316fd 100644
> --- a/net/packet/internal.h
> +++ b/net/packet/internal.h
> @@ -11,6 +11,7 @@ struct packet_mclist {
>  	unsigned short		type;
>  	unsigned short		alen;
>  	unsigned char		addr[MAX_ADDR_LEN];
> +	struct list_head	remove_list;

INIT_LIST_HEAD on alloc in packet_mc_add?

>  };
>  
>  /* kbdq - kernel block descriptor queue */
> -- 
> 2.49.0
>