Couple of BPF refcount fixes for mlx5

There are multiple issues in mlx5e_xdp_set():

1) The batched bpf_prog_add() is currently not checked for errors! When doing so, it should be done at an earlier point in time to makes sure that we cannot fail anymore at the time we want to set the program for each channel. This only means that we have to undo the bpf_prog_add() in case we return due to required reset.

2) When swapping the priv->xdp_prog, then no extra reference count must be taken since we got that from call path via dev_change_xdp_fd() already. Otherwise, we'd never be able to free the program. Also, bpf_prog_add() without checking the return code could fail.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")

Signed-off-by Daniel Borkmann <[email protected]>

---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25 ++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -0

index ab0c336..cf26672 100644
@@ -3142,6 +3142,17 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
                goto unlock;
        }
 
+       if (prog) {

Quoted 28 lines · Daniel Borkmann

There are multiple issues in mlx5e_xdp_set():

1) The batched bpf_prog_add() is currently not checked for errors! When doing so, it should be done at an earlier point in time to makes sure that we cannot fail anymore at the time we want to set the program for each channel. This only means that we have to undo the bpf_prog_add() in case we return due to required reset.

2) When swapping the priv->xdp_prog, then no extra reference count must be taken since we got that from call path via dev_change_xdp_fd() already. Otherwise, we'd never be able to free the program. Also, bpf_prog_add() without checking the return code could fail.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")

Signed-off-by Daniel Borkmann <[email protected]>

---
  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25 ++++++++++++++++++-----
  1 file changed, 20 insertions(+), 5 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -0

index ab0c336..cf26672 100644
@@ -3142,6 +3142,17 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
                 goto unlock;
         }
 
+       if (prog) {

drivers/net/ethernet/mellanox/mlx5/core/en_main.c static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog) · line 3155

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +10 -0

+               /* num_channels is invariant here, so we can take the
+                * batched reference right upfront.
+                */
+               prog = bpf_prog_add(prog, priv->params.num_channels);
+               if (IS_ERR(prog)) {
+                       err = PTR_ERR(prog);
+                       goto unlock;
+               }
+       }
+

With this approach you will end up taking a ref count twice per RQ! on the first time xdp_set is called i.e (old_prog = NULL, prog != NULL). One ref will be taken per RQ/Channel from the above code, and since reset will be TRUE mlx5e_open_locked will be called and another ref count will be taken on mlx5e_create_rq.

The issue here is that we have two places for ref count accounting, xdp_set and mlx5e_create_rq. Having ref-count updates in mlx5e_create_rq is essential for num_channels configuration changes (mlx5e_set_ringparam).

Our previous approach made sure that only one path will do the ref counting (mlx5e_open_locked vs. mlx5e_xdp_set batch ref update when reset == false).

2. Keep the current approach and make sure to not update the ref count twice, you can batch update only if (!reset && was_open) otherwise you can rely on mlx5e_open_locked to take the num_channels ref count for you.

Personally I prefer option 2, since we will keep the current logic which will allow configuration updates such as (mlx5e_set_ringparam) to not worry about ref counting since it will be done in the reset flow.

Quoted 57 lines · Saeed Mahameed

Quoted 36 lines · Daniel Borkmann

There are multiple issues in mlx5e_xdp_set():

1) The batched bpf_prog_add() is currently not checked for errors! When doing so, it should be done at an earlier point in time to makes sure that we cannot fail anymore at the time we want to set the program
for
each channel. This only means that we have to undo the bpf_prog_add() in case we return due to required reset.

2) When swapping the priv->xdp_prog, then no extra reference count must be
taken since we got that from call path via dev_change_xdp_fd()
already.
Otherwise, we'd never be able to free the program. Also,
bpf_prog_add()
without checking the return code could fail.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")

Signed-off-by Daniel Borkmann <[email protected]>

---
  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25
++++++++++++++++++-----
  1 file changed, 20 insertions(+), 5 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -0

