Re: [PATCH net v1] net: defer __dev_set_promiscuity() to avoid sleeping in atomic context

[Stanislav Fomichev]

On 02/14, Jiayuan Chen wrote:
> From: Jiayuan Chen <jiayuan.chen@xxxxxxxxxx>
> 
> __dev_set_rx_mode() is called with addr_list_lock (spinlock) held from
> many places in dev_addr_lists.c. When a device lacks IFF_UNICAST_FLT,
> __dev_set_rx_mode() calls __dev_set_promiscuity() which propagates
> through dev_change_rx_flags -> ndo_change_rx_flags -> dev_set_promiscuity
> on lower devices. Since commit 78cd408356fe ("net: add missing instance
> lock to dev_set_promiscuity"), dev_set_promiscuity() acquires the netdev
> instance lock (mutex) via netdev_lock_ops(). This leads to a
> "sleeping function called from invalid context" / "Invalid wait context"
> bug when the lower device has request_ops_lock or queue_mgmt_ops set.
> 
> The call chain is:
> 
>   dev_uc_add(bridge0)                      # e.g. from macsec_dev_open
>     netif_addr_lock_bh(bridge0)            # <- spinlock, BH disabled
>     __dev_set_rx_mode(bridge0)             # bridge has no IFF_UNICAST_FLT
>       __dev_set_promiscuity(bridge0)
>         ndo_change_rx_flags(bridge0)
>           br_manage_promisc -> dev_set_promiscuity(team0)
>             ndo_change_rx_flags(team0)
>               team_change_rx_flags -> dev_set_promiscuity(dummy0)
>                 netdev_lock_ops(dummy0)    # <- mutex! dummy has
>                                            #    request_ops_lock=true
> 
> This is not limited to bridge/team/dummy. Any combination of stacking
> devices (bridge, bond, macvlan, vlan, macsec, team, dsa, netvsc) over
> devices with instance lock (dummy, mlx5, bnxt, gve) can trigger this.
> 
> Fix this by deferring __dev_set_promiscuity() to after the spinlock is
> released:
> 
> 1. Change __dev_set_rx_mode() to return a promiscuity increment value
>    (+1, 0, -1) instead of calling __dev_set_promiscuity() directly.
>    The uc_promisc flag is still updated under the lock for correctness.
> 
> 2. Change dev_set_rx_mode() to call __dev_set_promiscuity() after
>    releasing addr_list_lock, based on the returned increment.
> 
> 3. Change all callers in dev_addr_lists.c to release their spinlock
>    first, then call dev_set_rx_mode() which handles both the rx mode
>    update and the deferred promiscuity change safely.

[..]

> Reproducer:
> 
>   ip link add dummy0 type dummy
>   ip link add team0 type team
>   ip link set dummy0 master team0
>   ip link set team0 up
>   ip link add bridge0 type bridge vlan_filtering 1
>   ip link set bridge0 up
>   ip link set team0 master bridge0
>   ip link add macsec0 link bridge0 type macsec
>   ip link set macsec0 up   # triggers the bug

Can you add it as a selftest under selftests/drivers/net/team/?