[bpf-next,2/3] bpf: add kfunc for skb refcounting

Commit Message

Maciej Fijalkowski Feb. 20, 2025, 1:45 p.m. UTC

These have been mostly taken from Amery Hung's work related to bpf qdisc
implementation. bpf_skb_{acquire,release}() are for increment/decrement
sk_buff::users whereas bpf_skb_destroy() is called for map entries that
have not been released and map is being wiped out from system.

Signed-off-by: Maciej Fijalkowski <[e-mail]>
---
 net/core/filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

Comments

Amery Hung Feb. 20, 2025, 11:25 p.m. UTC | #1

On 2/20/2025 5:45 AM, Maciej Fijalkowski wrote:
> These have been mostly taken from Amery Hung's work related to bpf qdisc
> implementation. bpf_skb_{acquire,release}() are for increment/decrement
> sk_buff::users whereas bpf_skb_destroy() is called for map entries that
> have not been released and map is being wiped out from system.
> 
> Signed-off-by: Maciej Fijalkowski <[e-mail]>
> ---
>   net/core/filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 2ec162dd83c4..9bd2701be088 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12064,6 +12064,56 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
>   
>   __bpf_kfunc_end_defs();
>   
> +__diag_push();
> +__diag_ignore_all("-Wmissing-prototypes",
> +		  "Global functions as their definitions will be in vmlinux BTF");
> +
> +/* bpf_skb_acquire - Acquire a reference to an skb. An skb acquired by this
> + * kfunc which is not stored in a map as a kptr, must be released by calling
> + * bpf_skb_release().
> + * @skb: The skb on which a reference is being acquired.
> + */
> +__bpf_kfunc struct sk_buff *bpf_skb_acquire(struct sk_buff *skb)
> +{
> +	if (refcount_inc_not_zero(&skb->users))
> +		return skb;
> +	return NULL;
> +}
> +
> +/* bpf_skb_release - Release the reference acquired on an skb.
> + * @skb: The skb on which a reference is being released.
> + */
> +__bpf_kfunc void bpf_skb_release(struct sk_buff *skb)
> +{
> +	skb_unref(skb);
> +}
> +
> +/* bpf_skb_destroy - Release an skb reference acquired and exchanged into
> + * an allocated object or a map.
> + * @skb: The skb on which a reference is being released.
> + */
> +__bpf_kfunc void bpf_skb_destroy(struct sk_buff *skb)
> +{
> +	(void)skb_unref(skb);
> +	consume_skb(skb);
> +}
> +
> +__diag_pop();
> +
> +BTF_KFUNCS_START(skb_kfunc_btf_ids)
> +BTF_ID_FLAGS(func, bpf_skb_acquire, KF_ACQUIRE | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_skb_release, KF_RELEASE)
> +BTF_KFUNCS_END(skb_kfunc_btf_ids)
> +
> +static const struct btf_kfunc_id_set skb_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set   = &skb_kfunc_btf_ids,
> +};
> +
> +BTF_ID_LIST(skb_kfunc_dtor_ids)
> +BTF_ID(struct, sk_buff)
> +BTF_ID_FLAGS(func, bpf_skb_destroy, KF_RELEASE)
> +
>   int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
>   			       struct bpf_dynptr *ptr__uninit)
>   {
> @@ -12117,6 +12167,13 @@ static const struct btf_kfunc_id_set bpf_kfunc_set_tcp_reqsk = {
>   
>   static int __init bpf_kfunc_init(void)
>   {
> +	const struct btf_id_dtor_kfunc skb_kfunc_dtors[] = {
> +		{
> +			.btf_id       = skb_kfunc_dtor_ids[0],
> +			.kfunc_btf_id = skb_kfunc_dtor_ids[1]
> +		},
> +	};
> +
>   	int ret;
>   
>   	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_skb);
> @@ -12133,6 +12190,11 @@ static int __init bpf_kfunc_init(void)
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
>   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
>   					       &bpf_kfunc_set_sock_addr);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &skb_kfunc_set);
> +
> +	ret = ret ?: register_btf_id_dtor_kfuncs(skb_kfunc_dtors,
> +						 ARRAY_SIZE(skb_kfunc_dtors),
> +						 THIS_MODULE);

