linux-kernel - Re: [Intel-wired-lan] [PATCH RFC net-next v4 3/9] iavf: drop page splitting and recycling

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: <f73daa0e-c201-5fba-8232-2107bc8e24de@intel.com>
Date:   Mon, 10 Jul 2023 15:13:22 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
CC:     Paul Menzel <pmenzel@...gen.mpg.de>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Larysa Zaremba <larysa.zaremba@...el.com>,
        <netdev@...r.kernel.org>, Alexander Duyck <alexanderduyck@...com>,
        "Ilias Apalodimas" <ilias.apalodimas@...aro.org>,
        <linux-kernel@...r.kernel.org>, Eric Dumazet <edumazet@...gle.com>,
        Michal Kubiak <michal.kubiak@...el.com>,
        <intel-wired-lan@...ts.osuosl.org>,
        Yunsheng Lin <linyunsheng@...wei.com>,
        David Christensen <drc@...ux.vnet.ibm.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH RFC net-next v4 3/9] iavf: drop page
 splitting and recycling

From: Alexander Duyck <alexander.duyck@...il.com>
Date: Thu, 6 Jul 2023 10:06:29 -0700

> On Thu, Jul 6, 2023 at 9:46 AM Alexander Lobakin
> <aleksander.lobakin@...el.com> wrote:
>>
>> From: Alexander Duyck <alexander.duyck@...il.com>
>> Date: Thu, 6 Jul 2023 07:47:03 -0700
>>
>>> On Wed, Jul 5, 2023 at 8:57 AM Alexander Lobakin
>>> <aleksander.lobakin@...el.com> wrote:
> 
> [...]
> 
>>>> @@ -1431,15 +1303,18 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>>>>                 else
>>>>                         skb = iavf_build_skb(rx_ring, rx_buffer, size);
>>>>
>>>> +               iavf_put_rx_buffer(rx_ring, rx_buffer);
>>>> +
>>>
>>> This should stay below where it was.
>>
>> Wait-wait-wait.
>>
>> if (!skb) break breaks the loop. put_rx_buffer() unmaps the page.
>> So in order to do the first, you need to do the second to avoid leaks.
>> Or you meant "why unmapping and freeing if we fail, just leave it in
>> place"? To make it easier to switch to Page Pool.
> 
> Specifically you don't want to be unmapping and freeing this page
> until after the !skb check. The problem is if skb is NULL the skb
> allocation failed and so processing of Rx is meant to stop in place
> without removing the page. It is where we will resume on the next pass
> assuming memory has been freed that can then be used. The problem is
> the skb allocation, not the page. We used to do the skb allocation
> before we would acquire the buffer, but with XDP there are cases where
> we aren't supposed to allocate it so it got moved to after which
> causes this confusion.
> 
>>>
>>>>                 /* exit if we failed to retrieve a buffer */
>>>>                 if (!skb) {
>>>>                         rx_ring->rx_stats.alloc_buff_failed++;
>>>> -                       if (rx_buffer && size)
>>>> -                               rx_buffer->pagecnt_bias++;
>>>> +                       __free_pages(rx_buffer->page,
>>>> +                                    iavf_rx_pg_order(rx_ring));
>>>> +                       rx_buffer->page = NULL;
>>>>                         break;
>>>>                 }
>>>
>>> This code was undoing the iavf_get_rx_buffer decrement of pagecnt_bias
>>> and then bailing since we have halted forward progress due to an skb
>>> allocation failure. As such we should just be removing the if
>>> statement and the increment of pagecnt_bias.
> 
> The key bit here is the allocation failure is the reason why we halted
> processing. So the page contains valid data and should not be freed.
> We just need to leave it in place and wait for an allocation to
> succeed and then we can resume processing.

Aaah, okay, you want to try once again with the same buffer next time.
I see. Makes sense!

> 
>>>
>>>>
>>>> -               iavf_put_rx_buffer(rx_ring, rx_buffer);
>>>> +               rx_buffer->page = NULL;
>>>>                 cleaned_count++;
>>>>
>>>>                 if (iavf_is_non_eop(rx_ring, rx_desc, skb))
>>>
>>> If iavf_put_rx_buffer just does the unmap and assignment of NULL then
>>> it could just be left here as is.
>>
>> I guess those two are tied with the one above.
> 
> Yeah, the iavf_put_rx_buffer should be left  down here.
Thanks,
Olek