netdev - Re: rcu locking issue in mpls output code?

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:	Mon, 20 Jun 2016 09:19:05 -0600
From:	David Ahern <dsa@...ulusnetworks.com>
To:	Lennert Buytenhek <buytenh@...tstofly.org>
Cc:	Roopa Prabhu <roopa@...ulusnetworks.com>,
	Robert Shearman <rshearma@...cade.com>, netdev@...r.kernel.org
Subject: Re: rcu locking issue in mpls output code?

On 6/20/16 12:30 AM, Lennert Buytenhek wrote:
> On Sun, Jun 19, 2016 at 08:19:20PM -0600, David Ahern wrote:
>
>>> diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
>>> index fb31aa8..802956b 100644
>>> --- a/net/mpls/mpls_iptunnel.c
>>> +++ b/net/mpls/mpls_iptunnel.c
>>> @@ -105,12 +105,15 @@ static int mpls_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>>>  		bos = false;
>>>  	}
>>>
>>> +	rcu_read_lock_bh();
>>>  	if (rt)
>>>  		err = neigh_xmit(NEIGH_ARP_TABLE, out_dev, &rt->rt_gateway,
>>>  				 skb);
>>>  	else if (rt6)
>>>  		err = neigh_xmit(NEIGH_ND_TABLE, out_dev, &rt6->rt6i_gateway,
>>>  				 skb);
>>> +	rcu_read_unlock_bh();
>>> +
>>>  	if (err)
>>>  		net_dbg_ratelimited("%s: packet transmission failed: %d\n",
>>>  				    __func__, err);
>>>
>>
>> I think those need to be added to neigh_xmit in the
>>
>> 	if (likely(index < NEIGH_NR_TABLES)) {
>>
>> 	}
>
> That'll force callers that don't need the extra protection (i.e.
> mpls_forward(), since that always runs from softirq and it's enough
> to protect the neigh state with rcu_read_lock() from softirq and we're
> already running under rcu_read_lock() when we get to neigh_xmit()) to
> eat the useless overhead of an extra rcu_read_{,un}lock_bh() pair, but
> sure, functionally that's correct, I think, and in my workload I don't
> care about MPLS forwarding performance anyway. ;-)

__neigh_lookup_noref expects bh level protection. Since the if block in 
neigh_xmit requires the locking seems like this the appropriate place 
for it.

>
> Want me to send a patch moving it to neigh_xmit() ?

Roopa/Robert: agree?