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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 10 May 2024 17:48:52 +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/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?

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