Re: [PATCH net-next v3 11/13] net: replace page_frag with page_frag_cache

[Mat Martineau <martineau@kernel.org>]

On Mon, 13 May 2024, Yunsheng Lin wrote:

> 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@gmail.com>
>>>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.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
>

Hi Yunsheng -

That static analyzer is correct that "offset" is *passed* uninitialized in 
that scenario, but it doesn't recognize that "offset" is never compared 
when page == NULL. So, it's a false positive in a way.

I don't think implementing fix #2 in the page_frag_alloc_probe() macro is 
best, since the warning is specific to the MPTCP code and other 
page_frag_cache users may have reasons to choose nonzero default values 
for the offset. I suggest initializing offset = 0 where it is declared in 
mptcp_sendmsg().


- Mat