I think we will need to deal with two versions of skb dtors here. Both 
qdisc and cls will register dtor associated for skb. The qdisc one just 
call kfree_skb(). While only one can exist for a specific btf id in the 
kernel if I understand correctly. Is it possible to have one that work
for both use cases?

>   	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
>   }
>   late_initcall(bpf_kfunc_init);

Maciej Fijalkowski Feb. 21, 2025, 2:56 p.m. UTC | #2

On Thu, Feb 20, 2025 at 03:25:03PM -0800, Amery Hung wrote:
> 
> 
> On 2/20/2025 5:45 AM, Maciej Fijalkowski wrote:
> > These have been mostly taken from Amery Hung's work related to bpf qdisc
> > implementation. bpf_skb_{acquire,release}() are for increment/decrement
> > sk_buff::users whereas bpf_skb_destroy() is called for map entries that
> > have not been released and map is being wiped out from system.
> > 
> > Signed-off-by: Maciej Fijalkowski <[e-mail]>
> > ---
> >   net/core/filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 62 insertions(+)
> > 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 2ec162dd83c4..9bd2701be088 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -12064,6 +12064,56 @@ __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
> >   __bpf_kfunc_end_defs();
> > +__diag_push();
> > +__diag_ignore_all("-Wmissing-prototypes",
> > +		  "Global functions as their definitions will be in vmlinux BTF");
> > +
> > +/* bpf_skb_acquire - Acquire a reference to an skb. An skb acquired by this
> > + * kfunc which is not stored in a map as a kptr, must be released by calling
> > + * bpf_skb_release().
> > + * @skb: The skb on which a reference is being acquired.
> > + */
> > +__bpf_kfunc struct sk_buff *bpf_skb_acquire(struct sk_buff *skb)
> > +{
> > +	if (refcount_inc_not_zero(&skb->users))
> > +		return skb;
> > +	return NULL;
> > +}
> > +
> > +/* bpf_skb_release - Release the reference acquired on an skb.
> > + * @skb: The skb on which a reference is being released.
> > + */
> > +__bpf_kfunc void bpf_skb_release(struct sk_buff *skb)
> > +{
> > +	skb_unref(skb);
> > +}
> > +
> > +/* bpf_skb_destroy - Release an skb reference acquired and exchanged into
> > + * an allocated object or a map.
> > + * @skb: The skb on which a reference is being released.
> > + */
> > +__bpf_kfunc void bpf_skb_destroy(struct sk_buff *skb)
> > +{
> > +	(void)skb_unref(skb);
> > +	consume_skb(skb);
> > +}
> > +
> > +__diag_pop();
> > +
> > +BTF_KFUNCS_START(skb_kfunc_btf_ids)
> > +BTF_ID_FLAGS(func, bpf_skb_acquire, KF_ACQUIRE | KF_RET_NULL)
> > +BTF_ID_FLAGS(func, bpf_skb_release, KF_RELEASE)
> > +BTF_KFUNCS_END(skb_kfunc_btf_ids)
> > +
> > +static const struct btf_kfunc_id_set skb_kfunc_set = {
> > +	.owner = THIS_MODULE,
> > +	.set   = &skb_kfunc_btf_ids,
> > +};
> > +
> > +BTF_ID_LIST(skb_kfunc_dtor_ids)
> > +BTF_ID(struct, sk_buff)
> > +BTF_ID_FLAGS(func, bpf_skb_destroy, KF_RELEASE)
> > +
> >   int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
> >   			       struct bpf_dynptr *ptr__uninit)
> >   {
> > @@ -12117,6 +12167,13 @@ static const struct btf_kfunc_id_set bpf_kfunc_set_tcp_reqsk = {
> >   static int __init bpf_kfunc_init(void)
> >   {
> > +	const struct btf_id_dtor_kfunc skb_kfunc_dtors[] = {
> > +		{
> > +			.btf_id       = skb_kfunc_dtor_ids[0],
> > +			.kfunc_btf_id = skb_kfunc_dtor_ids[1]
> > +		},
> > +	};
> > +
> >   	int ret;
> >   	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_skb);
> > @@ -12133,6 +12190,11 @@ static int __init bpf_kfunc_init(void)
> >   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
> >   	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
> >   					       &bpf_kfunc_set_sock_addr);
> > +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &skb_kfunc_set);
> > +
> > +	ret = ret ?: register_btf_id_dtor_kfuncs(skb_kfunc_dtors,
> > +						 ARRAY_SIZE(skb_kfunc_dtors),
> > +						 THIS_MODULE);
> 
> I think we will need to deal with two versions of skb dtors here. Both qdisc
> and cls will register dtor associated for skb. The qdisc one just call
> kfree_skb(). While only one can exist for a specific btf id in the kernel if
> I understand correctly. Is it possible to have one that work
> for both use cases?

