* [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 @ 2016-11-09 7:35 Zhiyi Sun 2016-11-09 9:05 ` Daniel Borkmann 0 siblings, 1 reply; 8+ messages in thread From: Zhiyi Sun @ 2016-11-09 7:35 UTC (permalink / raw) To: bblanco, Tariq Toukan, Yishai Hadas, netdev, linux-rdma, linux-kernel Cc: zhiyisun There are rx_ring_num queues. Each queue will load xdp prog. So bpf_prog_add() should add rx_ring_num to ref_cnt. Signed-off-by: Zhiyi Sun <zhiyisun@gmail.com> --- drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c index 12c99a2..d25e150 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c @@ -2650,7 +2650,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) */ if (priv->xdp_ring_num == xdp_ring_num) { if (prog) { - prog = bpf_prog_add(prog, priv->rx_ring_num - 1); + prog = bpf_prog_add(prog, priv->rx_ring_num); if (IS_ERR(prog)) return PTR_ERR(prog); } @@ -2680,7 +2680,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) } if (prog) { - prog = bpf_prog_add(prog, priv->rx_ring_num - 1); + prog = bpf_prog_add(prog, priv->rx_ring_num); if (IS_ERR(prog)) return PTR_ERR(prog); } -- 2.9.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 2016-11-09 7:35 [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 Zhiyi Sun @ 2016-11-09 9:05 ` Daniel Borkmann 2016-11-09 9:45 ` Zhiyi Sun 0 siblings, 1 reply; 8+ messages in thread From: Daniel Borkmann @ 2016-11-09 9:05 UTC (permalink / raw) To: Zhiyi Sun Cc: bblanco, Tariq Toukan, Yishai Hadas, netdev, linux-rdma, linux-kernel, alexei.starovoitov On 11/09/2016 08:35 AM, Zhiyi Sun wrote: > There are rx_ring_num queues. Each queue will load xdp prog. So > bpf_prog_add() should add rx_ring_num to ref_cnt. > > Signed-off-by: Zhiyi Sun <zhiyisun@gmail.com> Your analysis looks incorrect to me. Please elaborate in more detail why you think current code is buggy ... Call path is dev_change_xdp_fd(), which does bpf_prog_get_type() on the fd. This already takes a ref and only drops it in case of error. Thus in mlx4_xdp_set(), you only need priv->rx_ring_num - 1 refs for the rest of the rings, so that dropping refs from old_prog makes sure we release it again. Looks correct to me (maybe a comment would have helped there). > drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > index 12c99a2..d25e150 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > @@ -2650,7 +2650,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) > */ > if (priv->xdp_ring_num == xdp_ring_num) { > if (prog) { > - prog = bpf_prog_add(prog, priv->rx_ring_num - 1); > + prog = bpf_prog_add(prog, priv->rx_ring_num); > if (IS_ERR(prog)) > return PTR_ERR(prog); > } > @@ -2680,7 +2680,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) > } > > if (prog) { > - prog = bpf_prog_add(prog, priv->rx_ring_num - 1); > + prog = bpf_prog_add(prog, priv->rx_ring_num); > if (IS_ERR(prog)) > return PTR_ERR(prog); > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 2016-11-09 9:05 ` Daniel Borkmann @ 2016-11-09 9:45 ` Zhiyi Sun 2016-11-09 9:57 ` Daniel Borkmann 0 siblings, 1 reply; 8+ messages in thread From: Zhiyi Sun @ 2016-11-09 9:45 UTC (permalink / raw) To: Daniel Borkmann Cc: bblanco, Tariq Toukan, Yishai Hadas, netdev, linux-rdma, linux-kernel, alexei.starovoitov On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote: > On 11/09/2016 08:35 AM, Zhiyi Sun wrote: > > There are rx_ring_num queues. Each queue will load xdp prog. So > > bpf_prog_add() should add rx_ring_num to ref_cnt. > > > > Signed-off-by: Zhiyi Sun <zhiyisun@gmail.com> > > Your analysis looks incorrect to me. Please elaborate in more detail why > you think current code is buggy ... > Yes, you are correct. My patch is incorrect. It is not a bug. > Call path is dev_change_xdp_fd(), which does bpf_prog_get_type() on the > fd. This already takes a ref and only drops it in case of error. Thus > in mlx4_xdp_set(), you only need priv->rx_ring_num - 1 refs for the rest > of the rings, so that dropping refs from old_prog makes sure we release > it again. Looks correct to me (maybe a comment would have helped there). > I thought mlx4's code is incorrect because in mlx5's driver, function mlx5e_xdp_set() calls a pair of bpf_prog_add/put, the number of add and put to the refs are same. I didn't notice that one "add" has been called in its calller. So, it seems that mlx5's code is incorrect, right? > > drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > > index 12c99a2..d25e150 100644 > > --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > > +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > > @@ -2650,7 +2650,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) > > */ > > if (priv->xdp_ring_num == xdp_ring_num) { > > if (prog) { > > - prog = bpf_prog_add(prog, priv->rx_ring_num - 1); > > + prog = bpf_prog_add(prog, priv->rx_ring_num); > > if (IS_ERR(prog)) > > return PTR_ERR(prog); > > } > > @@ -2680,7 +2680,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) > > } > > > > if (prog) { > > - prog = bpf_prog_add(prog, priv->rx_ring_num - 1); > > + prog = bpf_prog_add(prog, priv->rx_ring_num); > > if (IS_ERR(prog)) > > return PTR_ERR(prog); > > } > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 2016-11-09 9:45 ` Zhiyi Sun @ 2016-11-09 9:57 ` Daniel Borkmann 2016-11-09 10:58 ` [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path kbuild test robot 2016-11-09 17:06 ` [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 Brenden Blanco 0 siblings, 2 replies; 8+ messages in thread From: Daniel Borkmann @ 2016-11-09 9:57 UTC (permalink / raw) To: Zhiyi Sun Cc: bblanco, Tariq Toukan, Yishai Hadas, netdev, linux-rdma, linux-kernel, alexei.starovoitov [-- Attachment #1: Type: text/plain, Size: 1365 bytes --] On 11/09/2016 10:45 AM, Zhiyi Sun wrote: > On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote: >> On 11/09/2016 08:35 AM, Zhiyi Sun wrote: >>> There are rx_ring_num queues. Each queue will load xdp prog. So >>> bpf_prog_add() should add rx_ring_num to ref_cnt. >>> >>> Signed-off-by: Zhiyi Sun <zhiyisun@gmail.com> >> >> Your analysis looks incorrect to me. Please elaborate in more detail why >> you think current code is buggy ... > > Yes, you are correct. My patch is incorrect. It is not a bug. > >> Call path is dev_change_xdp_fd(), which does bpf_prog_get_type() on the >> fd. This already takes a ref and only drops it in case of error. Thus >> in mlx4_xdp_set(), you only need priv->rx_ring_num - 1 refs for the rest >> of the rings, so that dropping refs from old_prog makes sure we release >> it again. Looks correct to me (maybe a comment would have helped there). > > I thought mlx4's code is incorrect because in mlx5's driver, function > mlx5e_xdp_set() calls a pair of bpf_prog_add/put, the number of add and > put to the refs are same. I didn't notice that one "add" has been called in its > calller. So, it seems that mlx5's code is incorrect, right? Yep, I think the two attached patches are needed. The other thing I noticed in mlx5e_create_rq() is that it calls bpf_prog_add(rq->xdp_prog, 1) without actually checking for errors. [-- Attachment #2: 0001-bpf-mlx4-fix-prog-refcount-in-mlx4_en_try_alloc_reso.patch --] [-- Type: text/x-patch, Size: 3022 bytes --] >From d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0 Mon Sep 17 00:00:00 2001 Message-Id: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> From: Daniel Borkmann <daniel@iogearbox.net> Date: Wed, 9 Nov 2016 10:31:19 +0100 Subject: [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path Commit 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings scheme") added a bug in that the prog's reference count is not dropped in the error path when mlx4_en_try_alloc_resources() is failing. We previously took bpf_prog_add(prog, priv->rx_ring_num - 1), that we need to release again. Earlier in the call-path, dev_change_xdp_fd() itself holds a ref to the prog as well, which is then released though bpf_prog_put() due to the propagated error. Fixes: 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings scheme") Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> --- drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 5 ++++- include/linux/bpf.h | 1 + kernel/bpf/syscall.c | 11 +++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c index 0f6225c..4104aec 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c @@ -2747,8 +2747,11 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) } err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof); - if (err) + if (err) { + if (prog) + bpf_prog_add_undo(prog, priv->rx_ring_num - 1); goto unlock_out; + } if (priv->port_up) { port_up = 1; diff --git a/include/linux/bpf.h b/include/linux/bpf.h index edcd96d..4f6a4f1 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -234,6 +234,7 @@ 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); +void bpf_prog_add_undo(struct bpf_prog *prog, int i); struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog); void bpf_prog_put(struct bpf_prog *prog); diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 228f962..a6e4dd8 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -680,6 +680,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i) } EXPORT_SYMBOL_GPL(bpf_prog_add); +void bpf_prog_add_undo(struct bpf_prog *prog, int i) +{ + /* Only to be used for undoing previous bpf_prog_add() in some + * error path. We still know that another entity in our call + * path holds a reference to the program, thus atomic_sub() can + * be safely used here! + */ + atomic_sub(i, &prog->aux->refcnt); +} +EXPORT_SYMBOL_GPL(bpf_prog_add_undo); + struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog) { return bpf_prog_add(prog, 1); -- 1.9.3 [-- Attachment #3: 0002-bpf-mlx5-fix-prog-refcount-in-mlx5e_xdp_set.patch --] [-- Type: text/x-patch, Size: 1474 bytes --] >From f0789544432bbb89c53c3b8ac6575d48fed97786 Mon Sep 17 00:00:00 2001 Message-Id: <f0789544432bbb89c53c3b8ac6575d48fed97786.1478685278.git.daniel@iogearbox.net> In-Reply-To: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> References: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> From: Daniel Borkmann <daniel@iogearbox.net> Date: Wed, 9 Nov 2016 10:51:26 +0100 Subject: [PATCH net-next 2/2] bpf, mlx5: fix prog refcount in mlx5e_xdp_set dev_change_xdp_fd() already holds a reference, so bpf_prog_add(prog, 1) is not correct as it takes one reference too much and will thus leak the prog eventually. Also, bpf_prog_add() can fail and is not checked for errors here. 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 | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index ba0c774..63309dd 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -3121,8 +3121,6 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog) /* exchange programs */ old_prog = xchg(&priv->xdp_prog, prog); - if (prog) - bpf_prog_add(prog, 1); if (old_prog) bpf_prog_put(old_prog); -- 1.9.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path 2016-11-09 9:57 ` Daniel Borkmann @ 2016-11-09 10:58 ` kbuild test robot 2016-11-09 11:04 ` Daniel Borkmann 2016-11-09 17:06 ` [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 Brenden Blanco 1 sibling, 1 reply; 8+ messages in thread From: kbuild test robot @ 2016-11-09 10:58 UTC (permalink / raw) To: Daniel Borkmann Cc: kbuild-all, Zhiyi Sun, bblanco, Tariq Toukan, Yishai Hadas, netdev, linux-rdma, linux-kernel, alexei.starovoitov [-- Attachment #1: Type: text/plain, Size: 1365 bytes --] Hi Daniel, [auto build test ERROR on net-next/master] url: https://github.com/0day-ci/linux/commits/Daniel-Borkmann/bpf-mlx4-fix-prog-refcount-in-mlx4_en_try_alloc_resources-error-path/20161109-182712 config: x86_64-acpi-redef (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/net/ethernet/mellanox/mlx4/en_netdev.c: In function 'mlx4_xdp_set': >> drivers/net/ethernet/mellanox/mlx4/en_netdev.c:2752:4: error: implicit declaration of function 'bpf_prog_add_undo' [-Werror=implicit-function-declaration] bpf_prog_add_undo(prog, priv->rx_ring_num - 1); ^~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +/bpf_prog_add_undo +2752 drivers/net/ethernet/mellanox/mlx4/en_netdev.c 2746 en_warn(priv, "Reducing the number of TX rings, to not exceed the max total rings number.\n"); 2747 } 2748 2749 err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof); 2750 if (err) { 2751 if (prog) > 2752 bpf_prog_add_undo(prog, priv->rx_ring_num - 1); 2753 goto unlock_out; 2754 } 2755 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 28646 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path 2016-11-09 10:58 ` [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path kbuild test robot @ 2016-11-09 11:04 ` Daniel Borkmann 0 siblings, 0 replies; 8+ messages in thread From: Daniel Borkmann @ 2016-11-09 11:04 UTC (permalink / raw) To: kbuild test robot Cc: kbuild-all, Zhiyi Sun, bblanco, Tariq Toukan, Yishai Hadas, netdev, linux-rdma, linux-kernel, alexei.starovoitov On 11/09/2016 11:58 AM, kbuild test robot wrote: [...] > All errors (new ones prefixed by >>): > > drivers/net/ethernet/mellanox/mlx4/en_netdev.c: In function 'mlx4_xdp_set': >>> drivers/net/ethernet/mellanox/mlx4/en_netdev.c:2752:4: error: implicit declaration of function 'bpf_prog_add_undo' [-Werror=implicit-function-declaration] > bpf_prog_add_undo(prog, priv->rx_ring_num - 1); > ^~~~~~~~~~~~~~~~~ > cc1: some warnings being treated as errors Ahh right, needs an empty variant for !CONFIG_BPF_SYSCALL. I'll fix that up before sending an official patch. Thanks, Daniel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 2016-11-09 9:57 ` Daniel Borkmann 2016-11-09 10:58 ` [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path kbuild test robot @ 2016-11-09 17:06 ` Brenden Blanco 2016-11-09 19:05 ` Daniel Borkmann 1 sibling, 1 reply; 8+ messages in thread From: Brenden Blanco @ 2016-11-09 17:06 UTC (permalink / raw) To: Daniel Borkmann Cc: Zhiyi Sun, Tariq Toukan, Yishai Hadas, netdev, linux-rdma, linux-kernel, alexei.starovoitov On Wed, Nov 09, 2016 at 10:57:32AM +0100, Daniel Borkmann wrote: > On 11/09/2016 10:45 AM, Zhiyi Sun wrote: > >On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote: > >>On 11/09/2016 08:35 AM, Zhiyi Sun wrote: > >>>There are rx_ring_num queues. Each queue will load xdp prog. So > >>>bpf_prog_add() should add rx_ring_num to ref_cnt. > >>> > >>>Signed-off-by: Zhiyi Sun <zhiyisun@gmail.com> > >> > >>Your analysis looks incorrect to me. Please elaborate in more detail why > >>you think current code is buggy ... > > > >Yes, you are correct. My patch is incorrect. It is not a bug. > > > >>Call path is dev_change_xdp_fd(), which does bpf_prog_get_type() on the > >>fd. This already takes a ref and only drops it in case of error. Thus > >>in mlx4_xdp_set(), you only need priv->rx_ring_num - 1 refs for the rest > >>of the rings, so that dropping refs from old_prog makes sure we release > >>it again. Looks correct to me (maybe a comment would have helped there). > > > >I thought mlx4's code is incorrect because in mlx5's driver, function > >mlx5e_xdp_set() calls a pair of bpf_prog_add/put, the number of add and > >put to the refs are same. I didn't notice that one "add" has been called in its > >calller. So, it seems that mlx5's code is incorrect, right? > > Yep, I think the two attached patches are needed. > > The other thing I noticed in mlx5e_create_rq() is that it calls > bpf_prog_add(rq->xdp_prog, 1) without actually checking for errors. > From d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0 Mon Sep 17 00:00:00 2001 > Message-Id: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> > From: Daniel Borkmann <daniel@iogearbox.net> > Date: Wed, 9 Nov 2016 10:31:19 +0100 > Subject: [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path > > Commit 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings > scheme") added a bug in that the prog's reference count is not dropped > in the error path when mlx4_en_try_alloc_resources() is failing. > > We previously took bpf_prog_add(prog, priv->rx_ring_num - 1), that we > need to release again. Earlier in the call-path, dev_change_xdp_fd() > itself holds a ref to the prog as well, which is then released though > bpf_prog_put() due to the propagated error. > > Fixes: 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings scheme") > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > --- > drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 5 ++++- > include/linux/bpf.h | 1 + > kernel/bpf/syscall.c | 11 +++++++++++ > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > index 0f6225c..4104aec 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c > @@ -2747,8 +2747,11 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) > } > > err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof); > - if (err) > + if (err) { > + if (prog) > + bpf_prog_add_undo(prog, priv->rx_ring_num - 1); Why not just move the above bpf_prog_add to be below the try_alloc? Nobody needs those references until all of the resources have been allocated, and then we can remove the need for bpf_prog_add_undo. > goto unlock_out; > + } > > if (priv->port_up) { > port_up = 1; > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index edcd96d..4f6a4f1 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -234,6 +234,7 @@ 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); > +void bpf_prog_add_undo(struct bpf_prog *prog, int i); > struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog); > void bpf_prog_put(struct bpf_prog *prog); > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 228f962..a6e4dd8 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -680,6 +680,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i) > } > EXPORT_SYMBOL_GPL(bpf_prog_add); > > +void bpf_prog_add_undo(struct bpf_prog *prog, int i) > +{ > + /* Only to be used for undoing previous bpf_prog_add() in some > + * error path. We still know that another entity in our call > + * path holds a reference to the program, thus atomic_sub() can > + * be safely used here! > + */ > + atomic_sub(i, &prog->aux->refcnt); > +} > +EXPORT_SYMBOL_GPL(bpf_prog_add_undo); > + > struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog) > { > return bpf_prog_add(prog, 1); > -- > 1.9.3 > > From f0789544432bbb89c53c3b8ac6575d48fed97786 Mon Sep 17 00:00:00 2001 > Message-Id: <f0789544432bbb89c53c3b8ac6575d48fed97786.1478685278.git.daniel@iogearbox.net> > In-Reply-To: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> > References: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> > From: Daniel Borkmann <daniel@iogearbox.net> > Date: Wed, 9 Nov 2016 10:51:26 +0100 > Subject: [PATCH net-next 2/2] bpf, mlx5: fix prog refcount in mlx5e_xdp_set > > dev_change_xdp_fd() already holds a reference, so bpf_prog_add(prog, 1) > is not correct as it takes one reference too much and will thus leak > the prog eventually. Also, bpf_prog_add() can fail and is not checked > for errors here. > > 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 | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > index ba0c774..63309dd 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > @@ -3121,8 +3121,6 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog) > > /* exchange programs */ > old_prog = xchg(&priv->xdp_prog, prog); > - if (prog) > - bpf_prog_add(prog, 1); There is also another use of bpf_prog_add down below, which does not check the error return. Same in mlx5e_create_rq. > if (old_prog) > bpf_prog_put(old_prog); > > -- > 1.9.3 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 2016-11-09 17:06 ` [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 Brenden Blanco @ 2016-11-09 19:05 ` Daniel Borkmann 0 siblings, 0 replies; 8+ messages in thread From: Daniel Borkmann @ 2016-11-09 19:05 UTC (permalink / raw) To: Brenden Blanco Cc: Zhiyi Sun, Tariq Toukan, Yishai Hadas, netdev, linux-rdma, linux-kernel, alexei.starovoitov On 11/09/2016 06:06 PM, Brenden Blanco wrote: > On Wed, Nov 09, 2016 at 10:57:32AM +0100, Daniel Borkmann wrote: >> On 11/09/2016 10:45 AM, Zhiyi Sun wrote: >>> On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote: >>>> On 11/09/2016 08:35 AM, Zhiyi Sun wrote: >>>>> There are rx_ring_num queues. Each queue will load xdp prog. So >>>>> bpf_prog_add() should add rx_ring_num to ref_cnt. >>>>> >>>>> Signed-off-by: Zhiyi Sun <zhiyisun@gmail.com> >>>> >>>> Your analysis looks incorrect to me. Please elaborate in more detail why >>>> you think current code is buggy ... >>> >>> Yes, you are correct. My patch is incorrect. It is not a bug. >>> >>>> Call path is dev_change_xdp_fd(), which does bpf_prog_get_type() on the >>>> fd. This already takes a ref and only drops it in case of error. Thus >>>> in mlx4_xdp_set(), you only need priv->rx_ring_num - 1 refs for the rest >>>> of the rings, so that dropping refs from old_prog makes sure we release >>>> it again. Looks correct to me (maybe a comment would have helped there). >>> >>> I thought mlx4's code is incorrect because in mlx5's driver, function >>> mlx5e_xdp_set() calls a pair of bpf_prog_add/put, the number of add and >>> put to the refs are same. I didn't notice that one "add" has been called in its >>> calller. So, it seems that mlx5's code is incorrect, right? >> >> Yep, I think the two attached patches are needed. >> >> The other thing I noticed in mlx5e_create_rq() is that it calls >> bpf_prog_add(rq->xdp_prog, 1) without actually checking for errors. > >> From d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0 Mon Sep 17 00:00:00 2001 >> Message-Id: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> >> From: Daniel Borkmann <daniel@iogearbox.net> >> Date: Wed, 9 Nov 2016 10:31:19 +0100 >> Subject: [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path >> >> Commit 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings >> scheme") added a bug in that the prog's reference count is not dropped >> in the error path when mlx4_en_try_alloc_resources() is failing. >> >> We previously took bpf_prog_add(prog, priv->rx_ring_num - 1), that we >> need to release again. Earlier in the call-path, dev_change_xdp_fd() >> itself holds a ref to the prog as well, which is then released though >> bpf_prog_put() due to the propagated error. >> >> Fixes: 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings scheme") >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> >> --- >> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 5 ++++- >> include/linux/bpf.h | 1 + >> kernel/bpf/syscall.c | 11 +++++++++++ >> 3 files changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c >> index 0f6225c..4104aec 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c >> @@ -2747,8 +2747,11 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog) >> } >> >> err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof); >> - if (err) >> + if (err) { >> + if (prog) >> + bpf_prog_add_undo(prog, priv->rx_ring_num - 1); > Why not just move the above bpf_prog_add to be below the try_alloc? > Nobody needs those references until all of the resources have been > allocated, and then we can remove the need for bpf_prog_add_undo. Right, looked into this and the convention is to call mlx4_en_try_alloc_resources() plus mlx4_en_safe_replace_resources(), which must always succeed (currently). Seems rather complex to go this route instead; bpf_prog_add_undo() or *_sub() [however we name it] is safe and straight forward, since we're guaranteed to have one reference already. >> goto unlock_out; >> + } >> >> if (priv->port_up) { >> port_up = 1; >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index edcd96d..4f6a4f1 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -234,6 +234,7 @@ 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); >> +void bpf_prog_add_undo(struct bpf_prog *prog, int i); >> struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog); >> void bpf_prog_put(struct bpf_prog *prog); >> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >> index 228f962..a6e4dd8 100644 >> --- a/kernel/bpf/syscall.c >> +++ b/kernel/bpf/syscall.c >> @@ -680,6 +680,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i) >> } >> EXPORT_SYMBOL_GPL(bpf_prog_add); >> >> +void bpf_prog_add_undo(struct bpf_prog *prog, int i) >> +{ >> + /* Only to be used for undoing previous bpf_prog_add() in some >> + * error path. We still know that another entity in our call >> + * path holds a reference to the program, thus atomic_sub() can >> + * be safely used here! >> + */ >> + atomic_sub(i, &prog->aux->refcnt); >> +} >> +EXPORT_SYMBOL_GPL(bpf_prog_add_undo); >> + >> struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog) >> { >> return bpf_prog_add(prog, 1); >> -- >> 1.9.3 > >> From f0789544432bbb89c53c3b8ac6575d48fed97786 Mon Sep 17 00:00:00 2001 >> Message-Id: <f0789544432bbb89c53c3b8ac6575d48fed97786.1478685278.git.daniel@iogearbox.net> >> In-Reply-To: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> >> References: <d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0.1478685278.git.daniel@iogearbox.net> >> From: Daniel Borkmann <daniel@iogearbox.net> >> Date: Wed, 9 Nov 2016 10:51:26 +0100 >> Subject: [PATCH net-next 2/2] bpf, mlx5: fix prog refcount in mlx5e_xdp_set >> >> dev_change_xdp_fd() already holds a reference, so bpf_prog_add(prog, 1) >> is not correct as it takes one reference too much and will thus leak >> the prog eventually. Also, bpf_prog_add() can fail and is not checked >> for errors here. >> >> 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 | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> index ba0c774..63309dd 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> @@ -3121,8 +3121,6 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog) >> >> /* exchange programs */ >> old_prog = xchg(&priv->xdp_prog, prog); >> - if (prog) >> - bpf_prog_add(prog, 1); > There is also another use of bpf_prog_add down below, which does not > check the error return. Same in mlx5e_create_rq. Yeah, saw that, too. These two unchecked bpf_prog_add() would be another issue to fix on top of this, ohh well. For net-next, I'll just add a __must_check to these functions, so we can avoid such issues in future and let the compiler complain early enough instead. Thanks, Daniel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-09 19:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-09 7:35 [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 Zhiyi Sun 2016-11-09 9:05 ` Daniel Borkmann 2016-11-09 9:45 ` Zhiyi Sun 2016-11-09 9:57 ` Daniel Borkmann 2016-11-09 10:58 ` [PATCH net-next 1/2] bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path kbuild test robot 2016-11-09 11:04 ` Daniel Borkmann 2016-11-09 17:06 ` [PATCH] net/mlx4_en: Fix bpf_prog_add ref_cnt in mlx4 Brenden Blanco 2016-11-09 19:05 ` Daniel Borkmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).