index ab0c336..cf26672 100644
@@ -3142,6 +3142,17 @@ static int mlx5e_xdp_set(struct net_device
 *netdev, struct bpf_prog *prog)
                 goto unlock;
         }
 
+       if (prog) {

drivers/net/ethernet/mellanox/mlx5/core/en_main.c static int mlx5e_xdp_set(struct net_device · line 3156

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +10 -0

+               /* num_channels is invariant here, so we can take the
+                * batched reference right upfront.
+                */
+               prog = bpf_prog_add(prog, priv->params.num_channels);
+               if (IS_ERR(prog)) {
+                       err = PTR_ERR(prog);
+                       goto unlock;
+               }
+       }
+

With this approach you will end up taking a ref count twice per RQ! on the first time xdp_set is called i.e (old_prog = NULL, prog != NULL). One ref will be taken per RQ/Channel from the above code, and since reset will be TRUE mlx5e_open_locked will be called and another ref count will be taken on mlx5e_create_rq.

Quoted 8 lines

The issue here is that we have two places for ref count accounting, xdp_set and mlx5e_create_rq. Having ref-count updates in mlx5e_create_rq is essential for num_channels configuration changes (mlx5e_set_ringparam).

Our previous approach made sure that only one path will do the ref counting (mlx5e_open_locked vs. mlx5e_xdp_set batch ref update when reset == false).

That is correct, for a short time bpf_prog_add() was charged also when we reset. When we reset, we will then jump to unlock_put and safely undo this since we took ref from mlx5e_create_rq() already in that case, and return successfully. That was intentional, so that eventually we end up just taking a /single/ ref per RQ when we exit mlx5e_xdp_set(), but more below ...

[...]

Quoted 6 lines

2. Keep the current approach and make sure to not update the ref count twice, you can batch update only if (!reset && was_open) otherwise you can rely on mlx5e_open_locked to take the num_channels ref count for you.

Quoted 4 lines

Personally I prefer option 2, since we will keep the current logic which will allow configuration updates such as (mlx5e_set_ringparam) to not worry about ref counting since it will be done in the reset flow.

... agree on keeping current approach. I actually like the idea, so we'd end up with this simpler version for the batched ref then.

Note, your "bpf_prog_add(prog, 1) // one ref for the device." is not needed since this we already got that one through dev_change_xdp_fd() as mentioned.

Quoted 79 lines · Daniel Borkmann

Quoted 55 lines · Saeed Mahameed

Quoted 36 lines · Daniel Borkmann

There are multiple issues in mlx5e_xdp_set():

1) The batched bpf_prog_add() is currently not checked for errors! When doing so, it should be done at an earlier point in time to makes sure that we cannot fail anymore at the time we want to set the program
for
each channel. This only means that we have to undo the bpf_prog_add() in case we return due to required reset.

2) When swapping the priv->xdp_prog, then no extra reference count must be
taken since we got that from call path via dev_change_xdp_fd()
already.
Otherwise, we'd never be able to free the program. Also,
bpf_prog_add()
without checking the return code could fail.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")

Signed-off-by Daniel Borkmann <[email protected]>

---
   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25
++++++++++++++++++-----
   1 file changed, 20 insertions(+), 5 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -0

index ab0c336..cf26672 100644
@@ -3142,6 +3142,17 @@ static int mlx5e_xdp_set(struct net_device
 *netdev, struct bpf_prog *prog)
                  goto unlock;
          }
 
