[PATCH bpf-next] net: Don't uninstall an XDP program when none is installed

[PATCH bpf-next] net: Don't uninstall an XDP program when none is installed

[Maxim Mikityanskiy]

dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
XDP program. It means that the driver's ndo_bpf can be called with
XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
case happens if the user runs `ip link set eth0 xdp off` when there is
no XDP program attached.

The drivers typically perform some heavy operations on XDP_SETUP_PROG,
so they all have to handle this case internally to return early if it
happens. This patch puts this check into the kernel code, so that all
drivers will benefit from it.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
Björn, please take a look at this, Saeed told me you were doing
something related, but I couldn't find it. If this fix is already
covered by your work, please tell about that.

 net/core/dev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 66f7508825bd..68b3e3320ceb 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8089,6 +8089,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 			bpf_prog_put(prog);
 			return -EINVAL;
 		}
+	} else {
+		if (!__dev_xdp_query(dev, bpf_op, query))
+			return 0;
 	}
 
 	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
-- 
2.19.1

Re: [PATCH bpf-next] net: Don't uninstall an XDP program when none is installed

[Björn Töpel]

On 2019-06-12 18:14, Maxim Mikityanskiy wrote:
> dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
> XDP program. It means that the driver's ndo_bpf can be called with
> XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
> case happens if the user runs `ip link set eth0 xdp off` when there is
> no XDP program attached.
> 
> The drivers typically perform some heavy operations on XDP_SETUP_PROG,
> so they all have to handle this case internally to return early if it
> happens. This patch puts this check into the kernel code, so that all
> drivers will benefit from it.
> 
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> ---
> Björn, please take a look at this, Saeed told me you were doing
> something related, but I couldn't find it. If this fix is already
> covered by your work, please tell about that.
>

Yeah, I'm trying make the query code more generic (pull common work out
from the drivers). However, my patch set is still not done (I'll try to
get a v4 out this week), but your improvement is!

I don't see why this should be held back because of my still not
finished work. I like this.

Acked-by: Björn Töpel <bjorn.topel@intel.com>


>   net/core/dev.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 66f7508825bd..68b3e3320ceb 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8089,6 +8089,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>   			bpf_prog_put(prog);
>   			return -EINVAL;
>   		}
> +	} else {
> +		if (!__dev_xdp_query(dev, bpf_op, query))
> +			return 0;
>   	}
>   
>   	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> 

Re: [PATCH bpf-next] net: Don't uninstall an XDP program when none is installed

[Jakub Kicinski]

On Wed, 12 Jun 2019 16:14:18 +0000, Maxim Mikityanskiy wrote:
> dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
> XDP program. It means that the driver's ndo_bpf can be called with
> XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
> case happens if the user runs `ip link set eth0 xdp off` when there is
> no XDP program attached.
> 
> The drivers typically perform some heavy operations on XDP_SETUP_PROG,
> so they all have to handle this case internally to return early if it
> happens. This patch puts this check into the kernel code, so that all
> drivers will benefit from it.
> 
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> ---
> Björn, please take a look at this, Saeed told me you were doing
> something related, but I couldn't find it. If this fix is already
> covered by your work, please tell about that.
> 
>  net/core/dev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 66f7508825bd..68b3e3320ceb 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8089,6 +8089,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  			bpf_prog_put(prog);
>  			return -EINVAL;
>  		}
> +	} else {
> +		if (!__dev_xdp_query(dev, bpf_op, query))
> +			return 0;

This will mask the error if program is installed with different flags.

You driver should do nothing is program installation state did not
change.  I.e.:

	if (!!prog == !!oldprog)

You can't remove the active -> active check anyway, this change should
make no difference.

>  	}
>  
>  	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);

Re: [PATCH bpf-next] net: Don't uninstall an XDP program when none is installed

[Björn Töpel]

