diff mbox series

[bpf,1/7] xdp: remove the xdp_attachment_flags_ok() callback

Message ID 160703131819.162669.2776807312730670823.stgit@toke.dk (mailing list archive)
State New, archived
Headers show
Series selftests/bpf: Restore test_offload.py to working order | expand

Commit Message

Toke Høiland-Jørgensen Dec. 3, 2020, 9:35 p.m. UTC 
From: Toke Høiland-Jørgensen <toke@redhat.com>

Since commit 7f0a838254bd ("bpf, xdp: Maintain info on attached XDP BPF
programs in net_device"), the XDP program attachment info is now maintained
in the core code. This interacts badly with the xdp_attachment_flags_ok()
check that prevents unloading an XDP program with different load flags than
it was loaded with. In practice, two kinds of failures are seen:

- An XDP program loaded without specifying a mode (and which then ends up
  in driver mode) cannot be unloaded if the program mode is specified on
  unload.

- The dev_xdp_uninstall() hook always calls the driver callback with the
  mode set to the type of the program but an empty flags argument, which
  means the flags_ok() check prevents the program from being removed,
  leading to bpf prog reference leaks.

Since we offloaded and non-offloaded programs can co-exist there doesn't
really seem to be any reason for the check anyway, and it's only used in
three drivers so let's just get rid of the callback entirely.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../net/ethernet/netronome/nfp/nfp_net_common.c    |    6 ------
 drivers/net/ethernet/ti/cpsw_priv.c                |    3 ---
 drivers/net/netdevsim/bpf.c                        |    3 ---
 include/net/xdp.h                                  |    2 --
 net/core/xdp.c                                     |   12 ------------
 5 files changed, 26 deletions(-)

Comments

Jakub Kicinski Dec. 4, 2020, 1:42 a.m. UTC | #1 
On Thu, 03 Dec 2020 22:35:18 +0100 Toke Høiland-Jørgensen wrote:
> Since we offloaded and non-offloaded programs can co-exist there doesn't
> really seem to be any reason for the check anyway, and it's only used in
> three drivers so let's just get rid of the callback entirely.

I don't remember exactly now, but I think the concern was that using 
the unspecified mode is pretty ambiguous when interface has multiple
programs attached.
Toke Høiland-Jørgensen Dec. 4, 2020, 9:38 a.m. UTC | #2 
Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 03 Dec 2020 22:35:18 +0100 Toke Høiland-Jørgensen wrote:
>> Since we offloaded and non-offloaded programs can co-exist there doesn't
>> really seem to be any reason for the check anyway, and it's only used in
>> three drivers so let's just get rid of the callback entirely.
>
> I don't remember exactly now, but I think the concern was that using 
> the unspecified mode is pretty ambiguous when interface has multiple
> programs attached.

Right. I did scratch my head a bit for why the check was there in the
first place, but that makes sense, actually :)

So how about we disallow unload without specifying a mode, but only if
more than one program is loaded? Since the core code tracks all the
programs now, this could just be enforced there and we would avoid all
the weird interactions with the drivers trying to enforce it...

-Toke
Jakub Kicinski Dec. 4, 2020, 4:48 p.m. UTC | #3 
On Fri, 04 Dec 2020 10:38:06 +0100 Toke Høiland-Jørgensen wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> > On Thu, 03 Dec 2020 22:35:18 +0100 Toke Høiland-Jørgensen wrote:  
> >> Since we offloaded and non-offloaded programs can co-exist there doesn't
> >> really seem to be any reason for the check anyway, and it's only used in
> >> three drivers so let's just get rid of the callback entirely.  
> >
> > I don't remember exactly now, but I think the concern was that using 
> > the unspecified mode is pretty ambiguous when interface has multiple
> > programs attached.  
> 
> Right. I did scratch my head a bit for why the check was there in the
> first place, but that makes sense, actually :)
> 
> So how about we disallow unload without specifying a mode, but only if
> more than one program is loaded?

Are you including replacing as a form of unload? :)

IMHO the simpler the definition of the API / constraint the better.
"You must specify the same flags" is pretty simple, as is copying 
the old behavior rather than trying to come up with new rules.

But up to you, I don't mind either way, really..

> Since the core code tracks all the programs now, this could just be
> enforced there and we would avoid all the weird interactions with the
> drivers trying to enforce it...

