netdev - Re: [PATCH net 2/2] geneve: Fix races between socket add and release.

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:	Wed, 17 Dec 2014 10:48:40 -0800
From:	Jesse Gross <jesse@...ira.com>
To:	Thomas Graf <tgraf@...g.ch>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>, Andy Zhou <azhou@...ira.com>,
	Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH net 2/2] geneve: Fix races between socket add and release.

On Wed, Dec 17, 2014 at 8:54 AM, Thomas Graf <tgraf@...g.ch> wrote:
> On 12/16/14 at 06:25pm, Jesse Gross wrote:
>> diff --git a/net/ipv4/geneve.c b/net/ipv4/geneve.c
>> index 5a47188..95e47c9 100644
>> --- a/net/ipv4/geneve.c
>> +++ b/net/ipv4/geneve.c
>> @@ -296,6 +296,7 @@ struct geneve_sock *geneve_sock_add(struct net *net, __be16 port,
>>                                   geneve_rcv_t *rcv, void *data,
>>                                   bool no_share, bool ipv6)
>>  {
>> +     struct geneve_net *gn = net_generic(net, geneve_net_id);
>>       struct geneve_sock *gs;
>>
>>       gs = geneve_socket_create(net, port, rcv, data, ipv6);
>> @@ -305,15 +306,15 @@ struct geneve_sock *geneve_sock_add(struct net *net, __be16 port,
>>       if (no_share)   /* Return error if sharing is not allowed. */
>>               return ERR_PTR(-EINVAL);
>>
>> +     spin_lock(&gn->sock_lock);
>>       gs = geneve_find_sock(net, port);
>
> Perhaps remove the _rcu of the iterator in the geneve_find_sock?
> Also, the kfree_rcu() seems no longer needed as all read accesses
> are protected by the spinlock.
>
>> -     if (gs) {
>> -             if (gs->rcv == rcv)
>> -                     atomic_inc(&gs->refcnt);
>> -             else
>> +     if (gs && ((gs->rcv != rcv) ||
>> +                !atomic_add_unless(&gs->refcnt, 1, 0)))
>>                       gs = ERR_PTR(-EBUSY);
>
> Since you are taking gn->sock_lock in geneve_sock_release()
> anyway, all accesses to refcnt could eventually be converted
> to non-atomic ops.

I generally agree (with the exception of kfree_rcu() - I believe that
is still needed since incoming packets reference it using RCU).
However, since this patch is targeted a net- I wanted to make a
minimal change and not completely redo the locking. A lot of the
locking here was pulled over from VXLAN and I think it can be
simplified since I don't expect that the Geneve code will bring in all
of that logic.

The one part that is not entirely clear is the workqueue in VXLAN used
for destroying the socket. This was added by Stephen in "vxlan: listen
on multiple ports" but it's not obvious to me what problem it is
trying to avoid and I don't see a comment. If possible, it would be
nice to simplify this as well if the issue doesn't apply to Geneve.
--
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