[PATCH net 0/3] Couple of BPF refcount fixes for mlx5

[PATCH net 0/3] Couple of BPF refcount fixes for mlx5

[Daniel Borkmann]

Various mlx5 bugs on eBPF program and refcount handling I found during review.
Since these kind of bugs happened multiple times here, I'll add a __must_check
to the bpf_prog_inc()/bpf_prog_add()/etc functions for net-next, so these things
will let the compiler (and thus kbuild bot) bark early enough. Note, turned out,
I had to take the hunk from c540594f864b ("bpf, mlx4: fix prog refcount in
mlx4_en_try_alloc_resources error path") to get bpf_prog_sub() function for net
as well, but the merge into net-next should add no conflicts.

Rana, please review.

Thanks a lot!

Daniel Borkmann (3):
  bpf, mlx5: fix mlx5e_create_rq taking reference on prog
  bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
  bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup

 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 42 ++++++++++++++++++-----
 include/linux/bpf.h                               |  5 +++
 kernel/bpf/syscall.c                              | 12 +++++++
 3 files changed, 51 insertions(+), 8 deletions(-)

-- 
1.9.3

[PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog

[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, 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 | 14 +++++++++++---
 kernel/bpf/syscall.c                              |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 84e8b25..2b83667 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -489,7 +489,16 @@ 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;
+	if (rq->xdp_prog) {
+		rq->xdp_prog = bpf_prog_inc(rq->xdp_prog);
+		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)
@@ -566,12 +575,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 237f3d6..751e806 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -686,6 +686,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

[PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set

[Daniel Borkmann]

There are multiple issues in mlx5e_xdp_set():

1) prog can be NULL, so calling unconditionally into bpf_prog_add(prog,
   priv->params.num_channels) can end badly.

2) The batched bpf_prog_add() should be done at an earlier point in
   time. This 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 early due to
   reset or device not in MLX5E_STATE_OPENED yet. Note, err is 0 here.

3) 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 <daniel@iogearbox.net>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25 ++++++++++++++++++-----
 include/linux/bpf.h                               |  5 +++++
 kernel/bpf/syscall.c                              | 11 ++++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 2b83667..c90610a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3125,6 +3125,17 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 		goto unlock;
 	}
 
+	if (prog) {
+		/* 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;
+		}
+	}
+
 	was_opened = test_bit(MLX5E_STATE_OPENED, &priv->state);
 	/* no need for full reset when exchanging programs */
 	reset = (!priv->xdp_prog || !prog);
@@ -3132,10 +3143,10 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 	if (was_opened && reset)
 		mlx5e_close_locked(netdev);
 
-	/* 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);
 
@@ -3146,12 +3157,11 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 		mlx5e_open_locked(netdev);
 
 	if (!test_bit(MLX5E_STATE_OPENED, &priv->state) || reset)
-		goto unlock;
+		goto unlock_put;
 
 	/* 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];
 
@@ -3173,6 +3183,11 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 unlock:
 	mutex_unlock(&priv->state_lock);
 	return err;
+unlock_put:
+	/* reference on priv->xdp_prog is still held at this point */
+	if (prog)
+		bpf_prog_sub(prog, priv->params.num_channels);
+	goto unlock;
 }
 
 static bool mlx5e_xdp_attached(struct net_device *dev)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c201017..ca495fd 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_sub(struct bpf_prog *prog, int i);
 struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog);
 void bpf_prog_put(struct bpf_prog *prog);
 
@@ -303,6 +304,10 @@ static inline struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline void bpf_prog_sub(struct bpf_prog *prog, int i)
+{
+}
+
 static inline void bpf_prog_put(struct bpf_prog *prog)
 {
 }
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 751e806..a0fca9f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -682,6 +682,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
 }
 EXPORT_SYMBOL_GPL(bpf_prog_add);
 
+void bpf_prog_sub(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 in such cases!
+	 */
+	WARN_ON(atomic_sub_return(i, &prog->aux->refcnt) == 0);
+}
+EXPORT_SYMBOL_GPL(bpf_prog_sub);
+
 struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog)
 {
 	return bpf_prog_add(prog, 1);
-- 
1.9.3

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

[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 <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 c90610a..930aa6f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3697,6 +3697,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

Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set

[Alexei Starovoitov]

On Mon, Nov 14, 2016 at 01:43:41AM +0100, Daniel Borkmann wrote:
> There are multiple issues in mlx5e_xdp_set():
> 
> 1) prog can be NULL, so calling unconditionally into bpf_prog_add(prog,
>    priv->params.num_channels) can end badly.
> 
> 2) The batched bpf_prog_add() should be done at an earlier point in
>    time. This 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 early due to
>    reset or device not in MLX5E_STATE_OPENED yet. Note, err is 0 here.
> 
> 3) 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 <daniel@iogearbox.net>
...
> +static inline void bpf_prog_sub(struct bpf_prog *prog, int i)
> +{
> +}
> +
>  static inline void bpf_prog_put(struct bpf_prog *prog)
>  {
>  }
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 751e806..a0fca9f 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -682,6 +682,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
>  }
>  EXPORT_SYMBOL_GPL(bpf_prog_add);
>  
> +void bpf_prog_sub(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 in such cases!
> +	 */
> +	WARN_ON(atomic_sub_return(i, &prog->aux->refcnt) == 0);
> +}
> +EXPORT_SYMBOL_GPL(bpf_prog_sub);

