public inbox for netdev@vger.kernel.org 
 help / color / mirror / Atom feed

* [PATCH net 0/3] Couple of BPF refcount fixes for mlx5
@ 2016-11-14  0:43 Daniel Borkmann
  2016-11-14  0:43 ` [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Daniel Borkmann @ 2016-11-14  0:43 UTC (permalink / raw)
  To: davem
  Cc: alexei.starovoitov, bblanco, tariqt, zhiyisun, ranas, netdev,
	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

^ permalink raw reply	[flat|nested] 12+ messages in thread

• * [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog
    2016-11-14  0:43 [PATCH net 0/3] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
  @ 2016-11-14  0:43 ` Daniel Borkmann
    2016-11-14 18:15   ` Saeed Mahameed
    2016-11-14  0:43 ` [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set Daniel Borkmann
    2016-11-14  0:43 ` [PATCH net 3/3] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup Daniel Borkmann
    2 siblings, 1 reply; 12+ messages in thread
  From: Daniel Borkmann @ 2016-11-14  0:43 UTC (permalink / raw)
    To: davem
    Cc: alexei.starovoitov, bblanco, tariqt, zhiyisun, ranas, 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, 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
  
  ^ permalink raw reply related	[flat|nested] 12+ messages in thread

• • * Re: [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog
      2016-11-14  0:43 ` [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
    @ 2016-11-14 18:15   ` Saeed Mahameed
      2016-11-14 18:26     ` Daniel Borkmann
      0 siblings, 1 reply; 12+ messages in thread
    From: Saeed Mahameed @ 2016-11-14 18:15 UTC (permalink / raw)
      To: Daniel Borkmann, davem
      Cc: alexei.starovoitov, bblanco, tariqt, zhiyisun, ranas, netdev
    
    
    
    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.
    
    ^ permalink raw reply	[flat|nested] 12+ messages in thread

  • • * Re: [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog
        2016-11-14 18:15   ` Saeed Mahameed
      @ 2016-11-14 18:26     ` Daniel Borkmann
        0 siblings, 0 replies; 12+ messages in thread
      From: Daniel Borkmann @ 2016-11-14 18:26 UTC (permalink / raw)
        To: Saeed Mahameed, davem
        Cc: alexei.starovoitov, bblanco, tariqt, zhiyisun, ranas, netdev
      
      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
      
      ^ permalink raw reply	[flat|nested] 12+ messages in thread

• * [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
    2016-11-14  0:43 [PATCH net 0/3] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
    2016-11-14  0:43 ` [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
  @ 2016-11-14  0:43 ` Daniel Borkmann
    2016-11-14  2:49   ` Alexei Starovoitov
    2016-11-14 18:27   ` Saeed Mahameed
    2016-11-14  0:43 ` [PATCH net 3/3] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup Daniel Borkmann
    2 siblings, 2 replies; 12+ messages in thread
  From: Daniel Borkmann @ 2016-11-14  0:43 UTC (permalink / raw)
    To: davem
    Cc: alexei.starovoitov, bblanco, tariqt, zhiyisun, ranas, netdev,
  	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
  
  ^ permalink raw reply related	[flat|nested] 12+ messages in thread

• • * Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
      2016-11-14  0:43 ` [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set Daniel Borkmann
    @ 2016-11-14  2:49   ` Alexei Starovoitov
      2016-11-14  8:49     ` Daniel Borkmann
      2016-11-14 18:27   ` Saeed Mahameed
      1 sibling, 1 reply; 12+ messages in thread
    From: Alexei Starovoitov @ 2016-11-14  2:49 UTC (permalink / raw)
      To: Daniel Borkmann; +Cc: davem, bblanco, tariqt, zhiyisun, ranas, netdev
    
    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.
    
    ^ permalink raw reply	[flat|nested] 12+ messages in thread

  • • * Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
        2016-11-14  2:49   ` Alexei Starovoitov
      @ 2016-11-14  8:49     ` Daniel Borkmann
        2016-11-14 17:35       ` Alexei Starovoitov
        0 siblings, 1 reply; 12+ messages in thread
      From: Daniel Borkmann @ 2016-11-14  8:49 UTC (permalink / raw)
        To: Alexei Starovoitov; +Cc: davem, bblanco, tariqt, zhiyisun, ranas, netdev
      
      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
      
      ^ permalink raw reply	[flat|nested] 12+ messages in thread

    • • * Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
          2016-11-14  8:49     ` Daniel Borkmann
        @ 2016-11-14 17:35       ` Alexei Starovoitov
          2016-11-14 18:23         ` Daniel Borkmann
          0 siblings, 1 reply; 12+ messages in thread
        From: Alexei Starovoitov @ 2016-11-14 17:35 UTC (permalink / raw)
          To: Daniel Borkmann; +Cc: davem, bblanco, tariqt, zhiyisun, ranas, netdev
        
        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.
        
        ^ permalink raw reply	[flat|nested] 12+ messages in thread

      • • * Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
            2016-11-14 17:35       ` Alexei Starovoitov
          @ 2016-11-14 18:23         ` Daniel Borkmann
            0 siblings, 0 replies; 12+ messages in thread
          From: Daniel Borkmann @ 2016-11-14 18:23 UTC (permalink / raw)
            To: Alexei Starovoitov; +Cc: davem, bblanco, tariqt, zhiyisun, ranas, netdev
          
          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.
          
          ^ permalink raw reply	[flat|nested] 12+ messages in thread

  • * Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
      2016-11-14  0:43 ` [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set Daniel Borkmann
      2016-11-14  2:49   ` Alexei Starovoitov
    @ 2016-11-14 18:27   ` Saeed Mahameed
      2016-11-14 19:05     ` Daniel Borkmann
      1 sibling, 1 reply; 12+ messages in thread
    From: Saeed Mahameed @ 2016-11-14 18:27 UTC (permalink / raw)
      To: Daniel Borkmann, davem
      Cc: alexei.starovoitov, bblanco, tariqt, zhiyisun, ranas, netdev
    
    
    
    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);
    > 
    
    ^ permalink raw reply	[flat|nested] 12+ messages in thread

  • • * Re: [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set
        2016-11-14 18:27   ` Saeed Mahameed
      @ 2016-11-14 19:05     ` Daniel Borkmann
        0 siblings, 0 replies; 12+ messages in thread
      From: Daniel Borkmann @ 2016-11-14 19:05 UTC (permalink / raw)
        To: Saeed Mahameed, davem
        Cc: alexei.starovoitov, bblanco, tariqt, zhiyisun, ranas, netdev
      
      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>
      
      ^ permalink raw reply	[flat|nested] 12+ messages in thread

• * [PATCH net 3/3] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup
    2016-11-14  0:43 [PATCH net 0/3] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
    2016-11-14  0:43 ` [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
    2016-11-14  0:43 ` [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set Daniel Borkmann
  @ 2016-11-14  0:43 ` Daniel Borkmann
    2 siblings, 0 replies; 12+ messages in thread
  From: Daniel Borkmann @ 2016-11-14  0:43 UTC (permalink / raw)
    To: davem
    Cc: alexei.starovoitov, bblanco, tariqt, zhiyisun, ranas, 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.
  
  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
  
  ^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-11-14 22:48 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-14  0:43 [PATCH net 0/3] Couple of BPF refcount fixes for mlx5 Daniel Borkmann
2016-11-14  0:43 ` [PATCH net 1/3] bpf, mlx5: fix mlx5e_create_rq taking reference on prog Daniel Borkmann
2016-11-14 18:15   ` Saeed Mahameed
2016-11-14 18:26     ` Daniel Borkmann
2016-11-14  0:43 ` [PATCH net 2/3] bpf, mlx5: fix various refcount/prog issues in mlx5e_xdp_set Daniel Borkmann
2016-11-14  2:49   ` Alexei Starovoitov
2016-11-14  8:49     ` Daniel Borkmann
2016-11-14 17:35       ` Alexei Starovoitov
2016-11-14 18:23         ` Daniel Borkmann
2016-11-14 18:27   ` Saeed Mahameed
2016-11-14 19:05     ` Daniel Borkmann
2016-11-14  0:43 ` [PATCH net 3/3] bpf, mlx5: drop priv->xdp_prog reference on netdev cleanup Daniel Borkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox