linux-kernel - Re: [PATCH RFC net-next 1/4] net: skbuff: don't include <net/page

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: <01388e7b-0178-924f-c0e1-a0c8b0cded8b@intel.com>
Date:   Fri, 30 Jun 2023 18:05:32 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Larysa Zaremba <larysa.zaremba@...el.com>,
        Yunsheng Lin <linyunsheng@...wei.com>,
        Alexander Duyck <alexanderduyck@...com>,
        "Jesper Dangaard Brouer" <hawk@...nel.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC net-next 1/4] net: skbuff: don't include
 <net/page_pool.h> to <linux/skbuff.h>

From: Alexander Duyck <alexander.duyck@...il.com>
Date: Fri, 30 Jun 2023 08:11:02 -0700

> On Fri, Jun 30, 2023 at 5:39 AM Alexander Lobakin
> <aleksander.lobakin@...el.com> wrote:
>>
>> From: Alexander H Duyck <alexander.duyck@...il.com>
>> Date: Thu, 29 Jun 2023 09:55:15 -0700
>>
>>> On Thu, 2023-06-29 at 17:23 +0200, Alexander Lobakin wrote:
>>>> Currently, touching <net/page_pool.h> triggers a rebuild of more than
>>>> a half of the kernel. That's because it's included in <linux/skbuff.h>.
>>>> And each new include to page_pool.h adds more [useless] data for the
>>>> toolchain to process per each source file from that pile.
>>
>> [...]
>>
>>>> +bool page_pool_return_skb_page(struct page *page, bool napi_safe)
>>>> +{
>>>> +    struct napi_struct *napi;
>>>> +    struct page_pool *pp;
>>>> +    bool allow_direct;
>>>> +
>>>> +    page = compound_head(page);
>>>> +    pp = page->pp;
>>>
>>> So this is just assuming that any page we pass thru is a page pool
>>> page. The problem is there may be some other pointer stored here that
>>> could cause issues.
>>
>> But that is exactly what you suggested in the previous revision's
>> thread... Hey! :D
>>
>> "I suspect we could look at pulling parts of it out as well. The
>> pp_magic check should always be succeeding unless we have pages getting
>> routed the wrong way somewhere. So maybe we should look at pulling it
>> out and moving it to another part of the path such as
>> __page_pool_put_page() and making it a bit more visible to catch those
>> cases".
> 
> Yeah, I have had a few off days recently, amazing what happens when
> you are running on only a few hours of sleep.. :-/

Aaah I see :D

> 
> Anyway, as I was saying it might make sense to wrap the whole thing up
> as a page pool accessor that would return NULL if the MAGIC value
> isn't present. Alternatively one other possibility would be to look at
> creating an inline function here that could check to see if the
> skb_frag is page pool and then just keep that in sk_buff.h since it
> looks like it should only need to rely on the page struct and
> PP_SIGNATURE which is a poison.h value. With that napi_frag_unref
> could avoid an unnecessary trip into the page_pool_return_skb_page
> function entirely if it isn't a page pool page and we could look at
> dropping the return value from page_pool_return_skb_page entirely.

I like this one. This is rather simple check and shouldn't cause code
size inflating, but I'll recheck with bloat-o-meter just in case.

> 
>> Anyway, since some drivers still mix PP pages with non-PP ones (mt76
>> IIRC, maybe more), I feel the check for magic is still relevant. I just
>> believed you and forgot about that T.T
> 
> Yeah, sorry about that, my bad. I was a bit too focused on the main
> drivers we use and not thinking outside the box enough.

Not a prob!

Thanks,
Olek