netdev - Re: [PATCH net 1/3] i40e: avoid premature Rx buffer reuse

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: <20200825112953.GB38865@ranger.igk.intel.com>
Date:   Tue, 25 Aug 2020 13:29:53 +0200
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Björn Töpel <bjorn.topel@...el.com>
Cc:     Björn Töpel <bjorn.topel@...il.com>,
        jeffrey.t.kirsher@...el.com, intel-wired-lan@...ts.osuosl.org,
        magnus.karlsson@...el.com, magnus.karlsson@...il.com,
        netdev@...r.kernel.org, piotr.raczynski@...el.com,
        maciej.machnikowski@...el.com, lirongqing@...du.com
Subject: Re: [PATCH net 1/3] i40e: avoid premature Rx buffer reuse

On Tue, Aug 25, 2020 at 01:25:16PM +0200, Björn Töpel wrote:
> On 2020-08-25 13:13, Maciej Fijalkowski wrote:
> > On Tue, Aug 25, 2020 at 11:16:27AM +0200, Björn Töpel wrote:
> [...]
> > >   	struct i40e_rx_buffer *rx_buffer;
> > >   	rx_buffer = i40e_rx_bi(rx_ring, rx_ring->next_to_clean);
> > > +	*rx_buffer_pgcnt = i40e_rx_buffer_page_count(rx_buffer);
> > 
> > What i previously meant was:
> > 
> > #if (PAGE_SIZE < 8192)
> > 	*rx_buffer_pgcnt = page_count(rx_buffer->page);
> > #endif
> > 
> > and see below
> > 
> 
> Right...
> 
> > >   	prefetchw(rx_buffer->page);
> > >   	/* we are reusing so sync this buffer for CPU use */
> > > @@ -2112,9 +2124,10 @@ static struct sk_buff *i40e_build_skb(struct i40e_ring *rx_ring,
> > >    * either recycle the buffer or unmap it and free the associated resources.
> > >    */
> > >   static void i40e_put_rx_buffer(struct i40e_ring *rx_ring,
> > > -			       struct i40e_rx_buffer *rx_buffer)
> > > +			       struct i40e_rx_buffer *rx_buffer,
> > > +			       int rx_buffer_pgcnt)
> > >   {
> > > -	if (i40e_can_reuse_rx_page(rx_buffer)) {
> > > +	if (i40e_can_reuse_rx_page(rx_buffer, rx_buffer_pgcnt)) {
> > >   		/* hand second half of page back to the ring */
> > >   		i40e_reuse_rx_page(rx_ring, rx_buffer);
> > >   	} else {
> > > @@ -2319,6 +2332,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
> > >   	unsigned int xdp_xmit = 0;
> > >   	bool failure = false;
> > >   	struct xdp_buff xdp;
> > > +	int rx_buffer_pgcnt;
> > 
> > you could move scope this variable only for the
> > 
> > while (likely(total_rx_packets < (unsigned int)budget))
> > 
> > loop and init this to 0. then you could drop the helper function you've
> > added. and BTW the page_count is not being used for big pages but i agree
> > that it's better to have it set to 0.
> > 
> 
> ...but isn't it a bit nasty with an output parameter that relies on the that
> the input was set to zero. I guess it's a matter of taste, but I find that
> more error prone.
> 
> Let me know if you have strong feelings about this, and I'll respin (but I
> rather not!).

Up to you. No strong feelings, i just think that i40e_rx_buffer_page_count
is not needed. But if you want to keep it, then i was usually asking
people to provide the doxygen descriptions for newly introduced
functions... :P

but scoping it still makes sense to me, static analysis tools would agree
with me I guess.

> 
> 
> Björn
> 
> 
> > >   #if (PAGE_SIZE < 8192)
> > >   	xdp.frame_sz = i40e_rx_frame_truesize(rx_ring, 0);
> > > @@ -2370,7 +2384,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
> > >   			break;
> > >   		i40e_trace(clean_rx_irq, rx_ring, rx_desc, skb);
> > > -		rx_buffer = i40e_get_rx_buffer(rx_ring, size);
> > > +		rx_buffer = i40e_get_rx_buffer(rx_ring, size, &rx_buffer_pgcnt);
> > >   		/* retrieve a buffer from the ring */
> > >   		if (!skb) {
> > > @@ -2413,7 +2427,7 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
> > >   			break;
> > >   		}
> > > -		i40e_put_rx_buffer(rx_ring, rx_buffer);
> > > +		i40e_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt);
> > >   		cleaned_count++;
> > >   		if (i40e_is_non_eop(rx_ring, rx_desc, skb))
> > > -- 
> > > 2.25.1
> > >