[PATCH net v2 0/3] net: fealnx: fix card-index overflow and PCI device teardown

[PATCH net v2 0/3] net: fealnx: fix card-index overflow and PCI device teardown

[Жамбакиев Радий Рикардинович]

From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>

Changes since v1:
  - Dropped the refactoring part from the patches.

Link: https://lore.kernel.org/all/20260930223720.GA93976@electric-eye.fr.zoreil.com/

Radiy Zhambakiev (3):
  net: fealnx: fix teardown order in remove
  net: fealnx: disable the PCI device on remove and probe failure
  net: fealnx: allocate the card index from an IDA

 drivers/net/ethernet/fealnx.c | 39 +++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 11 deletions(-)

-- 
2.53.0

[PATCH net v2 1/3] net: fealnx: fix teardown order in remove

[Жамбакиев Радий Рикардинович]

From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>

fealnx_remove_one() frees the DMA rings before unregistering the
netdev, while the interface may still be up, which leaves a
window where freed memory can be accessed.

Call unregister_netdev() first so dev_close() stops the Tx/Rx
engines, deletes the timers, and frees the IRQ before the rings are
freed.

Found by Linux Verification Center (linuxtesting.org)

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
---
 drivers/net/ethernet/fealnx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
index bdc38aac5850..68194c9ef332 100644
--- a/drivers/net/ethernet/fealnx.c
+++ b/drivers/net/ethernet/fealnx.c
@@ -682,11 +682,11 @@ static void fealnx_remove_one(struct pci_dev *pdev)
 	if (dev) {
 		struct netdev_private *np = netdev_priv(dev);
 
+		unregister_netdev(dev);
 		dma_free_coherent(&pdev->dev, TX_TOTAL_SIZE, np->tx_ring,
 				  np->tx_ring_dma);
 		dma_free_coherent(&pdev->dev, RX_TOTAL_SIZE, np->rx_ring,
 				  np->rx_ring_dma);
-		unregister_netdev(dev);
 		pci_iounmap(pdev, np->mem);
 		free_netdev(dev);
 		pci_release_regions(pdev);
-- 
2.53.0

[PATCH net v2 2/3] net: fealnx: disable the PCI device on remove and probe failure

[Жамбакиев Радий Рикардинович]

From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>

pci_enable_device() is called in probe, but the device is never
disabled on probe failure or remove. Device remains enabled and
bus mastering stays active.

Add pci_disable_device() to the probe error unwind and to
fealnx_remove_one(), after all other resources have been released.

Found by Linux Verification Center (linuxtesting.org)

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
---
 drivers/net/ethernet/fealnx.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
index 68194c9ef332..b5e96c7037f3 100644
--- a/drivers/net/ethernet/fealnx.c
+++ b/drivers/net/ethernet/fealnx.c
@@ -502,12 +502,13 @@ static int fealnx_init_one(struct pci_dev *pdev,
 	if (len < MIN_REGION_SIZE) {
 		dev_err(&pdev->dev,
 			   "region size %ld too small, aborting\n", len);
-		return -ENODEV;
+		err = -ENODEV;
+		goto err_out_disable;
 	}
 
-	i = pci_request_regions(pdev, boardname);
-	if (i)
-		return i;
+	err = pci_request_regions(pdev, boardname);
+	if (err)
+		goto err_out_disable;
 
 	irq = pdev->irq;
 
@@ -671,6 +672,8 @@ static int fealnx_init_one(struct pci_dev *pdev,
 	pci_iounmap(pdev, ioaddr);
 err_out_res:
 	pci_release_regions(pdev);
+err_out_disable:
+	pci_disable_device(pdev);
 	return err;
 }
 
@@ -690,6 +693,7 @@ static void fealnx_remove_one(struct pci_dev *pdev)
 		pci_iounmap(pdev, np->mem);
 		free_netdev(dev);
 		pci_release_regions(pdev);
+		pci_disable_device(pdev);
 	} else
 		printk(KERN_ERR "fealnx: remove for unknown device\n");
 }
-- 
2.53.0

[PATCH net v2 3/3] net: fealnx: allocate the card index from an IDA