Yup, enforcing in the core now would make perfect sense.
Toke Høiland-Jørgensen Dec. 4, 2020, 5:12 p.m. UTC | #4 
Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 04 Dec 2020 10:38:06 +0100 Toke Høiland-Jørgensen wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> > On Thu, 03 Dec 2020 22:35:18 +0100 Toke Høiland-Jørgensen wrote:  
>> >> Since we offloaded and non-offloaded programs can co-exist there doesn't
>> >> really seem to be any reason for the check anyway, and it's only used in
>> >> three drivers so let's just get rid of the callback entirely.  
>> >
>> > I don't remember exactly now, but I think the concern was that using 
>> > the unspecified mode is pretty ambiguous when interface has multiple
>> > programs attached.  
>> 
>> Right. I did scratch my head a bit for why the check was there in the
>> first place, but that makes sense, actually :)
>> 
>> So how about we disallow unload without specifying a mode, but only if
>> more than one program is loaded?
>
> Are you including replacing as a form of unload? :)

Yeah, that's what I ended up with (in v2): Any time there are multiple
programs loaded, callers have to specify a mode flag to avoid ambiguity.

> IMHO the simpler the definition of the API / constraint the better.
> "You must specify the same flags" is pretty simple, as is copying the
> old behavior rather than trying to come up with new rules.
>
> But up to you, I don't mind either way, really..

Well that old behaviour was what led me to investigate this in the first
place: If you just look at a program that's loaded, see it's in driver
mode, and then try to unload it with flags set to XDP_MODE_DRV, it will
sometimes work and sometimes not depending on what flags that program
happened to have been loaded with. In libxdp this is exactly what we do
(look at the loaded program and set the corresponding mode flag), so I
ended up getting some really odd bug reports...

So I really don't want to keep the current behaviour; if what I propose
in v2 is OK with you I think we should just go with that :)

-Toke
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index b150da43adb2..437226866ce8 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3562,9 +3562,6 @@  static int nfp_net_xdp_setup_drv(struct nfp_net *nn, struct netdev_bpf *bpf)
 	struct nfp_net_dp *dp;
 	int err;
 
-	if (!xdp_attachment_flags_ok(&nn->xdp, bpf))
-		return -EBUSY;
-
 	if (!prog == !nn->dp.xdp_prog) {
 		WRITE_ONCE(nn->dp.xdp_prog, prog);
 		xdp_attachment_setup(&nn->xdp, bpf);
@@ -3593,9 +3590,6 @@  static int nfp_net_xdp_setup_hw(struct nfp_net *nn, struct netdev_bpf *bpf)
 {
 	int err;
 
-	if (!xdp_attachment_flags_ok(&nn->xdp_hw, bpf))
-		return -EBUSY;
-
 	err = nfp_app_xdp_offload(nn->app, nn, bpf->prog, bpf->extack);
 	if (err)
 		return err;
diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c
index 31c5e36ff706..424e644724e4 100644
--- a/drivers/net/ethernet/ti/cpsw_priv.c
+++ b/drivers/net/ethernet/ti/cpsw_priv.c
@@ -1265,9 +1265,6 @@  static int cpsw_xdp_prog_setup(struct cpsw_priv *priv, struct netdev_bpf *bpf)
 	if (!priv->xdpi.prog && !prog)
 		return 0;
 
-	if (!xdp_attachment_flags_ok(&priv->xdpi, bpf))
-		return -EBUSY;
-
 	WRITE_ONCE(priv->xdp_prog, prog);
 
 	xdp_attachment_setup(&priv->xdpi, bpf);
diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
index 2e90512f3bbe..85546664bdd5 100644
--- a/drivers/net/netdevsim/bpf.c
+++ b/drivers/net/netdevsim/bpf.c
@@ -190,9 +190,6 @@  nsim_xdp_set_prog(struct netdevsim *ns, struct netdev_bpf *bpf,
 {
 	int err;
 
-	if (!xdp_attachment_flags_ok(xdp, bpf))
-		return -EBUSY;
-
 	if (bpf->command == XDP_SETUP_PROG && !ns->bpf_xdpdrv_accept) {
 		NSIM_EA(bpf->extack, "driver XDP disabled in DebugFS");
 		return -EOPNOTSUPP;
diff --git a/include/net/xdp.h b/include/net/xdp.h
index 3814fb631d52..9dab2bc6f187 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -240,8 +240,6 @@  struct xdp_attachment_info {
 };
 
 struct netdev_bpf;
-bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
-			     struct netdev_bpf *bpf);
 void xdp_attachment_setup(struct xdp_attachment_info *info,
 			  struct netdev_bpf *bpf);
 
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 491ad569a79c..d900cebc0acd 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -403,18 +403,6 @@  void __xdp_release_frame(void *data, struct xdp_mem_info *mem)
 }
 EXPORT_SYMBOL_GPL(__xdp_release_frame);
 
-bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
-			     struct netdev_bpf *bpf)
-{
-	if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES) {
-		NL_SET_ERR_MSG(bpf->extack,
-			       "program loaded with different flags");
-		return false;
-	}
-	return true;
-}
-EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);
-
 void xdp_attachment_setup(struct xdp_attachment_info *info,
 			  struct netdev_bpf *bpf)
 {