+       if (prog) {

drivers/net/ethernet/mellanox/mlx5/core/en_main.c static int mlx5e_xdp_set(struct net_device · line 3156

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +10 -0

+               /* num_channels is invariant here, so we can take the
+                * batched reference right upfront.
+                */
+               prog = bpf_prog_add(prog, priv->params.num_channels);
+               if (IS_ERR(prog)) {
+                       err = PTR_ERR(prog);
+                       goto unlock;
+               }
+       }
+

With this approach you will end up taking a ref count twice per RQ! on the first time xdp_set is called i.e (old_prog = NULL, prog != NULL). One ref will be taken per RQ/Channel from the above code, and since reset will be TRUE mlx5e_open_locked will be called and another ref count will be taken on mlx5e_create_rq.

Quoted 8 lines

The issue here is that we have two places for ref count accounting, xdp_set and mlx5e_create_rq. Having ref-count updates in mlx5e_create_rq is essential for num_channels configuration changes (mlx5e_set_ringparam).

Our previous approach made sure that only one path will do the ref counting (mlx5e_open_locked vs. mlx5e_xdp_set batch ref update when reset == false).

That is correct, for a short time bpf_prog_add() was charged also when we reset. When we reset, we will then jump to unlock_put and safely undo this since we took ref from mlx5e_create_rq() already in that case, and return successfully. That was intentional, so that eventually we end up just taking a /single/ ref per RQ when we exit mlx5e_xdp_set(), but more below ...

[...]

Quoted 6 lines

2. Keep the current approach and make sure to not update the ref count twice, you can batch update only if (!reset && was_open) otherwise you can rely on mlx5e_open_locked to take the num_channels ref count for you.

Quoted 7 lines

Quoted 4 lines

Personally I prefer option 2, since we will keep the current logic which will allow configuration updates such as (mlx5e_set_ringparam) to not worry about ref counting since it will be done in the reset flow.

... agree on keeping current approach. I actually like the idea, so we'd end up with this simpler version for the batched ref then.

Great :).

So let's do the batched update only if we are not going to reset (we already know that in advance), instead of the current patch where you batch update unconditionally and then
unlock_put in case reset was performed (which is just redundant and confusing).

Please note that if open_locked fails you need to goto unlock_put.

Quoted 2 lines

Note, your "bpf_prog_add(prog, 1) // one ref for the device." is not needed since this we already got that one through dev_change_xdp_fd() as mentioned.

If it is not needed then why we need bpf_prog_put on mlx5e_nic_cleanup in your next patch? this doesn't look symmetric (right) ! you shouldn't release a reference you didn't take.
Overall with this series the driver can take num_channels refs and will release num_channels refs on mlx5e_close. we shouldn't release one extra ref on NIC cleanup.

Quoted 109 lines · Saeed Mahameed

Quoted 88 lines · Daniel Borkmann

Quoted 61 lines · Saeed Mahameed

Quoted 40 lines · Daniel Borkmann

There are multiple issues in mlx5e_xdp_set():

1) The batched bpf_prog_add() is currently not checked for errors! When doing so, it should be done at an earlier point in time to makes
sure
that we cannot fail anymore at the time we want to set the program
for
each channel. This only means that we have to undo the
bpf_prog_add()
in case we return due to required reset.

2) When swapping the priv->xdp_prog, then no extra reference count must be
taken since we got that from call path via dev_change_xdp_fd()
already.
Otherwise, we'd never be able to free the program. Also,
bpf_prog_add()
without checking the return code could fail.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs

support")

Signed-off-by Daniel Borkmann <[email protected]>

---
   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25
++++++++++++++++++-----
   1 file changed, 20 insertions(+), 5 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -0

index ab0c336..cf26672 100644
@@ -3142,6 +3142,17 @@ static int mlx5e_xdp_set(struct net_device
 *netdev, struct bpf_prog *prog)
                  goto unlock;
          }
 
