What is lock_sock() before skb_free_datagram() for?

What is lock_sock() before skb_free_datagram() for?

[Tetsuo Handa]

Hello.

udp_recvmsg() and udpv6_recvmsg() call skb_free_datagram() with 
lock_sock().
But raw_recvmsg() and rawv6_recvmsg() call skb_free_datagram() 
without
lock_sock().
Is it OK?

Re: What is lock_sock() before skb_free_datagram() for?

[Eric Dumazet]

Tetsuo Handa a écrit :
> Hello.
> 
> udp_recvmsg() and udpv6_recvmsg() call skb_free_datagram() with 
> lock_sock().
> But raw_recvmsg() and rawv6_recvmsg() call skb_free_datagram() 
> without
> lock_sock().
> Is it OK?

Yes it is.

UDP protocol has memory accounting in recent kernels, not RAW.

Re: What is lock_sock() before skb_free_datagram() for?

[David Miller]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 18 Apr 2009 17:04:01 +0900

> Hello.
> 
> udp_recvmsg() and udpv6_recvmsg() call skb_free_datagram() with 
> lock_sock().
> But raw_recvmsg() and rawv6_recvmsg() call skb_free_datagram() 
> without
> lock_sock().
> Is it OK?

UDP does global socket memory accounting, and supports transient
state between sendmsg() calls via MSG_MORE, so it needs
locking.

RAW does not support those things, so doesn't need the locking.

Re: What is lock_sock() before skb_free_datagram() for?

[Tetsuo Handa]

Hello.

David Miller wrote:
> RAW does not support those things, so doesn't need the locking.
I see. Thanks.

It is harmless to call lock_sock() for protocols which don't 
support global
socket memory accounting, isn't it?

I want to introduce a new LSM hook which is called after picking
 up a datagram. Below is a patch.

--- security-testing-2.6.git.orig/net/core/datagram.c
+++ security-testing-2.6.git/net/core/datagram.c
@@ -55,6 +55,7 @@
 #include <net/checksum.h>
 #include <net/sock.h>
 #include <net/tcp_states.h>
+#include <linux/security.h>
 
 /*
  *	Is a socket 'connection oriented' ?
@@ -179,8 +180,13 @@ struct sk_buff *__skb_recv_datagram(stru
 		}
 		spin_unlock_irqrestore(&sk->sk_receive_queue.
lock, cpu_flags);
 
-		if (skb)
+		if (skb) {
+			error = security_socket_post_recv_datagram(sk, skb,
+								   flags);
+			if (error)
+				goto force_dequeue;
 			return skb;
+		}
 
 		/* User doesn't want to wait */
 		error = -EAGAIN;
