From: Zhiyi Sun <hidden> Date: 2016-11-09 07:35:53
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 <redacted>
---
drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2016-11-09 09:05:41
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 <redacted>
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).
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Zhiyi Sun <hidden> Date: 2016-11-09 09:45:59
On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote:
On 11/09/2016 08:35 AM, Zhiyi Sun wrote:
quoted
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 <redacted>
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2016-11-09 10:20:19
On 11/09/2016 10:45 AM, Zhiyi Sun wrote:
On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote:
quoted
On 11/09/2016 08:35 AM, Zhiyi Sun wrote:
quoted
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 <redacted>
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.
quoted
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.
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)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2016-11-09 11:04:35
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':
quoted
quoted
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
On Wed, Nov 09, 2016 at 10:57:32AM +0100, Daniel Borkmann wrote:
On 11/09/2016 10:45 AM, Zhiyi Sun wrote:
quoted
On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote:
quoted
On 11/09/2016 08:35 AM, Zhiyi Sun wrote:
quoted
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 <redacted>
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.
quoted
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.
quoted hunk
From d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0 Mon Sep 17 00:00:00 2001
Message-Id: [off-list ref]
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(-)
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.
quoted hunk
goto unlock_out;
+ }
if (priv->port_up) {
port_up = 1;
@@ -680,6 +680,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)}EXPORT_SYMBOL_GPL(bpf_prog_add);+voidbpf_prog_add_undo(structbpf_prog*prog,inti)+{+/* Only to be used for undoing previous bpf_prog_add() in some+*errorpath.Westillknowthatanotherentityinourcall+*pathholdsareferencetotheprogram,thusatomic_sub()can+*besafelyusedhere!+*/+atomic_sub(i,&prog->aux->refcnt);+}+EXPORT_SYMBOL_GPL(bpf_prog_add_undo);+structbpf_prog*bpf_prog_inc(structbpf_prog*prog){returnbpf_prog_add(prog,1);
--
1.9.3
quoted hunk
From f0789544432bbb89c53c3b8ac6575d48fed97786 Mon Sep 17 00:00:00 2001
Message-Id: [off-list ref]
In-Reply-To: [off-list ref]
References: [off-list ref]
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(-)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2016-11-09 19:05:46
On 11/09/2016 06:06 PM, Brenden Blanco wrote:
On Wed, Nov 09, 2016 at 10:57:32AM +0100, Daniel Borkmann wrote:
quoted
On 11/09/2016 10:45 AM, Zhiyi Sun wrote:
quoted
On Wed, Nov 09, 2016 at 10:05:31AM +0100, Daniel Borkmann wrote:
quoted
On 11/09/2016 08:35 AM, Zhiyi Sun wrote:
quoted
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 <redacted>
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.
quoted
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.
quoted
From d2bd6b3cd8636716a06b0ea3b1e041e16f87cce0 Mon Sep 17 00:00:00 2001
Message-Id: [off-list ref]
From: Daniel Borkmann <redacted>
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 <redacted>
---
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(-)
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.
quoted
goto unlock_out;
+ }
if (priv->port_up) {
port_up = 1;
@@ -680,6 +680,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)}EXPORT_SYMBOL_GPL(bpf_prog_add);+voidbpf_prog_add_undo(structbpf_prog*prog,inti)+{+/* Only to be used for undoing previous bpf_prog_add() in some+*errorpath.Westillknowthatanotherentityinourcall+*pathholdsareferencetotheprogram,thusatomic_sub()can+*besafelyusedhere!+*/+atomic_sub(i,&prog->aux->refcnt);+}+EXPORT_SYMBOL_GPL(bpf_prog_add_undo);+structbpf_prog*bpf_prog_inc(structbpf_prog*prog){returnbpf_prog_add(prog,1);--
1.9.3
quoted
From f0789544432bbb89c53c3b8ac6575d48fed97786 Mon Sep 17 00:00:00 2001
Message-Id: [off-list ref]
In-Reply-To: [off-list ref]
References: [off-list ref]
From: Daniel Borkmann <redacted>
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 <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 2 --
1 file changed, 2 deletions(-)
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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html