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

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

Date:   Mon, 29 Jul 2019 14:22:11 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     "Jonathan Lemon" <jonathan.lemon@...il.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 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)