[PATCH net v2] sfc: fix use-after-free of filter_work in efx_realloc_channels()

[PATCH net v2] sfc: fix use-after-free of filter_work in efx_realloc_channels()

[Sicong Huang]

With CONFIG_RFS_ACCEL, each channel has a delayed work item,
channel->filter_work, that expires aRFS filters. efx_poll() queues it
with mod_delayed_work(), and efx_filter_rfs_expire() always re-arms it
with schedule_delayed_work(dwork, 30 * HZ). Once efx_poll() has queued
it, the work is therefore always pending, queued or running. It is
only cancelled in efx_remove_filters(), which is not called on the ring
resize path.

Resizing the rings via ethtool -G calls efx_realloc_channels(). It
copies each channel, swaps the copies into efx->channel[] and frees the
old channel structures. Nothing on this path cancels the old channel's
filter_work. So the old filter_work can fire after its channel has
been freed. The timer core and delayed_work_timer_fn() then access
the freed delayed_work. efx_filter_rfs_expire() gets the freed channel
back via container_of(), dereferences it and re-arms the timer inside
the freed object.

A possible race is:
CPU0                             CPU1
efx_ethtool_set_ringparam()
efx_realloc_channels()
efx_stop_all()  //NAPI stopped, timer still armed
efx_copy_channel()
swap(efx->channel[i], ...)
efx_remove_channel(old)
kfree(old)  //free here!
                                 old->filter_work.timer fires
                                 delayed_work_timer_fn()
                                 efx_filter_rfs_expire()
                                 channel = container_of(dwork, ...)
                                 channel->rfs_last_expiry  //UAF!
                                 schedule_delayed_work(dwork, 30 * HZ)

Because the work re-arms itself every 30 seconds, this is not a narrow
timing window. One successful ethtool -G after efx_poll() has queued
filter_work is enough to trigger it.

The Siena driver has its own copy of this code in
efx_siena_realloc_channels() and has the same problem.

Introduce efx_free_channel() to cancel filter_work synchronously
before freeing the channel. Use it in efx_realloc_channels() and
efx_fini_channels(), and in their Siena equivalents. This keeps
cancellation of the per-channel expiry work inside the channel
deallocation API.

Fixes: 3af0f34290f6 ("sfc: replace asynchronous filter operations")
Signed-off-by: Sicong Huang <congei42@163.com>
---
Changes in v2:
- Introduce efx_free_channel() to cancel filter_work synchronously
  before freeing the channel. Replace the direct kfree() calls in 
  the reallocation and final teardown paths with this helper in 
  both drivers.
- Add the Fixes tag.

Link to v1: https://lore.kernel.org/netdev/20261022133927.3860186-1-congei42@163.com/

 drivers/net/ethernet/sfc/efx_channels.c       | 12 ++++++++++--
 drivers/net/ethernet/sfc/siena/efx_channels.c | 12 ++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
index f4dc3f3f4416..3e29b892e90c 100644
--- a/drivers/net/ethernet/sfc/efx_channels.c
+++ b/drivers/net/ethernet/sfc/efx_channels.c
@@ -561,6 +561,14 @@ static struct efx_channel *efx_alloc_channel(struct efx_nic *efx, int i)
 	return channel;
 }
 
+static void efx_free_channel(struct efx_channel *channel)
+{
+#ifdef CONFIG_RFS_ACCEL
+	cancel_delayed_work_sync(&channel->filter_work);
+#endif
+	kfree(channel);
+}
+
 int efx_init_channels(struct efx_nic *efx)
 {
 	unsigned int i;
@@ -589,7 +597,7 @@ void efx_fini_channels(struct efx_nic *efx)
 
 	for (i = 0; i < EFX_MAX_CHANNELS; i++)
 		if (efx->channel[i]) {
-			kfree(efx->channel[i]);
+			efx_free_channel(efx->channel[i]);
 			efx->channel[i] = NULL;
 		}
 }
@@ -898,7 +906,7 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
 		if (channel && channel->type->copy) {
 			efx_fini_napi_channel(channel);
 			efx_remove_channel(channel);
-			kfree(channel);
+			efx_free_channel(channel);
 		}
 	}
 
