Re: [PATCH net v3 1/2] sfc: fix use-after-free of filter_work in efx_realloc_channels()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 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).

- [High] Applied alone, the patch "sfc: fix use-after-free of filter_work
  in efx_realloc_channels()" only cancels channel->filter_work in the…
- [Medium] Applied alone, the patch "sfc: fix use-after-free of
  filter_work in efx_realloc_channels()" leaves the old channels' aRFS…

--

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

This adds efx_free_channel(), which cancels the per-channel aRFS expiry
work synchronously before freeing a channel. It is used in
efx_realloc_channels() and efx_fini_channels(), and the expiry work is
re-armed at the common exit of efx_realloc_channels() for channels that
still have filters. The same changes are applied to Siena.

Link: https://lore.kernel.org/netdev/179310908138.3134.11515982039281965575@kernel.org/

> diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> index f4dc3f3f4416c..44943345fa87a 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]
Is waiting for filter_work enough before this kfree()? The per-NIC aRFS
insertion works in efx->rps_slot[] can also hold a pointer to this
channel. efx_filter_rfs() queues them with schedule_work().

efx_filter_rfs_work() in rx_common.c looks up the channel when it
starts. It then sleeps in the MCDI insert before it touches the channel:

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);
    ...
    if (rc >= 0) {
        mutex_lock(&efx->rps_mutex);
        if (channel->rps_flow_id[rc] == RPS_FLOW_ID_INVALID)
            channel->rfs_filter_count++;
        ...
        channel->n_rfs_succeeded++;
    } else {
        ...
        channel->n_rfs_failed++;
        ...
        __efx_filter_rfs_expire(channel, min(channel->rfs_filter_count,
                                             100u));
    }
    ...
}

None of efx_device_detach_sync(), efx_stop_all() or
efx_soft_disable_interrupts() flushes these works.

Can efx_realloc_channels() call efx_free_channel() on an old channel
while one of these works is still blocked in filter_insert()? If so, the
work writes into the freed channel when it resumes.

The failure path also runs an expiry pass on the freed channel, which is
the problem this patch is meant to close. Siena has the same pattern, in
its efx_filter_rfs_work() and efx_siena_realloc_channels().

The next patch in this series, "sfc: quiesce aRFS work before copying
channels in efx_realloc_channels()", appears to fix this:

  - It adds a netif_device_present() check to efx_filter_rfs() and
    efx_siena_filter_rfs().
  - It adds efx_filter_rfs_flush(), which calls synchronize_net() and
    then flush_work() on each rps_slot[].work. This runs before the
    channels are copied.

Could that be folded into this patch so it is correct on its own?

[ ... ]

> @@ -898,10 +906,21 @@ 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);
>  		}
>  	}
>  
> +#ifdef CONFIG_RFS_ACCEL
> +	/* Re-arm the expiry work on the channels now in use: the copies on
> +	 * success, the originals after a rollback.  The copies inherit the
> +	 * aRFS filters, so they must inherit the expiry too, or a channel
> +	 * that gets no further NAPI polls would never expire its filters.
> +	 */
> +	efx_for_each_channel(channel, efx)
> +		if (channel->rfs_filter_count)
> +			schedule_delayed_work(&channel->filter_work, 30 * HZ);
> +#endif

[Severity: Medium]
Can the rfs_filter_count checked here be stale on the copies?

The copies come from efx_copy_channel(). It does a plain struct copy
without taking rps_mutex:

drivers/net/ethernet/sfc/efx_channels.c:efx_copy_channel() {
    ...
    *channel = *old_channel;
    ...
}

This copies rfs_filter_count, rfs_expire_index and rfs_last_expiry. The
copy also shares the rps_flow_id pointer with the old channel.

With this patch, the old channel's filter_work is only cancelled in
efx_free_channel(), in the out: loop above. By then the clone, the swap
and efx_probe_channel() on every copy have already run.

Until that cancel, __efx_filter_rfs_expire() and efx_filter_rfs_work()
can still update old->rfs_filter_count and the shared rps_flow_id[]
under rps_mutex:

drivers/net/ethernet/sfc/rx_common.c:__efx_filter_rfs_expire() {
    ...
    channel->rps_flow_id[index] = RPS_FLOW_ID_INVALID;
    channel->rfs_filter_count--;
    ...
}

So the copy's count could be too high after an expiry on the old
channel. It could also be too low after an insertion, and later wrap.

Suppose a copy's count ends up at 0 while rps_flow_id[] still holds
valid entries. Would this loop skip that channel, so its filters never
expire? That seems to be the case this block was added for.
efx_siena_realloc_channels() has the same ordering.

The next patch in this series, "sfc: quiesce aRFS work before copying
channels in efx_realloc_channels()", seems to fix this too. Before the
clone loop, it calls efx_filter_rfs_flush() and then
cancel_delayed_work_sync() on each channel's filter_work. It does this
in both efx_realloc_channels() and efx_siena_realloc_channels().

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