netdev - Re: [PATCH 1/3 net-next] linux: Add skb_frag_t 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: <932D725D-62F1-47D6-807A-45F81E66B1C6@gmail.com>
Date:   Mon, 29 Jul 2019 14:02:21 -0700
From:   "Jonathan Lemon" <jonathan.lemon@...il.com>
To:     "Jakub Kicinski" <jakub.kicinski@...ronome.com>
Cc:     davem@...emloft.net, kernel-team@...com, netdev@...r.kernel.org,
        "Matthew Wilcox" <willy@...radead.org>
Subject: Re: [PATCH 1/3 net-next] linux: Add skb_frag_t page_offset accessors



On 29 Jul 2019, at 13:50, Jakub Kicinski wrote:

> On Mon, 29 Jul 2019 10:19:39 -0700, Jonathan Lemon wrote:
>> Add skb_frag_off(), skb_frag_off_add(), skb_frag_off_set(),
>> and skb_frag_off_set_from() accessors for page_offset.
>>
>> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>
>> ---
>>  include/linux/skbuff.h | 61 
>> ++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 56 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 718742b1c505..7d94a78067ee 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -331,7 +331,7 @@ static inline void skb_frag_size_set(skb_frag_t 
>> *frag, unsigned int size)
>>  }
>>
>>  /**
>> - * skb_frag_size_add - Incrementes the size of a skb fragment by 
>> %delta
>> + * skb_frag_size_add - Increments the size of a skb fragment by 
>> %delta
>>   * @frag: skb fragment
>>   * @delta: value to add
>>   */
>> @@ -2857,6 +2857,46 @@ static inline void 
>> skb_propagate_pfmemalloc(struct page *page,
>>  		skb->pfmemalloc = true;
>>  }
>>
>> +/**
>> + * skb_frag_off - Returns the offset of a skb fragment
>> + * @frag: the paged fragment
>> + */
>> +static inline unsigned int skb_frag_off(const skb_frag_t *frag)
>> +{
>> +	return frag->page_offset;
>> +}
>> +
>> +/**
>> + * skb_frag_off_add - Increments the offset of a skb fragment by 
>> %delta
>
> I realize you're following the existing code, but should we perhaps 
> use
> the latest kdoc syntax? '()' after function name, and args should have
> '@' prefix, '%' would be for constants.

That would be a task for a different cleanup.  Not that I disagree with 
you,
but there's also nothing worse than mixing styles in the same file.



>> + * @frag: skb fragment
>> + * @delta: value to add
>> + */
>> +static inline void skb_frag_off_add(skb_frag_t *frag, int delta)
>> +{
>> +	frag->page_offset += delta;
>> +}
>> +
>> +/**
>> + * skb_frag_off_set - Sets the offset of a skb fragment
>> + * @frag: skb fragment
>> + * @offset: offset of fragment
>> + */
>> +static inline void skb_frag_off_set(skb_frag_t *frag, unsigned int 
>> offset)
>> +{
>> +	frag->page_offset = offset;
>> +}
>> +
>> +/**
>> + * skb_frag_off_set_from - Sets the offset of a skb fragment from 
>> another fragment
>> + * @fragto: skb fragment where offset is set
>> + * @fragfrom: skb fragment offset is copied from
>> + */
>> +static inline void skb_frag_off_set_from(skb_frag_t *fragto,
>> +					 const skb_frag_t *fragfrom)
>
> skb_frag_off_copy() ?

That was my initial inclination, but due to the often overloaded
connotations of the word "copy", opted to use the same "set" verbiage
that existed in the other functions.


>> +{
>> +	fragto->page_offset = fragfrom->page_offset;
>> +}
>> +
>>  /**
>>   * skb_frag_page - retrieve the page referred to by a paged fragment
>>   * @frag: the paged fragment
>> @@ -2923,7 +2963,7 @@ static inline void skb_frag_unref(struct 
>> sk_buff *skb, int f)
>>   */
>>  static inline void *skb_frag_address(const skb_frag_t *frag)
>>  {
>> -	return page_address(skb_frag_page(frag)) + frag->page_offset;
>> +	return page_address(skb_frag_page(frag)) + skb_frag_off(frag);
>>  }
>>
>>  /**
>> @@ -2939,7 +2979,18 @@ static inline void 
>> *skb_frag_address_safe(const skb_frag_t *frag)
>>  	if (unlikely(!ptr))
>>  		return NULL;
>>
>> -	return ptr + frag->page_offset;
>> +	return ptr + skb_frag_off(frag);
>> +}
>> +
>> +/**
>> + * skb_frag_page_set_from - sets the page in a fragment from another 
>> fragment
>
> skb_frag_page_copy() ?

Same reasoning as above.



>> + * @fragto: skb fragment where page is set
>> + * @fragfrom: skb fragment page is copied from
>> + */
>> +static inline void skb_frag_page_set_from(skb_frag_t *fragto,
>> +					  const skb_frag_t *fragfrom)
>> +{
>> +	fragto->bv_page = fragfrom->bv_page;
>>  }
>>
>>  /**