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

Message-ID: <378704b9-ad2c-ad63-2434-a2b34bb25f0f@intel.com>
Date:   Tue, 25 Aug 2020 13:37:35 +0200
From:   Björn Töpel <bjorn.topel@...el.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...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 2020-08-25 13:29, Maciej Fijalkowski wrote:
> 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.
>

Fair enough! I'll spin a v2! Thanks for taking a look!


Björn


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