Re: [PATCH net v4] net: e1000: fix warning in iounmap on probe failure

[netdev-bot+sashiko@kernel.org]

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

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

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] The commit message says something different from what the
  patch does.
- [Low] The Fixes: tag does not match itself.

--

net: e1000: fix warning in iounmap on probe failure

The error unwind in e1000_probe() unconditionally calls iounmap() on
hw->ce4100_gbe_mdio_base_virt. That pointer is NULL on non-CE4100
hardware, so the call triggers a WARN_ON_ONCE on x86. The patch makes
the MDIO unmap conditional.

Closes: https://syzkaller.appspot.com/bug?extid=ca1ef9e2e234b8d3599b

> Fix this by conditionally unmapping the CE4100 MDIO base only if the
> mac_type is e1000_ce4100, matching the exact logic used in e1000_remove().

[ ... ]

> Fixes: 13acde8fffc0af ("e1000: Fix the CE4100 bus type for the MDIO/PHY registers")

[Severity: Low]
The subject in this Fixes: tag doesn't match the commit. Commit
13acde8fffc0af has this subject:

    e1000: cleanup CE4100 MDIO registers access

That commit moved the global MDIO pointer into struct e1000_hw. It also
changed the error path to iounmap(hw->ce4100_gbe_mdio_base_virt), so the
hash looks like the right target. Could the subject in the tag be changed
to match? checkpatch.pl flags this kind of mismatch.

[ ... ]

> v3:
>   - Switched from pointer null-checks to checking hw->mac_type == e1000_ce4100

[ ... ]

> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index d7f5c6f16142..d7e279b739b8 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -1227,7 +1227,8 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	kfree(adapter->rx_ring);
>  err_dma:
>  err_sw_init:
> -	iounmap(hw->ce4100_gbe_mdio_base_virt);
> +	if (hw->ce4100_gbe_mdio_base_virt)
> +		iounmap(hw->ce4100_gbe_mdio_base_virt);

[Severity: Medium]
Does the commit message match this hunk? The message and the v3
changelog both say the unmap now depends on hw->mac_type ==
e1000_ce4100, "matching the exact logic used in e1000_remove()". The
code here checks the pointer for NULL instead. e1000_remove() does this:

    if (hw->mac_type == e1000_ce4100)
        iounmap(hw->ce4100_gbe_mdio_base_virt);

The NULL check actually looks like the more complete fix. In
e1000_probe(), e1000_init_hw_struct() sets hw->mac_type well before the
CE4100 MDIO ioremap runs. So on CE4100 hardware, these two gotos reach
the cleanup with mac_type == e1000_ce4100 and the pointer still NULL:

    err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
    if (err) {
        pr_err("No usable DMA config, aborting\n");
        goto err_dma;
    }
    ...
    err = e1000_sw_init(adapter);
    if (err)
        goto err_sw_init;

With a mac_type check, both paths would still call iounmap(NULL).

Could the commit message and changelog be updated to describe the NULL
check that is actually submitted? Since v3 was sent without the staged
changes, it would also help to confirm which version is intended. It's
also not clear which version the Reviewed-by tag from v2 covers.

>  	iounmap(hw->hw_addr);
>  err_ioremap:
>  	disable_dev = !test_and_set_bit(__E1000_DISABLED, &adapter->flags);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924095955.3471-1-nsvatoslav515%40gmail.com