Re: [PATCH net-next 1/6] net: add netif_rx_mode_schedule_fresh()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Claimed dead code: netif_rx_mode_schedule_fresh() is added and
  EXPORT_SYMBOL_GPL()'d with no in-tree caller in this patch.
- [Low] Claim that netif_rx_mode_schedule_fresh() cannot deliver the
  documented 'complete configuration replay' for ndo_set_rx_mode_async…

Pre-existing issues:
- [High] dev->rx_mode_retry_timer is initialized for the whole net_device
  lifetime in netif_rx_mode_init() (net/core/dev_addr_lists.c:1369) but…

--

net: add netif_rx_mode_schedule_fresh()

This adds a new exported helper that cancels any pending receive-mode retry
timer, resets the retry backoff counter, and then queues an immediate
receive-mode update via the netdev work core.  It is meant for callers that
need to replay the full receive configuration after a state transition,
as opposed to netif_rx_mode_schedule_retry() which is used after a failed
update.

No Link: tags present in the commit header.

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5d16737167eed..2246be72d8114 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -5197,6 +5197,7 @@ static inline void __dev_mc_unsync(struct net_device *dev,
>  
>  /* Functions used for secondary unicast and multicast support */
>  void dev_set_rx_mode(struct net_device *dev);
> +void netif_rx_mode_schedule_fresh(struct net_device *dev);
>  void netif_rx_mode_schedule_retry(struct net_device *dev);
>  int netif_set_promiscuity(struct net_device *dev, int inc);
>  int dev_set_promiscuity(struct net_device *dev, int inc);
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 08528ca0a8b31..4ae0510ce91fa 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -1337,6 +1337,27 @@ static void netif_rx_mode_queue(struct net_device *dev)
>  	__netdev_work_core_sched(dev, NETDEV_WORK_RX_MODE);
>  }
>  
> +/**
> + * netif_rx_mode_schedule_fresh() - schedule a fresh receive-mode update
> + * @dev: network device
> + *
> + * Cancel any pending retry and reset its backoff budget before scheduling an
> + * immediate receive-mode update. Callers use this after a state transition
> + * which requires the complete receive configuration to be replayed, rather
> + * than after a failed update (which uses netif_rx_mode_schedule_retry()).
> + *
> + * Context: sleepable. The caller must hold the device operations lock, or
> + * RTNL for a device which still uses RTNL-compatible operations.
> + */

[Severity: Low]
The kernel-doc says the helper is for callers which need "the complete
receive configuration to be replayed".  Does the helper on its own produce
that replay for an ndo_set_rx_mode_async driver?

The queued run ends up in netif_rx_mode_run(), which snapshots the address
lists and then reconciles them.  __hw_addr_list_snapshot() copies sync_cnt
and refcount, __hw_addr_sync_dev() skips entries whose sync_cnt is already
nonzero, and the reconcile step then computes a zero delta, so addresses
that were already marked synced are not pushed to the driver again.

Checking the caller added later in the series, enic does invalidate that
state first:

drivers/net/ethernet/cisco/enic/enic_main.c:enic_reset_addr_lists() {
	netif_addr_lock_bh(netdev);
	__dev_uc_unsync(netdev, NULL);
	__dev_mc_unsync(netdev, NULL);
	netif_addr_unlock_bh(netdev);
	...
}

and __hw_addr_unsync_dev() with a NULL unsync callback is documented and
implemented to just reset sync_cnt for each entry, so the following
netif_rx_mode_schedule_fresh() does re-push everything.  That makes the
behaviour correct as used, but the requirement is only visible in the
driver.

Would it be worth stating in the kernel-doc that the caller is responsible
for clearing the per-address sync state (for example via
__dev_uc_unsync(dev, NULL) / __dev_mc_unsync(dev, NULL)) before calling
this helper?

> +void netif_rx_mode_schedule_fresh(struct net_device *dev)
> +{
> +	might_sleep();
> +	netdev_assert_locked_ops_compat(dev);
> +	netif_rx_mode_cancel_retry(dev);
> +	netif_rx_mode_queue(dev);
> +}
> +EXPORT_SYMBOL_GPL(netif_rx_mode_schedule_fresh);
> +

[Severity: Low]
This isn't a bug, but at this commit netif_rx_mode_schedule_fresh() is
exported with no in-tree caller.

Grepping the tree at the end of the series shows eight references in
drivers/net/ethernet/cisco/enic/enic_main.c (enic_open(), enic_reset() and
enic_tx_hang_reset() among them), so the symbol does get its first user
inside the same series, which is the usual "add the API in one patch, add
the users in the following patches" layout.  Mentioning the intended first
consumer in the changelog would make that obvious to anyone bisecting or
reviewing this patch alone.

>  static void netif_rx_mode_retry(struct timer_list *t)
>  {
>  	struct net_device *dev =

[Severity: High]
This isn't a bug introduced by this patch, but since this patch adds a
second entry point that cancels dev->rx_mode_retry_timer, is the lifetime of
that timer fully covered anywhere?

netif_rx_mode_init() arms the timer for the whole net_device lifetime:

net/core/dev_addr_lists.c:netif_rx_mode_init() {
	__hw_addr_init(&dev->rx_mode_addr_cache);
	timer_setup(&dev->rx_mode_retry_timer, netif_rx_mode_retry, 0);
}

The only cancellation site in the tree is in __dev_close_many():

net/core/dev.c:__dev_close_many() {
	...
		netif_rx_mode_cancel_retry(dev);
		netif_set_up(dev, false);
	...
}

but netif_close_many() drops devices that are not IFF_UP before that:

net/core/dev.c:netif_close_many() {
	/* Remove the devices that don't need to be closed */
	list_for_each_entry_safe(dev, tmp, head, close_list)
		if (!(dev->flags & IFF_UP))
			list_del_init(&dev->close_list);
	...
}

netif_rx_mode_schedule_retry() has no IFF_UP, netif_device_present() or
dev_isalive() guard, and bnxt arms the timer from inside ndo_open, which
runs before __dev_open() sets IFF_UP:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_init_chip() {
	rc = bnxt_cfg_rx_mode(bp, &bp->dev->uc, true);
	if (rc == -EAGAIN) {
		netif_rx_mode_schedule_retry(bp->dev);
		rc = 0;
	}
	...
}

(bnge_netdev.c does the same.)  If that open subsequently fails, IFF_UP is
never set, so the close path skips the device and nothing cancels the timer.

On the teardown side, unregister_netdevice_many_notify() only calls
netdev_work_cancel_all(dev), and the free path only does
__hw_addr_flush(&dev->rx_mode_addr_cache); there is no
timer_shutdown_sync() or timer_delete_sync() for rx_mode_retry_timer.

Can free_netdev() then release the device while the timer is still queued,
so that netif_rx_mode_retry() dereferences freed memory through
timer_container_of() and netif_rx_mode_queue() -> netdev_work_enqueue() ->
dev_isalive(dev), and the timer wheel keeps a pointer into freed memory?
Would a timer_shutdown_sync(&dev->rx_mode_retry_timer) on the
unregister/free path, or an IFF_UP/present check in
netif_rx_mode_schedule_retry(), be the right fix here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com