netdev - Re: [PATCH net 1/2] net: stmmac: fix rx budget limit check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3enhwmz5sfuj6wb2h74xamyjsihiy4fagklqre7tojbwprnuib@t3w7ykzvofqx>
Date: Tue, 14 Nov 2023 14:25:54 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Baruch Siach <baruch@...s.co.il>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net 1/2] net: stmmac: fix rx budget limit check

On Mon, Nov 13, 2023 at 07:42:49PM +0200, Baruch Siach wrote:
> The while loop condition verifies 'count < limit'. Neither value change
> before the 'count >= limit' check. As is this check is dead code. But
> code inspection reveals a code path that modifies 'count' and then goto
> 'drain_data' and back to 'read_again'. So there is a need to verify
> count value sanity after 'read_again'.
> 
> Move 'read_again' up to fix the count limit check.

Nice catch! My local fix was to just drop the statement, but obviously
it was wrong. Indeed it's possible to have an implicit loop based on
two goto'es. So for this change definitely:
Reviewed-by: Serge Semin <fancer.lancer@...il.com>

>From the patch perspective seeing how clumsy the
stmmac_rx()/stmmac_xmit() methods look here and in several/multiple
other net-drivers here is a question to the subsystem maintainers. Is
it really a preferred practice to design them that way with gotos and
embed all the various stuff directly to a single function? Wouldn't it
be better at least from the readability point of view to split them up
into a set of smaller coherent functions and get rid from the gotos?

I am wondering because normally it would be indeed better, but network
subsystem may have some special requirements for such methods (if so
is it described anywhere in the kernel doc?), for instance, to reach a
greater performance by not relying on the compiler to embed the
sub-functions body into the denoted functions or by using the gotos so
not to increment the loop-counter and preserve the indentation level.
All of that may improve the code performance in some extent, but in
its turn it significantly reduces the code readability and
maintainability.

-Serge(y)

> 
> Fixes: ec222003bd94 ("net: stmmac: Prepare to add Split Header support")
> Signed-off-by: Baruch Siach <baruch@...s.co.il>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3e50fd53a617..f28838c8cdb3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5328,10 +5328,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  			len = 0;
>  		}
>  
> +read_again:
>  		if (count >= limit)
>  			break;
>  
> -read_again:
>  		buf1_len = 0;
>  		buf2_len = 0;
>  		entry = next_entry;
> -- 
> 2.42.0
> 
>