Re: [PATCH net] cxgb4: clear the sge maps and bitmaps freed on adap_init0() failure

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 (2) · 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] The patch does not fully fix the scenario it targets:
  remove_one() after adap_init0() fails in eeh_reset_done() still…

Pre-existing issues:
- [High] When adap_init0(adapter, 1) fails from eeh_reset_done(), the
  adapter is left registered and usable while its SGE maps are gone.
- [Medium] adap_init0() allocates adap->sge.egr_map, ingr_map,
  starving_fl, txq_maperr and (under CONFIG_DEBUG_FS) blocked_fl…
- [Medium] The debugfs 'blocked_fl' handlers read adap->sge.blocked_fl
  and adap->sge.egr_sz without any lock.

--

cxgb4: clear the sge maps and bitmaps freed on adap_init0() failure

This patch sets sge.egr_map, sge.ingr_map, sge.starving_fl, sge.txq_maperr
and sge.blocked_fl to NULL after the bye: label of adap_init0() frees them.
The goal is to avoid a second free in free_some_resources() from
remove_one() after adap_init0() fails when called from eeh_reset_done().

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index 1ced6df6eac8c..d4fe6aa185399 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -5402,11 +5402,16 @@ static int adap_init0(struct adapter *adap, int vpd_skip)
>  bye:
>  	adap_free_hma_mem(adap);
>  	kfree(adap->sge.egr_map);
> +	adap->sge.egr_map = NULL;
>  	kfree(adap->sge.ingr_map);
> +	adap->sge.ingr_map = NULL;

[Severity: High]
Does this fully close the remove_one() case described in the commit
message?

eeh_reset_prepare() doesn't free the ULD rx queues. It calls cxgb_down(),
and t4_free_sge_resources() only frees the ethernet, ctrl, fw_evtq, intrq
and ptp queues. After the reset, the ULD queues still have rspq.desc set
and adap->uld[type].handle is still set.

If adap_init0(adapter, 1) then fails in eeh_reset_done(), ingr_map is now
NULL. remove_one() calls t4_uld_clean_up() before it reaches
free_some_resources():

remove_one()
  detach_ulds()
  t4_uld_clean_up()
    cxgb4_shutdown_uld_adapter()
      free_sge_queues_uld()
        t4_free_uld_rxqs()
          free_rspq_fl()

drivers/net/ethernet/chelsio/cxgb4/sge.c:free_rspq_fl() {
    ...
	adap->sge.ingr_map[rq->cntxt_id - adap->sge.ingr_start] = NULL;
    ...
}

Wouldn't this write through a NULL ingr_map and oops whenever a ULD such
as iw_cxgb4, chcr, cxgbit or cxgb4i has rx queues allocated? ULD rx queues
get allocated whenever a ULD registers, because setup_sge_queues_uld() is
not gated on CXGB4_FULL_INIT_DONE.

Before this patch the same store was a use-after-free write, followed later
by the double free. So it looks like only the failure mode changes.

Would it be better to keep the maps valid on the EEH re-init path, instead
of freeing and clearing the live adapter's maps in bye:?

>  	bitmap_free(adap->sge.starving_fl);
> +	adap->sge.starving_fl = NULL;
>  	bitmap_free(adap->sge.txq_maperr);
> +	adap->sge.txq_maperr = NULL;

[Severity: High]
This is a pre-existing issue, but when adap_init0() fails from
eeh_reset_done(), the adapter stays registered with these maps gone.
sge.egr_sz and sge.ingr_sz keep their old non-zero values.

eeh_reset_done() just returns:

	err = adap_init0(adapter, 1);
	if (err) {
		dev_err(adapter->pdev_dev,
			"Adapter init failed, err %d", err);
		return;
	}

The netdevs, the debugfs files and the ULD registration all stay
reachable. Several users index the maps using only the sizes.

cxgb_open() only checks CXGB4_FULL_INIT_DONE, and eeh_reset_prepare()
cleared that flag through cxgb_down():

	if (!(adapter->flags & CXGB4_FULL_INIT_DONE)) {
		err = cxgb_up(adapter);

So bringing the interface up again goes into
cxgb_up()->setup_sge_queues(). If that succeeds, t4_sge_alloc_rxq() stores
into the maps:

	adap->sge.ingr_map[iq->cntxt_id - adap->sge.ingr_start] = iq;

If it fails, the freeout: path calls t4_free_sge_resources(), which does:

	memset(adap->sge.egr_map, 0,
	       adap->sge.egr_sz * sizeof(*adap->sge.egr_map));

A later ULD registration runs cxgb4_uld_alloc_resources()->
setup_sge_queues_uld()->t4_sge_alloc_rxq() and writes the same maps.

The debugfs blocked_fl_write() in cxgb4_debugfs.c also does:

	bitmap_copy(adap->sge.blocked_fl, t, adap->sge.egr_sz);

Before this patch these were use-after-free writes, and with it they
become NULL pointer dereferences.

Should a failed re-init in eeh_reset_done() leave the adapter in a state
where these paths can still run?

>  #ifdef CONFIG_DEBUG_FS
>  	bitmap_free(adap->sge.blocked_fl);
> +	adap->sge.blocked_fl = NULL;

[Severity: Medium]
This is a pre-existing issue, but does anything serialize the debugfs
blocked_fl handlers against this free?

blocked_fl_read() and blocked_fl_write() in cxgb4_debugfs.c use
adap->sge.blocked_fl and adap->sge.egr_sz without a lock. At the same time,
eeh_reset_done()->adap_init0() can reassign egr_sz, reallocate blocked_fl,
or free it here:

CPU1: blocked_fl_write()
        uses adap->sge.blocked_fl (old pointer)
CPU0: eeh_reset_done()->adap_init0()
        bye: bitmap_free(adap->sge.blocked_fl)
CPU1:   bitmap_copy(adap->sge.blocked_fl, t, adap->sge.egr_sz)

Can a blocked_fl_write() that runs at the same time as a failing re-init
write into the bitmap that bye: has just freed?

>  #endif
>  	if (ret != -ETIMEDOUT && ret != -EIO)
>  		t4_fw_bye(adap, adap->mbox);

[Severity: Medium]
This isn't a bug introduced by this patch, but does adap_init0() leak the
probe-time egr_map, ingr_map, starving_fl, txq_maperr and blocked_fl
buffers on every EEH/FLR reset?

adap_init0() allocates them unconditionally, without checking whether the
pointers are already set:

	adap->sge.egr_map = kzalloc_objs(*adap->sge.egr_map, adap->sge.egr_sz);

eeh_reset_prepare() only frees the HMA memory:

	adap_free_hma_mem(adapter);

So when eeh_reset_done() calls adap_init0(adapter, 1), the pointers from
probe are overwritten.

If the re-init fails partway through, for example if the ingr_map
allocation fails after egr_map was reallocated, bye: frees only the new
buffers. The overwritten probe-time buffers are still leaked.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104103309.24360-2-fourier.thomas%40gmail.com