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: [email protected]` 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: [email protected] > 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