On Wed, 12 Jun 2019 at 23:15, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 12 Jun 2019 16:14:18 +0000, Maxim Mikityanskiy wrote:
> > dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
> > XDP program. It means that the driver's ndo_bpf can be called with
> > XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
> > case happens if the user runs `ip link set eth0 xdp off` when there is
> > no XDP program attached.
> >
> > The drivers typically perform some heavy operations on XDP_SETUP_PROG,
> > so they all have to handle this case internally to return early if it
> > happens. This patch puts this check into the kernel code, so that all
> > drivers will benefit from it.
> >
> > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> > ---
> > Björn, please take a look at this, Saeed told me you were doing
> > something related, but I couldn't find it. If this fix is already
> > covered by your work, please tell about that.
> >
> >  net/core/dev.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 66f7508825bd..68b3e3320ceb 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8089,6 +8089,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> >                       bpf_prog_put(prog);
> >                       return -EINVAL;
> >               }
> > +     } else {
> > +             if (!__dev_xdp_query(dev, bpf_op, query))
> > +                     return 0;
>
> This will mask the error if program is installed with different flags.
>

Hmm, probably missing something, but I fail to see how the error is
being masked? This is to catch the NULL-to-NULL case early.

> You driver should do nothing is program installation state did not
> change.  I.e.:
>
>         if (!!prog == !!oldprog)
>
> You can't remove the active -> active check anyway, this change should
> make no difference.
>
> >       }
> >
> >       err = dev_xdp_install(dev, bpf_op, extack, flags, prog);

Re: [PATCH bpf-next] net: Don't uninstall an XDP program when none is installed

[Jakub Kicinski]

On Thu, 13 Jun 2019 11:04:45 +0200, Björn Töpel wrote:
> > > +     } else {
> > > +             if (!__dev_xdp_query(dev, bpf_op, query))
> > > +                     return 0;  
> >
> > This will mask the error if program is installed with different flags.
> >  
> 
> Hmm, probably missing something, but I fail to see how the error is
> being masked? This is to catch the NULL-to-NULL case early.

You're right, it shouldn't matter.

Re: [PATCH bpf-next] net: Don't uninstall an XDP program when none is installed

[Maxim Mikityanskiy]

On 2019-06-12 19:14, Maxim Mikityanskiy wrote:
> dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
> XDP program. It means that the driver's ndo_bpf can be called with
> XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
> case happens if the user runs `ip link set eth0 xdp off` when there is
> no XDP program attached.
> 
> The drivers typically perform some heavy operations on XDP_SETUP_PROG,
> so they all have to handle this case internally to return early if it
> happens. This patch puts this check into the kernel code, so that all
> drivers will benefit from it.
> 
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> ---
> Björn, please take a look at this, Saeed told me you were doing
> something related, but I couldn't find it. If this fix is already
> covered by your work, please tell about that.
> 
>   net/core/dev.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 66f7508825bd..68b3e3320ceb 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8089,6 +8089,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>   			bpf_prog_put(prog);
>   			return -EINVAL;
>   		}
> +	} else {
> +		if (!__dev_xdp_query(dev, bpf_op, query))
> +			return 0;
>   	}
>   
>   	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> 

Alexei, so what about this patch? It's marked as "Changed Requested" in 
patchwork, but Jakub's point looks resolved - I don't see any changes 
required from my side.

Re: [PATCH bpf-next] net: Don't uninstall an XDP program when none is installed

[Daniel Borkmann]

On 07/10/2019 01:16 PM, Maxim Mikityanskiy wrote:
> On 2019-06-12 19:14, Maxim Mikityanskiy wrote:
>> dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
>> XDP program. It means that the driver's ndo_bpf can be called with
>> XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
>> case happens if the user runs `ip link set eth0 xdp off` when there is
>> no XDP program attached.
>>
>> The drivers typically perform some heavy operations on XDP_SETUP_PROG,
>> so they all have to handle this case internally to return early if it
>> happens. This patch puts this check into the kernel code, so that all
>> drivers will benefit from it.
>>
>> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
>> ---
>> Björn, please take a look at this, Saeed told me you were doing
>> something related, but I couldn't find it. If this fix is already
>> covered by your work, please tell about that.
>>
>>   net/core/dev.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 66f7508825bd..68b3e3320ceb 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -8089,6 +8089,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>>   			bpf_prog_put(prog);
>>   			return -EINVAL;
>>   		}
>> +	} else {
>> +		if (!__dev_xdp_query(dev, bpf_op, query))
>> +			return 0;
>>   	}
>>   
>>   	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
>>
> 
> Alexei, so what about this patch? It's marked as "Changed Requested" in 
> patchwork, but Jakub's point looks resolved - I don't see any changes 
> required from my side.

I believe part of Jakub's feedback was that if we make this generic that this
does not generally address the case where both prog pointers are equal (whether
NULL or non-NULL).