the patches look good. I'm only worried about net/net-next merge
conflict here. (I would have to deal with it as well).
So instead of copying the above helper can we apply net-next's
'bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path'
patch to net without mlx4_xdp_set hunk and then apply
the rest of this patch?
Even better is to send this patch 2/3 to net-next?
yes, it's an issue, but very small one. There is no security
concern here, so I would prefer to avoid merge conflict.
Did you do a test merge of net/net-next by any chance?
May be I'm overreacting.

Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set

[Daniel Borkmann]

On 11/14/2016 03:49 AM, Alexei Starovoitov wrote:
> On Mon, Nov 14, 2016 at 01:43:41AM +0100, Daniel Borkmann wrote:
[...]
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 751e806..a0fca9f 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -682,6 +682,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
>>   }
>>   EXPORT_SYMBOL_GPL(bpf_prog_add);
>>
>> +void bpf_prog_sub(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 in such cases!
>> +	 */
>> +	WARN_ON(atomic_sub_return(i, &prog->aux->refcnt) == 0);
>> +}
>> +EXPORT_SYMBOL_GPL(bpf_prog_sub);
>
> the patches look good. I'm only worried about net/net-next merge
> conflict here. (I would have to deal with it as well).
> So instead of copying the above helper can we apply net-next's
> 'bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path'
> patch to net without mlx4_xdp_set hunk and then apply
> the rest of this patch?
> Even better is to send this patch 2/3 to net-next?
> yes, it's an issue, but very small one. There is no security
> concern here, so I would prefer to avoid merge conflict.
> Did you do a test merge of net/net-next by any chance?

Yes, I did a test merge and git resolved the above just fine w/o
any conflicts. I have no strong opinion whether net or net-next.
If preferred, I can just resend this series in the evening against
net-next instead, perhaps that's a bit better.

Thanks,
Daniel

Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set

[Alexei Starovoitov]

On Mon, Nov 14, 2016 at 09:49:49AM +0100, Daniel Borkmann wrote:
> On 11/14/2016 03:49 AM, Alexei Starovoitov wrote:
> >On Mon, Nov 14, 2016 at 01:43:41AM +0100, Daniel Borkmann wrote:
> [...]
> >>diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >>index 751e806..a0fca9f 100644
> >>--- a/kernel/bpf/syscall.c
> >>+++ b/kernel/bpf/syscall.c
> >>@@ -682,6 +682,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
> >>  }
> >>  EXPORT_SYMBOL_GPL(bpf_prog_add);
> >>
> >>+void bpf_prog_sub(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 in such cases!
> >>+	 */
> >>+	WARN_ON(atomic_sub_return(i, &prog->aux->refcnt) == 0);
> >>+}
> >>+EXPORT_SYMBOL_GPL(bpf_prog_sub);
> >
> >the patches look good. I'm only worried about net/net-next merge
> >conflict here. (I would have to deal with it as well).
> >So instead of copying the above helper can we apply net-next's
> >'bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path'
> >patch to net without mlx4_xdp_set hunk and then apply
> >the rest of this patch?
> >Even better is to send this patch 2/3 to net-next?
> >yes, it's an issue, but very small one. There is no security
> >concern here, so I would prefer to avoid merge conflict.
> >Did you do a test merge of net/net-next by any chance?
> 
> Yes, I did a test merge and git resolved the above just fine w/o
> any conflicts. I have no strong opinion whether net or net-next.
> If preferred, I can just resend this series in the evening against
> net-next instead, perhaps that's a bit better.

I have slight preference to go via net-next, but since it merges fine,
I don't mind net route too.

