linux-kernel - Re: [PATCH net-next 19/19] rxrpc: Use RCU to access a peer's service connection tree

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]

Message-ID: <20160630153049.GG30935@twins.programming.kicks-ass.net>
Date:	Thu, 30 Jun 2016 17:30:49 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	David Howells <dhowells@...hat.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-afs@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 19/19] rxrpc: Use RCU to access a peer's service
 connection tree

On Thu, Jun 30, 2016 at 03:12:58PM +0100, David Howells wrote:
> +struct rxrpc_connection *rxrpc_find_service_conn_rcu(struct rxrpc_peer *peer,
> +						     struct sk_buff *skb)
> +{
> +	struct rxrpc_connection *conn = NULL;
> +	struct rxrpc_conn_proto k;
> +	struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
> +	struct rb_node *p;
> +	unsigned int seq;
> +
> +	k.epoch	= sp->hdr.epoch;
> +	k.cid	= sp->hdr.cid & RXRPC_CIDMASK;
> +
> +	do {
> +		/* Unfortunately, rbtree walking doesn't give reliable results
> +		 * under just the RCU read lock, so we have to check for
> +		 * changes.
> +		 */
> +		read_seqbegin_or_lock(&peer->service_conn_lock, &seq);
> +
> +		p = peer->service_conns.rb_node;
> +		while (p) {
> +			conn = rb_entry(p, struct rxrpc_connection, service_node);
> +
> +			if (conn->proto.index_key < k.index_key)
> +				p = p->rb_left;
> +			else if (conn->proto.index_key > k.index_key)
> +				p = p->rb_right;

You still very much need rcu_dereference() for both left and right
pointers. As well as the first p load.

> +			else
> +				goto done;
> +			conn = NULL;
> +		}
> +	} while (need_seqretry(&peer->service_conn_lock, seq));
> +
> +done:
> +	done_seqretry(&peer->service_conn_lock, seq);
> +	_leave(" = %d", conn ? conn->debug_id : -1);
> +	return conn;
> +}


> +static struct rxrpc_connection *
> +rxrpc_publish_service_conn(struct rxrpc_peer *peer,
> +			   struct rxrpc_connection *conn)
> +{
> +	struct rxrpc_connection *cursor = NULL;
> +	struct rxrpc_conn_proto k = conn->proto;
> +	struct rb_node **pp, *parent;
> +
> +	write_seqlock_bh(&peer->service_conn_lock);
> +
> +	pp = &peer->service_conns.rb_node;
> +	parent = NULL;
> +	while (*pp) {
> +		parent = *pp;
> +		cursor = rb_entry(parent,
> +				  struct rxrpc_connection, service_node);
> +
> +		if (cursor->proto.index_key < k.index_key)
> +			pp = &(*pp)->rb_left;
> +		else if (cursor->proto.index_key > k.index_key)
> +			pp = &(*pp)->rb_right;
> +		else
> +			goto found_extant_conn;
> +	}
> +
> +	rb_link_node(&conn->service_node, parent, pp);

You want rb_link_node_rcu() here.

> +	rb_insert_color(&conn->service_node, &peer->service_conns);
> +conn_published:
> +	set_bit(RXRPC_CONN_IN_SERVICE_CONNS, &conn->flags);
> +	write_sequnlock_bh(&peer->service_conn_lock);
> +	_leave(" = %d [new]", conn->debug_id);
> +	return conn;