Index | Thread | Search

From:
Visa Hankala <visa@hankala.org>
Subject:
Re: sys/cnmac: fix command buffer leak on transmit failure
To:
"Kirill A. Korinsky" <kirill@korins.ky>
Cc:
tech@openbsd.org
Date:
Thu, 9 Jul 2026 14:22:51 +0000

Download raw body.

Thread
On Thu, Jul 09, 2026 at 12:28:47AM +0200, Kirill A. Korinsky wrote:
> visa@,
> 
> my ER-4 whcih I used as router had reached interesting state: it floods
> errors like this:
> 
> Jun 30 11:11:39 gw /bsd: cnmac2: cannot allocate command buffer from free pool allocator
> Jun 30 11:11:39 gw /bsd: cnmac2: failed to transmit packet
> Jun 30 11:11:40 gw /bsd: cnmac2: cannot allocate command buffer from free pool allocator
> Jun 30 11:11:40 gw /bsd: cnmac2: failed to transmit packet
> 
> tons of them, I have no idea what had happened before I discovered it in
> that state, but device was ok via serial, but network was dead.
> 
> After reading code near that error I think I had spotted a leak.

It seems to me that there is no leak in the code. The local cmdptr
pointer is derived using cmdptr_idx and discarded when the function
returns. cnmac_send_cmd() updates cmdptr_idx only if it was able to
add the command words to the queue.

> Index: sys/arch/octeon/dev/if_cnmac.c
> ===================================================================
> RCS file: /home/cvs/src/sys/arch/octeon/dev/if_cnmac.c,v
> diff -u -p -r1.91 if_cnmac.c
> --- sys/arch/octeon/dev/if_cnmac.c	19 Jun 2026 15:12:10 -0000	1.91
> +++ sys/arch/octeon/dev/if_cnmac.c	8 Jul 2026 22:20:34 -0000
> @@ -918,9 +918,6 @@ cnmac_send_cmd(struct cnmac_softc *sc, u
>  
>  	OCTEON_ETH_KASSERT(cmdptr != NULL);
>  
> -	*cmdptr++ = pko_cmd_w0;
> -	*cmdptr++ = pko_cmd_w1;
> -
>  	OCTEON_ETH_KASSERT(sc->sc_cmdptr.cmdptr_idx + 2 <= FPA_COMMAND_BUFFER_POOL_NWORDS - 1);
>  
>  	if (sc->sc_cmdptr.cmdptr_idx + 2 == FPA_COMMAND_BUFFER_POOL_NWORDS - 1) {
> @@ -934,10 +931,14 @@ cnmac_send_cmd(struct cnmac_softc *sc, u
>  			result = 1;
>  			goto done;
>  		}
> +		*cmdptr++ = pko_cmd_w0;
> +		*cmdptr++ = pko_cmd_w1;
>  		*cmdptr++ = buf;
>  		sc->sc_cmdptr.cmdptr = (uint64_t)buf;
>  		sc->sc_cmdptr.cmdptr_idx = 0;
>  	} else {
> +		*cmdptr++ = pko_cmd_w0;
> +		*cmdptr++ = pko_cmd_w1;
>  		sc->sc_cmdptr.cmdptr_idx += 2;
>  	}
>  
> 
> 
> -- 
> wbr, Kirill
>