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