[RFC PATCH bpf-next 0/2] Improve prog array uref semantics

[RFC PATCH bpf-next 0/2] Improve prog array uref semantics

[Daniel Xu]

This patchset changes the behavior of TC and XDP hooks during attachment
such that any BPF_MAP_TYPE_PROG_ARRAY that the prog uses has an extra
uref taken.

The goal behind this change is to try and prevent confusion for the
majority of use cases. The current behavior where when the last uref is
dropped the prog array map is emptied is quite confusing. Confusing
enough for there to be multiple references to it in ebpf-go [0][1].

Completely solving the problem is difficult. As stated in c9da161c6517
("bpf: fix clearing on persistent program array maps"), it is
difficult-to-impossible to walk the full dependency graph b/c it is too
dynamic.

However in practice, I've found that all progs in a tailcall chain
share the same prog array map. Knowing that, if we take a uref on any
used prog array map when the program is attached, we can simplify the
majority use case and make it more ergonomic.

I'll be the first to admit this is not a very clean solution. It does
not fully solve the problem. Nor does it make overall logic any simpler.
But I do think it makes a pretty big usability hole slightly smaller.

I've done some basic testing using a repro program [3] I wrote to debug
the original issue that eventually led me to this patchset. If we wanna
move forward with this approach, I'll resend with selftests.

[0]: https://github.com/cilium/ebpf/blob/01ebd4c1e2b9f8b3dd4fd2382aa1092c3c9bfc9d/doc.go#L22-L24
[1]: https://github.com/cilium/ebpf/blob/d1a52333f2c0fed085f8d742a5a3c164795d8492/collection.go#L320-L321
[2]: https://github.com/danobi/tc_tailcall_repro


Daniel Xu (2):
  net: bpf: Make xdp and cls_bpf use bpf_prog_put_dev()
  bpf: Take a uref on BPF_MAP_TYPE_PROG_ARRAY maps during dev attachment

 include/linux/bpf.h  |  1 +
 kernel/bpf/devmap.c  |  8 ++++----
 kernel/bpf/syscall.c | 46 +++++++++++++++++++++++++++++++++++++++++++-
 net/core/dev.c       | 16 +++++++--------
 net/sched/cls_bpf.c  |  4 ++--
 5 files changed, 60 insertions(+), 15 deletions(-)

-- 
2.41.0

[RFC PATCH bpf-next 1/2] net: bpf: Make xdp and cls_bpf use bpf_prog_put_dev()

[Daniel Xu]

This commit adds a stubbed bpf_prog_put_dev() that is symmetric to
bpf_prog_get_type_dev() such that all bpf device attachments are using a
*_dev() API.

This gives core bpf the ability to do special refcnt handling for device
attachments.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 include/linux/bpf.h  |  1 +
 kernel/bpf/devmap.c  |  8 ++++----
 kernel/bpf/syscall.c |  6 ++++++
 net/core/dev.c       | 16 ++++++++--------
 net/sched/cls_bpf.c  |  4 ++--
 5 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index eced6400f778..08269ad8cc45 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2030,6 +2030,7 @@ void bpf_prog_sub(struct bpf_prog *prog, int i);
 void bpf_prog_inc(struct bpf_prog *prog);
 struct bpf_prog * __must_check bpf_prog_inc_not_zero(struct bpf_prog *prog);
 void bpf_prog_put(struct bpf_prog *prog);
+void bpf_prog_put_dev(struct bpf_prog *prog);
 
 void bpf_prog_free_id(struct bpf_prog *prog);
 void bpf_map_free_id(struct bpf_map *map);
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 4d42f6ed6c11..b5d33a87a560 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -212,7 +212,7 @@ static void dev_map_free(struct bpf_map *map)
 			hlist_for_each_entry_safe(dev, next, head, index_hlist) {
 				hlist_del_rcu(&dev->index_hlist);
 				if (dev->xdp_prog)
-					bpf_prog_put(dev->xdp_prog);
+					bpf_prog_put_dev(dev->xdp_prog);
 				dev_put(dev->dev);
 				kfree(dev);
 			}
