[PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[Jakub Sitnicki]

sk->sk_user_data has multiple users, which are not compatible with each
other. To synchronize the users, any check-if-unused-and-set access to the
pointer has to happen with sock lock held.

l2tp currently fails to grab the lock when modifying the underlying tunnel
socket. Fix it by adding appropriate locking.

We don't to grab the lock when l2tp clears sk_user_data, because it happens
only in sk->sk_destruct, when the sock is going away.

v2:
- update Fixes to point to origin of the bug
- use real names in Reported/Tested-by tags

Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core")
Reported-by: Haowei Yan <g1042620637@gmail.com>
Tested-by: Haowei Yan <g1042620637@gmail.com>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
Cc: Tom Parkin <tparkin@katalix.com>

 net/l2tp/l2tp_core.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7499c51b1850..9f5f86bfc395 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1469,16 +1469,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 		sock = sockfd_lookup(tunnel->fd, &ret);
 		if (!sock)
 			goto err;
-
-		ret = l2tp_validate_socket(sock->sk, net, tunnel->encap);
-		if (ret < 0)
-			goto err_sock;
 	}
 
+	sk = sock->sk;
+	lock_sock(sk);
+
+	ret = l2tp_validate_socket(sk, net, tunnel->encap);
+	if (ret < 0)
+		goto err_sock;
+
 	tunnel->l2tp_net = net;
 	pn = l2tp_pernet(net);
 
-	sk = sock->sk;
 	sock_hold(sk);
 	tunnel->sock = sk;
 
@@ -1504,7 +1506,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 
 		setup_udp_tunnel_sock(net, sock, &udp_cfg);
 	} else {
-		sk->sk_user_data = tunnel;
+		rcu_assign_sk_user_data(sk, tunnel);
 	}
 
 	tunnel->old_sk_destruct = sk->sk_destruct;
@@ -1518,6 +1520,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 	if (tunnel->fd >= 0)
 		sockfd_put(sock);
 
+	release_sock(sk);
 	return 0;
 
 err_sock:
@@ -1525,6 +1528,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 		sock_release(sock);
 	else
 		sockfd_put(sock);
+
+	release_sock(sk);
 err:
 	return ret;
 }
-- 
2.35.3

Re: [PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]

On  Mon, Aug 15, 2022 at 15:01:07 +0200, Jakub Sitnicki wrote:
> sk->sk_user_data has multiple users, which are not compatible with each
> other. To synchronize the users, any check-if-unused-and-set access to the
> pointer has to happen with sock lock held.
> 
> l2tp currently fails to grab the lock when modifying the underlying tunnel
> socket. Fix it by adding appropriate locking.
> 
> We don't to grab the lock when l2tp clears sk_user_data, because it happens
> only in sk->sk_destruct, when the sock is going away.
> 
> v2:
> - update Fixes to point to origin of the bug
> - use real names in Reported/Tested-by tags
> 
> Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core")

This still seems wrong to me.

In 3557baabf280 pppol2tp_connect checks/sets sk_user_data with
lock_sock held.

> Reported-by: Haowei Yan <g1042620637@gmail.com>
> Tested-by: Haowei Yan <g1042620637@gmail.com>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
> Cc: Tom Parkin <tparkin@katalix.com>
> 
>  net/l2tp/l2tp_core.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 7499c51b1850..9f5f86bfc395 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1469,16 +1469,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  		sock = sockfd_lookup(tunnel->fd, &ret);
>  		if (!sock)
>  			goto err;
> -
> -		ret = l2tp_validate_socket(sock->sk, net, tunnel->encap);
> -		if (ret < 0)
> -			goto err_sock;
>  	}
>  
> +	sk = sock->sk;
> +	lock_sock(sk);
> +
> +	ret = l2tp_validate_socket(sk, net, tunnel->encap);
> +	if (ret < 0)
> +		goto err_sock;
> +
>  	tunnel->l2tp_net = net;
>  	pn = l2tp_pernet(net);
>  
> -	sk = sock->sk;
>  	sock_hold(sk);
>  	tunnel->sock = sk;
>  
> @@ -1504,7 +1506,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  
>  		setup_udp_tunnel_sock(net, sock, &udp_cfg);
>  	} else {
> -		sk->sk_user_data = tunnel;
> +		rcu_assign_sk_user_data(sk, tunnel);
>  	}
>  
>  	tunnel->old_sk_destruct = sk->sk_destruct;
> @@ -1518,6 +1520,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  	if (tunnel->fd >= 0)
>  		sockfd_put(sock);
>  
> +	release_sock(sk);
>  	return 0;
>  
>  err_sock:
> @@ -1525,6 +1528,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>  		sock_release(sock);
>  	else
>  		sockfd_put(sock);
> +
> +	release_sock(sk);
>  err:
>  	return ret;
>  }
> -- 
> 2.35.3
> 

-- 
Tom Parkin
Katalix Systems Ltd
https://katalix.com
Catalysts for your Embedded Linux software development

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[Jakub Sitnicki]

