This creates a new helper proto because the existing
bpf_get_socket_cookie_sock_proto has a ARG_PTR_TO_CTX argument and only
works for BPF programs where the context is a sock.
This helper could also be useful to other BPF program types such as LSM.
Signed-off-by: Florent Revest <[email protected]>
---
include/uapi/linux/bpf.h | 7 +++++++
kernel/trace/bpf_trace.c | 4 ++++
net/core/filter.c | 7 +++++++
tools/include/uapi/linux/bpf.h | 7 +++++++
4 files changed, 25 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c3458ec1f30a..3e0e33c43998 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1662,6 +1662,13 @@ union bpf_attr {
* Return
* A 8-byte long non-decreasing 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**.
+ * Return
+ * A 8-byte long non-decreasing number.
+ *
* 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 d255bc9b2bfa..14ad96579813 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1725,6 +1725,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
}
}
+extern const struct bpf_func_proto bpf_get_socket_cookie_sock_tracing_proto;
+
const struct bpf_func_proto *
tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
@@ -1748,6 +1750,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_storage_get_tracing_proto;
case BPF_FUNC_sk_storage_delete:
return &bpf_sk_storage_delete_tracing_proto;
+ case BPF_FUNC_get_socket_cookie:
+ return &bpf_get_socket_cookie_sock_tracing_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 2ca5eecebacf..177c4e5e529d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4631,6 +4631,13 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
.arg1_type = ARG_PTR_TO_CTX,
};
+const struct bpf_func_proto bpf_get_socket_cookie_sock_tracing_proto = {
+ .func = bpf_get_socket_cookie_sock,
+ .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 c3458ec1f30a..3e0e33c43998 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1662,6 +1662,13 @@ union bpf_attr {
* Return
* A 8-byte long non-decreasing 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**.
+ * Return
+ * A 8-byte long non-decreasing number.
+ *
* u32 bpf_get_socket_uid(struct sk_buff *skb)
* Return
* The owner UID of the socket associated to *skb*. If the socket
--
2.29.2.576.ga3fc446d84-goog
On 12/3/20 10:33 PM, Florent Revest wrote:
> This creates a new helper proto because the existing
> bpf_get_socket_cookie_sock_proto has a ARG_PTR_TO_CTX argument and only
> works for BPF programs where the context is a sock.
>
> This helper could also be useful to other BPF program types such as LSM.
>
> Signed-off-by: Florent Revest <[email protected]>
> ---
> include/uapi/linux/bpf.h | 7 +++++++
> kernel/trace/bpf_trace.c | 4 ++++
> net/core/filter.c | 7 +++++++
> tools/include/uapi/linux/bpf.h | 7 +++++++
> 4 files changed, 25 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index c3458ec1f30a..3e0e33c43998 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1662,6 +1662,13 @@ union bpf_attr {
> * Return
> * A 8-byte long non-decreasing 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**.
> + * Return
> + * A 8-byte long non-decreasing number.
I would not mention this here since it's not fully correct and we should avoid users
taking non-decreasing granted in their progs. The only assumption you can make is
that it can be considered a unique number. See also [0] with reverse counter..
[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=92acdc58ab11af66fcaef485433fde61b5e32fac
> + *
> * 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 d255bc9b2bfa..14ad96579813 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1725,6 +1725,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> }
> }
>
> +extern const struct bpf_func_proto bpf_get_socket_cookie_sock_tracing_proto;
> +
> const struct bpf_func_proto *
> tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> {
> @@ -1748,6 +1750,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_sk_storage_get_tracing_proto;
> case BPF_FUNC_sk_storage_delete:
> return &bpf_sk_storage_delete_tracing_proto;
> + case BPF_FUNC_get_socket_cookie:
> + return &bpf_get_socket_cookie_sock_tracing_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 2ca5eecebacf..177c4e5e529d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4631,6 +4631,13 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
> .arg1_type = ARG_PTR_TO_CTX,
> };
>
> +const struct bpf_func_proto bpf_get_socket_cookie_sock_tracing_proto = {
> + .func = bpf_get_socket_cookie_sock,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
> +};
> +
On 12/4/20 7:56 PM, Daniel Borkmann wrote:
> On 12/3/20 10:33 PM, Florent Revest wrote:
>> This creates a new helper proto because the existing
>> bpf_get_socket_cookie_sock_proto has a ARG_PTR_TO_CTX argument and only
>> works for BPF programs where the context is a sock.
>>
>> This helper could also be useful to other BPF program types such as LSM.
>>
>> Signed-off-by: Florent Revest <[email protected]>
>> ---
>> include/uapi/linux/bpf.h | 7 +++++++
>> kernel/trace/bpf_trace.c | 4 ++++
>> net/core/filter.c | 7 +++++++
>> tools/include/uapi/linux/bpf.h | 7 +++++++
>> 4 files changed, 25 insertions(+)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index c3458ec1f30a..3e0e33c43998 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1662,6 +1662,13 @@ union bpf_attr {
>> * Return
>> * A 8-byte long non-decreasing 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**.
>> + * Return
>> + * A 8-byte long non-decreasing number.
>
> I would not mention this here since it's not fully correct and we should avoid users
> taking non-decreasing granted in their progs. The only assumption you can make is
> that it can be considered a unique number. See also [0] with reverse counter..
>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=92acdc58ab11af66fcaef485433fde61b5e32fac
One more thought, in case you plan to use this from sleepable context, you would
need to use sock_gen_cookie() variant in the BPF helper instead.
On Thu, Dec 03, 2020 at 10:33:28PM +0100, Florent Revest wrote:
> This creates a new helper proto because the existing
> bpf_get_socket_cookie_sock_proto has a ARG_PTR_TO_CTX argument and only
> works for BPF programs where the context is a sock.
>
> This helper could also be useful to other BPF program types such as LSM.
>
> Signed-off-by: Florent Revest <[email protected]>
> ---
> include/uapi/linux/bpf.h | 7 +++++++
> kernel/trace/bpf_trace.c | 4 ++++
> net/core/filter.c | 7 +++++++
> tools/include/uapi/linux/bpf.h | 7 +++++++
> 4 files changed, 25 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index c3458ec1f30a..3e0e33c43998 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1662,6 +1662,13 @@ union bpf_attr {
> * Return
> * A 8-byte long non-decreasing 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**.
> + * Return
> + * A 8-byte long non-decreasing number.
> + *
> * 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 d255bc9b2bfa..14ad96579813 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1725,6 +1725,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> }
> }
>
> +extern const struct bpf_func_proto bpf_get_socket_cookie_sock_tracing_proto;
> +
> const struct bpf_func_proto *
> tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> {
> @@ -1748,6 +1750,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_sk_storage_get_tracing_proto;
> case BPF_FUNC_sk_storage_delete:
> return &bpf_sk_storage_delete_tracing_proto;
> + case BPF_FUNC_get_socket_cookie:
> + return &bpf_get_socket_cookie_sock_tracing_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 2ca5eecebacf..177c4e5e529d 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4631,6 +4631,13 @@ static const struct bpf_func_proto bpf_get_socket_cookie_sock_proto = {
> .arg1_type = ARG_PTR_TO_CTX,
> };
>
> +const struct bpf_func_proto bpf_get_socket_cookie_sock_tracing_proto = {
> + .func = bpf_get_socket_cookie_sock,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
In tracing where it gets a sk pointer, the sk could be NULL.
A NULL check is required in the helper. Please refer to
bpf_skc_to_tcp_sock[_proto] as an example.
This proto is in general also useful for non tracing context where
it can get a hold of a sk pointer. (e.g. another similar usage that will
have a hold on a sk pointer for BPF_PROG_TYPE_SK_REUSEPORT [0]).
In case if you don't need sleepable at this point as Daniel
mentioned in another thread. Does it make sense to rename this
proto to something like bpf_get_socket_pointer_cookie_proto?
[0]: https://lore.kernel.org/bpf/[email protected]/
On Fri, 2020-12-04 at 11:47 -0800, Martin KaFai Lau wrote:
> On Thu, Dec 03, 2020 at 10:33:28PM +0100, Florent Revest wrote:
> > +const struct bpf_func_proto
> > bpf_get_socket_cookie_sock_tracing_proto = {
> > + .func = bpf_get_socket_cookie_sock,
> > + .gpl_only = false,
> > + .ret_type = RET_INTEGER,
> > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
>
> In tracing where it gets a sk pointer, the sk could be NULL.
> A NULL check is required in the helper. Please refer to
> bpf_skc_to_tcp_sock[_proto] as an example.
Ah, good catch! :)
> This proto is in general also useful for non tracing context where
> it can get a hold of a sk pointer. (e.g. another similar usage that
> will have a hold on a sk pointer for BPF_PROG_TYPE_SK_REUSEPORT [0]).
Agreed.
> In case if you don't need sleepable at this point as Daniel
> mentioned in another thread. Does it make sense to rename this
> proto to something like bpf_get_socket_pointer_cookie_proto?
My understanding is that I could have two helpers definitions and
protos, one calling sock_gen_cookie and the other one calling
__sock_gen_cookie. Then I could just use:
return prog->aux->sleepable
? bpf_get_socket_pointer_cookie_sleepable_proto
: bpf_get_socket_pointer_cookie_proto;
Would that work ?
On Fri, 2020-12-04 at 20:03 +0100, Daniel Borkmann wrote:
> On 12/4/20 7:56 PM, Daniel Borkmann wrote:
> > On 12/3/20 10:33 PM, Florent Revest wrote:
> > > This creates a new helper proto because the existing
> > > bpf_get_socket_cookie_sock_proto has a ARG_PTR_TO_CTX argument
> > > and only
> > > works for BPF programs where the context is a sock.
> > >
> > > This helper could also be useful to other BPF program types such
> > > as LSM.
> > >
> > > Signed-off-by: Florent Revest <[email protected]>
> > > ---
> > > include/uapi/linux/bpf.h | 7 +++++++
> > > kernel/trace/bpf_trace.c | 4 ++++
> > > net/core/filter.c | 7 +++++++
> > > tools/include/uapi/linux/bpf.h | 7 +++++++
> > > 4 files changed, 25 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index c3458ec1f30a..3e0e33c43998 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -1662,6 +1662,13 @@ union bpf_attr {
> > > * Return
> > > * A 8-byte long non-decreasing 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**.
> > > + * Return
> > > + * A 8-byte long non-decreasing number.
> >
> > I would not mention this here since it's not fully correct and we
> > should avoid users taking non-decreasing granted in their progs.
> > The only assumption you can make is that it can be considered a
> > unique number. See also [0] with reverse counter..
> >
> > [0]
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=92acdc58ab11af66fcaef485433fde61b5e32fac
Ah this is a good point, thank you! I will send a v3 with an extra
patch that s/non-decreasing/unique/ in the other descriptions. I had
not given it any extra thought, I just stupidly copied/pasted existing
descriptions. :)
> One more thought, in case you plan to use this from sleepable
> context, you would need to use sock_gen_cookie() variant in the BPF
> helper instead.
Out of curiosity, why don't we just always call sock_gen_cookie? Is it
to avoid the performance impact of increasing the preempt counter and
introducing a memory barriers ?
On 12/8/20 8:30 PM, Florent Revest wrote:
> On Fri, 2020-12-04 at 20:03 +0100, Daniel Borkmann wrote:
>> On 12/4/20 7:56 PM, Daniel Borkmann wrote:
>>> On 12/3/20 10:33 PM, Florent Revest wrote:
>>>> This creates a new helper proto because the existing
>>>> bpf_get_socket_cookie_sock_proto has a ARG_PTR_TO_CTX argument
>>>> and only
>>>> works for BPF programs where the context is a sock.
>>>>
>>>> This helper could also be useful to other BPF program types such
>>>> as LSM.
>>>>
>>>> Signed-off-by: Florent Revest <[email protected]>
>>>> ---
>>>> include/uapi/linux/bpf.h | 7 +++++++
>>>> kernel/trace/bpf_trace.c | 4 ++++
>>>> net/core/filter.c | 7 +++++++
>>>> tools/include/uapi/linux/bpf.h | 7 +++++++
>>>> 4 files changed, 25 insertions(+)
>>>>
>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>> index c3458ec1f30a..3e0e33c43998 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -1662,6 +1662,13 @@ union bpf_attr {
>>>> * Return
>>>> * A 8-byte long non-decreasing 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**.
>>>> + * Return
>>>> + * A 8-byte long non-decreasing number.
>>>
>>> I would not mention this here since it's not fully correct and we
>>> should avoid users taking non-decreasing granted in their progs.
>>> The only assumption you can make is that it can be considered a
>>> unique number. See also [0] with reverse counter..
>>>
>>> [0]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=92acdc58ab11af66fcaef485433fde61b5e32fac
>
> Ah this is a good point, thank you! I will send a v3 with an extra
> patch that s/non-decreasing/unique/ in the other descriptions. I had
> not given it any extra thought, I just stupidly copied/pasted existing
> descriptions. :)
>
>> One more thought, in case you plan to use this from sleepable
>> context, you would need to use sock_gen_cookie() variant in the BPF
>> helper instead.
>
> Out of curiosity, why don't we just always call sock_gen_cookie? Is it
> to avoid the performance impact of increasing the preempt counter and
> introducing a memory barriers ?
Yes, all the other contexts where the existing helpers are used already have
preemption disabled, so the extra preempt_{disable,enable}() is unnecessary
overhead given we want the cookie generation be efficient.
Thanks,
Daniel