netdev - RE: [PATCH iwl-next v2 3/4] igb: add AF

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] [day] [month] [year] [list]

Message-ID:
 <DBBP189MB1433B99F674420A06C65C7549537A@DBBP189MB1433.EURP189.PROD.OUTLOOK.COM>
Date: Thu, 13 Jul 2023 10:25:21 +0000
From: Sriram Yagnaraman <sriram.yagnaraman@....tech>
To: Simon Horman <simon.horman@...igine.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"bpf@...r.kernel.org" <bpf@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, Jesse Brandeburg <jesse.brandeburg@...el.com>, Tony
 Nguyen <anthony.l.nguyen@...el.com>, "David S . Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Alexei Starovoitov
	<ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Jesper Dangaard
 Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>,
	Björn Töpel <bjorn@...nel.org>, Magnus Karlsson
	<magnus.karlsson@...el.com>, Maciej Fijalkowski
	<maciej.fijalkowski@...el.com>, Jonathan Lemon <jonathan.lemon@...il.com>
Subject: RE: [PATCH iwl-next v2 3/4] igb: add AF_XDP zero-copy Rx support

Hi Simon,

> -----Original Message-----
> From: Simon Horman <simon.horman@...igine.com>
> Sent: Thursday, 13 July 2023 11:29
> To: Sriram Yagnaraman <sriram.yagnaraman@....tech>
> Cc: intel-wired-lan@...ts.osuosl.org; bpf@...r.kernel.org;
> netdev@...r.kernel.org; Jesse Brandeburg <jesse.brandeburg@...el.com>;
> Tony Nguyen <anthony.l.nguyen@...el.com>; David S . Miller
> <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jakub
> Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; Alexei
> Starovoitov <ast@...nel.org>; Daniel Borkmann <daniel@...earbox.net>;
> Jesper Dangaard Brouer <hawk@...nel.org>; John Fastabend
> <john.fastabend@...il.com>; Björn Töpel <bjorn@...nel.org>; Magnus
> Karlsson <magnus.karlsson@...el.com>; Maciej Fijalkowski
> <maciej.fijalkowski@...el.com>; Jonathan Lemon
> <jonathan.lemon@...il.com>
> Subject: Re: [PATCH iwl-next v2 3/4] igb: add AF_XDP zero-copy Rx support
> 
> On Tue, Jul 11, 2023 at 01:47:04PM +0200, Sriram Yagnaraman wrote:
> > Add support for AF_XDP zero-copy receive path.
> >
> > When AF_XDP zero-copy is enabled, the rx buffers are allocated from
> > the xsk buff pool using igb_alloc_rx_buffers_zc.
> >
> > Use xsk_pool_get_rx_frame_size to set SRRCTL rx buf size when
> > zero-copy is enabled.
> >
> > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@....tech>
> 
> Hi Sriram,
> 
> > +bool igb_alloc_rx_buffers_zc(struct igb_ring *rx_ring, u16 count) {
> > +	union e1000_adv_rx_desc *rx_desc;
> > +	u32 nb_buffs_extra = 0, nb_buffs;
> > +	u16 ntu = rx_ring->next_to_use;
> > +	u16 total_count = count;
> > +	struct xdp_buff **xdp;
> > +
> > +	rx_desc = IGB_RX_DESC(rx_ring, ntu);
> > +	xdp = &rx_ring->rx_buffer_info_zc[ntu];
> > +
> > +	if (ntu + count >= rx_ring->count) {
> > +		nb_buffs_extra = igb_fill_rx_descs(rx_ring->xsk_pool, xdp,
> > +						   rx_desc,
> > +						   rx_ring->count - ntu);
> > +		if (nb_buffs_extra != rx_ring->count - ntu) {
> > +			ntu += nb_buffs_extra;
> > +			goto exit;
> 
> nb_buffs is uninitialised here...

Thank you, will fix it in v3

> 
> > +		}
> > +		rx_desc = IGB_RX_DESC(rx_ring, 0);
> > +		xdp = rx_ring->rx_buffer_info_zc;
> > +		ntu = 0;
> > +		count -= nb_buffs_extra;
> > +	}
> > +
> > +	nb_buffs = igb_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> > +	ntu += nb_buffs;
> > +	if (ntu == rx_ring->count)
> > +		ntu = 0;
> > +
> > +	/* clear the length for the next_to_use descriptor */
> > +	rx_desc = IGB_RX_DESC(rx_ring, ntu);
> > +	rx_desc->wb.upper.length = 0;
> > +
> > +exit:
> > +	if (rx_ring->next_to_use != ntu) {
> > +		rx_ring->next_to_use = ntu;
> > +
> > +		/* Force memory writes to complete before letting h/w
> > +		 * know there are new descriptors to fetch.  (Only
> > +		 * applicable for weak-ordered memory model archs,
> > +		 * such as IA-64).
> > +		 */
> > +		wmb();
> > +		writel(ntu, rx_ring->tail);
> > +	}
> > +
> > +	return total_count == (nb_buffs + nb_buffs_extra);
> 
> But it is used here.
> 
> ...
> 
> The following will tell you about this problem:
> - Smatch
> - clang-16 W=1 build [-Wsometimes-uninitialized]
> - gcc-12 build with -Wmaybe-uninitialized