netdev - Re: [RFC PATCH net-next v5 2/2] net: add netmem to skb_frag

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]

Date: Thu, 18 Jan 2024 16:52:04 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, Mina Almasry
	<almasrymina@...gle.com>, Jason Gunthorpe <jgg@...dia.com>
CC: <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Christian König <christian.koenig@....com>, Shakeel Butt
	<shakeelb@...gle.com>
Subject: Re: [RFC PATCH net-next v5 2/2] net: add netmem to skb_frag_t

On 2024/1/18 2:54, Willem de Bruijn wrote:
> 
> I agree. A concern with CONFIGs is that what matters in practice is
> which default the distros compile with. In this case, adding hurdles
> to using the feature likely for no real reason.
> 
> Static branches are used throughout the kernel in performance
> sensitive paths, exactly because they allow optional paths effectively
> for free. I'm quite surprised that this issue is being raised so
> strongly here, as they are hardly new or controversial.

The new or controversial part about its usage in the devmem patchset as my
understanding is:
1. It is assumed the devmem and normal page processing in networking does
   not have to be treated equally in the same system, either the performance
   of devmem is favored or the performance of normal page is favored. I think
   if distros is starting to worry about the CONFIG for devmem, devmem must be
   quite popular that we might need the best performance of both. IMHO, static
   branches might be just a convenient way to start supporting the devmem for
   now as we seems to not have a clear idea of unified handling or proper API
   for both devmem and normal page.

2. Specifically to skb_frag_page(), if the returning NULL is to catch its misuse
   for devmem, then I am agreed with this generally. But the NULL returning
   handling in kcm_write_msgs() seems to suggest otherwise to me. Isn't it
   reasonable to make the semantic obvious by using WARN_ON or BUG_ON directly in
   skb_frag_page(), and returning NULL does not 100% reliably crash the thread as
   suggested by jason?

> 
> But perhaps best is to show with data. Is there a representative page
> pool benchmark that exercises the most sensitive case (XDP_DROP?) that
> we can run with and without a static branch to demonstrate that any
> diff is within the noise?
> 
>>> But none of this is related to correctness. Code calling
>>> skb_frag_page() will fail or crash if it's not handled correctly
>>> regardless of the implementation details of skb_frag_page(). In the
>>> devmem series we add support to handle it correctly via [1] & [2].
>>>
>>> --
>>> Thanks,
>>> Mina
>>
>>
>>
>> -- 
>> Thanks,
>> Mina
> 
> 
> 
> .
>