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

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 3 Aug 2022 18:05:44 -0700 Joanne Koong wrote:
> I think the problem is that the skb may be cloned, so a write into any
> portion of the paged data requires a pull. If it weren't for this,
> then we could do the write and checksumming without pulling (eg kmap
> the page, get the csum_partial of the bytes you'll write over, do the
> write, get the csum_partial of the bytes you just wrote, then unkmap,
> then update skb->csum to be skb->csum - csum of the bytes you wrote
> over + csum of the bytes you wrote). I think we would even be able to
> provide a direct data slice to non-contiguous pages without needing
> the additional copy to a stack buffer (eg kmap the non-contiguous
> pages to a contiguous virtual address that we pass back to the bpf
> program, and then when the bpf program is finished do the cleanup for
> the mappings).

The whole read/write/data concept is not a great match for packet
parsing. Primary use for packet parsing is that you want to read
a header and not have to deal with frags or pulling. In that case
you should get a direct pointer or a copy on the stack, transparently.

Maybe before I go on talking nonsense I should read up on what dynptr
is and what it's trying to achieve. Stan says its like unique_ptr in
C++ which tells me.. nothing :)

$ git grep dynptr -- Documentation/
$

Any pointers?

> Three ideas I'm thinking through as a possible solution:
> 1) Enforce that the skb is always uncloned for skb-type bpf progs (we
> currently do this just for the skb head, see bpf_unclone_prologue()),
> but I'm not sure if the trade-off (pulling all the packet data, even
> if it won't be used) is acceptable.
> 
> 2) Don't support cloned skbs for bpf_dynptr_write/data and don't do
> any pulling. If the prog wants to use bpf_dynptr_write/data, then they
> have to pull it first

I think all output skbs from TCP are cloned, so that's not gonna work.

> 2) (uglier than #1 and #2) For bpf_dynptr_write()s, pull if the write
> is to a paged area and the skb is cloned, otherwise write to the paged
> area without pulling; if we do this, then we always have to invalidate
> all data slices associated with the skb (even for writes to the head)
> since at prog load time, the verifier doesn't know if the pull happens
> or not. For bpf_dynptr_data()s, follow the same policy.
> 
> I'm leaning towards 2. What are your thoughts?