[v2] Bonding: returns detailed error about XDP failures

[PATCHv2 net-next 2/3] bonding: use correct return value

Posted by Hangbin Liu 1 year, 3 months ago

When a slave already has an XDP program loaded, the correct return value
should be -EEXIST instead of -EOPNOTSUPP.

Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f0f76b6ac8be..6887a867fe8b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		if (dev_xdp_prog_count(slave_dev) > 0) {
 			SLAVE_NL_ERR(dev, slave_dev, extack,
 				     "Slave has XDP program loaded, please unload before enslaving");
-			err = -EOPNOTSUPP;
+			err = -EEXIST;
 			goto err;
 		}
 
-- 
2.46.0

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

Posted by Toke Høiland-Jørgensen 1 year, 3 months ago

Hangbin Liu <liuhangbin@gmail.com> writes:

> When a slave already has an XDP program loaded, the correct return value
> should be -EEXIST instead of -EOPNOTSUPP.
>
> Fixes: 9e2ee5c7e7c3 ("net, bonding: Add XDP support to the bonding driver")
> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index f0f76b6ac8be..6887a867fe8b 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>  		if (dev_xdp_prog_count(slave_dev) > 0) {
>  			SLAVE_NL_ERR(dev, slave_dev, extack,
>  				     "Slave has XDP program loaded, please unload before enslaving");
> -			err = -EOPNOTSUPP;
> +			err = -EEXIST;

Hmm, this has been UAPI since kernel 5.15, so can we really change it
now? What's the purpose of changing it, anyway?

-Toke

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

Posted by Hangbin Liu 1 year, 3 months ago

On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index f0f76b6ac8be..6887a867fe8b 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> >  				     "Slave has XDP program loaded, please unload before enslaving");
> > -			err = -EOPNOTSUPP;
> > +			err = -EEXIST;
> 
> Hmm, this has been UAPI since kernel 5.15, so can we really change it
> now? What's the purpose of changing it, anyway?

I just think it should return EXIST when the error is "Slave has XDP program
loaded". No special reason. If all others think we should not change it, I
can drop this patch.

Thanks
Hangbin

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

Posted by Simon Horman 1 year, 3 months ago

On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > index f0f76b6ac8be..6887a867fe8b 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> > >  				     "Slave has XDP program loaded, please unload before enslaving");
> > > -			err = -EOPNOTSUPP;
> > > +			err = -EEXIST;
> > 
> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
> > now? What's the purpose of changing it, anyway?
> 
> I just think it should return EXIST when the error is "Slave has XDP program
> loaded". No special reason. If all others think we should not change it, I
> can drop this patch.

Hi Toke,

Could you add some colour to what extent user's might rely on this error code?

Basically I think that if they do then we shouldn't change this.

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

Posted by Toke Høiland-Jørgensen 1 year, 3 months ago

Simon Horman <horms@kernel.org> writes:

> On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
>> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
>> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> > > index f0f76b6ac8be..6887a867fe8b 100644
>> > > --- a/drivers/net/bonding/bond_main.c
>> > > +++ b/drivers/net/bonding/bond_main.c
>> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
>> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
>> > >  				     "Slave has XDP program loaded, please unload before enslaving");
>> > > -			err = -EOPNOTSUPP;
>> > > +			err = -EEXIST;
>> > 
>> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
>> > now? What's the purpose of changing it, anyway?
>> 
>> I just think it should return EXIST when the error is "Slave has XDP program
>> loaded". No special reason. If all others think we should not change it, I
>> can drop this patch.
>
> Hi Toke,
>
> Could you add some colour to what extent user's might rely on this error code?
>
> Basically I think that if they do then we shouldn't change this.

Well, that's the trouble with UAPI, we don't really know. In libxdp and
xdp-tools we look at the return code to provide a nicer error message,
like:

https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L615

and as a signal to fall back to loading the programme without a dispatcher:

https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1824

Both of these cases would be unaffected (or even improved) by this
patch, so in that sense I don't have a concrete objection, just a
general "userspace may react to this". In other words, my concern is
more of a general "we don't know, so this seems risky". If any of you
have more information about how bonding XDP is generally used, that may
help get a better idea of this?

-Toke

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

Posted by Simon Horman 1 year, 3 months ago

On Fri, Oct 18, 2024 at 01:29:30PM +0200, Toke Høiland-Jørgensen wrote:
> Simon Horman <horms@kernel.org> writes:
> 
> > On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
> >> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> >> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >> > > index f0f76b6ac8be..6887a867fe8b 100644
> >> > > --- a/drivers/net/bonding/bond_main.c
> >> > > +++ b/drivers/net/bonding/bond_main.c
> >> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> >> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> >> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> >> > >  				     "Slave has XDP program loaded, please unload before enslaving");
> >> > > -			err = -EOPNOTSUPP;
> >> > > +			err = -EEXIST;
> >> > 
> >> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
> >> > now? What's the purpose of changing it, anyway?
> >> 
> >> I just think it should return EXIST when the error is "Slave has XDP program
> >> loaded". No special reason. If all others think we should not change it, I
> >> can drop this patch.
> >
> > Hi Toke,
> >
> > Could you add some colour to what extent user's might rely on this error code?
> >
> > Basically I think that if they do then we shouldn't change this.
> 
> Well, that's the trouble with UAPI, we don't really know. In libxdp and
> xdp-tools we look at the return code to provide a nicer error message,
> like:
> 
> https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L615
> 
> and as a signal to fall back to loading the programme without a dispatcher:
> 
> https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1824
> 
> Both of these cases would be unaffected (or even improved) by this
> patch, so in that sense I don't have a concrete objection, just a
> general "userspace may react to this". In other words, my concern is
> more of a general "we don't know, so this seems risky". If any of you
> have more information about how bonding XDP is generally used, that may
> help get a better idea of this?

