lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 23 Apr 2019 16:52:59 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     Vlad Buslov <vladbu@...lanox.com>
CC:     "jhs@...atatu.com" <jhs@...atatu.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>
Subject: Re: [PATCH net-next] net: sched: flower: refactor reoffload for
 concurrent access

On Tue, 2019-04-23 at 07:43 +0000, Vlad Buslov wrote:
> On Mon 22 Apr 2019 at 23:34, Saeed Mahameed <saeedm@...lanox.com>
> wrote:
> > On Mon, 2019-04-22 at 10:21 +0300, Vlad Buslov wrote:
> > > Recent changes that introduced unlocked flower did not properly
> > > account for
> > > case when reoffload is initiated concurrently with filter
> > > updates. To
> > > fix
> > > the issue, extend flower with 'hw_filters' list that is used to
> > > store
> > > filters that don't have 'skip_hw' flag set. Filter is added to
> > > the
> > > list
> > > when it is inserted to hardware and only removed from it after
> > > being
> > > unoffloaded from all drivers that parent block is attached to.
> > > This
> > > ensures
> > > that concurrent reoffload can still access filter that is being
> > > deleted and
> > > prevents race condition when driver callback can be removed when
> > > filter is
> > > no longer accessible trough idr, but is still present in
> > > hardware.
> > > 
> > > Refactor fl_change() to respect new filter reference counter and
> > > to
> > > release
> > > filter reference with __fl_put() in case of error, instead of
> > > directly
> > > deallocating filter memory. This allows for concurrent access to
> > > filter
> > > from fl_reoffload() and protects it with reference counting.
> > > Refactor
> > > fl_reoffload() to iterate over hw_filters list instead of idr.
> > > Implement
> > > fl_get_next_hw_filter() helper function that is used to iterate
> > > over
> > > hw_filters list with reference counting and skips filters that
> > > are
> > > being
> > > concurrently deleted.
> > > 
> > > Fixes: 92149190067d ("net: sched: flower: set unlocked flag for
> > > flower proto ops")
> > > Signed-off-by: Vlad Buslov <vladbu@...lanox.com>
> > > Reported-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> > > ---
> > >  net/sched/cls_flower.c | 77 +++++++++++++++++++++++++++++++-----
> > > ----
> > > --
> > >  1 file changed, 57 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> > > index 4b5585358699..524b86560af3 100644
> > > --- a/net/sched/cls_flower.c
> > > +++ b/net/sched/cls_flower.c
> > > @@ -90,6 +90,7 @@ struct cls_fl_head {
> > >  	struct rhashtable ht;
> > >  	spinlock_t masks_lock; /* Protect masks list */
> > >  	struct list_head masks;
> > > +	struct list_head hw_filters;
> > >  	struct rcu_work rwork;
> > >  	struct idr handle_idr;
> > >  };
> > > @@ -102,6 +103,7 @@ struct cls_fl_filter {
> > >  	struct tcf_result res;
> > >  	struct fl_flow_key key;
> > >  	struct list_head list;
> > > +	struct list_head hw_list;
> > >  	u32 handle;
> > >  	u32 flags;
> > >  	u32 in_hw_count;
> > > @@ -315,6 +317,7 @@ static int fl_init(struct tcf_proto *tp)
> > > 
> > >  	spin_lock_init(&head->masks_lock);
> > >  	INIT_LIST_HEAD_RCU(&head->masks);
> > > +	INIT_LIST_HEAD(&head->hw_filters);
> > >  	rcu_assign_pointer(tp->root, head);
> > >  	idr_init(&head->handle_idr);
> > > 
> > > @@ -352,6 +355,16 @@ static bool fl_mask_put(struct cls_fl_head
> > > *head, struct fl_flow_mask *mask)
> > >  	return true;
> > >  }
> > > 
> > > +static struct cls_fl_head *fl_head_dereference(struct tcf_proto
> > > *tp)
> > > +{
> > > +	/* Flower classifier only changes root pointer during init and
> > > destroy.
> > > +	 * Users must obtain reference to tcf_proto instance before
> > > calling its
> > > +	 * API, so tp->root pointer is protected from concurrent call
> > > to
> > > +	 * fl_destroy() by reference counting.
> > > +	 */
> > > +	return rcu_dereference_raw(tp->root);
> > > +}
> > > +
> > >  static void __fl_destroy_filter(struct cls_fl_filter *f)
> > >  {
> > >  	tcf_exts_destroy(&f->exts);
> > > @@ -382,6 +395,8 @@ static void fl_hw_destroy_filter(struct
> > > tcf_proto
> > > *tp, struct cls_fl_filter *f,
> > > 
> > >  	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower,
> > > false);
> > >  	spin_lock(&tp->lock);
> > > +	if (!list_empty(&f->hw_list))
> > > +		list_del_init(&f->hw_list);
> > >  	tcf_block_offload_dec(block, &f->flags);
> > >  	spin_unlock(&tp->lock);
> > > 
> > > @@ -393,6 +408,7 @@ static int fl_hw_replace_filter(struct
> > > tcf_proto
> > > *tp,
> > >  				struct cls_fl_filter *f, bool
> > > rtnl_held,
> > >  				struct netlink_ext_ack *extack)
> > >  {
> > > +	struct cls_fl_head *head = fl_head_dereference(tp);
> > >  	struct tc_cls_flower_offload cls_flower = {};
> > >  	struct tcf_block *block = tp->chain->block;
> > >  	bool skip_sw = tc_skip_sw(f->flags);
> > > @@ -444,6 +460,9 @@ static int fl_hw_replace_filter(struct
> > > tcf_proto
> > > *tp,
> > >  		goto errout;
> > >  	}
> > > 
> > > +	spin_lock(&tp->lock);
> > > +	list_add(&f->hw_list, &head->hw_filters);
> > > +	spin_unlock(&tp->lock);
> > >  errout:
> > >  	if (!rtnl_held)
> > >  		rtnl_unlock();
> > > @@ -475,23 +494,11 @@ static void fl_hw_update_stats(struct
> > > tcf_proto
> > > *tp, struct cls_fl_filter *f,
> > >  		rtnl_unlock();
> > >  }
> > > 
> > > -static struct cls_fl_head *fl_head_dereference(struct tcf_proto
> > > *tp)
> > > -{
> > > -	/* Flower classifier only changes root pointer during init and
> > > destroy.
> > > -	 * Users must obtain reference to tcf_proto instance before
> > > calling its
> > > -	 * API, so tp->root pointer is protected from concurrent call
> > > to
> > > -	 * fl_destroy() by reference counting.
> > > -	 */
> > > -	return rcu_dereference_raw(tp->root);
> > > -}
> > > -
> > >  static void __fl_put(struct cls_fl_filter *f)
> > >  {
> > >  	if (!refcount_dec_and_test(&f->refcnt))
> > >  		return;
> > > 
> > > -	WARN_ON(!f->deleted);
> > > -
> > >  	if (tcf_exts_get_net(&f->exts))
> > >  		tcf_queue_work(&f->rwork, fl_destroy_filter_work);
> > >  	else
> > > @@ -1522,6 +1529,7 @@ static int fl_change(struct net *net,
> > > struct
> > > sk_buff *in_skb,
> > >  		err = -ENOBUFS;
> > >  		goto errout_tb;
> > >  	}
> > > +	INIT_LIST_HEAD(&fnew->hw_list);
> > >  	refcount_set(&fnew->refcnt, 1);
> > > 
> > >  	err = tcf_exts_init(&fnew->exts, net, TCA_FLOWER_ACT, 0);
> > > @@ -1569,7 +1577,6 @@ static int fl_change(struct net *net,
> > > struct
> > > sk_buff *in_skb,
> > >  		goto errout_hw;
> > >  	}
> > > 
> > > -	refcount_inc(&fnew->refcnt);
> > >  	if (fold) {
> > >  		/* Fold filter was deleted concurrently. Retry lookup.
> > > */
> > >  		if (fold->deleted) {
> > > @@ -1591,6 +1598,7 @@ static int fl_change(struct net *net,
> > > struct
> > > sk_buff *in_skb,
> > >  			in_ht = true;
> > >  		}
> > > 
> > > +		refcount_inc(&fnew->refcnt);
> > >  		rhashtable_remove_fast(&fold->mask->ht,
> > >  				       &fold->ht_node,
> > >  				       fold->mask->filter_ht_params);
> > > @@ -1631,6 +1639,7 @@ static int fl_change(struct net *net,
> > > struct
> > > sk_buff *in_skb,
> > >  		if (err)
> > >  			goto errout_hw;
> > > 
> > > +		refcount_inc(&fnew->refcnt);
> > >  		fnew->handle = handle;
> > >  		list_add_tail_rcu(&fnew->list, &fnew->mask->filters);
> > >  		spin_unlock(&tp->lock);
> > > @@ -1642,19 +1651,20 @@ static int fl_change(struct net *net,
> > > struct
> > > sk_buff *in_skb,
> > >  	kfree(mask);
> > >  	return 0;
> > > 
> > > +errout_ht:
> > > +	spin_lock(&tp->lock);
> > >  errout_hw:
> > > +	fnew->deleted = true;
> > >  	spin_unlock(&tp->lock);
> > >  	if (!tc_skip_hw(fnew->flags))
> > >  		fl_hw_destroy_filter(tp, fnew, rtnl_held, NULL);
> > > -errout_ht:
> > >  	if (in_ht)
> > >  		rhashtable_remove_fast(&fnew->mask->ht, &fnew->ht_node,
> > >  				       fnew->mask->filter_ht_params);
> > >  errout_mask:
> > >  	fl_mask_put(head, fnew->mask);
> > >  errout:
> > > -	tcf_exts_get_net(&fnew->exts);
> > > -	tcf_queue_work(&fnew->rwork, fl_destroy_filter_work);
> > > +	__fl_put(fnew);
> > >  errout_tb:
> > >  	kfree(tb);
> > >  errout_mask_alloc:
> > > @@ -1699,16 +1709,44 @@ static void fl_walk(struct tcf_proto *tp,
> > > struct tcf_walker *arg,
> > >  	}
> > >  }
> > > 
> > > +static struct cls_fl_filter *
> > > +fl_get_next_hw_filter(struct tcf_proto *tp, struct cls_fl_filter
> > > *f,
> > > bool add)
> > > +{
> > > +	struct cls_fl_head *head = fl_head_dereference(tp);
> > > +
> > > +	spin_lock(&tp->lock);
> > > +	if (!f) {
> > > +		if (list_empty(&head->hw_filters)) {
> > > +			spin_unlock(&tp->lock);
> > > +			return NULL;
> > > +		}
> > 
> > Shouldn't this be a pre-condition to the whole function ? i mean
> > regardless of whether 'f' is NULL or not ?
> 
> List can't be empty if we already have an element of the list (f), so
> why check this on every iteration?
> 

Because you release the lock on every iteration, you can't just assume
that the list is still intact.

> > > +
> > > +		f = list_first_entry(&head->hw_filters, struct
> > > cls_fl_filter,
> > > +				     hw_list);
> > > +	} else {
> > > +		f = list_next_entry(f, hw_list);
> > > +	}
> > > +
> > 
> > Maybe if you use  list_for_each_entry_continue below, might
> > simplify
> > the above logic. it is weird that you need to figure out next entry
> > then call list_for_each_from, list 'continue' variation is made for
> > such use cases.
> 
> list_for_each_entry_continue requires initialized cursor and will
> skip
> first element if we obtain initial cursor with list_first_entry(). We
> can have two loops - one that uses list_for_each_entry for initial
> iteration, and another one that uses list_for_each_entry_continue for
> case when f!=NULL, but I don't see how that would be any simpler.
> 

you can set the initial cursor to list->head and not first entry.

It would be simpler if you do:

if (list_empty())
    return NULL;
pos = f ? f : list->head;
list_for_each_entry_continue(pos, ...);

> > > +	list_for_each_entry_from(f, &head->hw_filters, hw_list) {
> > > +		if (!(add && f->deleted) && refcount_inc_not_zero(&f-
> > > > refcnt)) {
> > > +			spin_unlock(&tp->lock);
> > > +			return f;
> > > +		}
> > > +	}
> > > +
> > > +	spin_unlock(&tp->lock);
> > > +	return NULL;
> > > +}
> > > +
> > >  static int fl_reoffload(struct tcf_proto *tp, bool add,
> > > tc_setup_cb_t *cb,
> > >  			void *cb_priv, struct netlink_ext_ack *extack)
> > >  {
> > >  	struct tc_cls_flower_offload cls_flower = {};
> > >  	struct tcf_block *block = tp->chain->block;
> > > -	unsigned long handle = 0;
> > > -	struct cls_fl_filter *f;
> > > +	struct cls_fl_filter *f = NULL;
> > >  	int err;
> > > 
> > > -	while ((f = fl_get_next_filter(tp, &handle))) {
> > > +	while ((f = fl_get_next_hw_filter(tp, f, add))) {
> > >  		if (tc_skip_hw(f->flags))
> > >  			goto next_flow;
> > 
> > this can never be true as it is already a pre-condition for
> > fl_hw_replace_filter which actually adds  to the hw_filters list, i
> > think it needs to be removed. if it is, then i think it should be
> > part
> > of fl_get_next_hw_filter and not the caller responsibility.
> 
> Good catch.
> 
> > > @@ -1757,7 +1795,6 @@ static int fl_reoffload(struct tcf_proto
> > > *tp,
> > > bool add, tc_setup_cb_t *cb,
> > >  					  add);
> > >  		spin_unlock(&tp->lock);
> > >  next_flow:
> > > -		handle++;
> > >  		__fl_put(f);
> > >  	}
> > > 

Powered by blists - more mailing lists