Re: [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog

[Saeed Mahameed]

On 11/14/2016 02:43 AM, Daniel Borkmann 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, so we really

Didn't know this, thanks for noticing, I wonder why taking a reference for an object would fail ?
especially when someone is requesting from the driver to take a reference to it ndo_xdp_set ?! sounds like a bad design.

Anyway I will check that later.

> 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 | 14 +++++++++++---
>  kernel/bpf/syscall.c                              |  1 +
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 84e8b25..2b83667 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -489,7 +489,16 @@ 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;

Why keeping this assignment ? just test priv->xdp_prog.

> +	if (rq->xdp_prog) {
> +		rq->xdp_prog = bpf_prog_inc(rq->xdp_prog);
> +		if (IS_ERR(rq->xdp_prog)) {
> +			err = PTR_ERR(rq->xdp_prog);
> +			rq->xdp_prog = NULL;
> +			goto err_rq_wq_destroy;
> +		}
> +	}

Try this, simpler and less indentations:
 
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;
}

Saeed.

Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set

[Daniel Borkmann]

On 11/14/2016 06:35 PM, Alexei Starovoitov wrote:
> On Mon, Nov 14, 2016 at 09:49:49AM +0100, Daniel Borkmann wrote:
>> On 11/14/2016 03:49 AM, Alexei Starovoitov wrote:
>>> On Mon, Nov 14, 2016 at 01:43:41AM +0100, Daniel Borkmann wrote:
>> [...]
>>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>>> index 751e806..a0fca9f 100644
>>>> --- a/kernel/bpf/syscall.c
>>>> +++ b/kernel/bpf/syscall.c
>>>> @@ -682,6 +682,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(bpf_prog_add);
>>>>
>>>> +void bpf_prog_sub(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 in such cases!
>>>> +	 */
>>>> +	WARN_ON(atomic_sub_return(i, &prog->aux->refcnt) == 0);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(bpf_prog_sub);
>>>
>>> the patches look good. I'm only worried about net/net-next merge
>>> conflict here. (I would have to deal with it as well).
>>> So instead of copying the above helper can we apply net-next's
>>> 'bpf, mlx4: fix prog refcount in mlx4_en_try_alloc_resources error path'
>>> patch to net without mlx4_xdp_set hunk and then apply
>>> the rest of this patch?
>>> Even better is to send this patch 2/3 to net-next?
>>> yes, it's an issue, but very small one. There is no security
>>> concern here, so I would prefer to avoid merge conflict.
>>> Did you do a test merge of net/net-next by any chance?
>>
>> Yes, I did a test merge and git resolved the above just fine w/o
>> any conflicts. I have no strong opinion whether net or net-next.
>> If preferred, I can just resend this series in the evening against
>> net-next instead, perhaps that's a bit better.
>
> I have slight preference to go via net-next, but since it merges fine,
> I don't mind net route too.

Ok, I'll rebase for net-next then.

Re: [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog

[Daniel Borkmann]

Hi Saeed,

On 11/14/2016 07:15 PM, Saeed Mahameed wrote:
> On 11/14/2016 02:43 AM, Daniel Borkmann 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, so we really
>
> Didn't know this, thanks for noticing, I wonder why taking a reference for an object would fail ?
> especially when someone is requesting from the driver to take a reference to it ndo_xdp_set ?! sounds like a bad design.
>
> Anyway I will check that later.

See 92117d8443bc ("bpf: fix refcnt overflow").

>> 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 | 14 +++++++++++---
>>   kernel/bpf/syscall.c                              |  1 +
>>   2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index 84e8b25..2b83667 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -489,7 +489,16 @@ 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;
>
> Why keeping this assignment ? just test priv->xdp_prog.
>
>> +	if (rq->xdp_prog) {
>> +		rq->xdp_prog = bpf_prog_inc(rq->xdp_prog);
>> +		if (IS_ERR(rq->xdp_prog)) {
>> +			err = PTR_ERR(rq->xdp_prog);
>> +			rq->xdp_prog = NULL;
>> +			goto err_rq_wq_destroy;
>> +		}
>> +	}
>
> Try this, simpler and less indentations:
>
> 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;
> }

Sure, I don't mind. Will do.

Thanks,
Daniel

Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set

[Saeed Mahameed]

On 11/14/2016 02:43 AM, Daniel Borkmann wrote:
> There are multiple issues in mlx5e_xdp_set():
> 
> 1) prog can be NULL, so calling unconditionally into bpf_prog_add(prog,
>    priv->params.num_channels) can end badly.

not correct, if prog is null we will never get to bpf_prog_add:

        reset = (!priv->xdp_prog || !prog);
        [...]
	if (!test_bit(MLX5E_STATE_OPENED, &priv->state) || reset)
		goto unlock;
        bpf_prog_add...
         

> 
> 2) The batched bpf_prog_add() should be done at an earlier point in
>    time. This 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 early due to
>    reset or device not in MLX5E_STATE_OPENED yet. Note, err is 0 here.
> 

