| Message ID | 20220713012621.2485047-1-joannelkoong@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v1] bpf: Fix bpf/sk_skb_pull_data for flags == 0 | expand |
On 7/13/22 3:26 AM, Joanne Koong wrote: > In the case where flags is 0, bpf_skb_pull_data and sk_skb_pull_data > should pull the entire skb payload including the bytes in the non-linear > page buffers. > > This is documented in the uapi: > "If a zero value is passed for *len*, then the whole length of the *skb* > is pulled" > > Fixes: 36bbef52c7eb6 ("bpf: direct packet write and access for helpers > for clsact progs") > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> This is not correct. We should fix the helper doc fa15601ab31e ("bpf: add documentation for eBPF helpers (33-41)"). It will make the head private for writing (e.g. for direct packet access), but not linearize the entire skb, so skb_headlen is correct here. > --- > net/core/filter.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 4ef77ec5255e..97eb15891bfc 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -1838,7 +1838,7 @@ BPF_CALL_2(bpf_skb_pull_data, struct sk_buff *, skb, u32, len) > * access case. By this we overcome limitations of only current > * headroom being accessible. > */ > - return bpf_try_make_writable(skb, len ? : skb_headlen(skb)); > + return bpf_try_make_writable(skb, len ? : skb->len); > } > > static const struct bpf_func_proto bpf_skb_pull_data_proto = { > @@ -1878,7 +1878,7 @@ BPF_CALL_2(sk_skb_pull_data, struct sk_buff *, skb, u32, len) > * access case. By this we overcome limitations of only current > * headroom being accessible. > */ > - return sk_skb_try_make_writable(skb, len ? : skb_headlen(skb)); > + return sk_skb_try_make_writable(skb, len ? : skb->len); > } > > static const struct bpf_func_proto sk_skb_pull_data_proto = { >
On Wed, Jul 13, 2022 at 12:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 7/13/22 3:26 AM, Joanne Koong wrote: > > In the case where flags is 0, bpf_skb_pull_data and sk_skb_pull_data > > should pull the entire skb payload including the bytes in the non-linear > > page buffers. > > > > This is documented in the uapi: > > "If a zero value is passed for *len*, then the whole length of the *skb* > > is pulled" > > > > Fixes: 36bbef52c7eb6 ("bpf: direct packet write and access for helpers > > for clsact progs") > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > > This is not correct. We should fix the helper doc fa15601ab31e ("bpf: add > documentation for eBPF helpers (33-41)"). It will make the head private > for writing (e.g. for direct packet access), but not linearize the entire > skb, so skb_headlen is correct here. > Great, I'll fix up the uapi doc. I'll change it to something like: "If a zero value is passed for *len*, then all bytes in the head of the skb will be made readable and writable". > > --- > > net/core/filter.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/core/filter.c b/net/core/filter.c > > index 4ef77ec5255e..97eb15891bfc 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -1838,7 +1838,7 @@ BPF_CALL_2(bpf_skb_pull_data, struct sk_buff *, skb, u32, len) > > * access case. By this we overcome limitations of only current > > * headroom being accessible. > > */ > > - return bpf_try_make_writable(skb, len ? : skb_headlen(skb)); > > + return bpf_try_make_writable(skb, len ? : skb->len); > > } > > > > static const struct bpf_func_proto bpf_skb_pull_data_proto = { > > @@ -1878,7 +1878,7 @@ BPF_CALL_2(sk_skb_pull_data, struct sk_buff *, skb, u32, len) > > * access case. By this we overcome limitations of only current > > * headroom being accessible. > > */ > > - return sk_skb_try_make_writable(skb, len ? : skb_headlen(skb)); > > + return sk_skb_try_make_writable(skb, len ? : skb->len); > > } > > > > static const struct bpf_func_proto sk_skb_pull_data_proto = { > > >
diff --git a/net/core/filter.c b/net/core/filter.c index 4ef77ec5255e..97eb15891bfc 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -1838,7 +1838,7 @@ BPF_CALL_2(bpf_skb_pull_data, struct sk_buff *, skb, u32, len) * access case. By this we overcome limitations of only current * headroom being accessible. */ - return bpf_try_make_writable(skb, len ? : skb_headlen(skb)); + return bpf_try_make_writable(skb, len ? : skb->len); } static const struct bpf_func_proto bpf_skb_pull_data_proto = { @@ -1878,7 +1878,7 @@ BPF_CALL_2(sk_skb_pull_data, struct sk_buff *, skb, u32, len) * access case. By this we overcome limitations of only current * headroom being accessible. */ - return sk_skb_try_make_writable(skb, len ? : skb_headlen(skb)); + return sk_skb_try_make_writable(skb, len ? : skb->len); } static const struct bpf_func_proto sk_skb_pull_data_proto = {
In the case where flags is 0, bpf_skb_pull_data and sk_skb_pull_data should pull the entire skb payload including the bytes in the non-linear page buffers. This is documented in the uapi: "If a zero value is passed for *len*, then the whole length of the *skb* is pulled" Fixes: 36bbef52c7eb6 ("bpf: direct packet write and access for helpers for clsact progs") Signed-off-by: Joanne Koong <joannelkoong@gmail.com> --- net/core/filter.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)