+       if (prog) {

drivers/net/ethernet/mellanox/mlx5/core/en_main.c static int mlx5e_xdp_set(struct net_device · line 3156

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +10 -0

+               /* num_channels is invariant here, so we can take the
+                * batched reference right upfront.
+                */
+               prog = bpf_prog_add(prog, priv->params.num_channels);
+               if (IS_ERR(prog)) {
+                       err = PTR_ERR(prog);
+                       goto unlock;
+               }
+       }
+

With this approach you will end up taking a ref count twice per RQ! on the first time xdp_set is called i.e (old_prog = NULL, prog != NULL). One ref will be taken per RQ/Channel from the above code, and since reset will be TRUE mlx5e_open_locked will be called and another ref count will be taken on mlx5e_create_rq.

Quoted 8 lines

The issue here is that we have two places for ref count accounting, xdp_set and mlx5e_create_rq. Having ref-count updates in mlx5e_create_rq is essential for num_channels configuration changes (mlx5e_set_ringparam).

Our previous approach made sure that only one path will do the ref counting (mlx5e_open_locked vs. mlx5e_xdp_set batch ref update when reset == false).

That is correct, for a short time bpf_prog_add() was charged also when we reset. When we reset, we will then jump to unlock_put and safely undo this since we took ref from mlx5e_create_rq() already in that case, and return successfully. That was intentional, so that eventually we end up just taking a /single/ ref per RQ when we exit mlx5e_xdp_set(), but more below ...

[...]

Quoted 7 lines

2. Keep the current approach and make sure to not update the ref count twice, you can batch update only if (!reset && was_open) otherwise you can rely on mlx5e_open_locked to take the num_channels ref count for you.

Quoted 8 lines

Quoted 4 lines

Personally I prefer option 2, since we will keep the current logic which will allow configuration updates such as (mlx5e_set_ringparam) to not worry about ref counting since it will be done in the reset flow.

... agree on keeping current approach. I actually like the idea, so we'd end up with this simpler version for the batched ref then.

Great :).

So let's do the batched update only if we are not going to reset (we already know that in advance), instead of the current patch where you batch update unconditionally and then
unlock_put in case reset was performed (which is just redundant and
confusing).

Quoted 1 line

Please note that if open_locked fails you need to goto unlock_put.

Sorry, I'm not quite sure I follow you here; are you /now/ commenting on the original patch or on the already updated diff I did from my very last email, that is, http://patchwork.ozlabs.org/patch/695564/ ?

Quoted 6 lines

Quoted 4 lines

Note, your "bpf_prog_add(prog, 1) // one ref for the device." is not needed
since this we already got that one through dev_change_xdp_fd() as mentioned.

Quoted 6 lines

If it is not needed then why we need bpf_prog_put on mlx5e_nic_cleanup in your next patch? this doesn't look symmetric (right) ! you shouldn't release a reference you didn't take.
Overall with this series the driver can take num_channels refs and will release num_channels refs on mlx5e_close. we shouldn't release one extra ref on NIC cleanup.

I already explained this in the commit description; when dev_change_xdp_fd() is called and sees a valid fd, it does bpf_prog_get_type(), which calls the __bpf_prog_get() taking a ref on the program (bpf_prog_inc()). That is then passed down to ops->ndo_xdp(). Only in case of error from the ->ndo_xdp() callback, we bpf_prog_put() this reference from dev_change_xdp_fd() side.

For drivers that implement against this ndo, it means that you need N-1 refs in addition. Have a look at the other two in-tree users, which do it correctly,
that is mlx4_xdp_set() and nfp_net_xdp_setup().

It's documented here (include/linux/netdevice.h) with ndo_xdp referring to it:

/* These structures hold the attributes of xdp state that are being passed
* to the netdevice through the xdp op.
*/
enum xdp_netdev_command {
/* Set or clear a bpf program used in the earliest stages of packet
* rx. The prog will have been loaded as BPF_PROG_TYPE_XDP. The
callee
* is responsible for calling bpf_prog_put on any old progs that are
* stored. In case of error, the callee need not release the new
prog
* reference, but on success it takes ownership and must
bpf_prog_put
* when it is no longer used.
*/
XDP_SETUP_PROG,
[...]
};

I think reason why this is rather confusing would be that initially, it was just a single prog for all queues, but it was requested along the way to move
prog pointer down to queues instead and let them have their own ref, so that some time in future individual progs from a subset of the queues can be exchanged.

Since the way xdp in mlx5 is implemented, you currently have the priv->xdp_prog
as the control prog. That's okay right now, but requires to drop the last ref
on priv->xdp_prog via bpf_prog_put() when netdev is dismantled and priv->xdp_prog
still present.

