netdev - 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:
 <PAXPR04MB85100F961EBDAFE532CE026D88C72@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Fri, 21 Feb 2025 01:26:23 +0000
From: Wei Fang <wei.fang@....com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Claudiu Manoil <claudiu.manoil@....com>, Clark Wang
	<xiaoning.wang@....com>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, Ioana Ciornei
	<ioana.ciornei@....com>, "Y.B. Lu" <yangbo.lu@....com>,
	"michal.swiatkowski@...ux.intel.com" <michal.swiatkowski@...ux.intel.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>, "stable@...r.kernel.org"
	<stable@...r.kernel.org>
Subject: RE: [PATCH v2 net 1/9] net: enetc: fix the off-by-one issue in
 enetc_map_tx_buffs()

> 
> 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?
> 
Yes, I plan to use a helper function to replace the same code blocks in net-next
tree.

> 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 :)