On Mon, Aug 15, 2022 at 02:21 PM +01, Tom Parkin wrote:
> [[PGP Signed Part:Undecided]]
> On  Mon, Aug 15, 2022 at 15:01:07 +0200, Jakub Sitnicki wrote:
>> sk->sk_user_data has multiple users, which are not compatible with each
>> other. To synchronize the users, any check-if-unused-and-set access to the
>> pointer has to happen with sock lock held.
>> 
>> l2tp currently fails to grab the lock when modifying the underlying tunnel
>> socket. Fix it by adding appropriate locking.
>> 
>> We don't to grab the lock when l2tp clears sk_user_data, because it happens
>> only in sk->sk_destruct, when the sock is going away.
>> 
>> v2:
>> - update Fixes to point to origin of the bug
>> - use real names in Reported/Tested-by tags
>> 
>> Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core")
>
> This still seems wrong to me.
>
> In 3557baabf280 pppol2tp_connect checks/sets sk_user_data with
> lock_sock held.

I think you are referring to the PPP-over-L2TP socket, not the UDP
socket. In pppol2tp_prepare_tunnel_socket() @ 3557baabf280 we're not
holding the sock lock over the UDP socket, AFAICT.

>
>> Reported-by: Haowei Yan <g1042620637@gmail.com>
>> Tested-by: Haowei Yan <g1042620637@gmail.com>
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>> Cc: Tom Parkin <tparkin@katalix.com>
>> 
>>  net/l2tp/l2tp_core.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>> 
>> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
>> index 7499c51b1850..9f5f86bfc395 100644
>> --- a/net/l2tp/l2tp_core.c
>> +++ b/net/l2tp/l2tp_core.c
>> @@ -1469,16 +1469,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>>  		sock = sockfd_lookup(tunnel->fd, &ret);
>>  		if (!sock)
>>  			goto err;
>> -
>> -		ret = l2tp_validate_socket(sock->sk, net, tunnel->encap);
>> -		if (ret < 0)
>> -			goto err_sock;
>>  	}
>>  
>> +	sk = sock->sk;
>> +	lock_sock(sk);
>> +
>> +	ret = l2tp_validate_socket(sk, net, tunnel->encap);
>> +	if (ret < 0)
>> +		goto err_sock;
>> +
>>  	tunnel->l2tp_net = net;
>>  	pn = l2tp_pernet(net);
>>  
>> -	sk = sock->sk;
>>  	sock_hold(sk);
>>  	tunnel->sock = sk;
>>  
>> @@ -1504,7 +1506,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>>  
>>  		setup_udp_tunnel_sock(net, sock, &udp_cfg);
>>  	} else {
>> -		sk->sk_user_data = tunnel;
>> +		rcu_assign_sk_user_data(sk, tunnel);
>>  	}
>>  
>>  	tunnel->old_sk_destruct = sk->sk_destruct;
>> @@ -1518,6 +1520,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>>  	if (tunnel->fd >= 0)
>>  		sockfd_put(sock);
>>  
>> +	release_sock(sk);
>>  	return 0;
>>  
>>  err_sock:
>> @@ -1525,6 +1528,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>>  		sock_release(sock);
>>  	else
>>  		sockfd_put(sock);
>> +
>> +	release_sock(sk);
>>  err:
>>  	return ret;
>>  }
>> -- 
>> 2.35.3
>> 

Re: [PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 3293 bytes --]

On  Mon, Aug 15, 2022 at 15:26:51 +0200, Jakub Sitnicki wrote:
> On Mon, Aug 15, 2022 at 02:21 PM +01, Tom Parkin wrote:
> > [[PGP Signed Part:Undecided]]
> > On  Mon, Aug 15, 2022 at 15:01:07 +0200, Jakub Sitnicki wrote:
> >> sk->sk_user_data has multiple users, which are not compatible with each
> >> other. To synchronize the users, any check-if-unused-and-set access to the
> >> pointer has to happen with sock lock held.
> >> 
> >> l2tp currently fails to grab the lock when modifying the underlying tunnel
> >> socket. Fix it by adding appropriate locking.
> >> 
> >> We don't to grab the lock when l2tp clears sk_user_data, because it happens
> >> only in sk->sk_destruct, when the sock is going away.
> >> 
> >> v2:
> >> - update Fixes to point to origin of the bug
> >> - use real names in Reported/Tested-by tags
> >> 
> >> Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core")
> >
> > This still seems wrong to me.
> >
> > In 3557baabf280 pppol2tp_connect checks/sets sk_user_data with
> > lock_sock held.
> 
> I think you are referring to the PPP-over-L2TP socket, not the UDP
> socket. In pppol2tp_prepare_tunnel_socket() @ 3557baabf280 we're not
> holding the sock lock over the UDP socket, AFAICT.

Yes, you're quite right -- my apologies.

