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:13:36 -0700
From:	Roopa Prabhu <roopa@...ulusnetworks.com>
To:	David Ahern <dsa@...ulusnetworks.com>
Cc:	Lennert Buytenhek <buytenh@...tstofly.org>,
	Robert Shearman <rshearma@...cade.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: rcu locking issue in mpls output code?

On Mon, Jun 20, 2016 at 8:19 AM, David Ahern <dsa@...ulusnetworks.com> wrote:
> 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?
>

yes, seems like an appropriate place for it.  provided it does not add
unnecessary overhead for others.
But then neigh_xmit seems to be only called from mpls_output and mpls_forward.

thanks!