Quoted 101 lines · Daniel Borkmann

Quoted 100 lines · Saeed Mahameed

Quoted 82 lines · Daniel Borkmann

Quoted 58 lines · Saeed Mahameed

Quoted 39 lines · Daniel Borkmann

There are multiple issues in mlx5e_xdp_set():

1) The batched bpf_prog_add() is currently not checked for errors! When doing so, it should be done at an earlier point in time to makes
sure
that we cannot fail anymore at the time we want to set the program
for
each channel. This only means that we have to undo the
bpf_prog_add()
in case we return due to required reset.

2) When swapping the priv->xdp_prog, then no extra reference count must be
taken since we got that from call path via dev_change_xdp_fd()
already.
Otherwise, we'd never be able to free the program. Also,
bpf_prog_add()
without checking the return code could fail.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs

support")

Signed-off-by Daniel Borkmann <[email protected]>

---
    drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25
++++++++++++++++++-----
    1 file changed, 20 insertions(+), 5 deletions(-)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +1 -0

index ab0c336..cf26672 100644
@@ -3142,6 +3142,17 @@ static int mlx5e_xdp_set(struct net_device
 *netdev, struct bpf_prog *prog)
                   goto unlock;
           }
 
+       if (prog) {

drivers/net/ethernet/mellanox/mlx5/core/en_main.c static int mlx5e_xdp_set(struct net_device · line 3156

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +10 -0

+               /* num_channels is invariant here, so we can take the
+                * batched reference right upfront.
+                */
+               prog = bpf_prog_add(prog, priv->params.num_channels);
+               if (IS_ERR(prog)) {
+                       err = PTR_ERR(prog);
+                       goto unlock;
+               }
+       }
+

With this approach you will end up taking a ref count twice per RQ! on the first time xdp_set is called i.e (old_prog = NULL, prog != NULL). One ref will be taken per RQ/Channel from the above code, and since reset will be TRUE mlx5e_open_locked will be called and another ref count will be taken on mlx5e_create_rq.

Quoted 8 lines

The issue here is that we have two places for ref count accounting, xdp_set and mlx5e_create_rq. Having ref-count updates in mlx5e_create_rq is essential for num_channels configuration changes (mlx5e_set_ringparam).

Our previous approach made sure that only one path will do the ref counting (mlx5e_open_locked vs. mlx5e_xdp_set batch ref update when reset == false).

That is correct, for a short time bpf_prog_add() was charged also when we reset. When we reset, we will then jump to unlock_put and safely undo this since we took ref from mlx5e_create_rq() already in that case, and return successfully. That was intentional, so that eventually we end up just taking a /single/ ref per RQ when we exit mlx5e_xdp_set(), but more below ...

[...]

Quoted 6 lines

2. Keep the current approach and make sure to not update the ref count twice, you can batch update only if (!reset && was_open) otherwise you can rely on mlx5e_open_locked to take the num_channels ref count for you.

Quoted 7 lines

Quoted 4 lines

Personally I prefer option 2, since we will keep the current logic which will allow configuration updates such as (mlx5e_set_ringparam) to not worry about ref counting since it will be done in the reset flow.

... agree on keeping current approach. I actually like the idea, so we'd end up with this simpler version for the batched ref then.

Great :).

So let's do the batched update only if we are not going to reset (we already know that in advance), instead of the current patch where you batch update unconditionally and then
unlock_put in case reset was performed (which is just redundant and
confusing).

Quoted 5 lines

Quoted 1 line

Please note that if open_locked fails you need to goto unlock_put.

Sorry, I'm not quite sure I follow you here; are you /now/ commenting on the original patch or on the already updated diff I did from my very last email, that is, http://patchwork.ozlabs.org/patch/695564/ ?

I am commenting on this patch "V2 2/4".
this patch will take batched ref count unconditionally and release the extra refs "unlock_put" if reset was performed.
I am asking to take the batched ref only if we are not going to reset in advance to save the extra "unlock_put"

Quoted 51 lines

Quoted 11 lines

