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

[Len Bao <len.bao@gmx.us>]

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