netdev - Re: [PATCH v9 bpf-next 3/5] bpf: Add skb dynptrs

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

Message-ID: <20230201004034.sea642affpiu7yfm@macbook-pro-6.dhcp.thefacebook.com>
Date:   Tue, 31 Jan 2023 16:40:34 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Martin KaFai Lau <martin.lau@...ux.dev>,
        Joanne Koong <joannelkoong@...il.com>, daniel@...earbox.net,
        andrii@...nel.org, martin.lau@...nel.org, ast@...nel.org,
        netdev@...r.kernel.org, memxor@...il.com, kernel-team@...com,
        bpf@...r.kernel.org
Subject: Re: [PATCH v9 bpf-next 3/5] bpf: Add skb dynptrs

On Tue, Jan 31, 2023 at 04:11:47PM -0800, Andrii Nakryiko wrote:
> >
> > When prog is just parsing the packet it doesn't need to finalize with bpf_dynptr_write.
> > The prog can always write into the pointer followed by if (p == buf) bpf_dynptr_write.
> > No need for rdonly flag, but extra copy is there in case of cloned which
> > could have been avoided with extra rd_only flag.
> 
> Yep, given we are designing bpf_dynptr_slice for performance, extra
> copy on reads is unfortunate. ro/rw flag or have separate
> bpf_dynptr_slice_rw vs bpf_dynptr_slice_ro?

Either flag or two kfuncs sound good to me.

> > Yes and No. bpf_skb_store_bytes is doing pull followed by memcpy,
> > while xdp_store_bytes does scatter gather copy into frags.
> > We should probably add similar copy to skb case to avoid allocations and pull.
> > Then in case of:
> >  if (p == buf) {
> >       bpf_dynptr_write(dp, buf, 16);
> >  }
> >
> > the write will guarantee to succeed for both xdp and skb and the user
> > doesn't need to add error checking for alloc failures in case of skb.
> >
> 
> That seems like a nice guarantee, agreed.

Just grepped through few projects that use skb_store_bytes.
Everywhere it looks like:
if (bpf_skb_store_byte(...))
   return error;

Not a pretty code to read.
I should prioritize bpf_assert() work, so we can assert from inside of
bpf_dynptr_write() eventually and remove all these IFs.

> > > >
> > > > > But I wonder if for simple cases when users are mostly sure that they
> > > > > are going to access only header data directly we can have an option
> > > > > for bpf_dynptr_from_skb() to specify what should be the behavior for
> > > > > bpf_dynptr_slice():
> > > > >
> > > > >   - either return NULL for anything that crosses into frags (no
> > > > > surprising perf penalty, but surprising NULLs);
> > > > >   - do bpf_skb_pull_data() if bpf_dynptr_data() needs to point to data
> > > > > beyond header (potential perf penalty, but on NULLs, if off+len is
> > > > > within packet).
> > > > >
> > > > > And then bpf_dynptr_from_skb() can accept a flag specifying this
> > > > > behavior and store it somewhere in struct bpf_dynptr.
> > > >
> > > > xdp does not have the bpf_skb_pull_data() equivalent, so xdp prog will still
> > > > need the write back handling.
> > > >
> > >
> > > Sure, unfortunately, can't have everything. I'm just thinking how to
> > > make bpf_dynptr_data() generically usable. Think about some common BPF
> > > routine that calculates hash for all bytes pointed to by dynptr,
> > > regardless of underlying dynptr type; it can iterate in small chunks,
> > > get memory slice, if possible, but fallback to generic
> > > bpf_dynptr_read() if doesn't. This will work for skb, xdp, LOCAL,
> > > RINGBUF, any other dynptr type.
> >
> > It looks to me that dynptr on top of skb, xdp, local can work as generic reader,
> > but dynptr as a generic writer doesn't look possible.
> > BPF_F_RECOMPUTE_CSUM and BPF_F_INVALIDATE_HASH are special to skb.
> > There is also bpf_skb_change_proto and crazy complex bpf_skb_adjust_room.
> > I don't think writing into skb vs xdp vs ringbuf are generalizable.
> > The prog needs to do a ton more work to write into skb correctly.
> 
> If that's the case, then yeah, bpf_dynptr_write() can just return
> error for skb/xdp dynptrs?

You mean to error when these skb only flags are present, but dynptr->type == xdp ?
Yep. I don't see another option. My point was that dynptr doesn't quite work as an
abstraction for writing into networking things.
While libraries like: parse_http(&dynptr), compute_hash(&dynptr), find_string(&dynptr)
can indeed be generic and work with raw bytes, skb, xdp as an input,
which I think was on top of your wishlist for dynptr.