netdev - Re: [net-next PATCH v3 07/15] net: sched: RCU cls

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, 09 Sep 2014 05:59:13 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	John Fastabend <john.fastabend@...il.com>
Cc:	xiyou.wangcong@...il.com, davem@...emloft.net, jhs@...atatu.com,
	netdev@...r.kernel.org, paulmck@...ux.vnet.ibm.com,
	brouer@...hat.com
Subject: Re: [net-next PATCH v3 07/15] net: sched: RCU cls_route

On Mon, 2014-09-08 at 22:57 -0700, John Fastabend wrote:
> RCUify the route classifier. For now however spinlock's are used to
> protect fastmap cache.
> 
> The issue here is the fastmap may be read by one CPU while the
> cache is being updated by another. An array of pointers could be
> one possible solution.

Yep, this doesnt seem like an urgent issue anyway.

> 
> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
> ---

...

>  
>  static void route4_destroy(struct tcf_proto *tp)
>  {
> -	struct route4_head *head = tp->root;
> +	struct route4_head *head = rtnl_dereference(tp->root);
>  	int h1, h2;
>  
>  	if (head == NULL)
> @@ -266,28 +287,35 @@ static void route4_destroy(struct tcf_proto *tp)
>  	for (h1 = 0; h1 <= 256; h1++) {
>  		struct route4_bucket *b;
>  
> -		b = head->table[h1];
> +		b = rtnl_dereference(head->table[h1]);
>  		if (b) {
>  			for (h2 = 0; h2 <= 32; h2++) {
>  				struct route4_filter *f;
>  
> -				while ((f = b->ht[h2]) != NULL) {
> -					b->ht[h2] = f->next;
> -					route4_delete_filter(tp, f);
> +				while ((f = rtnl_dereference(b->ht[h2])) != NULL) {
> +					struct route4_filter *next;
> +
> +					next = rtnl_dereference(f->next);
> +					rcu_assign_pointer(b->ht[h2], next);

	RCU_INIT_POINTER(), or simply : b->ht[h2] = f->next;

> +					call_rcu(&f->rcu, route4_delete_filter);
>  				}
>  			}
> -			kfree(b);
> +			RCU_INIT_POINTER(head->table[h1], NULL);
> +			kfree_rcu(b, rcu);
>  		}
>  	}
> -	kfree(head);
> +	RCU_INIT_POINTER(tp->root, NULL);
> +	kfree_rcu(head, rcu);
>  }
>  
>  static int route4_delete(struct tcf_proto *tp, unsigned long arg)
>  {
> -	struct route4_head *head = tp->root;
> -	struct route4_filter **fp, *f = (struct route4_filter *)arg;
> -	unsigned int h = 0;
> +	struct route4_head *head = rtnl_dereference(tp->root);
> +	struct route4_filter *f = (struct route4_filter *)arg;
> +	struct route4_filter __rcu **fp;
> +	struct route4_filter *nf;
>  	struct route4_bucket *b;
> +	unsigned int h = 0;
>  	int i;
>  
>  	if (!head || !f)
> @@ -296,27 +324,35 @@ static int route4_delete(struct tcf_proto *tp, unsigned long arg)
>  	h = f->handle;
>  	b = f->bkt;
>  
> -	for (fp = &b->ht[from_hash(h >> 16)]; *fp; fp = &(*fp)->next) {
> -		if (*fp == f) {
> -			tcf_tree_lock(tp);
> +	fp = &b->ht[from_hash(h >> 16)];
> +	for (nf = rtnl_dereference(*fp); nf;
> +	     fp = &nf->next, nf = rtnl_dereference(*fp)) {
> +		if (nf == f) {
> +			/* unlink it */
>  			*fp = f->next;

Strange you left this without annotations, while rest of your patches
always use rcu_assign_pointer(*fp, rtnl_dereference(f->next)

Note that it is absolutely fine to use *fp = f->next;

;)

> -			tcf_tree_unlock(tp);
>  



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html