[PATCH net 1/2] geneve: Remove socket and offload handlers at destruction.

[PATCH net 1/2] geneve: Remove socket and offload handlers at destruction.

[Jesse Gross]

Sockets aren't currently removed from the the global list when
they are destroyed. In addition, offload handlers need to be cleaned
up as well.

Fixes: 0b5e8b8e ("net: Add Geneve tunneling protocol driver")
CC: Andy Zhou <azhou@nicira.com>
Signed-off-by: Jesse Gross <jesse@nicira.com>
---
 net/ipv4/geneve.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/ipv4/geneve.c b/net/ipv4/geneve.c
index a457232..5a47188 100644
--- a/net/ipv4/geneve.c
+++ b/net/ipv4/geneve.c
@@ -159,6 +159,15 @@ static void geneve_notify_add_rx_port(struct geneve_sock *gs)
 	}
 }
 
+static void geneve_notify_del_rx_port(struct geneve_sock *gs)
+{
+	struct sock *sk = gs->sock->sk;
+	sa_family_t sa_family = sk->sk_family;
+
+	if (sa_family == AF_INET)
+		udp_del_offload(&gs->udp_offloads);
+}
+
 /* Callback from net/ipv4/udp.c to receive packets */
 static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
@@ -312,9 +321,17 @@ EXPORT_SYMBOL_GPL(geneve_sock_add);
 
 void geneve_sock_release(struct geneve_sock *gs)
 {
+	struct net *net = sock_net(gs->sock->sk);
+	struct geneve_net *gn = net_generic(net, geneve_net_id);
+
 	if (!atomic_dec_and_test(&gs->refcnt))
 		return;
 
+	spin_lock(&gn->sock_lock);
+	hlist_del_rcu(&gs->hlist);
+	geneve_notify_del_rx_port(gs);
+	spin_unlock(&gn->sock_lock);
+
 	queue_work(geneve_wq, &gs->del_work);
 }
 EXPORT_SYMBOL_GPL(geneve_sock_release);
-- 
1.9.1

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

[Jesse Gross]

Currently, searching for a socket to add a reference to is not
synchronized with deletion of sockets. This can result in use
after free if there is another operation that is removing a
socket at the same time. Solving this requires both holding the
appropriate lock and checking the refcount to ensure that it
has not already hit zero.

Inspired by a related (but not exactly the same) issue in the
VXLAN driver.

Fixes: 0b5e8b8e ("net: Add Geneve tunneling protocol driver")
CC: Andy Zhou <azhou@nicira.com>
Signed-off-by: Jesse Gross <jesse@nicira.com>
---
 net/ipv4/geneve.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

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);
-	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);
-	} else {
+	spin_unlock(&gn->sock_lock);
+
+	if (!gs)
 		gs = ERR_PTR(-EINVAL);
-	}
 
 	return gs;
 }
-- 
1.9.1

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

[Thomas Graf]

On 12/16/14 at 06:25pm, Jesse Gross wrote:
> Currently, searching for a socket to add a reference to is not
> synchronized with deletion of sockets. This can result in use
> after free if there is another operation that is removing a
> socket at the same time. Solving this requires both holding the
> appropriate lock and checking the refcount to ensure that it
> has not already hit zero.
> 
> Inspired by a related (but not exactly the same) issue in the
> VXLAN driver.
> 
> Fixes: 0b5e8b8e ("net: Add Geneve tunneling protocol driver")
> CC: Andy Zhou <azhou@nicira.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
> ---
>  net/ipv4/geneve.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> 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.

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

[Jesse Gross]

On Wed, Dec 17, 2014 at 8:54 AM, Thomas Graf <tgraf@suug.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.

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

[Thomas Graf]

On 12/17/14 at 10:48am, Jesse Gross wrote:
> I generally agree (with the exception of kfree_rcu() - I believe that
> is still needed since incoming packets reference it using RCU).

I didn't inspect this in full detail but seems like the data path
should only care about gs->sock which is properly refcnt'ed.

> 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.

Makes sense. Feel free to take:
Acked-by: Thomas Graf <tgraf@suug.ch>

> 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.

I don't have an explanation for that either. Each entry on the
vni_list[] takes a vs->refcnt.

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

[Jesse Gross]

On Wed, Dec 17, 2014 at 1:15 PM, Thomas Graf <tgraf@suug.ch> wrote:
> On 12/17/14 at 10:48am, Jesse Gross wrote:
>> I generally agree (with the exception of kfree_rcu() - I believe that
>> is still needed since incoming packets reference it using RCU).
>
> I didn't inspect this in full detail but seems like the data path
> should only care about gs->sock which is properly refcnt'ed.

The data path (if you include the consumer in OVS), goes from sk to gs
to gs->rcv_data so it does care about the stuff that is being freed
here.

>> 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.
>
> I don't have an explanation for that either. Each entry on the
> vni_list[] takes a vs->refcnt.

I don't think the workqueue is about refcounting - it looks to me like
the issue that is being avoided is either that RTNL is held and a
lower layer tries to take it again or another lock that nests in the
opposite way with RTNL. However, looking through the code, the problem
wasn't obvious to me.

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

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Tue, 16 Dec 2014 18:25:32 -0800

> Currently, searching for a socket to add a reference to is not
> synchronized with deletion of sockets. This can result in use
> after free if there is another operation that is removing a
> socket at the same time. Solving this requires both holding the
> appropriate lock and checking the refcount to ensure that it
> has not already hit zero.
> 
> Inspired by a related (but not exactly the same) issue in the
> VXLAN driver.
> 
> Fixes: 0b5e8b8e ("net: Add Geneve tunneling protocol driver")
> CC: Andy Zhou <azhou@nicira.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Applied.

Re: [PATCH net 1/2] geneve: Remove socket and offload handlers at destruction.

[Thomas Graf]

On 12/16/14 at 06:25pm, Jesse Gross wrote:
> Sockets aren't currently removed from the the global list when
> they are destroyed. In addition, offload handlers need to be cleaned
> up as well.
> 
> Fixes: 0b5e8b8e ("net: Add Geneve tunneling protocol driver")
> CC: Andy Zhou <azhou@nicira.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Acked-by: Thomas Graf <tgraf@suug.ch>

Re: [PATCH net 1/2] geneve: Remove socket and offload handlers at destruction.

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Tue, 16 Dec 2014 18:25:31 -0800

> Sockets aren't currently removed from the the global list when
> they are destroyed. In addition, offload handlers need to be cleaned
> up as well.
> 
> Fixes: 0b5e8b8e ("net: Add Geneve tunneling protocol driver")
> CC: Andy Zhou <azhou@nicira.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>

Applied.