netdev - Re: [RFC net-next 2/3] skbuff: rewrite the doc for data-only skbs

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] [day] [month] [year] [list]

Message-ID: <20220324113114.3d3e6ba8@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 24 Mar 2022 11:31:14 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, corbet@....net,
        imagedong@...cent.com, edumazet@...gle.com, dsahern@...nel.org,
        talalahmad@...gle.com, linux-doc@...r.kernel.org
Subject: Re: [RFC net-next 2/3] skbuff: rewrite the doc for data-only skbs

On Thu, 24 Mar 2022 09:50:06 +0100 Paolo Abeni wrote:
> > +/**
> > + * DOC: dataref and headerless skbs
> > + *
> > + * Transport layers send out clones of data skbs they hold for retransmissions.
> > + * To allow lower layers of the stack to prepend their headers
> > + * we split &skb_shared_info.dataref into two halves.
> > + * The lower 16 bits count the overall number of references.
> > + * The higher 16 bits indicate number of data-only references.
> > + * skb_header_cloned() checks if skb is allowed to add / write the headers.  
> 
> Thank you very much for the IMHO much needed documentation!
> 
> Please allow me to do some non-native-english-speaker biased comments;)
> 
> The previous patch uses the form  "payload data" instead of data-only,
> I think it would be clearer using consistently one or the other. I
> personally would go for "payload-data-only" (which is probably a good
> reason to pick a different option).

That starts to get long. Let me go for payload everywhere.

> > - * All users must obey the rule that the skb->data reference count must be
> > - * greater than or equal to the payload reference count.
> > + * The creator of the skb (e.g. TCP) marks its data-only skb as &sk_buff.nohdr
> > + * (via __skb_header_release()). Any clone created from marked skb will get
> > + * &sk_buff.hdr_len populated with the available headroom.
> > + * If it's the only clone in existence it's able to modify the headroom at will.  
> 
> I think it would be great if we explicitly list the expected sequence,
> e.g.
> 	<alloc skb>
> 	skb_reserve
> 	__skb_header_release
> 	skb_clone

Will do!