Skip to contentSkip to conversation

Couple of BPF refcount fixes for mlx5

PATCH In review Daniel Borkmann 3 participants 9 messages Nov 19, 2016 – Nov 21, 2016

Subsystems (6)
Networking Network drivers BPF BPF core +2 more subsystems
lore
Share & tools
lore

Daniel Borkmann

author maintainer
#0 lore raw

v3 cover Couple of BPF refcount fixes for mlx5

Various mlx5 bugs on eBPF refcount handling found during review. Last patch in series adds a __must_check to BPF helpers to make sure we won't run into it again w/o compiler complaining first.

v2 -> v3:

- Just reworked patch 2/4 so we don't need bpf_prog_sub().
- Rebased, rest as is.

v1 -> v2:

- After discussion with Alexei, we agreed upon rebasing the patches against net-next.
- Since net-next, I've also added the __must_check to enforce future users to check for errors.
- Fixed up commit message #2.
- Simplify assignment from patch #1 based on Saeed's feedback on previous set.

Thanks a lot!

Daniel Borkmann (4):
bpf, mlx5: fix mlx5e_create_rq taking reference on prog bpf, mlx5: fix various refcount issues in mlx5e_xdp_set bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup
bpf: add __must_check attributes to refcount manipulating helpers

 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 33 +++++++++++++++++------
 include/linux/bpf.h                               | 12 +++++----
 kernel/bpf/syscall.c                              |  1 +
 3 files changed, 33 insertions(+), 13 deletions(-)
Signature · 1 line
1.9.3
author maintainer
#1 lore raw

PATCH v3 1/4 bpf, mlx5: fix mlx5e_create_rq taking reference on prog

In mlx5e_create_rq(), when creating a new queue, we call bpf_prog_add() but without checking the return value. bpf_prog_add() can fail since 92117d8443bc ("bpf: fix refcnt overflow"), so we really must check it. Take the reference right when we assign it to the rq from priv->xdp_prog, and just drop the reference on error path. Destruction in mlx5e_destroy_rq() looks good, though.

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 | 13 +++++++++----
 kernel/bpf/syscall.c                              |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en_main.c +9 -4
