linux-kernel - Re: [PATCH net-next v19 09/14] net: rename skb_copy_to_page

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]

Message-ID: <c9860411-fa9c-4e1b-bca2-a10e6737f9b0@gmail.com>
Date: Sat, 5 Oct 2024 21:44:31 +0800
From: Yunsheng Lin <yunshenglin0825@...il.com>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Yunsheng Lin <linyunsheng@...wei.com>, Eric Dumazet <edumazet@...gle.com>,
 David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH net-next v19 09/14] net: rename skb_copy_to_page_nocache()
 helper

On 10/4/2024 11:00 AM, Alexander Duyck wrote:
> On Tue, Oct 1, 2024 at 12:59 AM Yunsheng Lin <yunshenglin0825@...il.com> wrote:
>>
>> Rename skb_copy_to_page_nocache() to skb_copy_to_va_nocache()
>> to avoid calling virt_to_page() as we are about to pass virtual
>> address directly.
>>
>> CC: Alexander Duyck <alexander.duyck@...il.com>
>> Signed-off-by: Yunsheng Lin <linyunsheng@...wei.com>
>> ---
>>   include/net/sock.h | 10 ++++------
>>   net/ipv4/tcp.c     |  7 +++----
>>   net/kcm/kcmsock.c  |  7 +++----
>>   3 files changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index c58ca8dd561b..7d0b606d6251 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -2185,15 +2185,13 @@ static inline int skb_add_data_nocache(struct sock *sk, struct sk_buff *skb,
>>          return err;
>>   }
>>
>> -static inline int skb_copy_to_page_nocache(struct sock *sk, struct iov_iter *from,
>> -                                          struct sk_buff *skb,
>> -                                          struct page *page,
>> -                                          int off, int copy)
>> +static inline int skb_copy_to_va_nocache(struct sock *sk, struct iov_iter *from,
>> +                                        struct sk_buff *skb, char *va,
>> +                                        int copy)
>>   {
> 
> This new naming is kind of confusing. Currently the only other
> "skb_copy_to" functions are skb_copy_to_linear_data and
> skb_copy_to_linear_data_offset. The naming before basically indicated

I am not sure if the above "skb_copy_to" functions are really related
here, as they are in include/linux/skbuff.h and don't take '*sk' as
first input param.

As "skb_copy_to" function in include/net/sock.h does take '*sk' as first
input param, perhaps the "skb_copy_to" functions in include/net/sock.h
can be renamed to "sk_skb_copy_to" in the future as most of functions
do in include/net/sock.h

> which part of the skb the data was being copied into. So before we
> were copying into the "page" frags. With the new naming this function
> is much less clear as technically the linear data can also be a
> virtual address.

I guess it is ok to use it for linear data if there is a need, why
invent another function for the linear data when both linear data and
non-linear data can be used as a virtual address?

> 
> I would recommend maybe replacing "va" with "frag", "page_frag" or
> maybe "pfrag" as what we are doing is copying the data to one of the
> pages in the paged frags section of the skb before they are added to
> the skb itself.

Don't "frag", "page_frag" or "pfrag" also seem confusing enough that
it does not take any 'frag' as the input param?

Does skb_copy_data() make more sense here as it can work on both
linear and non-linear data, as skb_do_copy_data_nocache() and
skb_copy_to_page_nocache() in the same header file seem to have a
similar style?