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

[netdev-bot+sashiko@kernel.org]

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

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

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 and v3 changelog describe a different fix
  from the diff that was submitted.
- [Low] The new `if (hw->hw_addr)` check before `iounmap(hw->hw_addr)` in
  e1000_probe() (drivers/net/ethernet/intel/e1000/e1000_main.c:1232) is…
- [Low] The subject quoted in the Fixes tag is wrong.

--

net: e1000: fix warning in iounmap on probe failure

This patch changes the iounmap() calls in the e1000_probe() error unwind
so that a NULL hw->ce4100_gbe_mdio_base_virt is never passed to
iounmap(). On non-CE4100 hardware that pointer stays NULL, and on x86 a
NULL argument to iounmap() triggers a WARN_ON_ONCE.

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

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

[Severity: Low]
The subject quoted here doesn't match the commit it points to. The title
of 13acde8fffc0af is:

  e1000: cleanup CE4100 MDIO registers access

The SHA itself looks right. That commit added the unconditional
iounmap(hw->ce4100_gbe_mdio_base_virt) to the probe error path.

Could the tag use the real subject and the usual 12 character abbreviated
SHA?

> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index d7f5c6f16142..156903089fa6 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -1227,8 +1227,10 @@ 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);
> -	iounmap(hw->hw_addr);
> +	if (hw->ce4100_gbe_mdio_base_virt)
> +		iounmap(hw->ce4100_gbe_mdio_base_virt);

[Severity: Medium]
Is the commit message describing this version of the code? The message
says:

    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().
    The hw->hw_addr iounmap() remains unconditional since it is guaranteed to
    be valid for all error paths reaching the err_sw_init label.

The v3 changelog says:

  - Switched from pointer null-checks to checking hw->mac_type == e1000_ce4100
  - Removed redundant check for hw->hw_addr since it cannot be NULL here

The diff has no mac_type check, and iounmap(hw->hw_addr) is still behind
a NULL check. That looks like the v2 approach.

Was the wrong diff sent with the v3 message, or is the message out of
date? The Reviewed-by may also have been given for a different version.

The pointer check in the diff looks like the better choice. A mac_type
check would still reach iounmap(NULL) when the CE4100 ioremap fails in
e1000_probe():

	if (hw->mac_type == e1000_ce4100) {
		hw->ce4100_gbe_mdio_base_virt =
					ioremap(pci_resource_start(pdev, BAR_1),
						pci_resource_len(pdev, BAR_1));

		if (!hw->ce4100_gbe_mdio_base_virt)
			goto err_mdio_ioremap;
	}

err_mdio_ioremap frees the tx and rx rings, then falls through to
err_sw_init. On that path mac_type is e1000_ce4100 but the MDIO pointer
is NULL. A mac_type check would therefore still hit
WARN_ON_ONCE(!is_ioremap_addr(addr)) in the x86 iounmap().

Could the commit message and changelog be updated to describe the NULL
pointer check, instead of changing the code to match the message?

> +	if (hw->hw_addr)
> +		iounmap(hw->hw_addr);

[Severity: Low]
Can hw->hw_addr ever be NULL here? It is mapped earlier in e1000_probe():

	hw->hw_addr = pci_ioremap_bar(pdev, BAR_0);
	if (!hw->hw_addr)
		goto err_ioremap;

The err_ioremap label comes after this block. Every other path into
err_sw_init runs after the mapping has succeeded, so the check is always
true.

The commit message says the hw_addr iounmap() "remains unconditional",
and the v3 changelog says this check was removed. Should it be dropped?

>  err_ioremap:
>  	disable_dev = !test_and_set_bit(__E1000_DISABLED, &adapter->flags);
>  	free_netdev(netdev);

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