* [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5
@ 2016-11-19 0:44 Daniel Borkmann
2016-11-19 0:45 ` [PATCH net-next v3 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Daniel Borkmann @ 2016-11-19 0:44 UTC (permalink / raw)
To: davem
Cc: alexei.starovoitov, bblanco, zhiyisun, ranas, saeedm, netdev,
Daniel Borkmann
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(-)
--
1.9.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v3 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog
2016-11-19 0:44 [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
@ 2016-11-19 0:45 ` Daniel Borkmann
2016-11-21 9:27 ` Saeed Mahameed
2016-11-19 0:45 ` [PATCH net-next v3 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set Daniel Borkmann
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2016-11-19 0:45 UTC (permalink / raw)
To: davem
Cc: alexei.starovoitov, bblanco, zhiyisun, ranas, saeedm, netdev,
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 <daniel@iogearbox.net>
---
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 13 +++++++++----
kernel/bpf/syscall.c | 1 +
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index bd0732d..54bae79 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -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;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ce1b7de..eb15498 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -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)
{
--
1.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next v3 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set
2016-11-19 0:44 [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
2016-11-19 0:45 ` [PATCH net-next v3 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
@ 2016-11-19 0:45 ` Daniel Borkmann
2016-11-21 9:30 ` Saeed Mahameed
2016-11-19 0:45 ` [PATCH net-next v3 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup Daniel Borkmann
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2016-11-19 0:45 UTC (permalink / raw)
To: davem
Cc: alexei.starovoitov, bblanco, zhiyisun, ranas, saeedm, netdev,
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 <daniel@iogearbox.net>
---
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 54bae79..491cff9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -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];
--
1.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next v3 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup
2016-11-19 0:44 [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
2016-11-19 0:45 ` [PATCH net-next v3 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
2016-11-19 0:45 ` [PATCH net-next v3 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set Daniel Borkmann
@ 2016-11-19 0:45 ` Daniel Borkmann
2016-11-21 9:31 ` Saeed Mahameed
2016-11-19 0:45 ` [PATCH net-next v3 4/4] bpf: add __must_check attributes to refcount manipulating helpers Daniel Borkmann
2016-11-21 16:26 ` [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 David Miller
4 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2016-11-19 0:45 UTC (permalink / raw)
To: davem
Cc: alexei.starovoitov, bblanco, zhiyisun, ranas, saeedm, netdev,
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 <daniel@iogearbox.net>
---
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 491cff9..6957608 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -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)
--
1.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next v3 4/4] bpf: add __must_check attributes to refcount manipulating helpers
2016-11-19 0:44 [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
` (2 preceding siblings ...)
2016-11-19 0:45 ` [PATCH net-next v3 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup Daniel Borkmann
@ 2016-11-19 0:45 ` Daniel Borkmann
2016-11-21 16:26 ` [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 David Miller
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Borkmann @ 2016-11-19 0:45 UTC (permalink / raw)
To: davem
Cc: alexei.starovoitov, bblanco, zhiyisun, ranas, saeedm, netdev,
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 <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 01c1487..69d0a7f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -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);
}
--
1.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v3 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog
2016-11-19 0:45 ` [PATCH net-next v3 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
@ 2016-11-21 9:27 ` Saeed Mahameed
0 siblings, 0 replies; 9+ messages in thread
From: Saeed Mahameed @ 2016-11-21 9:27 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Alexei Starovoitov, Brenden Blanco, Zhiyi Sun,
Rana Shahout, Saeed Mahameed, Linux Netdev List
On Sat, Nov 19, 2016 at 2:45 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> 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 <daniel@iogearbox.net>
Acked-by: Saeed Mahameed <saeedm@mellanox.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v3 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set
2016-11-19 0:45 ` [PATCH net-next v3 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set Daniel Borkmann
@ 2016-11-21 9:30 ` Saeed Mahameed
0 siblings, 0 replies; 9+ messages in thread
From: Saeed Mahameed @ 2016-11-21 9:30 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Alexei Starovoitov, Brenden Blanco, Zhiyi Sun,
Rana Shahout, Saeed Mahameed, Linux Netdev List
On Sat, Nov 19, 2016 at 2:45 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> 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 <daniel@iogearbox.net>
Acked-by: Saeed Mahameed <saeedm@mellanox.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v3 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup
2016-11-19 0:45 ` [PATCH net-next v3 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup Daniel Borkmann
@ 2016-11-21 9:31 ` Saeed Mahameed
0 siblings, 0 replies; 9+ messages in thread
From: Saeed Mahameed @ 2016-11-21 9:31 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Alexei Starovoitov, Brenden Blanco, Zhiyi Sun,
Rana Shahout, Saeed Mahameed, Linux Netdev List
On Sat, Nov 19, 2016 at 2:45 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> 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 <daniel@iogearbox.net>
Acked-by: Saeed Mahameed <saeedm@mellanox.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5
2016-11-19 0:44 [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
` (3 preceding siblings ...)
2016-11-19 0:45 ` [PATCH net-next v3 4/4] bpf: add __must_check attributes to refcount manipulating helpers Daniel Borkmann
@ 2016-11-21 16:26 ` David Miller
4 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2016-11-21 16:26 UTC (permalink / raw)
To: daniel; +Cc: alexei.starovoitov, bblanco, zhiyisun, ranas, saeedm, netdev
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Sat, 19 Nov 2016 01:44:59 +0100
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-11-21 16:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-19 0:44 [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
2016-11-19 0:45 ` [PATCH net-next v3 1/4] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
2016-11-21 9:27 ` Saeed Mahameed
2016-11-19 0:45 ` [PATCH net-next v3 2/4] bpf, mlx5: fix various refcount issues in mlx5e_xdp_set Daniel Borkmann
2016-11-21 9:30 ` Saeed Mahameed
2016-11-19 0:45 ` [PATCH net-next v3 3/4] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup Daniel Borkmann
2016-11-21 9:31 ` Saeed Mahameed
2016-11-19 0:45 ` [PATCH net-next v3 4/4] bpf: add __must_check attributes to refcount manipulating helpers Daniel Borkmann
2016-11-21 16:26 ` [PATCH net-next v3 0/4] Couple of BPF refcount fixes for mlx5 David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.