It is delayed for a reason, we do delayed batched bpf_prog_add() 
only when reset is not required (exchanging prog/old_prg) when both prog and old_prog are not null,
which means the only thing that could fail in this case is bpf_prog_add.

so i don't see any reason for changing the logic, checking for  bpf_prog_add return value would be sufficient.

Sorry I need to go for now, I will continue reviewing this patch later.  but this patch looks a little bit exaggerated.

> 3) 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 <daniel@iogearbox.net>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 25 ++++++++++++++++++-----
>  include/linux/bpf.h                               |  5 +++++
>  kernel/bpf/syscall.c                              | 11 ++++++++++
>  3 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 2b83667..c90610a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -3125,6 +3125,17 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
>  		goto unlock;
>  	}
>  
> +	if (prog) {
> +		/* 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;
> +		}
> +	}
> +
>  	was_opened = test_bit(MLX5E_STATE_OPENED, &priv->state);
>  	/* no need for full reset when exchanging programs */
>  	reset = (!priv->xdp_prog || !prog);
> @@ -3132,10 +3143,10 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
>  	if (was_opened && reset)
>  		mlx5e_close_locked(netdev);
>  
> -	/* 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);
>  
> @@ -3146,12 +3157,11 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
>  		mlx5e_open_locked(netdev);
>  
>  	if (!test_bit(MLX5E_STATE_OPENED, &priv->state) || reset)
> -		goto unlock;
> +		goto unlock_put;
>  
>  	/* 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];
>  
> @@ -3173,6 +3183,11 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
>  unlock:
>  	mutex_unlock(&priv->state_lock);
>  	return err;
> +unlock_put:
> +	/* reference on priv->xdp_prog is still held at this point */
> +	if (prog)
> +		bpf_prog_sub(prog, priv->params.num_channels);
> +	goto unlock;
>  }
>  
>  static bool mlx5e_xdp_attached(struct net_device *dev)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index c201017..ca495fd 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_sub(struct bpf_prog *prog, int i);
>  struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog);
>  void bpf_prog_put(struct bpf_prog *prog);
>  
> @@ -303,6 +304,10 @@ static inline struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
>  	return ERR_PTR(-EOPNOTSUPP);
>  }
>  
> +static inline void bpf_prog_sub(struct bpf_prog *prog, int i)
> +{
> +}
> +
>  static inline void bpf_prog_put(struct bpf_prog *prog)
>  {
>  }
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 751e806..a0fca9f 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -682,6 +682,17 @@ struct bpf_prog *bpf_prog_add(struct bpf_prog *prog, int i)
>  }
>  EXPORT_SYMBOL_GPL(bpf_prog_add);
>  
> +void bpf_prog_sub(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 in such cases!
> +	 */
> +	WARN_ON(atomic_sub_return(i, &prog->aux->refcnt) == 0);
> +}
> +EXPORT_SYMBOL_GPL(bpf_prog_sub);
> +
>  struct bpf_prog *bpf_prog_inc(struct bpf_prog *prog)
>  {
>  	return bpf_prog_add(prog, 1);
> 

Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set

[Daniel Borkmann]

On 11/14/2016 07:27 PM, Saeed Mahameed wrote:
> On 11/14/2016 02:43 AM, Daniel Borkmann wrote:
>> There are multiple issues in mlx5e_xdp_set():
>>
>> 1) prog can be NULL, so calling unconditionally into bpf_prog_add(prog,
>>     priv->params.num_channels) can end badly.
>
> not correct, if prog is null we will never get to bpf_prog_add:
>
>          reset = (!priv->xdp_prog || !prog);
>          [...]
> 	if (!test_bit(MLX5E_STATE_OPENED, &priv->state) || reset)
> 		goto unlock;
>          bpf_prog_add...

Yep, I noticed already, dropped this from the changelog for net-next
rebase anyway.

>> 2) The batched bpf_prog_add() should be done at an earlier point in
>>     time. This 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 early due to
>>     reset or device not in MLX5E_STATE_OPENED yet. Note, err is 0 here.
>
> It is delayed for a reason, we do delayed batched bpf_prog_add()
> only when reset is not required (exchanging prog/old_prg) when both prog and old_prog are not null,
> which means the only thing that could fail in this case is bpf_prog_add.

Right, plus 3) below.

> so i don't see any reason for changing the logic, checking for  bpf_prog_add return value would be sufficient.

Yes, but if that fails, it would be better to check early for bailing out
since at this point in time undoing logic becomes unnecessary ugly.

> Sorry I need to go for now, I will continue reviewing this patch later.  but this patch looks a little bit exaggerated.
>
>> 3) 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 <daniel@iogearbox.net>