Thanks,
Daniel

[PATCH bpf-next v2] net: Don't call XDP_SETUP_PROG when nothing is changed

[Maxim Mikityanskiy]

Don't uninstall an XDP program when none is installed, and don't install
an XDP program that has the same ID as the one already installed.

dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
XDP program. It means that the driver's ndo_bpf can be called with
XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
case happens if the user runs `ip link set eth0 xdp off` when there is
no XDP program attached.

The symmetrical case is possible when the user tries to set the program
that is already set.

The drivers typically perform some heavy operations on XDP_SETUP_PROG,
so they all have to handle these cases internally to return early if
they happen. This patch puts this check into the kernel code, so that
all drivers will benefit from it.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
v2 changes: Cover the case when the program is set, but isn't changed.

 net/core/dev.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 49589ed2018d..b1afafee3e2a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8126,12 +8126,15 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		bpf_chk = generic_xdp_install;
 
 	if (fd >= 0) {
+		u32 prog_id;
+
 		if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
 			NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time");
 			return -EEXIST;
 		}
-		if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) &&
-		    __dev_xdp_query(dev, bpf_op, query)) {
+
+		prog_id = __dev_xdp_query(dev, bpf_op, query);
+		if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && prog_id) {
 			NL_SET_ERR_MSG(extack, "XDP program already attached");
 			return -EBUSY;
 		}
@@ -8146,6 +8149,14 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 			bpf_prog_put(prog);
 			return -EINVAL;
 		}
+
+		if (prog->aux->id == prog_id) {
+			bpf_prog_put(prog);
+			return 0;
+		}
+	} else {
+		if (!__dev_xdp_query(dev, bpf_op, query))
+			return 0;
 	}
 
 	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
-- 
2.20.1

Re: [PATCH bpf-next v2] net: Don't call XDP_SETUP_PROG when nothing is changed

[Jonathan Lemon]

On 14 Aug 2019, at 7:34, Maxim Mikityanskiy wrote:

> Don't uninstall an XDP program when none is installed, and don't install
> an XDP program that has the same ID as the one already installed.
>
> dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
> XDP program. It means that the driver's ndo_bpf can be called with
> XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
> case happens if the user runs `ip link set eth0 xdp off` when there is
> no XDP program attached.
>
> The symmetrical case is possible when the user tries to set the program
> that is already set.
>
> The drivers typically perform some heavy operations on XDP_SETUP_PROG,
> so they all have to handle these cases internally to return early if
> they happen. This patch puts this check into the kernel code, so that
> all drivers will benefit from it.
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>

Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>

Re: [PATCH bpf-next v2] net: Don't call XDP_SETUP_PROG when nothing is changed

[Jakub Kicinski]

On Wed, 14 Aug 2019 14:34:06 +0000, Maxim Mikityanskiy wrote:
> Don't uninstall an XDP program when none is installed, and don't install
> an XDP program that has the same ID as the one already installed.
> 
> dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
> XDP program. It means that the driver's ndo_bpf can be called with
> XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
> case happens if the user runs `ip link set eth0 xdp off` when there is
> no XDP program attached.
> 
> The symmetrical case is possible when the user tries to set the program
> that is already set.
> 
> The drivers typically perform some heavy operations on XDP_SETUP_PROG,
> so they all have to handle these cases internally to return early if
> they happen. This patch puts this check into the kernel code, so that
> all drivers will benefit from it.
> 
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Re: [PATCH bpf-next v2] net: Don't call XDP_SETUP_PROG when nothing is changed

[Daniel Borkmann]

On 8/14/19 4:34 PM, Maxim Mikityanskiy wrote:
> Don't uninstall an XDP program when none is installed, and don't install
> an XDP program that has the same ID as the one already installed.
> 
> dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
> XDP program. It means that the driver's ndo_bpf can be called with
> XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
> case happens if the user runs `ip link set eth0 xdp off` when there is
> no XDP program attached.
> 
> The symmetrical case is possible when the user tries to set the program
> that is already set.
> 
> The drivers typically perform some heavy operations on XDP_SETUP_PROG,
> so they all have to handle these cases internally to return early if
> they happen. This patch puts this check into the kernel code, so that
> all drivers will benefit from it.
> 
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>

Applied, thanks!