Quoted 4 lines

Note, your "bpf_prog_add(prog, 1) // one ref for the device." is not needed
since this we already got that one through dev_change_xdp_fd() as mentioned.

If it is not needed then why we need bpf_prog_put on mlx5e_nic_cleanup in your next patch? this doesn't look symmetric (right) ! you shouldn't release a reference you didn't take.
Overall with this series the driver can take num_channels refs and will release num_channels refs on mlx5e_close. we shouldn't release one extra ref on NIC cleanup.

I already explained this in the commit description; when dev_change_xdp_fd() is called and sees a valid fd, it does bpf_prog_get_type(), which calls the __bpf_prog_get() taking a ref on the program (bpf_prog_inc()). That is then passed down to ops->ndo_xdp(). Only in case of error from the ->ndo_xdp() callback, we bpf_prog_put() this reference from dev_change_xdp_fd() side.

For drivers that implement against this ndo, it means that you need N-1 refs in addition. Have a look at the other two in-tree users, which do it correctly,
that is mlx4_xdp_set() and nfp_net_xdp_setup().

It's documented here (include/linux/netdevice.h) with ndo_xdp referring to it:

/* These structures hold the attributes of xdp state that are being passed
* to the netdevice through the xdp op.
*/
enum xdp_netdev_command {
/* Set or clear a bpf program used in the earliest stages of packet
* rx. The prog will have been loaded as BPF_PROG_TYPE_XDP. The
callee
* is responsible for calling bpf_prog_put on any old progs that are
* stored. In case of error, the callee need not release the new
prog
* reference, but on success it takes ownership and must
bpf_prog_put
* when it is no longer used.
*/
XDP_SETUP_PROG,
[...]
};

I think reason why this is rather confusing would be that initially, it was just a single prog for all queues, but it was requested along the way to move
prog pointer down to queues instead and let them have their own ref, so that some time in future individual progs from a subset of the queues can be exchanged.

Quoted 7 lines

Since the way xdp in mlx5 is implemented, you currently have the priv->xdp_prog
as the control prog. That's okay right now, but requires to drop the last ref
on priv->xdp_prog via bpf_prog_put() when netdev is dismantled and priv->xdp_prog
still present.

Got it, but i would rather make the stack cleanup those refs once it receives an unregester_netdev event instead of counting on netdev to do it, as i said before, like any other resource added to the netdev by the stack (vlan/mac/etc..).

but this is a general discussion of xdp API, it won't block this series :).

Saeed.

mlx5e_xdp_set() is currently the only place where we drop reference on the prog sitting in priv->xdp_prog when it's exchanged by a new one. We also need to make sure that we eventually release that reference, for example, in case the netdev is dismantled.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")

Signed-off-by Daniel Borkmann <[email protected]>

---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +0 -0

index cf26672..60fe54c 100644

Quoted 17 lines · Daniel Borkmann

mlx5e_xdp_set() is currently the only place where we drop reference on the prog sitting in priv->xdp_prog when it's exchanged by a new one. We also need to make sure that we eventually release that reference, for example, in case the netdev is dismantled.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")

Signed-off-by Daniel Borkmann <[email protected]>

---
  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +0 -0

index cf26672..60fe54c 100644
@@ -3715,6 +3715,9 @@ static void mlx5e_nic_cleanup(struct mlx5e_priv *priv)
 

drivers/net/ethernet/mellanox/mlx5/core/en_main.c static void mlx5e_nic_cleanup(struct mlx5e_priv *priv) · line 3721

Quoted 6 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +3 -0

         if (MLX5_CAP_GEN(mdev, vport_group_manager))
                 mlx5_eswitch_unregister_vport_rep(esw, 0);
+
+       if (priv->xdp_prog)
+               bpf_prog_put(priv->xdp_prog);
  }

I thought that on unregister_netdev ndo_xdp_set will be called with NULL prog to cleanup. like any other resources (Vlans/mac_lists/ etc..), why xdp should be different ?
Anyway if this is the case, I am ok with this fix, you can even send it to net (it looks like a serious leak).

