[PATCH] arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

[PATCH] arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

[Len Bao]

The 'cards' and 'numcards' variables are initialized only during the
init phase in the 'com90xx_found' function and never changed. So, mark
these as __ro_after_init.

Signed-off-by: Len Bao <len.bao@gmx.us>
---
 drivers/net/arcnet/com90xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/arcnet/com90xx.c b/drivers/net/arcnet/com90xx.c
index b3b287c16..bdcdccbfb 100644
--- a/drivers/net/arcnet/com90xx.c
+++ b/drivers/net/arcnet/com90xx.c
@@ -67,8 +67,8 @@ static void com90xx_copy_from_card(struct net_device *dev, int bufnum,
 
 /* Known ARCnet cards */
 
-static struct net_device *cards[16];
-static int numcards;
+static struct net_device *cards[16] __ro_after_init;
+static int numcards __ro_after_init;
 
 /* Handy defines for ARCnet specific stuff */
 
-- 
2.43.0

Re: [PATCH] arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says: This is a driver that doesn't seem to have seen any
            real attention for some time. I do wonder if we should
	    leave it be.

	    But as we are here, I'm passing on the AI generated review
	    with some commentary.

---
arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

This commit marks the cards and numcards variables as __ro_after_init since
they are only initialized during the init phase and are never changed
afterward.

> diff --git a/drivers/net/arcnet/com90xx.c b/drivers/net/arcnet/com90xx.c
> index b3b287c165617..bdcdccbfb0017 100644
> --- a/drivers/net/arcnet/com90xx.c
> +++ b/drivers/net/arcnet/com90xx.c
> @@ -67,8 +67,8 @@ static void com90xx_copy_from_card(struct net_device *dev, int bufnum,
>  
>  /* Known ARCnet cards */
>  
> -static struct net_device *cards[16];
> -static int numcards;
> +static struct net_device *cards[16] __ro_after_init;
> +static int numcards __ro_after_init;

Simon says: I believe this is an existing bug rather than a regression.
            But I am concerned that this patch may make it worse.
	    So I'm wondering if it should be fixed at the same time
	    by a series containing:
	    1. a patch to fix this overflow
	    2. this patch (to set __ro_after_init)

Could this change exacerbate an existing regression where we overflow the
cards array?

com90xx_probe() can test up to 32 default I/O ports. For each valid device
found, com90xx_found() unconditionally executes:

drivers/net/arcnet/com90xx.c:com90xx_found() {
    ...
	if (register_netdev(dev))
		goto err_unmap;

	cards[numcards++] = dev;
	return 0;
    ...
}

If more than 16 devices are successfully probed, this writes past the cards
array. Since this patch marks these variables as __ro_after_init, they are
now placed adjacently in memory.

Is it possible that the overflow will now clobber numcards with a pointer
value, leading to an out-of-bounds index on the next iteration?

Simon says: The issue flagged below also appears to be an existing bug
            rather than a regression. And I don't believe it impacts
	    this patch or needs to block progress of it.

Additionally, while reviewing this code, I noticed a second potential
regression in com90xx_found(). Are we registering the IRQ handler before
initializing the function pointers?

drivers/net/arcnet/com90xx.c:com90xx_found() {
    ...
	/* reserve the irq */
	if (request_irq(airq, arcnet_interrupt, 0, "arcnet (90xx)", dev)) {
		arc_printk(D_NORMAL, dev, "Can't get IRQ %d!\n", airq);
		goto err_release_mem;
	}
	dev->irq = airq;

	/* Initialize the rest of the device structure. */
	lp->card_name = "COM90xx";
	lp->hw.command = com90xx_command;
	lp->hw.status = com90xx_status;
    ...
}

If an interrupt triggers immediately after request_irq() completes (e.g., due
to a spurious interrupt), arcnet_interrupt() will execute, evaluate
!netif_running(dev) as true, and invoke lp->hw.status(dev).

Because alloc_arcdev() zero-allocates the private data, lp->hw.status is NULL
at this point. Can this lead to a NULL pointer dereference?

Re: [PATCH] arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

[Len Bao]

On Wed, May 20, 2026 at 09:16:46PM +0100, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> 
> Simon says: This is a driver that doesn't seem to have seen any
>             real attention for some time. I do wonder if we should
> 	    leave it be.
> 
> 	    But as we are here, I'm passing on the AI generated review
> 	    with some commentary.
> 
> ---
> arcnet: com90xx: Mark cards and numcards variables as __ro_after_init
> 
> This commit marks the cards and numcards variables as __ro_after_init since
> they are only initialized during the init phase and are never changed
> afterward.
> 
> > diff --git a/drivers/net/arcnet/com90xx.c b/drivers/net/arcnet/com90xx.c
> > index b3b287c165617..bdcdccbfb0017 100644
> > --- a/drivers/net/arcnet/com90xx.c
> > +++ b/drivers/net/arcnet/com90xx.c
> > @@ -67,8 +67,8 @@ static void com90xx_copy_from_card(struct net_device *dev, int bufnum,
> >  
> >  /* Known ARCnet cards */
> >  
> > -static struct net_device *cards[16];
> > -static int numcards;
> > +static struct net_device *cards[16] __ro_after_init;
> > +static int numcards __ro_after_init;
> 
> Simon says: I believe this is an existing bug rather than a regression.
>             But I am concerned that this patch may make it worse.
> 	    So I'm wondering if it should be fixed at the same time
> 	    by a series containing:
> 	    1. a patch to fix this overflow
> 	    2. this patch (to set __ro_after_init)
> 
> Could this change exacerbate an existing regression where we overflow the
> cards array?
> 
> com90xx_probe() can test up to 32 default I/O ports. For each valid device
> found, com90xx_found() unconditionally executes:
> 
> drivers/net/arcnet/com90xx.c:com90xx_found() {
>     ...
> 	if (register_netdev(dev))
> 		goto err_unmap;
> 
> 	cards[numcards++] = dev;
> 	return 0;
>     ...
> }
> 
> If more than 16 devices are successfully probed, this writes past the cards
> array. Since this patch marks these variables as __ro_after_init, they are
> now placed adjacently in memory.
> 
> Is it possible that the overflow will now clobber numcards with a pointer
> value, leading to an out-of-bounds index on the next iteration?
> 
> Simon says: The issue flagged below also appears to be an existing bug
>             rather than a regression. And I don't believe it impacts
> 	    this patch or needs to block progress of it.
> 
> Additionally, while reviewing this code, I noticed a second potential
> regression in com90xx_found(). Are we registering the IRQ handler before
> initializing the function pointers?
> 
> drivers/net/arcnet/com90xx.c:com90xx_found() {
>     ...
> 	/* reserve the irq */
> 	if (request_irq(airq, arcnet_interrupt, 0, "arcnet (90xx)", dev)) {
> 		arc_printk(D_NORMAL, dev, "Can't get IRQ %d!\n", airq);
> 		goto err_release_mem;
> 	}
> 	dev->irq = airq;
> 
> 	/* Initialize the rest of the device structure. */
> 	lp->card_name = "COM90xx";
> 	lp->hw.command = com90xx_command;
> 	lp->hw.status = com90xx_status;
>     ...
> }
> 
> If an interrupt triggers immediately after request_irq() completes (e.g., due
> to a spurious interrupt), arcnet_interrupt() will execute, evaluate
> !netif_running(dev) as true, and invoke lp->hw.status(dev).
> 
> Because alloc_arcdev() zero-allocates the private data, lp->hw.status is NULL
> at this point. Can this lead to a NULL pointer dereference?

Hi Simon,

I'm a newbie in the linux kernel development and I believe I don't have the
experience to fix all the issues showed in this review by myself.

Also, you tell me to leave it be as is because it has no real attention.
So, my question is:

Is it worth that I try to solve the issues mentioned or not?
I am totally confused.

Best regards,

Len