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

[PATCH net 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. While at it use dev_err() instead of printk() for the 
unknown-device case.

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 | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
index bdc38aac5850..d7cd1644a375 100644
--- a/drivers/net/ethernet/fealnx.c
+++ b/drivers/net/ethernet/fealnx.c
@@ -678,20 +678,23 @@ static int fealnx_init_one(struct pci_dev *pdev,
 static void fealnx_remove_one(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
+	struct netdev_private *np;
+
+	if (!dev) {
+		dev_err(&pdev->dev, "remove for unknown device\n");
+		return;
+	}
+
+	np = netdev_priv(dev);
 
-	if (dev) {
-		struct netdev_private *np = netdev_priv(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);
-	} else
-		printk(KERN_ERR "fealnx: remove for unknown device\n");
+	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);
+	pci_iounmap(pdev, np->mem);
+	pci_release_regions(pdev);
+	free_netdev(dev);
 }
 
 
-- 
2.53.0

[PATCH net 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 | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
index d7cd1644a375..46a40d00b59a 100644
--- a/drivers/net/ethernet/fealnx.c
+++ b/drivers/net/ethernet/fealnx.c
@@ -494,20 +494,22 @@ static int fealnx_init_one(struct pci_dev *pdev,
 
 	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)
+		return err;
 	pci_set_master(pdev);
 
 	len = pci_resource_len(pdev, bar);
 	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 +673,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;
 }
 
@@ -694,6 +698,7 @@ static void fealnx_remove_one(struct pci_dev *pdev)
 			  np->rx_ring_dma);
 	pci_iounmap(pdev, np->mem);
 	pci_release_regions(pdev);
+	pci_disable_device(pdev);
 	free_netdev(dev);
 }
 
-- 
2.53.0

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

[Francois Romieu]

Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru> :
> 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 | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
> index d7cd1644a375..46a40d00b59a 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c
> @@ -494,20 +494,22 @@ static int fealnx_init_one(struct pci_dev *pdev,
>  
>  	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)
> +		return err;

Unrelated change.

-- 
Ueimor

[PATCH net 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>
---
 drivers/net/ethernet/fealnx.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
index 46a40d00b59a..c474dd5ac4c6 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,20 +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;
 
 	err = pci_enable_device(pdev);
 	if (err)
-		return err;
+		goto err_out_ida;
 	pci_set_master(pdev);
 
 	len = pci_resource_len(pdev, bar);
@@ -536,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;
@@ -675,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;
 }
 
@@ -699,6 +710,7 @@ static void fealnx_remove_one(struct pci_dev *pdev)
 	pci_iounmap(pdev, np->mem);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
+	ida_free(&fealnx_ida, np->card_idx);
 	free_netdev(dev);
 }
 
-- 
2.53.0

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

[Andrew Lunn]

On Thu, Sep 24, 2026 at 10:44:25AM +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.

Seems very theoretical. For something which is never going to happen,
please keep it KISS:

	if (card_idx > 100000) {
      		dev_error(..., "Please stop doing that!");
		return -ENODEV;
	 }

    Andrew

---
pw-bot: cr

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

[Andrew Lunn]