[Жамбакиев Радий Рикардинович]

From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>

card_idx is a static counter that is incremented on every probe.
It can overflow and wrap to a negative value, which then indexes
options[] and full_duplex[] out of bounds. Large values also no
longer fit in the 12-byte boardname[] buffer.

Allocate the card index from an IDA and free it on probe failure and
remove. The IDA reuses ids on re-add, preserving the options[] and
full_duplex[] mapping by probe order.

Store the id in the driver-private data so fealnx_remove_one() can
free it, and size boardname to hold a full 32-bit id.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
---
Note on the IDA approach:

I really would like to go that way, as the proposed KISS way does
not really fix the main problem of infinitely incremented static
counter.

Constraining the probe to some arbitrary value also does not feel
right to me.

card_idx is incremented at the top of the probe, so even failing
probes will increment it. A failing device will eventually 
exhaust this counter at its retry rate.

 drivers/net/ethernet/fealnx.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
index b5e96c7037f3..627e570fd399 100644
--- a/drivers/net/ethernet/fealnx.c
+++ b/drivers/net/ethernet/fealnx.c
@@ -83,6 +83,7 @@ static int full_duplex[MAX_UNITS] = { -1, -1, -1, -1, -1, -1, -1, -1 };
 #include <linux/crc32.h>
 #include <linux/delay.h>
 #include <linux/bitops.h>
+#include <linux/idr.h>
 
 #include <asm/processor.h>	/* Processor type for cache alignment. */
 #include <asm/io.h>
@@ -143,6 +144,8 @@ struct chip_info {
 	int flags;
 };
 