Yes, that is the trouble with the UAPI. I was hoping you might be able to
provide the clarity you ask for above. But alas, things are as clear as
mud.

In lieu of more information I suggest caution and dropping this change for
now.

-- 
pw-bot: cr

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

Posted by Hangbin Liu 1 year, 3 months ago

On Fri, Oct 18, 2024 at 03:21:04PM +0100, Simon Horman wrote:
> On Fri, Oct 18, 2024 at 01:29:30PM +0200, Toke Høiland-Jørgensen wrote:
> > Simon Horman <horms@kernel.org> writes:
> > 
> > > On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
> > >> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> > >> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > >> > > index f0f76b6ac8be..6887a867fe8b 100644
> > >> > > --- a/drivers/net/bonding/bond_main.c
> > >> > > +++ b/drivers/net/bonding/bond_main.c
> > >> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > >> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> > >> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> > >> > >  				     "Slave has XDP program loaded, please unload before enslaving");
> > >> > > -			err = -EOPNOTSUPP;
> > >> > > +			err = -EEXIST;
> > >> > 
> > >> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
> > >> > now? What's the purpose of changing it, anyway?
> > >> 
> > >> I just think it should return EXIST when the error is "Slave has XDP program
> > >> loaded". No special reason. If all others think we should not change it, I
> > >> can drop this patch.
> > >
> > > Hi Toke,
> > >
> > > Could you add some colour to what extent user's might rely on this error code?
> > >
> > > Basically I think that if they do then we shouldn't change this.
> > 
> > Well, that's the trouble with UAPI, we don't really know. In libxdp and
> > xdp-tools we look at the return code to provide a nicer error message,
> > like:
> > 
> > https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L615
> > 
> > and as a signal to fall back to loading the programme without a dispatcher:
> > 
> > https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1824
> > 
> > Both of these cases would be unaffected (or even improved) by this
> > patch, so in that sense I don't have a concrete objection, just a
> > general "userspace may react to this". In other words, my concern is
> > more of a general "we don't know, so this seems risky". If any of you
> > have more information about how bonding XDP is generally used, that may
> > help get a better idea of this?
> 
> Yes, that is the trouble with the UAPI. I was hoping you might be able to
> provide the clarity you ask for above. But alas, things are as clear as
> mud.
> 
> In lieu of more information I suggest caution and dropping this change for
> now.

OK, I will drop this one.

Thanks
Hangbin

Re: [PATCHv2 net-next 2/3] bonding: use correct return value

Posted by Simon Horman 1 year, 3 months ago

On Sat, Oct 19, 2024 at 12:51:00AM +0000, Hangbin Liu wrote:
> On Fri, Oct 18, 2024 at 03:21:04PM +0100, Simon Horman wrote:
> > On Fri, Oct 18, 2024 at 01:29:30PM +0200, Toke Høiland-Jørgensen wrote:
> > > Simon Horman <horms@kernel.org> writes:
> > > 
> > > > On Fri, Oct 18, 2024 at 12:46:18AM +0000, Hangbin Liu wrote:
> > > >> On Thu, Oct 17, 2024 at 04:47:19PM +0200, Toke Høiland-Jørgensen wrote:
> > > >> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > >> > > index f0f76b6ac8be..6887a867fe8b 100644
> > > >> > > --- a/drivers/net/bonding/bond_main.c
> > > >> > > +++ b/drivers/net/bonding/bond_main.c
> > > >> > > @@ -5699,7 +5699,7 @@ static int bond_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > > >> > >  		if (dev_xdp_prog_count(slave_dev) > 0) {
> > > >> > >  			SLAVE_NL_ERR(dev, slave_dev, extack,
> > > >> > >  				     "Slave has XDP program loaded, please unload before enslaving");
> > > >> > > -			err = -EOPNOTSUPP;
> > > >> > > +			err = -EEXIST;
> > > >> > 
> > > >> > Hmm, this has been UAPI since kernel 5.15, so can we really change it
> > > >> > now? What's the purpose of changing it, anyway?
> > > >> 
> > > >> I just think it should return EXIST when the error is "Slave has XDP program
> > > >> loaded". No special reason. If all others think we should not change it, I
> > > >> can drop this patch.
> > > >
> > > > Hi Toke,
> > > >
> > > > Could you add some colour to what extent user's might rely on this error code?
> > > >
> > > > Basically I think that if they do then we shouldn't change this.
> > > 
> > > Well, that's the trouble with UAPI, we don't really know. In libxdp and
> > > xdp-tools we look at the return code to provide a nicer error message,
> > > like:
> > > 
> > > https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L615
> > > 
> > > and as a signal to fall back to loading the programme without a dispatcher:
> > > 
> > > https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1824
> > > 
> > > Both of these cases would be unaffected (or even improved) by this
> > > patch, so in that sense I don't have a concrete objection, just a
> > > general "userspace may react to this". In other words, my concern is
> > > more of a general "we don't know, so this seems risky". If any of you
> > > have more information about how bonding XDP is generally used, that may
> > > help get a better idea of this?
> > 
> > Yes, that is the trouble with the UAPI. I was hoping you might be able to
> > provide the clarity you ask for above. But alas, things are as clear as
> > mud.
> > 
> > In lieu of more information I suggest caution and dropping this change for
> > now.
> 
> OK, I will drop this one.

Thanks.