diff --git a/drivers/net/ethernet/sfc/siena/efx_channels.c b/drivers/net/ethernet/sfc/siena/efx_channels.c
index 1fc343598771..1a8985c153f9 100644
--- a/drivers/net/ethernet/sfc/siena/efx_channels.c
+++ b/drivers/net/ethernet/sfc/siena/efx_channels.c
@@ -563,6 +563,14 @@ static struct efx_channel *efx_alloc_channel(struct efx_nic *efx, int i)
 	return channel;
 }
 
+static void efx_free_channel(struct efx_channel *channel)
+{
+#ifdef CONFIG_RFS_ACCEL
+	cancel_delayed_work_sync(&channel->filter_work);
+#endif
+	kfree(channel);
+}
+
 int efx_siena_init_channels(struct efx_nic *efx)
 {
 	unsigned int i;
@@ -591,7 +599,7 @@ void efx_siena_fini_channels(struct efx_nic *efx)
 
 	for (i = 0; i < EFX_MAX_CHANNELS; i++)
 		if (efx->channel[i]) {
-			kfree(efx->channel[i]);
+			efx_free_channel(efx->channel[i]);
 			efx->channel[i] = NULL;
 		}
 }
@@ -932,7 +940,7 @@ int efx_siena_realloc_channels(struct efx_nic *efx, u32 rxq_entries,
 		if (channel && channel->type->copy) {
 			efx_fini_napi_channel(channel);
 			efx_siena_remove_channel(channel);
-			kfree(channel);
+			efx_free_channel(channel);
 		}
 	}
 

base-commit: f4377bf85887f57b59abb5479f7660a81ffe4449
-- 
2.34.1

Re: [PATCH net v2] sfc: fix use-after-free of filter_work in efx_realloc_channels()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2] sfc: fix use-after-free of filter_work in efx_realloc_channels()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After a ring resize, the replacement channel's aRFS expiry
  work is never started, so periodic expiry stops on that channel.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the async aRFS
  insertion work can still use a channel that efx_realloc_channels() has…
- [Medium] This problem wasn't introduced by this patch, but the cancel
  is placed late, so a race remains in efx_realloc_channels() /…

--

sfc: fix use-after-free of filter_work in efx_realloc_channels()

This patch adds an efx_free_channel() helper that synchronously cancels
the per-channel aRFS expiry work (filter_work) before freeing the
channel. The helper is used in efx_realloc_channels() and
efx_fini_channels(), and in their Siena equivalents. This closes a
use-after-free where the self-re-arming expiry work outlives a channel
freed on the ethtool -G ring resize path.

> diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> index f4dc3f3f4416c..3e29b892e90cb 100644
> --- a/drivers/net/ethernet/sfc/efx_channels.c
> +++ b/drivers/net/ethernet/sfc/efx_channels.c
> @@ -561,6 +561,14 @@ static struct efx_channel *efx_alloc_channel(struct efx_nic *efx, int i)
>  	return channel;
>  }
>  
> +static void efx_free_channel(struct efx_channel *channel)
> +{
> +#ifdef CONFIG_RFS_ACCEL
> +	cancel_delayed_work_sync(&channel->filter_work);
> +#endif
> +	kfree(channel);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but efx_free_channel() only
syncs filter_work. Can the async aRFS insertion work still touch a
channel that has been freed here?

efx_filter_rfs() queues efx->rps_slot[slot].work on system_wq and pins
only net_dev via netdev_hold(). efx_filter_rfs_work() caches the channel
pointer before it sleeps:

drivers/net/ethernet/sfc/rx_common.c:efx_filter_rfs_work() {
	struct efx_channel *channel = efx_get_channel(efx, req->rxq_index);
	...
	rc = efx->type->filter_insert(efx, &req->spec, true);
	...
		mutex_lock(&efx->rps_mutex);
		if (channel->rps_flow_id[rc] == RPS_FLOW_ID_INVALID)
			channel->rfs_filter_count++;
		channel->rps_flow_id[rc] = req->flow_id;
		mutex_unlock(&efx->rps_mutex);
	...
}

Suppose ethtool -G runs efx_realloc_channels() while this work is
sleeping in filter_insert() (MCDI) or waiting on rps_mutex. The old
channel can then be swapped out and kfree()d by efx_free_channel()
before the work reaches those accesses. None of
efx_device_detach_sync(), efx_stop_all() or efx_realloc_channels() waits
for in-flight rps_slot works.

The work would then read and write the freed channel. That includes
storing req->flow_id through an rps_flow_id pointer loaded from freed
memory, the n_rfs_succeeded / n_rfs_failed updates, and the
__efx_filter_rfs_expire(channel, ...) call on the failure path.

Should the channel teardown also wait for outstanding rps_slot works?
The Siena copy of efx_filter_rfs_work() has the same pattern.

> +
>  int efx_init_channels(struct efx_nic *efx)
>  {
>  	unsigned int i;

[ ... ]

> @@ -898,7 +906,7 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
>  		if (channel && channel->type->copy) {
>  			efx_fini_napi_channel(channel);
>  			efx_remove_channel(channel);
> -			kfree(channel);
> +			efx_free_channel(channel);

[Severity: Medium]
After the resize, what arms filter_work on the replacement channel?

efx_copy_channel() copies rfs_filter_count, rfs_last_expiry,
rfs_expire_index and the rps_flow_id pointer from the old channel, so
the aRFS filters stay installed. It only re-initializes the work:

	*channel = *old_channel;
	...
#ifdef CONFIG_RFS_ACCEL
	INIT_DELAYED_WORK(&channel->filter_work, efx_filter_rfs_expire);
#endif

With this patch, the old channel's self-re-arming work is cancelled here.
The only other place that arms filter_work is efx_poll():

		if (channel->rfs_filter_count * time >= 600 * HZ)
			mod_delayed_work(system_percpu_wq, &channel->filter_work, 0);

The self-re-arm in efx_filter_rfs_expire() exists to cover the case
where no NAPI poll happens:

	/* Ensure we do more work eventually even if NAPI poll is not happening */
	schedule_delayed_work(dwork, 30 * HZ);

Take a channel that still has aRFS filters after ethtool -G but sees no
more events. Does expiry ever run on it again? Its stale filters would
seem to stay in the NIC filter table, and its rfs_filter_count would stay
non-zero indefinitely.

Could the new channel's filter_work be re-armed after the swap, for
example when the old work was pending or rfs_filter_count is non-zero?
efx_siena_realloc_channels() has the same gap. The commit message also
doesn't mention this change in behaviour.

[Severity: Medium]
This is a pre-existing issue, but the cancel happens quite late in
efx_realloc_channels(). Can the old channel's filter_work still run
between efx_copy_channel() and this point?

efx_stop_all() and efx_soft_disable_interrupts() don't cancel
filter_work. efx_copy_channel() then does *channel = *old_channel
without rps_mutex. After that, the old and new channels share one
rps_flow_id[] array but keep separate rfs_filter_count and
rfs_expire_index values.

The old work could fire after the copy but before efx_free_channel().
That window covers the swap and efx_probe_channel() for every copied
channel. If it does, __efx_filter_rfs_expire(old) removes filters from
the NIC and does:

				channel->rps_flow_id[index] = RPS_FLOW_ID_INVALID;
				channel->rfs_filter_count--;

The rps_flow_id[] write lands in the shared array. The decrement and the
rfs_expire_index update go to the old channel, which is freed right
after.

The live channel's rfs_filter_count then stays above the number of
filled rps_flow_id slots. That breaks the invariant in net_driver.h:

 * @rfs_filter_count: number of accelerated RFS filters currently in place;
 *	equals the count of @rps_flow_id slots filled

Only efx_probe_filters() resets the count, so the ethtool
rfs_filter_count stat and the expiry quota calculation would stay
inflated.

Would cancelling each old channel's filter_work right after
efx_stop_all(), before the clone loop, close this window? The same
ordering exists in efx_siena_realloc_channels().

>  		}
>  	}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023134214.322796-1-congei42%40163.com