@@ -191,6 +197,19 @@ struct sk_buff *__skb_recv_datagram(stru
 
 	return NULL;
 
+force_dequeue:
+	if (flags & MSG_PEEK) {
+		unsigned long cpu_flags;
+		spin_lock_irqsave(&sk->sk_receive_queue.lock, cpu_flags);
+		if (skb == skb_peek(&sk->sk_receive_queue)) {
+			__skb_unlink(skb, &sk->sk_receive_queue);
+			atomic_dec(&skb->users);
+		}
+		spin_unlock_irqrestore(&sk->sk_receive_queue.
lock, cpu_flags);
+	}
+	lock_sock(sk);
+	skb_free_datagram(sk, skb);
+	release_sock(sk);
 no_packet:
 	*err = error;
 	return NULL;

If it is harmful to call lock_sock() for protocols which don't 
support global
socket memory accounting, I need to make lock_sock(sk);/release_
sock(sk); calls
conditional.

Regards.

Re: What is lock_sock() before skb_free_datagram() for?

[David Miller]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 18 Apr 2009 18:04:24 +0900

> If it is harmful to call lock_sock() for protocols which don't
> support global socket memory accounting, I need to make
> lock_sock(sk);/release_ sock(sk); calls conditional.

Great, more complexity in the kernel for the sake of TOMOYO
:-(

Re: What is lock_sock() before skb_free_datagram() for?

[Tetsuo Handa]

David Miller wrote:
> > If it is harmful to call lock_sock() for protocols which don't
> > support global socket memory accounting, I need to make
> > lock_sock(sk);/release_ sock(sk); calls conditional.
> 
> Great, more complexity in the kernel for the sake of TOMOYO
> :-(
> 
You meant that I should leave lock_sock(sk);/release_sock(sk); calls
unconditional?
This error path is not frequently called. If there are no problems but
performance, I'll leave them unconditional.

Re: What is lock_sock() before skb_free_datagram() for?

[David Miller]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 18 Apr 2009 21:23:35 +0900

> David Miller wrote:
>> > If it is harmful to call lock_sock() for protocols which don't
>> > support global socket memory accounting, I need to make
>> > lock_sock(sk);/release_ sock(sk); calls conditional.
>> 
>> Great, more complexity in the kernel for the sake of TOMOYO
>> :-(
>> 
> You meant that I should leave lock_sock(sk);/release_sock(sk); calls
> unconditional?
> This error path is not frequently called. If there are no problems but
> performance, I'll leave them unconditional.

I mean you should not make generic so no longer generic.

We worked so hard to split out this common code, it is simply
a non-starter for anyone to start putting protocol specific test
into here, or even worse to move this code back to being locally
copied into every protocol implementation.

You may want to think about how you can achieve your goals by putting
these unpleasant hooks into some other location.

Re: What is lock_sock() before skb_free_datagram() for?

[Tetsuo Handa]

David Miller wrote:
> We worked so hard to split out this common code, it is simply
> a non-starter for anyone to start putting protocol specific test
> into here, or even worse to move this code back to being locally
> copied into every protocol implementation.
You don't want LSM modules to perform protocol specific test inside
__skb_recv_datagram(). I see.

> You may want to think about how you can achieve your goals by putting
> these unpleasant hooks into some other location.
May I insert security_socket_post_recv_datagram() into the caller of
skb_recv_datagram() (as shown below)?

 include/linux/security.h |   39 +++++++++++++++++++++++++++++++++++++++
 net/ipv4/raw.c           |    5 +++++
 net/ipv4/udp.c           |    7 +++++++
 net/ipv6/raw.c           |    5 +++++
 net/ipv6/udp.c           |    7 +++++++
 net/socket.c             |    5 +++++
 security/capability.c    |   13 +++++++++++++
 security/security.c      |   11 +++++++++++
 8 files changed, 92 insertions(+)

--- security-testing-2.6.git.orig/net/ipv4/raw.c
+++ security-testing-2.6.git/net/ipv4/raw.c
@@ -666,6 +666,11 @@ static int raw_recvmsg(struct kiocb *ioc
 	skb = skb_recv_datagram(sk, flags, noblock, &err);
 	if (!skb)
 		goto out;
+	err = security_socket_post_recv_datagram(sk, skb, flags);
+	if (err) {
+		skb_kill_datagram(sk, skb, flags);
+		goto out;
+	}
 
 	copied = skb->len;
 	if (len < copied) {
--- security-testing-2.6.git.orig/net/ipv4/udp.c
+++ security-testing-2.6.git/net/ipv4/udp.c
@@ -901,6 +901,13 @@ try_again:
 				  &peeked, &err);
 	if (!skb)
 		goto out;
+	err = security_socket_post_recv_datagram(sk, skb, flags);
+	if (err) {
+		lock_sock(sk);
+		skb_kill_datagram(sk, skb, flags);
+		release_sock(sk);
+		goto out;
+	}
 
 	ulen = skb->len - sizeof(struct udphdr);
 	copied = len;
--- security-testing-2.6.git.orig/net/ipv6/raw.c
+++ security-testing-2.6.git/net/ipv6/raw.c
@@ -465,6 +465,11 @@ static int rawv6_recvmsg(struct kiocb *i
 	skb = skb_recv_datagram(sk, flags, noblock, &err);
 	if (!skb)
 		goto out;
+	err = security_socket_post_recv_datagram(sk, skb, flags);
+	if (err) {
+		skb_kill_datagram(sk, skb, flags);
+		goto out;
+	}
 
 	copied = skb->len;
 	if (copied > len) {
--- security-testing-2.6.git.orig/net/ipv6/udp.c
+++ security-testing-2.6.git/net/ipv6/udp.c
@@ -208,6 +208,13 @@ try_again:
 				  &peeked, &err);
 	if (!skb)
 		goto out;
+	err = security_socket_post_recv_datagram(sk, skb, flags);
+	if (err) {
+		lock_sock(sk);
+		skb_kill_datagram(sk, skb, flags);
+		release_sock(sk);
+		goto out;
+	}
 
 	ulen = skb->len - sizeof(struct udphdr);
 	copied = len;

Re: What is lock_sock() before skb_free_datagram() for?

[David Miller]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sun, 19 Apr 2009 14:12:15 +0900

> David Miller wrote:
>> We worked so hard to split out this common code, it is simply
>> a non-starter for anyone to start putting protocol specific test
>> into here, or even worse to move this code back to being locally
>> copied into every protocol implementation.
> You don't want LSM modules to perform protocol specific test inside
> __skb_recv_datagram(). I see.
> 
>> You may want to think about how you can achieve your goals by putting
>> these unpleasant hooks into some other location.
> May I insert security_socket_post_recv_datagram() into the caller of
> skb_recv_datagram() (as shown below)?

This definitely looks better, yes.

Re: What is lock_sock() before skb_free_datagram() for?

[Samir Bellabes]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> David Miller wrote:
>> We worked so hard to split out this common code, it is simply
>> a non-starter for anyone to start putting protocol specific test
>> into here, or even worse to move this code back to being locally
>> copied into every protocol implementation.
> You don't want LSM modules to perform protocol specific test inside
> __skb_recv_datagram(). I see.
>
>> You may want to think about how you can achieve your goals by putting
>> these unpleasant hooks into some other location.
> May I insert security_socket_post_recv_datagram() into the caller of
> skb_recv_datagram() (as shown below)?

what is the purpose of having such hooks ?

>  include/linux/security.h |   39 +++++++++++++++++++++++++++++++++++++++
>  net/ipv4/raw.c           |    5 +++++
>  net/ipv4/udp.c           |    7 +++++++
>  net/ipv6/raw.c           |    5 +++++
>  net/ipv6/udp.c           |    7 +++++++
>  net/socket.c             |    5 +++++
>  security/capability.c    |   13 +++++++++++++
>  security/security.c      |   11 +++++++++++
>  8 files changed, 92 insertions(+)
>
> --- security-testing-2.6.git.orig/net/ipv4/raw.c
> +++ security-testing-2.6.git/net/ipv4/raw.c
> @@ -666,6 +666,11 @@ static int raw_recvmsg(struct kiocb *ioc
>  	skb = skb_recv_datagram(sk, flags, noblock, &err);
>  	if (!skb)
>  		goto out;
> +	err = security_socket_post_recv_datagram(sk, skb, flags);
> +	if (err) {
> +		skb_kill_datagram(sk, skb, flags);
> +		goto out;
> +	}
>  
>  	copied = skb->len;
>  	if (len < copied) {
> --- security-testing-2.6.git.orig/net/ipv4/udp.c
> +++ security-testing-2.6.git/net/ipv4/udp.c
> @@ -901,6 +901,13 @@ try_again:
>  				  &peeked, &err);
>  	if (!skb)
>  		goto out;
> +	err = security_socket_post_recv_datagram(sk, skb, flags);
> +	if (err) {
> +		lock_sock(sk);
> +		skb_kill_datagram(sk, skb, flags);
> +		release_sock(sk);
> +		goto out;
> +	}
>  
>  	ulen = skb->len - sizeof(struct udphdr);
>  	copied = len;
> --- security-testing-2.6.git.orig/net/ipv6/raw.c
> +++ security-testing-2.6.git/net/ipv6/raw.c
> @@ -465,6 +465,11 @@ static int rawv6_recvmsg(struct kiocb *i
>  	skb = skb_recv_datagram(sk, flags, noblock, &err);
>  	if (!skb)
>  		goto out;
> +	err = security_socket_post_recv_datagram(sk, skb, flags);
> +	if (err) {
> +		skb_kill_datagram(sk, skb, flags);
> +		goto out;
> +	}
>  
>  	copied = skb->len;
>  	if (copied > len) {
> --- security-testing-2.6.git.orig/net/ipv6/udp.c
> +++ security-testing-2.6.git/net/ipv6/udp.c
> @@ -208,6 +208,13 @@ try_again:
>  				  &peeked, &err);
>  	if (!skb)
>  		goto out;
> +	err = security_socket_post_recv_datagram(sk, skb, flags);
> +	if (err) {
> +		lock_sock(sk);
> +		skb_kill_datagram(sk, skb, flags);
> +		release_sock(sk);
> +		goto out;
> +	}
>  
>  	ulen = skb->len - sizeof(struct udphdr);
>  	copied = len;
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is lock_sock() before skb_free_datagram() for?

[Tetsuo Handa]

Samir Bellabes wrote:
> what is the purpose of having such hooks ?
Same as security_socket_post_accept() (i.e. to drop datagrams from unwanted
peers).

I need to understand the meaning of "poll()" returning "ready" to understand
why security_socket_accept() and security_socket_recvmsg() are permitted to
return an error (though these hooks don't remove from the queue).

My understanding is that "poll()" returning "ready" does not guarantee that
accept()/recvmsg() shall return a valid file descriptor/datagram;
"poll()" returning "ready" guarantees merely accept()/recvmsg() does not
need to wait for connection/datagram. (Otherwise, security_socket_accept()
and security_socket_recvmsg() have to be gone.)