linux-kernel - Re: [PATCH net-next v3 11/13] net: replace page_frag with page_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: Mon, 13 May 2024 19:53:00 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Mat Martineau <martineau@...nel.org>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Alexander Duyck
	<alexander.duyck@...il.com>, Ayush Sawal <ayush.sawal@...lsio.com>, Eric
 Dumazet <edumazet@...gle.com>, Willem de Bruijn
	<willemdebruijn.kernel@...il.com>, Jason Wang <jasowang@...hat.com>, Ingo
 Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>, Juri Lelli
	<juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt
	<rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman
	<mgorman@...e.de>, Daniel Bristot de Oliveira <bristot@...hat.com>, Valentin
 Schneider <vschneid@...hat.com>, John Fastabend <john.fastabend@...il.com>,
	Jakub Sitnicki <jakub@...udflare.com>, David Ahern <dsahern@...nel.org>,
	Matthieu Baerts <matttbe@...nel.org>, Geliang Tang <geliang@...nel.org>,
	Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>,
	Jiri Pirko <jiri@...nulli.us>, Boris Pismenny <borisp@...dia.com>,
	<bpf@...r.kernel.org>, <mptcp@...ts.linux.dev>
Subject: Re: [PATCH net-next v3 11/13] net: replace page_frag with
 page_frag_cache

On 2024/5/11 1:29, Mat Martineau wrote:
> On Fri, 10 May 2024, Yunsheng Lin wrote:
> 
>> On 2024/5/10 0:22, Mat Martineau wrote:
>>> On Wed, 8 May 2024, Yunsheng Lin wrote:
>>>
>>>> Use the newly introduced prepare/probe/commit API to
>>>> replace page_frag with page_frag_cache for sk_page_frag().
>>>>
>>>> CC: Alexander Duyck <alexander.duyck@...il.com>
>>>> Signed-off-by: Yunsheng Lin <linyunsheng@...wei.com>
>>>> ---
>>>> .../chelsio/inline_crypto/chtls/chtls.h       |   3 -
>>>> .../chelsio/inline_crypto/chtls/chtls_io.c    | 100 ++++---------
>>>> .../chelsio/inline_crypto/chtls/chtls_main.c  |   3 -
>>>> drivers/net/tun.c                             |  28 ++--
>>>> include/linux/sched.h                         |   4 +-
>>>> include/net/sock.h                            |  14 +-
>>>> kernel/exit.c                                 |   3 +-
>>>> kernel/fork.c                                 |   3 +-
>>>> net/core/skbuff.c                             |  32 ++--
>>>> net/core/skmsg.c                              |  22 +--
>>>> net/core/sock.c                               |  46 ++++--
>>>> net/ipv4/ip_output.c                          |  33 +++--
>>>> net/ipv4/tcp.c                                |  35 ++---
>>>> net/ipv4/tcp_output.c                         |  28 ++--
>>>> net/ipv6/ip6_output.c                         |  33 +++--
>>>> net/kcm/kcmsock.c                             |  30 ++--
>>>> net/mptcp/protocol.c                          |  70 +++++----
>>>> net/sched/em_meta.c                           |   2 +-
>>>> net/tls/tls_device.c                          | 139 ++++++++++--------
>>>> 19 files changed, 331 insertions(+), 297 deletions(-)
>>>>
>>>
>>> <snip>
>>>
>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>> index bb8f96f2b86f..ab844011d442 100644
>>>> --- a/net/mptcp/protocol.c
>>>> +++ b/net/mptcp/protocol.c
>>>> @@ -960,17 +960,18 @@ static bool mptcp_skb_can_collapse_to(u64 write_seq,
>>>> }
>>>>
>>>> /* we can append data to the given data frag if:
>>>> - * - there is space available in the backing page_frag
>>>> - * - the data frag tail matches the current page_frag free offset
>>>> + * - there is space available for the current page
>>>> + * - the data frag tail matches the current page and offset
>>>>  * - the data frag end sequence number matches the current write seq
>>>>  */
>>>> static bool mptcp_frag_can_collapse_to(const struct mptcp_sock *msk,
>>>> -                       const struct page_frag *pfrag,
>>>> +                       const struct page *page,
>>>> +                       const unsigned int offset,
>>>> +                       const unsigned int size,
>>>
>>> Hi Yunsheng -
>>>
>>> Why add the 'size' parameter here? It's checked to be a nonzero value, but it can only be 0 if page is also NULL. In this case "page == df->page" will be false, so the function will return false even without checking 'size'.
>>
>> Is it possible that the pfrag->page is also NULL, which may cause
>> mptcp_frag_can_collapse_to() to return true?
> 
> Not sure. But I do know that df->page will never be NULL, so "page == df->page" will always be false when page == NULL.
> 
>>
>> I just found out that the 'size' is not set to zero when return
>> NULL for the implementation of probe API for the current version.
>> Perhaps it makes more sense to expect the API caller to make sure
>> the the returned 'page' not being NULL before using the 'offset',
>> 'size' and 'va', like below:
>>
>> df && page && page == df->page
>>
> 
> Given that df->page is never NULL, I don't think the extra "&& page" is needed.

Not checking the extra "&& page" seems to cause the below warning, it seems we
have the below options:
1. ignore the warning.
2. set offset to zero if there is no enough space when probe API asks for a specific
   amount of available space as you suggested.
3. add the "&& page" in mptcp_frag_can_collapse_to()

what is your favour option? or any other better option?

net-mptcp-protocol.c:warning:variable-offset-is-used-uninitialized-whenever-if-condition-is-false

> 
> - Mat
> .
>