index bd0732d..54bae79 100644
@@ -513,7 +513,13 @@ static int mlx5e_create_rq(struct mlx5e_channel *c,
 	rq->channel = c;
 	rq->ix      = c->ix;
 	rq->priv    = c->priv;
-	rq->xdp_prog = priv->xdp_prog;
+
+	rq->xdp_prog = priv->xdp_prog ? bpf_prog_inc(priv->xdp_prog) : NULL;
+	if (IS_ERR(rq->xdp_prog)) {
+		err = PTR_ERR(rq->xdp_prog);
+		rq->xdp_prog = NULL;
+		goto err_rq_wq_destroy;
+	}
 
 	rq->buff.map_dir = DMA_FROM_DEVICE;
 	if (rq->xdp_prog)

@@ -590,12 +596,11 @@ static int mlx5e_create_rq(struct mlx5e_channel *c,
 	rq->page_cache.head = 0;
 	rq->page_cache.tail = 0;
 
-	if (rq->xdp_prog)
-		bpf_prog_add(rq->xdp_prog, 1);
-
 	return 0;
 
 err_rq_wq_destroy:
+	if (rq->xdp_prog)
+		bpf_prog_put(rq->xdp_prog);
 	mlx5_wq_destroy(&rq->wq_ctrl);
 
 	return err;
kernel/bpf/syscall.c +1 -0
index ce1b7de..eb15498 100644
@@ -696,6 +696,7 @@ struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog)
 {
 	return bpf_prog_add(prog, 1);
 }
+EXPORT_SYMBOL_GPL(bpf_prog_inc);
 
 static struct bpf_prog *__bpf_prog_get(u32 ufd, enum bpf_prog_type *type)
 {
Signature · 1 line
1.9.3
Acked-by on v3 1/4
#2 lore raw

Re: [PATCH net-next v3 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog

Quoted 8 lines · Daniel Borkmann

In mlx5e_create_rq(), when creating a new queue, we call bpf_prog_add() but without checking the return value. bpf_prog_add() can fail since 92117d8443bc ("bpf: fix refcnt overflow"), so we really must check it. Take the reference right when we assign it to the rq from priv->xdp_prog, and just drop the reference on error path. Destruction in mlx5e_destroy_rq() looks good, though.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")
Signed-off-by Daniel Borkmann <[email protected]>
Acked-by Saeed Mahameed <[email protected]>
author maintainer
#3 lore raw

PATCH v3 2/4 bpf, mlx5: fix various refcount issues in mlx5e_xdp_set

In reply to 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. The batched refs short-cut can only be performed when we don't need to perform a reset for changing the rq type and the device was in opened state. In case the device was not in opened state, then the next mlx5e_open_locked() will aquire the refs from the control prog via mlx5e_create_rq(), same when we need to perform a 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 release 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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en_main.c +13 -4
index 54bae79..491cff9 100644
@@ -3144,11 +3144,21 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 
 	if (was_opened && reset)
 		mlx5e_close_locked(netdev);
+	if (was_opened && !reset) {
+		/* 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;
+		}
+	}
 
-	/* exchange programs */
+	/* exchange programs, extra prog reference we got from caller
+	 * as long as we don't fail from this point onwards.
+	 */
 	old_prog = xchg(&priv->xdp_prog, prog);
-	if (prog)
-		bpf_prog_add(prog, 1);
 	if (old_prog)
 		bpf_prog_put(old_prog);
 

@@ -3164,7 +3174,6 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 	/* exchanging programs w/o reset, we update ref counts on behalf
 	 * of the channels RQs here.
 	 */
-	bpf_prog_add(prog, priv->params.num_channels);
 	for (i = 0; i < priv->params.num_channels; i++) {
 		struct mlx5e_channel *c = priv->channel[i];
Signature · 1 line
1.9.3
Acked-by on v3 2/4
#4 lore raw

Re: [PATCH net-next v3 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set

Quoted 11 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. The batched refs short-cut can only be performed when we don't need to perform a reset for changing the rq type and the device was in opened state. In case the device was not in opened state, then the next mlx5e_open_locked() will aquire the refs from the control prog via mlx5e_create_rq(), same when we need to perform a reset.

Quoted 7 lines

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 release 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]>
Acked-by Saeed Mahameed <[email protected]>
author maintainer
#5 lore raw

PATCH v3 3/4 bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup

In reply to 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, otherwise we leak the program.

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 +3 -0
index 491cff9..6957608 100644
@@ -3705,6 +3705,9 @@ static void mlx5e_nic_cleanup(struct mlx5e_priv *priv)
 
 	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);
 }
 
 static int mlx5e_init_nic_rx(struct mlx5e_priv *priv)
Signature · 1 line
1.9.3
Acked-by on v3 3/4
#6 lore raw

Re: [PATCH net-next v3 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup

Quoted 7 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, otherwise we leak the program.

Fixes 86994156c736 ("net/mlx5e: XDP fast RX drop bpf programs support")
Signed-off-by Daniel Borkmann <[email protected]>
Acked-by Saeed Mahameed <[email protected]>
author maintainer
#7 lore raw

PATCH v3 4/4 bpf: add __must_check attributes to refcount manipulating helpers

In reply to Daniel Borkmann

Helpers like bpf_prog_add(), bpf_prog_inc(), bpf_map_inc() can fail with an error, so make sure the caller properly checks their return value and not just ignores it, which could worst-case lead to use after free.

Signed-off-by Daniel Borkmann <[email protected]>
Acked-by Alexei Starovoitov <[email protected]>
---
 include/linux/bpf.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
include/linux/bpf.h +7 -5
index 01c1487..69d0a7f 100644
@@ -233,14 +233,14 @@ u64 bpf_event_output(struct bpf_map *map, u64 flags, void *meta, u64 meta_size,
 
 struct bpf_prog *bpf_prog_get(u32 ufd);
 struct bpf_prog *bpf_prog_get_type(u32 ufd, enum bpf_prog_type type);
-struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i);
+struct bpf_prog * __must_check bpf_prog_add(struct bpf_prog *prog, int i);
 void bpf_prog_sub(struct bpf_prog *prog, int i);
-struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog);
+struct bpf_prog * __must_check bpf_prog_inc(struct bpf_prog *prog);
 void bpf_prog_put(struct bpf_prog *prog);
 
 struct bpf_map *bpf_map_get_with_uref(u32 ufd);
 struct bpf_map *__bpf_map_get(struct fd f);
-struct bpf_map *bpf_map_inc(struct bpf_map *map, bool uref);
+struct bpf_map * __must_check bpf_map_inc(struct bpf_map *map, bool uref);
 void bpf_map_put_with_uref(struct bpf_map *map);
 void bpf_map_put(struct bpf_map *map);
 int bpf_map_precharge_memlock(u32 pages);

@@ -299,7 +299,8 @@ static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
-static inline struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
+static inline struct bpf_prog * __must_check bpf_prog_add(struct bpf_prog *prog,
+							  int i)
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }

@@ -311,7 +312,8 @@ static inline void bpf_prog_sub(struct bpf_prog *prog, int i)
 static inline void bpf_prog_put(struct bpf_prog *prog)
 {
 }
-static inline struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog)
+
+static inline struct bpf_prog * __must_check bpf_prog_inc(struct bpf_prog *prog)
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
Signature · 1 line
1.9.3
maintainer
#8 lore raw
In reply to Daniel Borkmann

From: Daniel Borkmann <[email protected]>
Date: Sat, 19 Nov 2016 01:44:59 +0100

Quoted 3 lines

Various mlx5 bugs on eBPF refcount handling found during review. Last patch in series adds a __must_check to BPF helpers to make sure we won't run into it again w/o compiler complaining first.

Series applied, thanks Daniel.