Re: [PATCH] net: 8390: pcnet_cs: release PCMCIA window on setup_shmem_window() error

[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://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260731161740.44955-1-mhun512@gmail.com?part=1
---
--- Patch [1]: [PATCH] net: 8390: pcnet_cs: release PCMCIA window on setup_shmem_window() error ---
commit 74f57c39e62b4fcd24846cf9fc7fe9cbcd03cfe3
Author: Myeonghun Pak <mhun512@gmail.com>

net: 8390: pcnet_cs: release PCMCIA window on setup_shmem_window() error

This routes the post-pcmcia_request_window() error paths in
setup_shmem_window() through a new "release" label that calls
pcmcia_release_window(), and folds the existing buffer-verification
cleanup into it. The pcmcia_request_window() failure path is left
returning directly.

A few questions below, mostly about the changelog and about the
surrounding code in the function being reworked.

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5]
The changelog states:

    The requested window is leaked.

and carries Cc: stable. Is "leaked" the right description here? On the
success path pcmcia_request_window() stamps the map bits into the
resource flags:

    res->flags |= (win->map << 2) | IORESOURCE_MEM;

and pcmcia_disable_device() then walks every window resource and
releases the ones still carrying those bits:

    for (i = 0; i < MAX_WIN; i++) {
            struct resource *res = p_dev->resource[MAX_IO_WIN + i];
            if (res->flags & WIN_FLAGS_REQ)
                    pcmcia_release_window(p_dev, res);
    }

Both the pcnet_config() failure path and pcnet_detach() reach
pcnet_release() -> pcmcia_disable_device(), so the window looks like it
is reclaimed at device teardown rather than lost for good.

Would it be more accurate to say socket window 3 and its reserved iomem
range stay held for the whole bound lifetime of the device even though
the driver has fallen back to setup_dma_config()? That wording would
also help stable backport triage, since the current text reads like an
unbounded leak that survives card removal or module unload.

