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

From: Jakub Kicinski <kuba@kernel.org>
To: Martin KaFai Lau <kafai@fb.com>
Cc: Joanne Koong <joannelkoong@gmail.com>, bpf <bpf@vger.kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Alexei Starovoitov <ast@kernel.org>
Subject: Re: [PATCH bpf-next v1 1/3] bpf: Add skb dynptrs
Date: Wed, 3 Aug 2022 18:44:18 -0700	[thread overview]
Message-ID: <20220803184418.38f9ed42@kernel.org> (raw)
In-Reply-To: <20220804012756.2eqvkofecpthzcoi@kafai-mbp.dhcp.thefacebook.com>

On Wed, 3 Aug 2022 18:27:56 -0700 Martin KaFai Lau wrote:
> On Wed, Aug 03, 2022 at 04:25:40PM -0700, Jakub Kicinski wrote:
> > The point of skb_header_pointer() is to expose the chunk of the packet
> > pointed to by [skb, offset, len] as a linear buffer. Potentially coping
> > it out to a stack buffer *IIF* the header is not contiguous inside the
> > skb head, which should very rarely happen.
> > 
> > Here it seems we return an error so that user must pull if the data is
> > not linear, which is defeating the purpose. The user of
> > skb_header_pointer() wants to avoid the copy while _reliably_ getting 
> > a contiguous pointer. Plus pulling in the header may be far more
> > expensive than a small copy to the stack.
> > 
> > The pointer returned by skb_header_pointer is writable, but it's not
> > guaranteed that the writes go to the packet, they may go to the
> > on-stack buffer, so the caller must do some sort of:
> > 
> > 	if (data_ptr == stack_buf)
> > 		skb_store_bits(...);
> > 
> > Which we were thinking of wrapping in some sort of flush operation.  
> Curious on the idea.  don't know whether this is a dynptr helper or
> should be a specific pkt helper though.

Yeah, I could well pattern matched the dynptr because it sounded
similar but it's a completely different beast.

> The idea is to have the prog keeps writing to a ptr (skb->data or stack_buf).

To be clear writing is a lot more rare than reading in this case.

> When the prog is done, call a bpf helper to flush.  The helper
> decides if it needs to flush from stack_buf to skb and
> will take care of the cloned skb ?

Yeah, I'd think for skb it'd just pull. Normally dealing with skbs
you'd indeed probably just pull upfront if you knew you're gonna write.
Hence saving yourself from the unnecessary trip thru the stack. But XDP
does not have strong pulling support, so if the interface must support
both then it's the lower common denominator.

> > If I'm reading this right dynptr as implemented here do not provide
> > such semantics, am I confused in thinking that this is a continuation
> > of the XDP multi-buff discussion? Is it a completely separate thing
> > and we'll still need a header_pointer like helper?  
> Can you share a pointer to the XDP multi-buff discussion?

https://lore.kernel.org/all/20210916095539.4696ae27@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/