linux-kernel - Re: [PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in enetc_map_tx

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: <20250220130121.fq3irlaunowyvfc4@skbuf>
Date: Thu, 20 Feb 2025 15:01:21 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Wei Fang <wei.fang@....com>
Cc: claudiu.manoil@....com, xiaoning.wang@....com, andrew+netdev@...n.ch,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, ioana.ciornei@....com, yangbo.lu@....com,
	michal.swiatkowski@...ux.intel.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev,
	stable@...r.kernel.org
Subject: Re: [PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in
 enetc_map_tx_buffs()

On Wed, Feb 19, 2025 at 01:42:39PM +0800, Wei Fang wrote:
> When a DMA mapping error occurs while processing skb frags, it will free
> one more tx_swbd than expected, so fix this off-by-one issue.
> 
> Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
> Cc: stable@...r.kernel.org
> Signed-off-by: Wei Fang <wei.fang@....com>
> ---

After running with some test data, I agree that the bug exists and that
the fix is correct.

Reviewed-by: Vladimir Oltean <vladimir.oltean@....com>

It's just that there's yet one more (correct) dma_err snippet in
enetc_lso_hw_offload() which achieves the same thing, but expressed
differently, added by you in December 2024.

For fixing a bug from 2019, I agree that you've made the right choice in
not creating a dependency on that later code. But I like slightly less
the fact that it leaves 2 slightly different, both non-obvious, code
paths for unmapping DMA buffers. You could have at least copied the
dma_err handling from enetc_lso_hw_offload(), to make it obvious that
one is correct and the other is not, and not complicate things with yet
a 3rd implementation.

You don't need to change this unless there's some other reason to resend
the patch set, but at least, once "net" merges back into "net-next",
could you please make a mental note to consolidate the 2 code snippets
into a single function?

Also, the first dma_mapping_error() from enetc_map_tx_buffs() does not
need to "goto dma_err". It would be dead code. Maybe you could simplify
that as well. In general, the convention of naming error path labels is
to name them after what they do, rather than after the function that
failed when you jump to them. It's easier to manually verify correctness
this way.

Also, the dev_err(tx_ring->dev, "DMA map error"); message should be rate
limited, since it's called from a fast path and can kill the console if
the error is persistent.

A lot of things to improve still, thanks for doing this :)