On Thu, Sep 24, 2026 at 10:44:21AM +0000, Жамбакиев Радий Рикардинович wrote:
> 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. While at it use dev_err() instead of printk() for the 
> unknown-device case.
> 
> 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 | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
> index bdc38aac5850..d7cd1644a375 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c
> @@ -678,20 +678,23 @@ static int fealnx_init_one(struct pci_dev *pdev,
>  static void fealnx_remove_one(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);
> +	struct netdev_private *np;
> +
> +	if (!dev) {
> +		dev_err(&pdev->dev, "remove for unknown device\n");
> +		return;
> +	}

I know you are just moving code around, but is that possible?  We try
avoid defensive code. It is better to actually understand the code and
stop bad things happening.

     Andrew

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

[Francois Romieu]

Жамбакиев Радий Рикардинович <r.zhambakiev@prosoftsystems.ru> :
> 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.

Fine. A single line of code needs to be moved.

> While at it use dev_err() instead of printk() for the unknown-device case.

Unrelated stuff.

-- 
Ueimor

Re: [PATCH net 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: 2 · Medium: 0 · Low: 0 (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).

- [High] The commit says unregister_netdev() -> dev_close() ->
  netdev_close() "deletes the timers ... before the rings are freed".
- [High] The fix assumes that once unregister_netdev() returns,
  netdev_close() has stopped the Rx/Tx DMA engines, so…

Pre-existing issues:
- [Low] At this commit, fealnx_init_one() calls pci_enable_device() and
  pci_set_master() (lines 497/499).

--

net: fealnx: fix teardown order in remove

This patch reorders fealnx_remove_one() so unregister_netdev() runs before
the Tx/Rx descriptor rings are freed. The intent is for netdev_close() to
stop the hardware, delete the timers and free the IRQ first. It also
changes the unknown-device printk() to dev_err().

> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
> index bdc38aac5850..d7cd1644a375 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c
> @@ -678,20 +678,23 @@ static int fealnx_init_one(struct pci_dev *pdev,
>  static void fealnx_remove_one(struct pci_dev *pdev)
>  {
>  	struct net_device *dev = pci_get_drvdata(pdev);

[ ... ]

> -	if (dev) {
> -		struct netdev_private *np = netdev_priv(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);
> -	} else
> -		printk(KERN_ERR "fealnx: remove for unknown device\n");
> +	unregister_netdev(dev);

[Severity: High]
The commit message says:

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

Can reset_timer still be pending when free_netdev() runs below?

netdev_close() cancels the timers before it frees the IRQ, and it never
takes 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 and may still be running on
another CPU. When it uses up max_interrupt_work, it arms the timer:

drivers/net/ethernet/fealnx.c: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);
    ...
}

The handler can also stay busy during close. Its RBU path and netdev_rx()
write np->crvalue, which still has RXEN/TXEN set, back into TCRRCR.

One possible sequence:

CPU A                                  CPU B
netdev_close()                         intr_handler()
  timer_delete_sync(&np->reset_timer)
    (nothing pending yet)
                                         add_timer(&np->reset_timer)
  free_irq()
    (waits for CPU B to leave handler)
fealnx_remove_one()
  dma_free_coherent(tx_ring, rx_ring)
  pci_iounmap()
  free_netdev()

free_netdev() frees netdev_private, and the pending reset_timer is embedded
in it. About HZ/2 later, reset_timer() runs on the freed np. It calls
reset_and_disable_rxtx() and then enable_rxtx(). Those write the freed
np->tx_ring_dma and np->rx_ring_dma into TXLBA and RXLBA through the stale
np->mem mapping, and they turn Rx/Tx and IMR back on.

Would netdev_close() need to quiesce the IRQ first, with free_irq() or
synchronize_irq(), and then call timer_shutdown_sync() on both timers?
Alternatively, intr_handler() could be stopped from re-arming the timer
under np->lock during close.

> +	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: High]
Are the Tx/Rx DMA engines guaranteed to be stopped when these rings are
freed?

netdev_close() calls stop_nic_rxtx(ioaddr, 0) and sets IMR to 0 before it
quiesces the contexts that can turn the engines back on. It also leaves
np->crvalue and np->imrvalue unchanged, so RXEN/TXEN and the interrupt mask
are still set there. Each of these paths can undo the stop.

netdev_timer(), on link-up with a crvalue change:

			if (np->crvalue != old_crvalue) {
				stop_nic_rxtx(ioaddr, np->crvalue);
				iowrite32(np->crvalue, ioaddr + TCRRCR);
			}

reset_timer()->enable_rxtx(), which reprograms TXLBA/RXLBA, BCR and TCRRCR
(through __set_rx_mode()) and then does:

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

intr_handler(), on RBU (and similarly from netdev_rx()):

				iowrite32(np->crvalue, ioaddr + TCRRCR);

and on every exit from the handler:

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

timer_delete_sync() and free_irq() wait for these paths to finish, but
they do not undo the register writes. After that, neither netdev_close()
nor fealnx_remove_one() resets the chip (BCR bit 0) or stops the engines
again.

That means the rings can be freed here while the device is still reading
and writing descriptors in them. The Rx buffers were already unmapped and
freed in netdev_close(). Even with the later patch in this series,
pci_disable_device(), which clears bus mastering, still runs only after
dma_free_coherent().

Should the chip be stopped or reset after the IRQ and both timers are
quiesced, and before the rings are freed?

> +	pci_iounmap(pdev, np->mem);
> +	pci_release_regions(pdev);

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The
rewritten remove sequence still has no pci_disable_device().

fealnx_init_one() calls pci_enable_device() and pci_set_master(), and
nothing undoes them. The early error returns skip it:

drivers/net/ethernet/fealnx.c:fealnx_init_one() {
    ...
	i = pci_enable_device(pdev);
	if (i) return i;
	pci_set_master(pdev);

	len = pci_resource_len(pdev, bar);
	if (len < MIN_REGION_SIZE) {
		...
		return -ENODEV;
	}

	i = pci_request_regions(pdev, boardname);
	if (i)
		return i;
    ...
}

The goto unwind also ends at err_out_res -> pci_release_regions() without
disabling the device. So enable_cnt is left unbalanced, and bus mastering
stays on after unbind or a failed probe.

The later patch in this series, "net: fealnx: disable the PCI device on
remove and probe failure", already fixes this. It adds an err_out_disable
label to the probe unwind and calls pci_disable_device() in
fealnx_remove_one().

> +	free_netdev(dev);
>  }

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