Re: [PATCH bpf-next v1 1/3] bpf: Add skb dynptrs

[Martin KaFai Lau <kafai@fb.com>]

On Mon, Aug 01, 2022 at 03:58:41PM -0700, Andrii Nakryiko wrote:
> On Mon, Aug 1, 2022 at 3:33 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Mon, Aug 01, 2022 at 02:16:23PM -0700, Joanne Koong wrote:
> > > On Mon, Aug 1, 2022 at 12:38 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > > >
> > > > On Mon, Aug 01, 2022 at 10:52:14AM -0700, Joanne Koong wrote:
> > > > > > Since we are on bpf_dynptr_write, what is the reason
> > > > > > on limiting it to the skb_headlen() ?  Not implying one
> > > > > > way is better than another.  would like to undertand the reason
> > > > > > behind it since it is not clear in the commit message.
> > > > > For bpf_dynptr_write, if we don't limit it to skb_headlen() then there
> > > > > may be writes that pull the skb, so any existing data slices to the
> > > > > skb must be invalidated. However, in the verifier we can't detect when
> > > > > the data slice should be invalidated vs. when it shouldn't (eg
> > > > > detecting when a write goes into the paged area vs when the write is
> > > > > only in the head). If the prog wants to write into the paged area, I
> > > > > think the only way it can work is if it pulls the data first with
> > > > > bpf_skb_pull_data before calling bpf_dynptr_write. I will add this to
> > > > > the commit message in v2
> > > > Note that current verifier unconditionally invalidates PTR_TO_PACKET
> > > > after bpf_skb_store_bytes().  Potentially the same could be done for
> > > > other new helper like bpf_dynptr_write().  I think this bpf_dynptr_write()
> > > > behavior cannot be changed later, so want to raise this possibility here
> > > > just in case it wasn't considered before.
> > >
> > > Thanks for raising this possibility. To me, it seems more intuitive
> > > from the user standpoint to have bpf_dynptr_write() on a paged area
> > > fail (even if bpf_dynptr_read() on that same offset succeeds) than to
> > > have bpf_dynptr_write() always invalidate all dynptr slices related to
> > > that skb. I think most writes will be to the data in the head area,
> > > which seems unfortunate that bpf_dynptr_writes to the head area would
> > > invalidate the dynptr slices regardless.
> > >
> > > What are your thoughts? Do you think you prefer having
> > > bpf_dynptr_write() always work regardless of where the data is? If so,
> > > I'm happy to make that change for v2 :)
> > Yeah, it sounds like an optimization to avoid unnecessarily
> > invalidating the sliced data.
> >
> > To be honest, I am not sure how often the dynptr_data()+dynptr_write() combo will
> > be used considering there is usually a pkt read before a pkt write in
> > the pkt modification use case.  If I got that far to have a sliced data pointer
> > to satisfy what I need for reading,  I would try to avoid making extra call
> > to dyptr_write() to modify it.
> >
> > I would prefer user can have similar expectation (no need to worry pkt layout)
> > between dynptr_read() and dynptr_write(), and also has similar experience to
> > the bpf_skb_load_bytes() and bpf_skb_store_bytes().  Otherwise, it is just
> > unnecessary rules for user to remember while there is no clear benefit on
> > the chance of this optimization.
> >
> 
> Are you saying that bpf_dynptr_read() shouldn't read from non-linear
> part of skb (and thus match more restrictive bpf_dynptr_write), or are
> you saying you'd rather have bpf_dynptr_write() write into non-linear
> part but invalidate bpf_dynptr_data() pointers?
The latter.  Read and write without worrying about the skb layout.

Also, if the prog needs to call a helper to write, it knows the bytes are
not in the data pointer.  Then it needs to bpf_skb_pull_data() before
it can call write.  However, after bpf_skb_pull_data(), why the prog
needs to call the write helper instead of directly getting a new
data pointer and write to it?  If the prog needs to write many many
bytes, a write helper may then help.

> 
> I guess I agree about consistency and that it seems like in practice
> you'd use bpf_dynptr_data() to work with headers and stuff like that
> at known locations, and then if you need to modify the rest of payload
> you'd do either bpf_skb_load_bytes()/bpf_skb_store_bytes() or
> bpf_dynptr_read()/bpf_dynptr_write() which would invalidate
> bpf_dynptr_data() pointers (but that would be ok by that time).
imo, read, write and then go back to read is less common.
writing bytes without first reading them is also less common.

> 
> 
> > I won't insist though.  User can always stay with the bpf_skb_load_bytes()
> > and bpf_skb_store_bytes() to avoid worrying about the skb layout.
> >
> > > >
> > > > Thinking from the existing bpf_skb_{load,store}_bytes() and skb->data perspective.
> > > > If the user changes the skb by directly using skb->data to avoid calling
> > > > load_bytes()/store_bytes(), the user will do the necessary bpf_skb_pull_data()
> > > > before reading/writing the skb->data.  If load_bytes()+store_bytes() is used instead,
> > > > it would be hard to reason why the earlier bpf_skb_load_bytes() can load a particular
> > > > byte but [may] need to make an extra bpf_skb_pull_data() call before it can use
> > > > bpf_skb_store_bytes() to store a modified byte at the same offset.