@@ -228,7 +228,7 @@ static void dev_map_free(struct bpf_map *map)
 				continue;
 
 			if (dev->xdp_prog)
-				bpf_prog_put(dev->xdp_prog);
+				bpf_prog_put_dev(dev->xdp_prog);
 			dev_put(dev->dev);
 			kfree(dev);
 		}
@@ -800,7 +800,7 @@ static void __dev_map_entry_free(struct rcu_head *rcu)
 
 	dev = container_of(rcu, struct bpf_dtab_netdev, rcu);
 	if (dev->xdp_prog)
-		bpf_prog_put(dev->xdp_prog);
+		bpf_prog_put_dev(dev->xdp_prog);
 	dev_put(dev->dev);
 	kfree(dev);
 }
@@ -884,7 +884,7 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
 
 	return dev;
 err_put_prog:
-	bpf_prog_put(prog);
+	bpf_prog_put_dev(prog);
 err_put_dev:
 	dev_put(dev->dev);
 err_out:
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 10666d17b9e3..d8e5530598f3 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2164,6 +2164,12 @@ void bpf_prog_put(struct bpf_prog *prog)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_put);
 
+void bpf_prog_put_dev(struct bpf_prog *prog)
+{
+	bpf_prog_put(prog);
+}
+EXPORT_SYMBOL_GPL(bpf_prog_put_dev);
+
 static int bpf_prog_release(struct inode *inode, struct file *filp)
 {
 	struct bpf_prog *prog = filp->private_data;
diff --git a/net/core/dev.c b/net/core/dev.c
index 17e6281e408c..ed0ece344416 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5676,7 +5676,7 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
 	case XDP_SETUP_PROG:
 		rcu_assign_pointer(dev->xdp_prog, new);
 		if (old)
-			bpf_prog_put(old);
+			bpf_prog_put_dev(old);
 
 		if (old && !new) {
 			static_branch_dec(&generic_xdp_needed_key);
@@ -9167,7 +9167,7 @@ static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
 	err = bpf_op(dev, &xdp);
 	if (err) {
 		if (prog)
-			bpf_prog_put(prog);
+			bpf_prog_put_dev(prog);
 		return err;
 	}
 
@@ -9202,7 +9202,7 @@ static void dev_xdp_uninstall(struct net_device *dev)
 		if (link)
 			link->dev = NULL;
 		else
-			bpf_prog_put(prog);
+			bpf_prog_put_dev(prog);
 
 		dev_xdp_set_link(dev, mode, NULL);
 	}
@@ -9326,7 +9326,7 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
 	else
 		dev_xdp_set_prog(dev, mode, new_prog);
 	if (cur_prog)
-		bpf_prog_put(cur_prog);
+		bpf_prog_put_dev(cur_prog);
 
 	return 0;
 }
@@ -9445,7 +9445,7 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
 
 	if (old_prog == new_prog) {
 		/* no-op, don't disturb drivers */
-		bpf_prog_put(new_prog);
+		bpf_prog_put_dev(new_prog);
 		goto out_unlock;
 	}
 
@@ -9457,7 +9457,7 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
 		goto out_unlock;
 
 	old_prog = xchg(&link->prog, new_prog);
-	bpf_prog_put(old_prog);
+	bpf_prog_put_dev(old_prog);
 
 out_unlock:
 	rtnl_unlock();
@@ -9568,9 +9568,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 
 err_out:
 	if (err && new_prog)
-		bpf_prog_put(new_prog);
+		bpf_prog_put_dev(new_prog);
 	if (old_prog)
