RE: [PATCH v2 net-next 5/7] net: dsa: netc: enable ingress port filtering lookup by default

> > @@ -1753,8 +1757,6 @@ static int netc_port_add_host_flood_rule(struct
> netc_port *np,
> >  	np->uc = uc;
> >  	np->mc = mc;
> >  	np->ipft_hf_eid = host_flood->entry_id;
> > -	/* Enable ingress port filter table lookup */
> > -	netc_port_wr(np, NETC_PIPFCR, PIPFCR_EN);
> >
> >  free_host_flood:
> >  	kfree(host_flood);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does
> netc_port_add_host_flood_rule() execute sleepable operations in atomic
> context?
> 
> When userspace changes interface flags (e.g., ip link set dev ethX promisc on),
> the network core calls ndo_change_rx_flags while holding
> netif_addr_lock_bh(),
> which disables bottom halves and establishes an atomic context.
> 
> The DSA core delegates this to the port_set_host_flood callback, which calls
> netc_port_set_host_flood() -> netc_port_add_host_flood_rule() synchronously.
> 
> Looking at the code, do these functions execute sleepable operations?
> 
> - Allocating memory using GFP_KERNEL (via kzalloc_obj() in
>   netc_port_add_host_flood_rule())
> - Acquiring a sleeping lock (via mutex_lock() in ntmp_select_and_lock_cbdr())
> - Yielding to the scheduler (via read_poll_timeout() with sleep=true in
>   netc_xmit_ntmp_cmd())
> 
> Could this lead to a "scheduling while atomic" panic if
> CONFIG_DEBUG_ATOMIC_SLEEP is enabled? Should this hardware
> configuration
> be deferred to a workqueue?
> 

This a false positive, dev_change_rx_flags() is not called in the
netif_addr_lock_bh context.

dev_change_rx_flags()
├── __dev_set_promiscuity()
│   ├── netif_set_promiscuity()
│   │   └── dev_set_promiscuity()          [netdev_lock_ops = mutex]
│   ├── __dev_change_flags()
│   │   ├── netif_change_flags()
│   │   │   ├── do_setlink()               [rtnl_nets_lock = mutex]
│   │   │   ├── dev_change_flags()         [netdev_lock_ops = mutex]
│   │   │   └── vlan_dev_work/ndo_work     [netdev_lock_ops = mutex]
│   │   └── rtnl_configure_link()          [netdev_lock_ops = mutex]
│   └── netif_rx_mode_run()                [might_sleep() + netdev_assert_locked_ops_compat]
│       ├── netif_rx_mode_sync()           [netdev_lock_ops = mutex]
│       └── netdev_work_proc()             [rtnl_lock + netdev_lock_ops = mutex]
├── netif_set_allmulti()
│   ├── __dev_change_flags()               (Same as above)
│   └── dev_set_allmulti()                 [netdev_lock_ops = mutex]
└── __dev_change_flags()                   (Same as above)

__dev_set_rx_mode() is called with netif_addr_lock_bh() held, placing it
in atomic context. However, drivers implementing ndo_change_rx_flags
are not invoked directly from this path.

Inside __dev_set_rx_mode(), there is an explicit guard:

if (ops->ndo_set_rx_mode_async || ops->ndo_change_rx_flags ||
    netdev_need_ops_lock(dev)) {
    netif_rx_mode_queue(dev);
    return;
}

If a driver implements ndo_change_rx_flags, the function
immediately enqueues a work item via netif_rx_mode_queue() and
returns without invoking any driver callback. The actual
ndo_change_rx_flags call is deferred to a workqueue, which runs in
process context under rtnl_lock and netdev_lock_ops() — both mutexes
that allow sleeping.