linux-kernel - Re: [PATCH net-next v23 3/7] mm: page_frag: use initial zero offset for page_frag_alloc

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: <371240e0-5f95-4d5b-9eea-9029e5dcb4b1@broadcom.com>
Date: Fri, 24 Jan 2025 10:55:22 -0800
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Yunsheng Lin <linyunsheng@...wei.com>, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, Eric Dumazet <edumazet@...gle.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Alexander Duyck <alexander.duyck@...il.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Linux-MM <linux-mm@...ck.org>,
 Alexander Duyck <alexanderduyck@...com>
Subject: Re: [PATCH net-next v23 3/7] mm: page_frag: use initial zero offset
 for page_frag_alloc_align()

On 1/24/25 01:52, Yunsheng Lin wrote:
> On 2025/1/24 3:15, Florian Fainelli wrote:
>>
>> Sorry for the late feedback, this patch causes the bgmac driver in is .ndo_open() function to return -ENOMEM, the call trace looks like this:
> 
> Hi, Florian
> Thanks for the report.
> 
>>
>>   bgmac_open
>>    -> bgmac_dma_init
>>      -> bgmac_dma_rx_skb_for_slot
>>        -> netdev_alloc_frag
>>
>> BGMAC_RX_ALLOC_SIZE = 10048 and PAGE_FRAG_CACHE_MAX_SIZE = 32768.
> 
> I guess BGMAC_RX_ALLOC_SIZE being bigger than PAGE_SIZE is the
> problem here, as the frag API is not really supporting allocating
> fragment that is bigger than PAGE_SIZE, as it will fall back to
> allocate the base page when order-3 compound page allocation fails,
> see __page_frag_cache_refill().
> 
> Also, it seems strange that bgmac driver seems to always use jumbo
> frame size to allocate fragment, isn't more  appropriate to allocate
> the fragment according to MTU?

Totally, even though my email domain would suggest otherwise, I am just 
an user of that driver here, not its maintainer, though I do have some 
familiarity with it, I don't know why that choice was made.

> 
>>
>> Eventually we land into __page_frag_alloc_align() with the following parameters across multiple successive calls:
>>
>> __page_frag_alloc_align: fragsz=10048, align_mask=-1, size=32768, offset=0
>> __page_frag_alloc_align: fragsz=10048, align_mask=-1, size=32768, offset=10048
>> __page_frag_alloc_align: fragsz=10048, align_mask=-1, size=32768, offset=20096
>> __page_frag_alloc_align: fragsz=10048, align_mask=-1, size=32768, offset=30144
>>
>> So in that case we do indeed have offset + fragsz (40192) > size (32768) and so we would eventually return NULL.
> 
> It seems the changing of '(unlikely(offset < 0))' checking to
> "fragsz > PAGE_SIZE" causes bgmac driver to break more easily
> here. But bgmac driver might likely break too if the system
> memory is severely fragmented when falling back to alllocate
> base page before this patch.
> 
>>
>> Any idea on how to best fix that within the bgmac driver?
> 
> Maybe use the page allocator API directly when allocating fragment
> with BGMAC_RX_ALLOC_SIZE > PAGE_SIZE for a quick fix.
> 
> In the long term, maybe it makes sense to use the page_pool API
> as more drivers are converting to use page_pool API already.

Short term, I think I am going to submit a quick fix that is inspired by 
the out of tree patches carried in OpenWrt:

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/bcm53xx/patches-6.6/700-bgmac-reduce-max-frame-size-to-support-just-MTU-1500.patch;h=3a2f4b06ed6d8cda1f3f0be23e1066267234766b;hb=HEAD

Thanks!
-- 
Florian