+static DEFINE_IDA(fealnx_ida);
+
 static const struct chip_info skel_netdrv_tbl[] = {
 	{ "100/10M Ethernet PCI Adapter",	HAS_MII_XCVR },
 	{ "100/10M Ethernet PCI Adapter",	HAS_CHIP_XCVR },
@@ -411,6 +414,8 @@ struct netdev_private {
 	unsigned char phys[2];	/* MII device addresses. */
 	struct mii_if_info mii;
 	void __iomem *mem;
+
+	int card_idx;
 };
 
 
@@ -473,9 +478,8 @@ static int fealnx_init_one(struct pci_dev *pdev,
 			   const struct pci_device_id *ent)
 {
 	struct netdev_private *np;
-	int i, option, err, irq;
-	static int card_idx = -1;
-	char boardname[12];
+	int option, err, irq, i;
+	char boardname[18];
 	void __iomem *ioaddr;
 	unsigned long len;
 	unsigned int chip_id = ent->driver_data;
@@ -483,19 +487,24 @@ static int fealnx_init_one(struct pci_dev *pdev,
 	void *ring_space;
 	dma_addr_t ring_dma;
 	u8 addr[ETH_ALEN];
+	int card_idx;
 #ifdef USE_IO_OPS
 	int bar = 0;
 #else
 	int bar = 1;
 #endif
 
-	card_idx++;
+	card_idx = ida_alloc(&fealnx_ida, GFP_KERNEL);
+	if (card_idx < 0)
+		return card_idx;
+
 	sprintf(boardname, "fealnx%d", card_idx);
 
 	option = card_idx < MAX_UNITS ? options[card_idx] : 0;
 
-	i = pci_enable_device(pdev);
-	if (i) return i;
+	err = pci_enable_device(pdev);
+	if (err)
+		goto err_out_ida;
 	pci_set_master(pdev);
 
 	len = pci_resource_len(pdev, bar);
@@ -535,6 +544,7 @@ static int fealnx_init_one(struct pci_dev *pdev,
 
 	/* Make certain the descriptor lists are aligned. */
 	np = netdev_priv(dev);
+	np->card_idx = card_idx;
 	np->mem = ioaddr;
 	spin_lock_init(&np->lock);
 	np->pci_dev = pdev;
@@ -674,6 +684,8 @@ static int fealnx_init_one(struct pci_dev *pdev,
 	pci_release_regions(pdev);
 err_out_disable:
 	pci_disable_device(pdev);
+err_out_ida:
+	ida_free(&fealnx_ida, card_idx);
 	return err;
 }
 
@@ -691,6 +703,7 @@ static void fealnx_remove_one(struct pci_dev *pdev)
 		dma_free_coherent(&pdev->dev, RX_TOTAL_SIZE, np->rx_ring,
 				  np->rx_ring_dma);
 		pci_iounmap(pdev, np->mem);
+		ida_free(&fealnx_ida, np->card_idx);
 		free_netdev(dev);
 		pci_release_regions(pdev);
 		pci_disable_device(pdev);
-- 
2.53.0

Re: [PATCH net v2 0/3] net: fealnx: fix card-index overflow and PCI device teardown

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

RE: [PATCH net v2 1/3] net: fealnx: fix teardown order in remove

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>
> Sent: Friday, October 2, 2026 4:10 PM
> To: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Michael Grzeschik <mgr@kernel.org>; Jijie Shao
> <shaojijie@huawei.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Denis Benato
> <benato.denis96@gmail.com>; Uwe Kleine-König (The Capable Hub)
> <u.kleine-koenig@baylibre.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; lvc-project@linuxtesting.org;
> stable@vger.kernel.org
> Subject: [PATCH net v2 1/3] net: fealnx: fix teardown order in remove
> 
> From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
> 
> fealnx_remove_one() frees the DMA rings before unregistering the
> netdev, while the interface may still be up, which leaves a window
> where freed memory can be accessed.
> 
> Call unregister_netdev() first so dev_close() stops the Tx/Rx engines,
> deletes the timers, and frees the IRQ before the rings are freed.
> 
> Found by Linux Verification Center (linuxtesting.org)
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
> ---
>  drivers/net/ethernet/fealnx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/fealnx.c
> b/drivers/net/ethernet/fealnx.c index bdc38aac5850..68194c9ef332
> 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c
> @@ -682,11 +682,11 @@ static void fealnx_remove_one(struct pci_dev
> *pdev)
>  	if (dev) {
>  		struct netdev_private *np = netdev_priv(dev);
> 
> +		unregister_netdev(dev);
>  		dma_free_coherent(&pdev->dev, TX_TOTAL_SIZE, np-
> >tx_ring,
>  				  np->tx_ring_dma);
>  		dma_free_coherent(&pdev->dev, RX_TOTAL_SIZE, np-
> >rx_ring,
>  				  np->rx_ring_dma);
> -		unregister_netdev(dev);
>  		pci_iounmap(pdev, np->mem);
>  		free_netdev(dev);
>  		pci_release_regions(pdev);
> --
> 2.53.0

[PATCH net v2 1/3] net: fealnx: fix teardown order in remove

RE: [PATCH net v2 1/3] net: fealnx: fix teardown order in remove

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>
> Sent: Friday, October 2, 2026 4:10 PM
> To: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Michael Grzeschik <mgr@kernel.org>; Jijie Shao
> <shaojijie@huawei.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Denis Benato
> <benato.denis96@gmail.com>; Uwe Kleine-König (The Capable Hub)
> <u.kleine-koenig@baylibre.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; lvc-project@linuxtesting.org;
> stable@vger.kernel.org
> Subject: [PATCH net v2 1/3] net: fealnx: fix teardown order in remove
> 
> From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
> 
> fealnx_remove_one() frees the DMA rings before unregistering the
> netdev, while the interface may still be up, which leaves a window
> where freed memory can be accessed.
> 
> Call unregister_netdev() first so dev_close() stops the Tx/Rx engines,
> deletes the timers, and frees the IRQ before the rings are freed.
> 
> Found by Linux Verification Center (linuxtesting.org)
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
> ---
>  drivers/net/ethernet/fealnx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/fealnx.c
> b/drivers/net/ethernet/fealnx.c index bdc38aac5850..68194c9ef332
> 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c
> @@ -682,11 +682,11 @@ static void fealnx_remove_one(struct pci_dev
> *pdev)
>  	if (dev) {
>  		struct netdev_private *np = netdev_priv(dev);
> 
> +		unregister_netdev(dev);
>  		dma_free_coherent(&pdev->dev, TX_TOTAL_SIZE, np-
> >tx_ring,
>  				  np->tx_ring_dma);
>  		dma_free_coherent(&pdev->dev, RX_TOTAL_SIZE, np-
> >rx_ring,
>  				  np->rx_ring_dma);
> -		unregister_netdev(dev);
>  		pci_iounmap(pdev, np->mem);
>  		free_netdev(dev);
>  		pci_release_regions(pdev);
> --
> 2.53.0

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [PATCH net v2 0/3] net: fealnx: fix card-index overflow and PCI device teardown

[Жамбакиев Радий Рикардинович]

On Fri, 2026-10-02 at 14:13 +0000, netdev-bot+sinfo@kernel.org wrote:
> ВНИМАНИЕ! Письмо получено от внешнего отправителя. Не переходите по
> ссылкам и не открывайте вложения, пока не убедитесь, что они
> безопасны.
> 
> 
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.
> 
The change was not tested on real hardware.

Re: [PATCH net v2 3/3] net: fealnx: allocate the card index from an IDA

[Andrew Lunn]

On Fri, Oct 02, 2026 at 02:10:10PM +0000, Жамбакиев Радий Рикардинович wrote:
> From: Radiy Zhambakiev <r.zhambakiev@prosoftsystems.ru>
> 
> card_idx is a static counter that is incremented on every probe.
> It can overflow and wrap to a negative value, which then indexes
> options[] and full_duplex[] out of bounds. Large values also no
> longer fit in the 12-byte boardname[] buffer.
> 
> Allocate the card index from an IDA and free it on probe failure and
> remove. The IDA reuses ids on re-add, preserving the options[] and
> full_duplex[] mapping by probe order.
> 
> Store the id in the driver-private data so fealnx_remove_one() can
> free it, and size boardname to hold a full 32-bit id.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

How have you tested this?

The advantage of the KISS approach is it is stupid, so unlikely to be
wrong. The complexity here is much higher, so it is more likely to be
wrong. That is something we have to considered.

Have you put it into a loop, and probed it 10342432341 times, on real
hardware?

     Andrew

Re: [PATCH net v2 1/3] net: fealnx: fix teardown order in remove

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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 makes fealnx_remove_one() free both DMA rings, unmap
  np->mem and free the netdev right after unregister_netdev().
- [Medium] The patch's safety argument is that dev_close() stops the
  Tx/Rx engines before fealnx_remove_one() frees the descriptor rings.

--

net: fealnx: fix teardown order in remove

This commit moves unregister_netdev() ahead of the dma_free_coherent()
calls in fealnx_remove_one(). The goal is for dev_close() to stop the
Tx/Rx engines, delete the timers and free the IRQ before the DMA rings
are freed.

> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
> index bdc38aac58509..68194c9ef3326 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c
> @@ -682,11 +682,11 @@ static void fealnx_remove_one(struct pci_dev *pdev)
>  	if (dev) {
>  		struct netdev_private *np = netdev_priv(dev);
>  
> +		unregister_netdev(dev);

[Severity: High]

Does netdev_close() in drivers/net/ethernet/fealnx.c really guarantee
what the commit message relies on? It runs this sequence without taking
np->lock:

drivers/net/ethernet/fealnx.c:netdev_close() {
    ...
	iowrite32(0x0000, ioaddr + IMR);
    ...
	stop_nic_rxtx(ioaddr, 0);

	timer_delete_sync(&np->timer);
	timer_delete_sync(&np->reset_timer);

	free_irq(np->pci_dev->irq, dev);
    ...
}

intr_handler() is registered with IRQF_SHARED. It can already be running
on another CPU while holding np->lock. Its loop checks the software
np->imrvalue, so the IMR=0 write above does not stop it.

Suppose the handler reaches the too-much-work path after
timer_delete_sync(&np->reset_timer) has returned:

intr_handler() {
    ...
		if (--boguscnt < 0) {
			...
			if (!np->reset_timer_armed) {
				np->reset_timer_armed = 1;
				np->reset_timer.expires = RUN_AT(HZ/2);
				add_timer(&np->reset_timer);
    ...
}

free_irq() waits for the handler to finish, but it does not cancel the
timer the handler armed. Can reset_timer still be pending when
unregister_netdev() returns?

If it can, fealnx_remove_one() then frees the rings, unmaps np->mem and
calls free_netdev(). That frees the timer_list embedded in np while it
is still queued.

When reset_timer() later fires, it takes np->lock in freed memory and
calls enable_rxtx(). enable_rxtx() touches rx_ring through
reset_rx_descriptors(). It then writes the freed tx_ring_dma and
rx_ring_dma into TXLBA/RXLBA through the unmapped np->mem.

Three other paths write np->crvalue, with RXEN/TXEN still set, back to
TCRRCR after close's stop_nic_rxtx():

- the RBU branch in intr_handler()
- the rx error branch in netdev_rx()
- the media change branch in netdev_timer(), which can run before
  timer_delete_sync(&np->timer)

The handler also restores IMR on exit:

	iowrite32(np->imrvalue, ioaddr + IMR);

Could any of these turn the engines and interrupts back on before the
rings are freed? Remove does no BCR soft reset and does not clear bus
mastering before dma_free_coherent(), so a re-enabled chip could DMA
into the freed ring pages.

Later patches in the series leave netdev_close() unchanged. They add
pci_disable_device() only after the rings are freed.

Would netdev_close() need to clear the enable bits under np->lock, and
sync the IRQ before a final timer_shutdown_sync()? Would the chip also
need a reset before the rings are freed?

>  		dma_free_coherent(&pdev->dev, TX_TOTAL_SIZE, np->tx_ring,
>  				  np->tx_ring_dma);
>  		dma_free_coherent(&pdev->dev, RX_TOTAL_SIZE, np->rx_ring,
>  				  np->rx_ring_dma);

[Severity: Medium]

Are the engines guaranteed to have stopped before these rings are freed?
The only hardware quiesce step in netdev_close() is stop_nic_rxtx(), and
it does not report failure:

drivers/net/ethernet/fealnx.c:stop_nic_rxtx() {
	int delay = 0x1000;
	iowrite32(crvalue & ~(CR_W_RXEN+CR_W_TXEN), ioaddr + TCRRCR);
	while (--delay) {
		if ( (ioread32(ioaddr + TCRRCR) & (CR_R_RXSTOP+CR_R_TXSTOP))
					    == (CR_R_RXSTOP+CR_R_TXSTOP) )
			break;
	}
}

The poll has no delay between reads. To netdev_close(), running out of
the budget looks the same as a successful stop.

A Tx frame might still be in collision backoff on a 10 Mb/s half-duplex
link, or the chip might be wedged, for example after tx_timeout. In
either case, could the chip still write descriptor status into tx_ring
or rx_ring after dma_free_coherent()?

The driver already has reset_and_disable_rxtx(), which does the stop,
IMR=0 and a BCR soft reset. Neither close nor remove uses it, and bus
mastering is not cleared before the rings are freed.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002140954.261779-1-r.zhambakiev%40prosoftsystems.ru

Re: [PATCH net v2 3/3] net: fealnx: allocate the card index from an IDA

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 (1) · 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).

- [Medium] The commit message says the IDA "reuses ids on re-add,
  preserving the options[] and full_duplex[] mapping by probe order".

Pre-existing issues:
- [High] fealnx_init_one() (drivers/net/ethernet/fealnx.c:482-518) builds
  the PCI resource name in the stack array `char boardname[18]` and…
- [Medium] Other legacy Ethernet drivers have the same static per-probe
  card counter used to index fixed-size module-parameter arrays.

--

net: fealnx: allocate the card index from an IDA

fealnx_init_one() used a static card_idx counter that went up on every
probe. This patch replaces it with an id taken from an IDA, which is
freed when the probe fails and in fealnx_remove_one(). The id is kept in
netdev_private, and boardname[] grows to 18 bytes so it can hold a full
32-bit id.

> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
> index b5e96c7037f37..627e570fd399d 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c

[ ... ]

> @@ -473,9 +478,8 @@ static int fealnx_init_one(struct pci_dev *pdev,
>  			   const struct pci_device_id *ent)
>  {
>  	struct netdev_private *np;
> -	int i, option, err, irq;
> -	static int card_idx = -1;

[Severity: Medium]
This is a pre-existing issue in other drivers and was not introduced by
this patch, but the same static counter pattern shows up elsewhere.

amd8111e_probe_one() in drivers/net/ethernet/amd/amd8111e.c indexes
module parameter arrays with MAX_UNITS (8) entries and does no bounds
check at all:

	lp->ext_phy_option = speed_duplex[card_idx];
	if (coalesce[card_idx])
		lp->options |= OPTION_INTR_COAL_ENABLE;
	if (dynamic_ipg[card_idx++])

Here card_idx is declared as "static int card_idx;" in amd8111e.h. Does
the 9th probe read past the end of speed_duplex[], coalesce[] and
dynamic_ipg[]? Repeated sysfs unbind/bind would be enough to get there.

rio_probe1() in drivers/net/ethernet/dlink/dl2k.c and sundance_probe1()
in drivers/net/ethernet/dlink/sundance.c only check
"card_idx < MAX_UNITS" on a signed static counter. That is the same
wraparound case this patch fixes for fealnx. Should these get a
follow-up?

> -	char boardname[12];
> +	int option, err, irq, i;
> +	char boardname[18];

[Severity: High]
This isn't a bug introduced by this patch, but since boardname[] is being
resized here: is it safe to keep it on the stack at all?

Later in fealnx_init_one() the buffer is passed to the resource core:

	err = pci_request_regions(pdev, boardname);

__request_region_locked() in kernel/resource.c stores the pointer and
does not copy the string:

	res->name = name;

The regions are only released in fealnx_remove_one(). So once
fealnx_init_one() returns, the names of the BAR resources point into a
dead stack frame for as long as the device stays bound.

r_show() prints r->name with %s for /proc/iomem and /proc/ioports. Both
files are world-readable, and for readers without CAP_SYS_ADMIN only the
addresses are masked, not the name.

Can an unprivileged reader see stale kernel stack contents here? And
could the read fault if the probing task has exited and its vmapped
stack has been freed?

Passing DRV_NAME or pci_name(pdev), or keeping the name in
netdev_private, would avoid this. It would also make the 12 to 18 byte
resize unnecessary.

>  	void __iomem *ioaddr;
>  	unsigned long len;
>  	unsigned int chip_id = ent->driver_data;
> @@ -483,19 +487,24 @@ static int fealnx_init_one(struct pci_dev *pdev,

[ ... ]

> -	card_idx++;
> +	card_idx = ida_alloc(&fealnx_ida, GFP_KERNEL);
> +	if (card_idx < 0)
> +		return card_idx;
> +
>  	sprintf(boardname, "fealnx%d", card_idx);
>  
>  	option = card_idx < MAX_UNITS ? options[card_idx] : 0;

[Severity: Medium]
The commit message says:

  The IDA reuses ids on re-add, preserving the options[] and
  full_duplex[] mapping by probe order.

Does this still hold when an earlier probe fails?

With the old static counter, every probe attempt used up an index. The
Nth fealnx device probed always got options[N-1] and full_duplex[N-1],
even if an earlier probe had failed.

With this patch, the error path gives the id back:

err_out_ida:
	ida_free(&fealnx_ida, card_idx);

The next ida_alloc() then returns the lowest free id. Say card 0 fails
in pci_enable_device(), or at any later error label, and card 1 probes
next. Card 1 then gets index 0 and applies options[0] and
full_duplex[0], which were meant for card 0. That can force the wrong
media type or duplex setting on card 1's link.

The same happens on unbind/bind. The mapping follows the order in which
devices are re-probed, not the device itself. If both cards are unbound
and the second one is rebound first, it gets id 0.

Could the commit message describe this change in behaviour? Or should a
failed probe keep its slot, as it did before?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002140954.261779-1-r.zhambakiev%40prosoftsystems.ru