Looking at the current code it seems bpf_find_btf_id() (which
btf_parse_kptr() calls) will go through modules and return the first match
against sk_buff btf but that's currently a wild guess from my side. So
your concern stands as we have no mechanism that would distinguish the
dtors for same btf id.

I would have to take a deeper look at btf_parse_kptr() and find some way
to associate dtor with its module during registering and then use it
within btf_find_dtor_kfunc(). Would this be sufficient?

> 
> >   	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
> >   }
> >   late_initcall(bpf_kfunc_init);
>

Patch

diff --git a/net/core/filter.c b/net/core/filter.c
index 2ec162dd83c4..9bd2701be088 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -12064,6 +12064,56 @@  __bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct __sk_buff *s, struct sock *sk,
 
 __bpf_kfunc_end_defs();
 
+__diag_push();
+__diag_ignore_all("-Wmissing-prototypes",
+		  "Global functions as their definitions will be in vmlinux BTF");
+
+/* bpf_skb_acquire - Acquire a reference to an skb. An skb acquired by this
+ * kfunc which is not stored in a map as a kptr, must be released by calling
+ * bpf_skb_release().
+ * @skb: The skb on which a reference is being acquired.
+ */
+__bpf_kfunc struct sk_buff *bpf_skb_acquire(struct sk_buff *skb)
+{
+	if (refcount_inc_not_zero(&skb->users))
+		return skb;
+	return NULL;
+}
+
+/* bpf_skb_release - Release the reference acquired on an skb.
+ * @skb: The skb on which a reference is being released.
+ */
+__bpf_kfunc void bpf_skb_release(struct sk_buff *skb)
+{
+	skb_unref(skb);
+}
+
+/* bpf_skb_destroy - Release an skb reference acquired and exchanged into
+ * an allocated object or a map.
+ * @skb: The skb on which a reference is being released.
+ */
+__bpf_kfunc void bpf_skb_destroy(struct sk_buff *skb)
+{
+	(void)skb_unref(skb);
+	consume_skb(skb);
+}
+
+__diag_pop();
+
+BTF_KFUNCS_START(skb_kfunc_btf_ids)
+BTF_ID_FLAGS(func, bpf_skb_acquire, KF_ACQUIRE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_skb_release, KF_RELEASE)
+BTF_KFUNCS_END(skb_kfunc_btf_ids)
+
+static const struct btf_kfunc_id_set skb_kfunc_set = {
+	.owner = THIS_MODULE,
+	.set   = &skb_kfunc_btf_ids,
+};
+
+BTF_ID_LIST(skb_kfunc_dtor_ids)
+BTF_ID(struct, sk_buff)
+BTF_ID_FLAGS(func, bpf_skb_destroy, KF_RELEASE)
+
 int bpf_dynptr_from_skb_rdonly(struct __sk_buff *skb, u64 flags,
 			       struct bpf_dynptr *ptr__uninit)
 {
@@ -12117,6 +12167,13 @@  static const struct btf_kfunc_id_set bpf_kfunc_set_tcp_reqsk = {
 
 static int __init bpf_kfunc_init(void)
 {
+	const struct btf_id_dtor_kfunc skb_kfunc_dtors[] = {
+		{
+			.btf_id       = skb_kfunc_dtor_ids[0],
+			.kfunc_btf_id = skb_kfunc_dtor_ids[1]
+		},
+	};
+
 	int ret;
 
 	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_skb);
@@ -12133,6 +12190,11 @@  static int __init bpf_kfunc_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
 					       &bpf_kfunc_set_sock_addr);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &skb_kfunc_set);
+
+	ret = ret ?: register_btf_id_dtor_kfuncs(skb_kfunc_dtors,
+						 ARRAY_SIZE(skb_kfunc_dtors),
+						 THIS_MODULE);
 	return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
 }
 late_initcall(bpf_kfunc_init);