> >
> >> Reported-by: Haowei Yan <g1042620637@gmail.com>
> >> Tested-by: Haowei Yan <g1042620637@gmail.com>
> >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> >> ---
> >> Cc: Tom Parkin <tparkin@katalix.com>
> >> 
> >>  net/l2tp/l2tp_core.c | 17 +++++++++++------
> >>  1 file changed, 11 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> >> index 7499c51b1850..9f5f86bfc395 100644
> >> --- a/net/l2tp/l2tp_core.c
> >> +++ b/net/l2tp/l2tp_core.c
> >> @@ -1469,16 +1469,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
> >>  		sock = sockfd_lookup(tunnel->fd, &ret);
> >>  		if (!sock)
> >>  			goto err;
> >> -
> >> -		ret = l2tp_validate_socket(sock->sk, net, tunnel->encap);
> >> -		if (ret < 0)
> >> -			goto err_sock;
> >>  	}
> >>  
> >> +	sk = sock->sk;
> >> +	lock_sock(sk);
> >> +
> >> +	ret = l2tp_validate_socket(sk, net, tunnel->encap);
> >> +	if (ret < 0)
> >> +		goto err_sock;
> >> +
> >>  	tunnel->l2tp_net = net;
> >>  	pn = l2tp_pernet(net);
> >>  
> >> -	sk = sock->sk;
> >>  	sock_hold(sk);
> >>  	tunnel->sock = sk;
> >>  
> >> @@ -1504,7 +1506,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
> >>  
> >>  		setup_udp_tunnel_sock(net, sock, &udp_cfg);
> >>  	} else {
> >> -		sk->sk_user_data = tunnel;
> >> +		rcu_assign_sk_user_data(sk, tunnel);
> >>  	}
> >>  
> >>  	tunnel->old_sk_destruct = sk->sk_destruct;
> >> @@ -1518,6 +1520,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
> >>  	if (tunnel->fd >= 0)
> >>  		sockfd_put(sock);
> >>  
> >> +	release_sock(sk);
> >>  	return 0;
> >>  
> >>  err_sock:
> >> @@ -1525,6 +1528,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
> >>  		sock_release(sock);
> >>  	else
> >>  		sockfd_put(sock);
> >> +
> >> +	release_sock(sk);
> >>  err:
> >>  	return ret;
> >>  }
> >> -- 
> >> 2.35.3
> >> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[Jakub Kicinski]

On Mon, 15 Aug 2022 15:01:07 +0200 Jakub Sitnicki wrote:
> sk->sk_user_data has multiple users, which are not compatible with each
> other. To synchronize the users, any check-if-unused-and-set access to the
> pointer has to happen with sock lock held.
> 
> l2tp currently fails to grab the lock when modifying the underlying tunnel
> socket. Fix it by adding appropriate locking.
> 
> We don't to grab the lock when l2tp clears sk_user_data, because it happens
> only in sk->sk_destruct, when the sock is going away.

Note to other netdev maintainers that based on the discussion about 
the reuseport locking it's unclear whether we shouldn't also take 
the callback lock...

Re: [PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[Jakub Sitnicki]

On Tue, Aug 16, 2022 at 06:41 PM -07, Jakub Kicinski wrote:
> On Mon, 15 Aug 2022 15:01:07 +0200 Jakub Sitnicki wrote:
>> sk->sk_user_data has multiple users, which are not compatible with each
>> other. To synchronize the users, any check-if-unused-and-set access to the
>> pointer has to happen with sock lock held.
>> 
>> l2tp currently fails to grab the lock when modifying the underlying tunnel
>> socket. Fix it by adding appropriate locking.
>> 
>> We don't to grab the lock when l2tp clears sk_user_data, because it happens
>> only in sk->sk_destruct, when the sock is going away.
>
> Note to other netdev maintainers that based on the discussion about 
> the reuseport locking it's unclear whether we shouldn't also take 
> the callback lock...

You're right. reuseport_array, psock, and kcm protect sk_user_data with
the callback lock, not the sock lock. Need to fix it.

Re: [PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[Jakub Kicinski]

On Wed, 17 Aug 2022 16:33:33 +0200 Jakub Sitnicki wrote:
> > Note to other netdev maintainers that based on the discussion about 
> > the reuseport locking it's unclear whether we shouldn't also take 
> > the callback lock...  
> 
> You're right. reuseport_array, psock, and kcm protect sk_user_data with
> the callback lock, not the sock lock. Need to fix it.

Where 'it' == current patch? Would you mind adding to the kdoc on
sk_user_data that it's protected by the callback lock while at it?

Re: [PATCH net v2] l2tp: Serialize access to sk_user_data with sock lock

[Jakub Sitnicki]

On Wed, Aug 17, 2022 at 08:51 AM -07, Jakub Kicinski wrote:
> On Wed, 17 Aug 2022 16:33:33 +0200 Jakub Sitnicki wrote:
>> > Note to other netdev maintainers that based on the discussion about 
>> > the reuseport locking it's unclear whether we shouldn't also take 
>> > the callback lock...  
>> 
>> You're right. reuseport_array, psock, and kcm protect sk_user_data with
>> the callback lock, not the sock lock. Need to fix it.
>
> Where 'it' == current patch? Would you mind adding to the kdoc on
> sk_user_data that it's protected by the callback lock while at it?

Yes, will prepare a v3 for review. Sorry, should have been explicit.

Will add a kdoc. Great idea.