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: <20230201004647.jyesy2dqqx2a7ytb@macbook-pro-6.dhcp.thefacebook.com>
Date:   Tue, 31 Jan 2023 16:46:47 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Joanne Koong <joannelkoong@...il.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        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 03:17:08PM -0800, Joanne Koong wrote:
> >
> > It's not clear how to deal with BPF_F_RECOMPUTE_CSUM though.
> > Expose __skb_postpull_rcsum/__skb_postpush_rcsum as kfuncs?
> > But that defeats Andrii's goal to use dynptr as a generic wrapper.
> > skb is quite special.
> 
> If it's the common case that skbs use the same flag across writes in
> their bpf prog, then we can have bpf_dynptr_from_skb take in
> BPF_F_RECOMPUTE_CSUM/BPF_F_INVALIDATE_HASH in its flags arg and then
> always apply this when the skb does a write to packet data.

Remembering these flags at creation of dynptr is an interesting idea,
but it doesn't help with direct write into ptr returned from bpf_dynptr_slice.
The __skb_postpull_rcsum needs to be done before the write and
__skb_postpush_rcsum after the write.

> >
> > Maybe something like:
> > void *bpf_dynptr_slice(const struct bpf_dynptr *ptr, u32 offset, u32 len,
> >                        void *buffer, u32 buffer__sz)
> > {
> >   if (skb_cloned()) {
> >     skb_copy_bits(skb, offset, buffer, len);
> >     return buffer;
> >   }
> >   return skb_header_pointer(...);
> > }
> >
> > 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.
> 
> We're able to track in the verifier whether the slice gets written to
> or not, so if it does get written to in the skb case, can't we just
> add in a call to bpf_try_make_writable() as a post-processing fixup
> that gets called before bpf_dynptr_slice? Then bpf_dynptr_slice() can
> just return a directly writable ptr and avoid the extra memcpy

It's doable, but bpf_try_make_writable can fail and it's much slower than memcpy.
I'm not sure what you're optimizing here.