| Message ID | 20210126183559.1302406-2-revest@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v6,1/5] bpf: Be less specific about socket cookies guarantees | expand |
On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote: > > This needs a new helper that: > - can work in a sleepable context (using sock_gen_cookie) > - takes a struct sock pointer and checks that it's not NULL > > Signed-off-by: Florent Revest <revest@chromium.org> > Acked-by: KP Singh <kpsingh@kernel.org> > --- > include/linux/bpf.h | 1 + > include/uapi/linux/bpf.h | 8 ++++++++ > kernel/trace/bpf_trace.c | 2 ++ > net/core/filter.c | 12 ++++++++++++ > tools/include/uapi/linux/bpf.h | 8 ++++++++ > 5 files changed, 31 insertions(+) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 1aac2af12fed..26219465e1f7 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto; > extern const struct bpf_func_proto bpf_this_cpu_ptr_proto; > extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto; > extern const struct bpf_func_proto bpf_sock_from_file_proto; > +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto; > > const struct bpf_func_proto *bpf_tracing_func_proto( > enum bpf_func_id func_id, const struct bpf_prog *prog); > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 0b735c2729b2..5855c398d685 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -1673,6 +1673,14 @@ union bpf_attr { > * Return > * A 8-byte long unique number. > * > + * u64 bpf_get_socket_cookie(void *sk) should the type be `struct sock *` then? > + * Description > + * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts > + * *sk*, but gets socket from a BTF **struct sock**. This helper > + * also works for sleepable programs. > + * Return > + * A 8-byte long unique number or 0 if *sk* is NULL. > + * > * u32 bpf_get_socket_uid(struct sk_buff *skb) > * Return > * The owner UID of the socket associated to *skb*. If the socket > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 6c0018abe68a..845b2168e006 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -1760,6 +1760,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_sk_storage_delete_tracing_proto; > case BPF_FUNC_sock_from_file: > return &bpf_sock_from_file_proto; > + case BPF_FUNC_get_socket_cookie: > + return &bpf_get_socket_ptr_cookie_proto; > #endif > case BPF_FUNC_seq_printf: > return prog->expected_attach_type == BPF_TRACE_ITER ? > diff --git a/net/core/filter.c b/net/core/filter.c > index 9ab94e90d660..606e2b6115ed 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -4631,6 +4631,18 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = { > .arg1_type = ARG_PTR_TO_CTX, > }; > > +BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk) > +{ > + return sk ? sock_gen_cookie(sk) : 0; > +} > + > +const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = { > + .func = bpf_get_socket_ptr_cookie, > + .gpl_only = false, > + .ret_type = RET_INTEGER, > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, > +}; > + > BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx) > { > return __sock_gen_cookie(ctx->sk); > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index 0b735c2729b2..5855c398d685 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -1673,6 +1673,14 @@ union bpf_attr { > * Return > * A 8-byte long unique number. > * > + * u64 bpf_get_socket_cookie(void *sk) > + * Description > + * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts > + * *sk*, but gets socket from a BTF **struct sock**. This helper > + * also works for sleepable programs. > + * Return > + * A 8-byte long unique number or 0 if *sk* is NULL. > + * > * u32 bpf_get_socket_uid(struct sk_buff *skb) > * Return > * The owner UID of the socket associated to *skb*. If the socket > -- > 2.30.0.280.ga3ce27912f-goog >
On Wed, Jan 27, 2021 at 10:01 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote: > > > > This needs a new helper that: > > - can work in a sleepable context (using sock_gen_cookie) > > - takes a struct sock pointer and checks that it's not NULL > > > > Signed-off-by: Florent Revest <revest@chromium.org> > > Acked-by: KP Singh <kpsingh@kernel.org> > > --- > > include/linux/bpf.h | 1 + > > include/uapi/linux/bpf.h | 8 ++++++++ > > kernel/trace/bpf_trace.c | 2 ++ > > net/core/filter.c | 12 ++++++++++++ > > tools/include/uapi/linux/bpf.h | 8 ++++++++ > > 5 files changed, 31 insertions(+) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 1aac2af12fed..26219465e1f7 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto; > > extern const struct bpf_func_proto bpf_this_cpu_ptr_proto; > > extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto; > > extern const struct bpf_func_proto bpf_sock_from_file_proto; > > +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto; > > > > const struct bpf_func_proto *bpf_tracing_func_proto( > > enum bpf_func_id func_id, const struct bpf_prog *prog); > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index 0b735c2729b2..5855c398d685 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -1673,6 +1673,14 @@ union bpf_attr { > > * Return > > * A 8-byte long unique number. > > * > > + * u64 bpf_get_socket_cookie(void *sk) > > should the type be `struct sock *` then? Oh, absolutely. :) Thank you Andrii
On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 1/29/21 11:57 AM, Daniel Borkmann wrote: > > On 1/27/21 10:01 PM, Andrii Nakryiko wrote: > >> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote: > >>> > >>> This needs a new helper that: > >>> - can work in a sleepable context (using sock_gen_cookie) > >>> - takes a struct sock pointer and checks that it's not NULL > >>> > >>> Signed-off-by: Florent Revest <revest@chromium.org> > >>> Acked-by: KP Singh <kpsingh@kernel.org> > >>> --- > >>> include/linux/bpf.h | 1 + > >>> include/uapi/linux/bpf.h | 8 ++++++++ > >>> kernel/trace/bpf_trace.c | 2 ++ > >>> net/core/filter.c | 12 ++++++++++++ > >>> tools/include/uapi/linux/bpf.h | 8 ++++++++ > >>> 5 files changed, 31 insertions(+) > >>> > >>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h > >>> index 1aac2af12fed..26219465e1f7 100644 > >>> --- a/include/linux/bpf.h > >>> +++ b/include/linux/bpf.h > >>> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto; > >>> extern const struct bpf_func_proto bpf_this_cpu_ptr_proto; > >>> extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto; > >>> extern const struct bpf_func_proto bpf_sock_from_file_proto; > >>> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto; > >>> > >>> const struct bpf_func_proto *bpf_tracing_func_proto( > >>> enum bpf_func_id func_id, const struct bpf_prog *prog); > >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > >>> index 0b735c2729b2..5855c398d685 100644 > >>> --- a/include/uapi/linux/bpf.h > >>> +++ b/include/uapi/linux/bpf.h > >>> @@ -1673,6 +1673,14 @@ union bpf_attr { > >>> * Return > >>> * A 8-byte long unique number. > >>> * > >>> + * u64 bpf_get_socket_cookie(void *sk) > >> > >> should the type be `struct sock *` then? > > > > Checking libbpf's generated bpf_helper_defs.h it generates: > > > > /* > > * bpf_get_socket_cookie > > * > > * If the **struct sk_buff** pointed by *skb* has a known socket, > > * retrieve the cookie (generated by the kernel) of this socket. > > * If no cookie has been set yet, generate a new cookie. Once > > * generated, the socket cookie remains stable for the life of the > > * socket. This helper can be useful for monitoring per socket > > * networking traffic statistics as it provides a global socket > > * identifier that can be assumed unique. > > * > > * Returns > > * A 8-byte long non-decreasing number on success, or 0 if the > > * socket field is missing inside *skb*. > > */ > > static __u64 (*bpf_get_socket_cookie)(void *ctx) = (void *) 46; > > > > So in terms of helper comment it's picking up the description from the > > `u64 bpf_get_socket_cookie(struct sk_buff *skb)` signature. With that > > in mind it would likely make sense to add the actual `struct sock *` type > > to the comment to make it more clear in here. > > One thought that still came to mind when looking over the series again, do > we need to blacklist certain functions from bpf_get_socket_cookie() under > tracing e.g. when attaching to, say fexit? For example, if sk_prot_free() > would be temporary uninlined/exported for testing and bpf_get_socket_cookie() > was invoked from a prog upon fexit where sock was already passed back to > allocator, I presume there's risk of mem corruption, no? Mh, this is interesting. I can try to add a deny list in v7 but I'm not sure whether I'll be able to catch them all. I'm assuming that __sk_destruct, sk_destruct, __sk_free, sk_free would be other problematic functions but potentially there would be more.
On 1/30/21 12:45 PM, Florent Revest wrote: > On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 1/29/21 11:57 AM, Daniel Borkmann wrote: >>> On 1/27/21 10:01 PM, Andrii Nakryiko wrote: >>>> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote: >>>>> >>>>> This needs a new helper that: >>>>> - can work in a sleepable context (using sock_gen_cookie) >>>>> - takes a struct sock pointer and checks that it's not NULL >>>>> >>>>> Signed-off-by: Florent Revest <revest@chromium.org> >>>>> Acked-by: KP Singh <kpsingh@kernel.org> >>>>> --- >>>>> include/linux/bpf.h | 1 + >>>>> include/uapi/linux/bpf.h | 8 ++++++++ >>>>> kernel/trace/bpf_trace.c | 2 ++ >>>>> net/core/filter.c | 12 ++++++++++++ >>>>> tools/include/uapi/linux/bpf.h | 8 ++++++++ >>>>> 5 files changed, 31 insertions(+) >>>>> >>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >>>>> index 1aac2af12fed..26219465e1f7 100644 >>>>> --- a/include/linux/bpf.h >>>>> +++ b/include/linux/bpf.h >>>>> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto; >>>>> extern const struct bpf_func_proto bpf_this_cpu_ptr_proto; >>>>> extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto; >>>>> extern const struct bpf_func_proto bpf_sock_from_file_proto; >>>>> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto; >>>>> >>>>> const struct bpf_func_proto *bpf_tracing_func_proto( >>>>> enum bpf_func_id func_id, const struct bpf_prog *prog); >>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >>>>> index 0b735c2729b2..5855c398d685 100644 >>>>> --- a/include/uapi/linux/bpf.h >>>>> +++ b/include/uapi/linux/bpf.h >>>>> @@ -1673,6 +1673,14 @@ union bpf_attr { >>>>> * Return >>>>> * A 8-byte long unique number. >>>>> * >>>>> + * u64 bpf_get_socket_cookie(void *sk) >>>> >>>> should the type be `struct sock *` then? >>> >>> Checking libbpf's generated bpf_helper_defs.h it generates: >>> >>> /* >>> * bpf_get_socket_cookie >>> * >>> * If the **struct sk_buff** pointed by *skb* has a known socket, >>> * retrieve the cookie (generated by the kernel) of this socket. >>> * If no cookie has been set yet, generate a new cookie. Once >>> * generated, the socket cookie remains stable for the life of the >>> * socket. This helper can be useful for monitoring per socket >>> * networking traffic statistics as it provides a global socket >>> * identifier that can be assumed unique. >>> * >>> * Returns >>> * A 8-byte long non-decreasing number on success, or 0 if the >>> * socket field is missing inside *skb*. >>> */ >>> static __u64 (*bpf_get_socket_cookie)(void *ctx) = (void *) 46; >>> >>> So in terms of helper comment it's picking up the description from the >>> `u64 bpf_get_socket_cookie(struct sk_buff *skb)` signature. With that >>> in mind it would likely make sense to add the actual `struct sock *` type >>> to the comment to make it more clear in here. >> >> One thought that still came to mind when looking over the series again, do >> we need to blacklist certain functions from bpf_get_socket_cookie() under >> tracing e.g. when attaching to, say fexit? For example, if sk_prot_free() >> would be temporary uninlined/exported for testing and bpf_get_socket_cookie() >> was invoked from a prog upon fexit where sock was already passed back to >> allocator, I presume there's risk of mem corruption, no? > > Mh, this is interesting. I can try to add a deny list in v7 but I'm > not sure whether I'll be able to catch them all. I'm assuming that > __sk_destruct, sk_destruct, __sk_free, sk_free would be other > problematic functions but potentially there would be more. I was just looking at bpf_skb_output() from a7658e1a4164 ("bpf: Check types of arguments passed into helpers") which afaiu has similar issue, back at the time this was introduced there was no fentry/fexit but rather raw tp progs, so you could still safely dump skb this way including for kfree_skb() tp. Presumably if you bpf_skb_output() at 'fexit/kfree_skb' you might be able to similarly crash your kernel which I don't think is intentional (also given we go above and beyond in other areas to avoid crashing or destabilizing e.g. [0] to mention one). Maybe these should really only be for BPF_TRACE_RAW_TP (or BPF_PROG_TYPE_LSM) where it can be audited that it's safe to use like a7658e1a4164's original intention ... or have some sort of function annotation like __acquires/__releases but for tracing e.g. __frees(skb) where use would then be blocked (not sure iff feasible). [0] https://lore.kernel.org/bpf/20210126001219.845816-1-yhs@fb.com/
On Mon, Feb 1, 2021 at 2:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 1/30/21 12:45 PM, Florent Revest wrote: > > On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > >> On 1/29/21 11:57 AM, Daniel Borkmann wrote: > >>> On 1/27/21 10:01 PM, Andrii Nakryiko wrote: > >>>> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote: > >>>>> > >>>>> This needs a new helper that: > >>>>> - can work in a sleepable context (using sock_gen_cookie) > >>>>> - takes a struct sock pointer and checks that it's not NULL > >>>>> > >>>>> Signed-off-by: Florent Revest <revest@chromium.org> > >>>>> Acked-by: KP Singh <kpsingh@kernel.org> > >>>>> --- > >>>>> include/linux/bpf.h | 1 + > >>>>> include/uapi/linux/bpf.h | 8 ++++++++ > >>>>> kernel/trace/bpf_trace.c | 2 ++ > >>>>> net/core/filter.c | 12 ++++++++++++ > >>>>> tools/include/uapi/linux/bpf.h | 8 ++++++++ > >>>>> 5 files changed, 31 insertions(+) > >>>>> > >>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h > >>>>> index 1aac2af12fed..26219465e1f7 100644 > >>>>> --- a/include/linux/bpf.h > >>>>> +++ b/include/linux/bpf.h > >>>>> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto; > >>>>> extern const struct bpf_func_proto bpf_this_cpu_ptr_proto; > >>>>> extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto; > >>>>> extern const struct bpf_func_proto bpf_sock_from_file_proto; > >>>>> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto; > >>>>> > >>>>> const struct bpf_func_proto *bpf_tracing_func_proto( > >>>>> enum bpf_func_id func_id, const struct bpf_prog *prog); > >>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > >>>>> index 0b735c2729b2..5855c398d685 100644 > >>>>> --- a/include/uapi/linux/bpf.h > >>>>> +++ b/include/uapi/linux/bpf.h > >>>>> @@ -1673,6 +1673,14 @@ union bpf_attr { > >>>>> * Return > >>>>> * A 8-byte long unique number. > >>>>> * > >>>>> + * u64 bpf_get_socket_cookie(void *sk) > >>>> > >>>> should the type be `struct sock *` then? > >>> > >>> Checking libbpf's generated bpf_helper_defs.h it generates: > >>> > >>> /* > >>> * bpf_get_socket_cookie > >>> * > >>> * If the **struct sk_buff** pointed by *skb* has a known socket, > >>> * retrieve the cookie (generated by the kernel) of this socket. > >>> * If no cookie has been set yet, generate a new cookie. Once > >>> * generated, the socket cookie remains stable for the life of the > >>> * socket. This helper can be useful for monitoring per socket > >>> * networking traffic statistics as it provides a global socket > >>> * identifier that can be assumed unique. > >>> * > >>> * Returns > >>> * A 8-byte long non-decreasing number on success, or 0 if the > >>> * socket field is missing inside *skb*. > >>> */ > >>> static __u64 (*bpf_get_socket_cookie)(void *ctx) = (void *) 46; > >>> > >>> So in terms of helper comment it's picking up the description from the > >>> `u64 bpf_get_socket_cookie(struct sk_buff *skb)` signature. With that > >>> in mind it would likely make sense to add the actual `struct sock *` type > >>> to the comment to make it more clear in here. > >> > >> One thought that still came to mind when looking over the series again, do > >> we need to blacklist certain functions from bpf_get_socket_cookie() under > >> tracing e.g. when attaching to, say fexit? For example, if sk_prot_free() > >> would be temporary uninlined/exported for testing and bpf_get_socket_cookie() > >> was invoked from a prog upon fexit where sock was already passed back to > >> allocator, I presume there's risk of mem corruption, no? > > > > Mh, this is interesting. I can try to add a deny list in v7 but I'm > > not sure whether I'll be able to catch them all. I'm assuming that > > __sk_destruct, sk_destruct, __sk_free, sk_free would be other > > problematic functions but potentially there would be more. > > I was just looking at bpf_skb_output() from a7658e1a4164 ("bpf: Check types of > arguments passed into helpers") which afaiu has similar issue, back at the time > this was introduced there was no fentry/fexit but rather raw tp progs, so you > could still safely dump skb this way including for kfree_skb() tp. Presumably if > you bpf_skb_output() at 'fexit/kfree_skb' you might be able to similarly crash the verifier cannot check absolutely everything. Whitelisting and blacklisting all combinations is not practical. > your kernel which I don't think is intentional (also given we go above and beyond > in other areas to avoid crashing or destabilizing e.g. [0] to mention one). Maybe > these should really only be for BPF_TRACE_RAW_TP (or BPF_PROG_TYPE_LSM) where it > can be audited that it's safe to use like a7658e1a4164's original intention ... > or have some sort of function annotation like __acquires/__releases but for tracing > e.g. __frees(skb) where use would then be blocked (not sure iff feasible). I think this set is useful and corner cases like fexit in sk_free are not worth the hassle. One can install xdp prog that drops all packets. The server is dead at this point.
On Mon, Feb 1, 2021 at 11:37 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Feb 1, 2021 at 2:32 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > > On 1/30/21 12:45 PM, Florent Revest wrote: > > > On Fri, Jan 29, 2021 at 1:49 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > >> On 1/29/21 11:57 AM, Daniel Borkmann wrote: > > >>> On 1/27/21 10:01 PM, Andrii Nakryiko wrote: > > >>>> On Tue, Jan 26, 2021 at 10:36 AM Florent Revest <revest@chromium.org> wrote: > > >>>>> > > >>>>> This needs a new helper that: > > >>>>> - can work in a sleepable context (using sock_gen_cookie) > > >>>>> - takes a struct sock pointer and checks that it's not NULL > > >>>>> > > >>>>> Signed-off-by: Florent Revest <revest@chromium.org> > > >>>>> Acked-by: KP Singh <kpsingh@kernel.org> > > >>>>> --- > > >>>>> include/linux/bpf.h | 1 + > > >>>>> include/uapi/linux/bpf.h | 8 ++++++++ > > >>>>> kernel/trace/bpf_trace.c | 2 ++ > > >>>>> net/core/filter.c | 12 ++++++++++++ > > >>>>> tools/include/uapi/linux/bpf.h | 8 ++++++++ > > >>>>> 5 files changed, 31 insertions(+) > > >>>>> > > >>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > >>>>> index 1aac2af12fed..26219465e1f7 100644 > > >>>>> --- a/include/linux/bpf.h > > >>>>> +++ b/include/linux/bpf.h > > >>>>> @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto; > > >>>>> extern const struct bpf_func_proto bpf_this_cpu_ptr_proto; > > >>>>> extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto; > > >>>>> extern const struct bpf_func_proto bpf_sock_from_file_proto; > > >>>>> +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto; > > >>>>> > > >>>>> const struct bpf_func_proto *bpf_tracing_func_proto( > > >>>>> enum bpf_func_id func_id, const struct bpf_prog *prog); > > >>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > >>>>> index 0b735c2729b2..5855c398d685 100644 > > >>>>> --- a/include/uapi/linux/bpf.h > > >>>>> +++ b/include/uapi/linux/bpf.h > > >>>>> @@ -1673,6 +1673,14 @@ union bpf_attr { > > >>>>> * Return > > >>>>> * A 8-byte long unique number. > > >>>>> * > > >>>>> + * u64 bpf_get_socket_cookie(void *sk) > > >>>> > > >>>> should the type be `struct sock *` then? > > >>> > > >>> Checking libbpf's generated bpf_helper_defs.h it generates: > > >>> > > >>> /* > > >>> * bpf_get_socket_cookie > > >>> * > > >>> * If the **struct sk_buff** pointed by *skb* has a known socket, > > >>> * retrieve the cookie (generated by the kernel) of this socket. > > >>> * If no cookie has been set yet, generate a new cookie. Once > > >>> * generated, the socket cookie remains stable for the life of the > > >>> * socket. This helper can be useful for monitoring per socket > > >>> * networking traffic statistics as it provides a global socket > > >>> * identifier that can be assumed unique. > > >>> * > > >>> * Returns > > >>> * A 8-byte long non-decreasing number on success, or 0 if the > > >>> * socket field is missing inside *skb*. > > >>> */ > > >>> static __u64 (*bpf_get_socket_cookie)(void *ctx) = (void *) 46; > > >>> > > >>> So in terms of helper comment it's picking up the description from the > > >>> `u64 bpf_get_socket_cookie(struct sk_buff *skb)` signature. With that > > >>> in mind it would likely make sense to add the actual `struct sock *` type > > >>> to the comment to make it more clear in here. > > >> > > >> One thought that still came to mind when looking over the series again, do > > >> we need to blacklist certain functions from bpf_get_socket_cookie() under > > >> tracing e.g. when attaching to, say fexit? For example, if sk_prot_free() > > >> would be temporary uninlined/exported for testing and bpf_get_socket_cookie() > > >> was invoked from a prog upon fexit where sock was already passed back to > > >> allocator, I presume there's risk of mem corruption, no? > > > > > > Mh, this is interesting. I can try to add a deny list in v7 but I'm > > > not sure whether I'll be able to catch them all. I'm assuming that > > > __sk_destruct, sk_destruct, __sk_free, sk_free would be other > > > problematic functions but potentially there would be more. > > > > I was just looking at bpf_skb_output() from a7658e1a4164 ("bpf: Check types of > > arguments passed into helpers") which afaiu has similar issue, back at the time > > this was introduced there was no fentry/fexit but rather raw tp progs, so you > > could still safely dump skb this way including for kfree_skb() tp. Presumably if > > you bpf_skb_output() at 'fexit/kfree_skb' you might be able to similarly crash > > the verifier cannot check absolutely everything. > Whitelisting and blacklisting all combinations is not practical. Ok, I'm sending a v7 that only changes the signature to take a struct sock * argument then and I won't be adding an allow or deny list in this series. :) > > your kernel which I don't think is intentional (also given we go above and beyond > > in other areas to avoid crashing or destabilizing e.g. [0] to mention one). Maybe > > these should really only be for BPF_TRACE_RAW_TP (or BPF_PROG_TYPE_LSM) where it > > can be audited that it's safe to use like a7658e1a4164's original intention ... > > or have some sort of function annotation like __acquires/__releases but for tracing > > e.g. __frees(skb) where use would then be blocked (not sure iff feasible). > > I think this set is useful and corner cases like fexit in sk_free are > not worth the hassle. > One can install xdp prog that drops all packets. The server is dead at > this point.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 1aac2af12fed..26219465e1f7 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1874,6 +1874,7 @@ extern const struct bpf_func_proto bpf_per_cpu_ptr_proto; extern const struct bpf_func_proto bpf_this_cpu_ptr_proto; extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto; extern const struct bpf_func_proto bpf_sock_from_file_proto; +extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto; const struct bpf_func_proto *bpf_tracing_func_proto( enum bpf_func_id func_id, const struct bpf_prog *prog); diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 0b735c2729b2..5855c398d685 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1673,6 +1673,14 @@ union bpf_attr { * Return * A 8-byte long unique number. * + * u64 bpf_get_socket_cookie(void *sk) + * Description + * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts + * *sk*, but gets socket from a BTF **struct sock**. This helper + * also works for sleepable programs. + * Return + * A 8-byte long unique number or 0 if *sk* is NULL. + * * u32 bpf_get_socket_uid(struct sk_buff *skb) * Return * The owner UID of the socket associated to *skb*. If the socket diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 6c0018abe68a..845b2168e006 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1760,6 +1760,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_sk_storage_delete_tracing_proto; case BPF_FUNC_sock_from_file: return &bpf_sock_from_file_proto; + case BPF_FUNC_get_socket_cookie: + return &bpf_get_socket_ptr_cookie_proto; #endif case BPF_FUNC_seq_printf: return prog->expected_attach_type == BPF_TRACE_ITER ? diff --git a/net/core/filter.c b/net/core/filter.c index 9ab94e90d660..606e2b6115ed 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -4631,6 +4631,18 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = { .arg1_type = ARG_PTR_TO_CTX, }; +BPF_CALL_1(bpf_get_socket_ptr_cookie, struct sock *, sk) +{ + return sk ? sock_gen_cookie(sk) : 0; +} + +const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto = { + .func = bpf_get_socket_ptr_cookie, + .gpl_only = false, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, +}; + BPF_CALL_1(bpf_get_socket_cookie_sock_ops, struct bpf_sock_ops_kern *, ctx) { return __sock_gen_cookie(ctx->sk); diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 0b735c2729b2..5855c398d685 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1673,6 +1673,14 @@ union bpf_attr { * Return * A 8-byte long unique number. * + * u64 bpf_get_socket_cookie(void *sk) + * Description + * Equivalent to **bpf_get_socket_cookie**\ () helper that accepts + * *sk*, but gets socket from a BTF **struct sock**. This helper + * also works for sleepable programs. + * Return + * A 8-byte long unique number or 0 if *sk* is NULL. + * * u32 bpf_get_socket_uid(struct sk_buff *skb) * Return * The owner UID of the socket associated to *skb*. If the socket