Re: [PATCH net v2] 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] Half of the patch is provably dead code and the commit message
  over-claims the defect.
- [Low] The commit carries `Reported-by:
  syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com` and asserts…

--

net: e1000: fix warning in iounmap on probe failure

This patch adds NULL checks before the two iounmap() calls in the
err_sw_init/err_dma error unwind path of e1000_probe(), so that a probe
failure does not pass a NULL pointer to iounmap().  It carries two Fixes:
tags and a syzbot Reported-by:.

No Link: tags were present in the commit header.

Two questions below, one on the commit message tags and one on the diff.

> net: e1000: fix warning in iounmap on probe failure
>
> During a probe failure, the e1000 driver error handling path calls
> iounmap() on hw->ce4100_gbe_mdio_base_virt and hw->hw_addr without
> checking if they were previously mapped. This triggers a kernel warning
> (WARN) rather than a fatal crash when passed an uninitialized pointer.
>
> This issue was found by Syzbot. The exact kernel configuration (.config)
> and the C reproducer used to trigger this warning are available on the
> Syzbot dashboard.
>
> Fix this by adding NULL checks before calling iounmap().
>
> Reported-by: syzbot+ca1ef9e2e234b8d3599b@syzkaller.appspotmail.com
> Fixes: 13acde8fffc0a ("e1000: cleanup CE4100 MDIO registers access")

[Severity: Low]
The Reported-by: line is followed directly by a Fixes: line, with no
Closes: or Link: URL pointing at the syzbot report.  Could that URL be
added?

Documentation/process/submitting-patches.rst asks for it:

    The tag should be followed by a Closes: tag pointing to the report,
    unless the report is not available on the web

and scripts/checkpatch.pl warns on this pattern:

    BAD_REPORTED_BY_LINK
    "Reported-by: should be immediately followed by Closes: or Link:
     with a URL to the report"

The commit message says the .config and the C reproducer "are available on
the Syzbot dashboard", but without the link there is no way to check which
error path syzbot actually reached, which is relevant to the question
below.

Also, the description of the value handed to iounmap() as an
"uninitialized pointer" does not look accurate: the adapter private area
comes from alloc_etherdev()/alloc_netdev_mqs(), which zero the allocation,
so hw->ce4100_gbe_mdio_base_virt is deterministically NULL rather than
garbage.  Could that wording be adjusted?

> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index d7f5c6f161420..156903089fa6b 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);
> +	if (hw->hw_addr)
> +		iounmap(hw->hw_addr);
>  err_ioremap:

[Severity: Medium]
Can the new "if (hw->hw_addr)" test ever be false?

hw->hw_addr is assigned in exactly one place in the driver, in
e1000_probe():

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

The err_ioremap: label sits below the iounmap(hw->hw_addr) that is being
guarded here, so the only path that leaves hw_addr NULL skips this code
entirely.  Every other goto that lands on this label (err_sw_init,
err_dma, err_mdio_ioremap, err_eeprom, err_register) is reachable only
after pci_ioremap_bar() already returned a valid mapping.

If that reading is right, the hw_addr half of the change is dead code, and
the second tag

	Fixes: 1dc329180fe22 ("e1000: Use hw, er32, and ew32")

refers to a defect that is not present.  Should that tag be dropped, along
with the mention of hw->hw_addr in the commit message, so a stable
backport is not aimed at a path that cannot trigger?

The ce4100_gbe_mdio_base_virt half does look like a real fix, since that
mapping is only created for one MAC type:

	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));

so on every other device the field stays NULL while the unwind path still
calls iounmap() on it, and x86 iounmap() does:

	if (WARN_ON_ONCE(!is_ioremap_addr((void __force *)addr)))
		return;

Would it be clearer to describe only that case in the changelog?

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