Quoted 18 lines · Saeed Mahameed

Quoted 17 lines · Daniel Borkmann

mlx5e_xdp_set() is currently the only place where we drop reference on the prog sitting in priv->xdp_prog when it's exchanged by a new one. We also need to make sure that we eventually release that reference, for example, in case the netdev is dismantled.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")

Signed-off-by Daniel Borkmann <[email protected]>

---
  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +0 -0

index cf26672..60fe54c 100644
@@ -3715,6 +3715,9 @@ static void mlx5e_nic_cleanup(struct mlx5e_priv *priv)
 

Quoted 12 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c static void mlx5e_nic_cleanup(struct mlx5e_priv *priv) · line 3721

Quoted 6 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +3 -0

         if (MLX5_CAP_GEN(mdev, vport_group_manager))
                 mlx5_eswitch_unregister_vport_rep(esw, 0);
+
+       if (priv->xdp_prog)
+               bpf_prog_put(priv->xdp_prog);
  }

I thought that on unregister_netdev ndo_xdp_set will be called with NULL prog to cleanup. like any other resources (Vlans/mac_lists/ etc..), why xdp should be different ?
Anyway if this is the case, I am ok with this fix, you can even send it to net (it looks like a serious leak).

The only interaction with ndo_xdp() right now is dev_change_xdp_fd() and the currently a bit terse dump via rtnl_xdp_fill(). The latter only tells whether something is actually attached and will have more info in near future, but doesn't alter anything.

dev_change_xdp_fd() is only triggered from user side via netlink when IFLA_XDP container attr is around, so no automatic cleanup here. This means as per documentation in enum xdp_netdev_command, that the driver has full ownership, thus needs to bpf_prog_put().

Quoted 47 lines · Daniel Borkmann

Quoted 25 lines · Saeed Mahameed

Quoted 22 lines · Daniel Borkmann

mlx5e_xdp_set() is currently the only place where we drop reference on the
prog sitting in priv->xdp_prog when it's exchanged by a new one. We also need to make sure that we eventually release that reference, for example,
in case the netdev is dismantled.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")

Signed-off-by Daniel Borkmann <[email protected]>

---
  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
  1 file changed, 3 insertions(+)

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +0 -0

index cf26672..60fe54c 100644
@@ -3715,6 +3715,9 @@ static void mlx5e_nic_cleanup(struct mlx5e_priv
 *priv)
 

Quoted 13 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c static void mlx5e_nic_cleanup(struct mlx5e_priv *priv) · line 3721

Quoted 6 lines

drivers/net/ethernet/mellanox/mlx5/core/en_main.c +3 -0

         if (MLX5_CAP_GEN(mdev, vport_group_manager))
                 mlx5_eswitch_unregister_vport_rep(esw, 0);
+
+       if (priv->xdp_prog)
+               bpf_prog_put(priv->xdp_prog);
  }

I thought that on unregister_netdev ndo_xdp_set will be called with NULL prog to cleanup. like any other resources (Vlans/mac_lists/ etc..), why xdp should be different ?
Anyway if this is the case, I am ok with this fix, you can even send it to net (it looks like a serious leak).

The only interaction with ndo_xdp() right now is dev_change_xdp_fd() and the currently a bit terse dump via rtnl_xdp_fill(). The latter only tells whether something is actually attached and will have more info in near future, but doesn't alter anything.

Quoted 4 lines

dev_change_xdp_fd() is only triggered from user side via netlink when IFLA_XDP container attr is around, so no automatic cleanup here. This means as per documentation in enum xdp_netdev_command, that the driver has full ownership, thus needs to bpf_prog_put().

Note that without patch 2/4, just sending this one to net doesn't really solve anything, since there the mlx5e_xdp_set() still has the incorrect bpf_prog_add(prog, 1) around. So it's the whole series if so. I had it originally targeted at net, but Alexei suggested net-next; I don't really mind either way, so I agreed to go for net-next.