> diff --git a/drivers/net/ethernet/8390/pcnet_cs.c b/drivers/net/ethernet/8390/pcnet_cs.c
> index 19f9c5db3f3ba..a913674be60ac 100644
> --- a/drivers/net/ethernet/8390/pcnet_cs.c
> +++ b/drivers/net/ethernet/8390/pcnet_cs.c
> @@ -1434,14 +1434,14 @@ static int setup_shmem_window(struct pcmcia_device *link, int start_pg,
>      offset -= offset % window_size;
>      ret = pcmcia_map_mem_page(link, link->resource[3], offset);
>      if (ret)
> -	    goto failed;
> +	goto release;
>  
>      /* Try scribbling on the buffer */
>      info->base = ioremap(link->resource[3]->start,
>  			resource_size(link->resource[3]));
>      if (unlikely(!info->base)) {
>  	    ret = -ENOMEM;
> +	goto release;
>      }

[Severity: Low]
[Finding: main-1-1]
[Sources: opus-5]
This isn't a bug, but the indentation of the two new goto statements
differs from the code they replace and from the statements around them.
The file uses tab+4 spaces at this depth, and the new gotos use a single
tab. In the ioremap() block the two statements of the same brace block
now sit at different columns:

    if (unlikely(!info->base)) {
	    ret = -ENOMEM;
	goto release;
    }

which makes the goto read as if it were outside the if block. Could
these keep the surrounding style?

>  
>      for (i = 0; i < (TX_PAGES<<8); i += 2)
> @@ -1452,9 +1452,8 @@ static int setup_shmem_window(struct pcmcia_device *link, int start_pg,
>      pcnet_reset_8390(dev);
>      if (i != (TX_PAGES<<8)) {
>  	iounmap(info->base);
> -	pcmcia_release_window(link, link->resource[3]);
>  	info->base = NULL;
> -	goto failed;
> +	goto release;
>      }

[Severity: High]
[Finding: main-7-3]
[Sources: opus-5]
This is a pre-existing issue and not introduced by this patch, but it is
in the function being reworked, so it seems worth raising.

Does setup_shmem_window() mix up the card-side window base with the
host-side displacement into the ioremap() mapping? offset is computed as
a card address rounded down to a window boundary:

    offset = (start_pg << 8) + cm_offset;
    offset -= offset % window_size;
    ret = pcmcia_map_mem_page(link, link->resource[3], offset);

That is correct for pcmcia_map_mem_page(), which stores it as
s->win[w].card_start. But the same value is then used as a byte offset
inside the host mapping:

    for (i = 0; i < (TX_PAGES<<8); i += 2)
	__raw_writew((i>>1), info->base+offset+i);
    udelay(100);
    for (i = 0; i < (TX_PAGES<<8); i += 2)
	if (__raw_readw(info->base+offset+i) != (i>>1)) break;

With the default parameters (start_pg == PCNET_START_PG == 0x40,
cm_offset == 0) window_size is 0x4000 and offset stays 0x4000, while the
ioremap() covers exactly resource_size(link->resource[3]) == 0x4000
bytes. Doesn't that put the whole scribble loop at info->base + 0x4000
through +0x4bff, entirely past the end of the mapping, so the write
either faults on the ioremap guard page or lands in a neighbouring
device mapping?

Should the host displacement be the remainder instead, i.e.
((start_pg << 8) + cm_offset) % window_size?

The derived values look affected the same way:

    ei_status.mem = info->base + offset;
    ei_status.priv = resource_size(link->resource[3]);
    ...
    ei_status.stop_page = start_pg + (
	    (resource_size(link->resource[3]) - offset) >> 8);

In the default case that gives stop_page == start_pg, and for
USE_BIG_BUF (start_pg 0x01, cm_offset 0x10000, window_size 0x8000)
offset becomes 0x10000 so resource_size() - offset underflows in
unsigned resource_size_t arithmetic.

Reaching this needs use_shmem=1 on the module line plus a supported
card, since has_shmem is always 0 (pcnet_confcheck() does *priv &= 0x10
on a value that is only ever 0 or 1). Given that, is the shared-memory
path here exercised at all, and does that change how the changelog's
"probe can continue and even succeed" claim should be phrased?

>  
>      ei_status.mem = info->base + offset;
> @@ -1475,6 +1474,8 @@ static int setup_shmem_window(struct pcmcia_device *link, int start_pg,
>      info->flags |= USE_SHMEM;
>      return 0;
>  
> +release:
> +	pcmcia_release_window(link, link->resource[3]);
>  failed:
>      return 1;
>  }

Simon says: I think the pre-existing problem described below
            is out of scope for your patch.

[Severity: Medium]
[Finding: main-2-2]
[Sources: opus-5]
This is a pre-existing problem in the PCMCIA core rather than something
this patch introduces, but it touches the assumption stated in the
changelog:

    The pcmcia_request_window() failure path still returns
    directly, as no window is held there.

Is that always true? pcmcia_request_window() allocates the system memory
region and sets the client bit before programming the mapping:

    p_dev->_win |= CLIENT_WIN_REQ(w);

    /* Configure the socket controller */
    win->map = w+1;
    ...
    if (s->ops->set_mem_map(s, win) != 0) {
            dev_dbg(&p_dev->dev, "failed to set memory mapping\n");
            mutex_unlock(&s->ops_mutex);
            return -EIO;
    }

On that -EIO return win->res stays allocated, the _win bit stays set,
and res->flags never receives the (win->map << 2) marker that is only
written on the success path. Both cleanup helpers key off exactly that
marker:

    w = ((res->flags & IORESOURCE_BITS & WIN_FLAGS_REQ) >> 2) - 1;
    if (w >= MAX_WIN)
            return -EINVAL;

and in pcmcia_disable_device():

    if (res->flags & WIN_FLAGS_REQ)
            pcmcia_release_window(p_dev, res);

WIN_FLAGS_REQ is 0x1c, and the flags pcnet_cs sets here
(WIN_DATA_WIDTH_16 | WIN_MEMORY_TYPE_CM | WIN_ENABLE | WIN_USE_WAIT)
contain none of those bits, so the half-held window would be invisible
to pcnet_release() -> pcmcia_disable_device() too.

This looks reachable from pcnet_cs with mem_speed above 1000, where
yenta_set_mem_map() rejects the mapping on mem->speed > 1000 and
pcmcia_request_window() takes that -EIO path. Does that mean the
struct resource from pcmcia_find_mem_region() and the reserved host
iomem range can no longer be released by any in-tree caller, and that
window index w can later be reused with win->res overwritten?

Adding a release label for that branch would not help while res->flags
lacks the marker bits, so it may be worth noting that the core is where
this one has to be fixed.