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: <552A5F1D-1A1A-4B4B-A88C-7D303A2D0B82@gmail.com>
Date:   Mon, 29 Jul 2019 14:37:49 -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 14:22, Jakub Kicinski wrote:

> On Mon, 29 Jul 2019 14:02:21 -0700, Jonathan Lemon wrote:
>> 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>
>
>>>> 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.
>
> Funny you should say that given that (a) I'm commenting on the new 
> code
> you're adding, and (b) you did do an unrelated spelling fix above ;)
>
>>>> + * @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.
>
> There is no need to ponder the connotations of verbs. Please just
> look at other function names in skbuff.h, especially those which
> copy fields :)
>
> static inline void skb_copy_hash(struct sk_buff *to, const struct 
> sk_buff *from)
> static inline void skb_copy_secmark(struct sk_buff *to, const struct 
> sk_buff *from)
> static inline void skb_copy_queue_mapping(struct sk_buff *to, const 
> struct sk_buff *from)
> static inline void skb_ext_copy(struct sk_buff *dst, const struct 
> sk_buff *src)

Okay, I missed those, let me respin.
-- 
Jonathan