-		bpf_prog_put(old_prog);
+		bpf_prog_put_dev(old_prog);
 	return err;
 }
 
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 382c7a71f81f..20129d73dab4 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -258,7 +258,7 @@ static int cls_bpf_init(struct tcf_proto *tp)
 static void cls_bpf_free_parms(struct cls_bpf_prog *prog)
 {
 	if (cls_bpf_is_ebpf(prog))
-		bpf_prog_put(prog->filter);
+		bpf_prog_put_dev(prog->filter);
 	else
 		bpf_prog_destroy(prog->filter);
 
@@ -391,7 +391,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
 	if (tb[TCA_BPF_NAME]) {
 		name = nla_memdup(tb[TCA_BPF_NAME], GFP_KERNEL);
 		if (!name) {
-			bpf_prog_put(fp);
+			bpf_prog_put_dev(fp);
 			return -ENOMEM;
 		}
 	}
-- 
2.41.0

Re: [RFC PATCH bpf-next 0/2] Improve prog array uref semantics

[Daniel Xu]

Hey Daniel,

On Wed, Aug 23, 2023, at 9:08 AM, Daniel Xu wrote:
> This patchset changes the behavior of TC and XDP hooks during attachment
> such that any BPF_MAP_TYPE_PROG_ARRAY that the prog uses has an extra
> uref taken.
>
> The goal behind this change is to try and prevent confusion for the
> majority of use cases. The current behavior where when the last uref is
> dropped the prog array map is emptied is quite confusing. Confusing
> enough for there to be multiple references to it in ebpf-go [0][1].
>
> Completely solving the problem is difficult. As stated in c9da161c6517
> ("bpf: fix clearing on persistent program array maps"), it is
> difficult-to-impossible to walk the full dependency graph b/c it is too
> dynamic.
>
> However in practice, I've found that all progs in a tailcall chain
> share the same prog array map. Knowing that, if we take a uref on any
> used prog array map when the program is attached, we can simplify the
> majority use case and make it more ergonomic.
>
> I'll be the first to admit this is not a very clean solution. It does
> not fully solve the problem. Nor does it make overall logic any simpler.
> But I do think it makes a pretty big usability hole slightly smaller.
>
> I've done some basic testing using a repro program [3] I wrote to debug
> the original issue that eventually led me to this patchset. If we wanna
> move forward with this approach, I'll resend with selftests.
>
> [0]: 
> https://github.com/cilium/ebpf/blob/01ebd4c1e2b9f8b3dd4fd2382aa1092c3c9bfc9d/doc.go#L22-L24
> [1]: 
> https://github.com/cilium/ebpf/blob/d1a52333f2c0fed085f8d742a5a3c164795d8492/collection.go#L320-L321
> [2]: https://github.com/danobi/tc_tailcall_repro

I recently remembered about this again. Was suggested I poke you in case you're interested.
I looked again and I think this is kinda a neat hack. I probably won't have time to pick this back
up either way.

Re: [RFC PATCH bpf-next 0/2] Improve prog array uref semantics

[Alexei Starovoitov]

On Tue, Oct 29, 2024 at 11:36 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Hey Daniel,
>
> On Wed, Aug 23, 2023, at 9:08 AM, Daniel Xu wrote:
> > This patchset changes the behavior of TC and XDP hooks during attachment
> > such that any BPF_MAP_TYPE_PROG_ARRAY that the prog uses has an extra
> > uref taken.
> >
> > The goal behind this change is to try and prevent confusion for the
> > majority of use cases. The current behavior where when the last uref is
> > dropped the prog array map is emptied is quite confusing. Confusing
> > enough for there to be multiple references to it in ebpf-go [0][1].
> >
> > Completely solving the problem is difficult. As stated in c9da161c6517
> > ("bpf: fix clearing on persistent program array maps"), it is
> > difficult-to-impossible to walk the full dependency graph b/c it is too
> > dynamic.
> >
> > However in practice, I've found that all progs in a tailcall chain
> > share the same prog array map. Knowing that, if we take a uref on any
> > used prog array map when the program is attached, we can simplify the
> > majority use case and make it more ergonomic.

Are you proposing to inc map uref when prog is attached?

But that re-adds the circular dependency that uref concept is solving.
When prog is inserted into prog array prog refcnt is incremented.
So if prog also incremented uref. The user space can exit
but prog array and progs will stay there though nothing is using them.
I guess I'm missing the idea.

> >
> > I'll be the first to admit this is not a very clean solution. It does
> > not fully solve the problem. Nor does it make overall logic any simpler.
> > But I do think it makes a pretty big usability hole slightly smaller.
> >
> > I've done some basic testing using a repro program [3] I wrote to debug
> > the original issue that eventually led me to this patchset. If we wanna
> > move forward with this approach, I'll resend with selftests.
> >
> > [0]:
> > https://github.com/cilium/ebpf/blob/01ebd4c1e2b9f8b3dd4fd2382aa1092c3c9bfc9d/doc.go#L22-L24
> > [1]:
> > https://github.com/cilium/ebpf/blob/d1a52333f2c0fed085f8d742a5a3c164795d8492/collection.go#L320-L321
> > [2]: https://github.com/danobi/tc_tailcall_repro
>
> I recently remembered about this again. Was suggested I poke you in case you're interested.
> I looked again and I think this is kinda a neat hack. I probably won't have time to pick this back
> up either way.
>

Re: [RFC PATCH bpf-next 0/2] Improve prog array uref semantics

[Daniel Xu]

On Sat, Nov 16, 2024, at 2:17 PM, Alexei Starovoitov wrote:
> On Tue, Oct 29, 2024 at 11:36 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>>
>> Hey Daniel,
>>
>> On Wed, Aug 23, 2023, at 9:08 AM, Daniel Xu wrote:
>> > This patchset changes the behavior of TC and XDP hooks during attachment
>> > such that any BPF_MAP_TYPE_PROG_ARRAY that the prog uses has an extra
>> > uref taken.
>> >
>> > The goal behind this change is to try and prevent confusion for the
>> > majority of use cases. The current behavior where when the last uref is
>> > dropped the prog array map is emptied is quite confusing. Confusing
>> > enough for there to be multiple references to it in ebpf-go [0][1].
>> >
>> > Completely solving the problem is difficult. As stated in c9da161c6517
>> > ("bpf: fix clearing on persistent program array maps"), it is
>> > difficult-to-impossible to walk the full dependency graph b/c it is too
>> > dynamic.
>> >
>> > However in practice, I've found that all progs in a tailcall chain
>> > share the same prog array map. Knowing that, if we take a uref on any
>> > used prog array map when the program is attached, we can simplify the
>> > majority use case and make it more ergonomic.
>
> Are you proposing to inc map uref when prog is attached?
>
> But that re-adds the circular dependency that uref concept is solving.
> When prog is inserted into prog array prog refcnt is incremented.
> So if prog also incremented uref. The user space can exit
> but prog array and progs will stay there though nothing is using them.
> I guess I'm missing the idea.

IIRC the old-style tc/xdp attachment is the one incrementing the uref. Once
whatever program there is detached the uref is dropped. So I don't think
any circular refs can happen unless a prog can somehow prevent its own
detachment.

Could be mis-remembering though.

Thanks,
Daniel

Re: [RFC PATCH bpf-next 0/2] Improve prog array uref semantics

[Alexei Starovoitov]

On Wed, Nov 20, 2024 at 7:55 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
>
>
> On Sat, Nov 16, 2024, at 2:17 PM, Alexei Starovoitov wrote:
> > On Tue, Oct 29, 2024 at 11:36 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >>
> >> Hey Daniel,
> >>
> >> On Wed, Aug 23, 2023, at 9:08 AM, Daniel Xu wrote:
> >> > This patchset changes the behavior of TC and XDP hooks during attachment
> >> > such that any BPF_MAP_TYPE_PROG_ARRAY that the prog uses has an extra
> >> > uref taken.
> >> >
> >> > The goal behind this change is to try and prevent confusion for the
> >> > majority of use cases. The current behavior where when the last uref is
> >> > dropped the prog array map is emptied is quite confusing. Confusing
> >> > enough for there to be multiple references to it in ebpf-go [0][1].
> >> >
> >> > Completely solving the problem is difficult. As stated in c9da161c6517
> >> > ("bpf: fix clearing on persistent program array maps"), it is
> >> > difficult-to-impossible to walk the full dependency graph b/c it is too
> >> > dynamic.
> >> >
> >> > However in practice, I've found that all progs in a tailcall chain
> >> > share the same prog array map. Knowing that, if we take a uref on any
> >> > used prog array map when the program is attached, we can simplify the
> >> > majority use case and make it more ergonomic.
> >
> > Are you proposing to inc map uref when prog is attached?
> >
> > But that re-adds the circular dependency that uref concept is solving.
> > When prog is inserted into prog array prog refcnt is incremented.
> > So if prog also incremented uref. The user space can exit
> > but prog array and progs will stay there though nothing is using them.
> > I guess I'm missing the idea.
>
> IIRC the old-style tc/xdp attachment is the one incrementing the uref.

uref is incremented when FD is given to user space and
file->release() callback decrements uref.

I don't think any of the attach operations mess with uref.
At least they shouldn't.

> Once
> whatever program there is detached the uref is dropped. So I don't think
> any circular refs can happen unless a prog can somehow prevent its own
> detachment.
>
> Could be mis-remembering though.
>
> Thanks,
> Daniel

Re: [RFC PATCH bpf-next 0/2] Improve prog array uref semantics

[Daniel Xu]

On Wed, Nov 20, 2024, at 8:07 AM, Alexei Starovoitov wrote:
> On Wed, Nov 20, 2024 at 7:55 AM Daniel Xu <dxu@dxuuu.xyz> wrote:
>>
>>
>>
>> On Sat, Nov 16, 2024, at 2:17 PM, Alexei Starovoitov wrote:
>> > On Tue, Oct 29, 2024 at 11:36 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>> >>
>> >> Hey Daniel,
>> >>
>> >> On Wed, Aug 23, 2023, at 9:08 AM, Daniel Xu wrote:
>> >> > This patchset changes the behavior of TC and XDP hooks during attachment
>> >> > such that any BPF_MAP_TYPE_PROG_ARRAY that the prog uses has an extra
>> >> > uref taken.
>> >> >
>> >> > The goal behind this change is to try and prevent confusion for the
>> >> > majority of use cases. The current behavior where when the last uref is
>> >> > dropped the prog array map is emptied is quite confusing. Confusing
>> >> > enough for there to be multiple references to it in ebpf-go [0][1].
>> >> >
>> >> > Completely solving the problem is difficult. As stated in c9da161c6517
>> >> > ("bpf: fix clearing on persistent program array maps"), it is
>> >> > difficult-to-impossible to walk the full dependency graph b/c it is too
>> >> > dynamic.
>> >> >
>> >> > However in practice, I've found that all progs in a tailcall chain
>> >> > share the same prog array map. Knowing that, if we take a uref on any
>> >> > used prog array map when the program is attached, we can simplify the
>> >> > majority use case and make it more ergonomic.
>> >
>> > Are you proposing to inc map uref when prog is attached?
>> >
>> > But that re-adds the circular dependency that uref concept is solving.
>> > When prog is inserted into prog array prog refcnt is incremented.
>> > So if prog also incremented uref. The user space can exit
>> > but prog array and progs will stay there though nothing is using them.
>> > I guess I'm missing the idea.
>>
>> IIRC the old-style tc/xdp attachment is the one incrementing the uref.
>
> uref is incremented when FD is given to user space and
> file->release() callback decrements uref.
>
> I don't think any of the attach operations mess with uref.
> At least they shouldn't.

None yet. My patch was adding it. It's fine if it's too much of a hack -
was just an idea.