* What is lock_sock() before skb_free_datagram() for? @ 2009-04-18 8:04 Tetsuo Handa 2009-04-18 8:09 ` Eric Dumazet 2009-04-18 8:35 ` David Miller 0 siblings, 2 replies; 11+ messages in thread From: Tetsuo Handa @ 2009-04-18 8:04 UTC (permalink / raw) To: netdev 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-18 8:04 What is lock_sock() before skb_free_datagram() for? Tetsuo Handa @ 2009-04-18 8:09 ` Eric Dumazet 2009-04-18 8:35 ` David Miller 1 sibling, 0 replies; 11+ messages in thread From: Eric Dumazet @ 2009-04-18 8:09 UTC (permalink / raw) To: Tetsuo Handa; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-18 8:04 What is lock_sock() before skb_free_datagram() for? Tetsuo Handa 2009-04-18 8:09 ` Eric Dumazet @ 2009-04-18 8:35 ` David Miller 2009-04-18 9:04 ` Tetsuo Handa 1 sibling, 1 reply; 11+ messages in thread From: David Miller @ 2009-04-18 8:35 UTC (permalink / raw) To: penguin-kernel; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-18 8:35 ` David Miller @ 2009-04-18 9:04 ` Tetsuo Handa 2009-04-18 9:08 ` David Miller 0 siblings, 1 reply; 11+ messages in thread From: Tetsuo Handa @ 2009-04-18 9:04 UTC (permalink / raw) To: davem; +Cc: netdev, linux-security-module 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-18 9:04 ` Tetsuo Handa @ 2009-04-18 9:08 ` David Miller 2009-04-18 12:23 ` Tetsuo Handa 0 siblings, 1 reply; 11+ messages in thread From: David Miller @ 2009-04-18 9:08 UTC (permalink / raw) To: penguin-kernel; +Cc: netdev, linux-security-module 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 :-( ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-18 9:08 ` David Miller @ 2009-04-18 12:23 ` Tetsuo Handa 2009-04-19 4:28 ` David Miller 0 siblings, 1 reply; 11+ messages in thread From: Tetsuo Handa @ 2009-04-18 12:23 UTC (permalink / raw) To: davem; +Cc: netdev, linux-security-module 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-18 12:23 ` Tetsuo Handa @ 2009-04-19 4:28 ` David Miller 2009-04-19 5:12 ` Tetsuo Handa 0 siblings, 1 reply; 11+ messages in thread From: David Miller @ 2009-04-19 4:28 UTC (permalink / raw) To: penguin-kernel; +Cc: netdev, linux-security-module 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-19 4:28 ` David Miller @ 2009-04-19 5:12 ` Tetsuo Handa 2009-04-19 7:44 ` David Miller 2009-04-23 14:57 ` Samir Bellabes 0 siblings, 2 replies; 11+ messages in thread From: Tetsuo Handa @ 2009-04-19 5:12 UTC (permalink / raw) To: davem; +Cc: netdev, linux-security-module 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-19 5:12 ` Tetsuo Handa @ 2009-04-19 7:44 ` David Miller 2009-04-23 14:57 ` Samir Bellabes 1 sibling, 0 replies; 11+ messages in thread From: David Miller @ 2009-04-19 7:44 UTC (permalink / raw) To: penguin-kernel; +Cc: netdev, linux-security-module 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-19 5:12 ` Tetsuo Handa 2009-04-19 7:44 ` David Miller @ 2009-04-23 14:57 ` Samir Bellabes 2009-04-23 21:56 ` Tetsuo Handa 1 sibling, 1 reply; 11+ messages in thread From: Samir Bellabes @ 2009-04-23 14:57 UTC (permalink / raw) To: Tetsuo Handa; +Cc: davem, netdev, linux-security-module 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: What is lock_sock() before skb_free_datagram() for? 2009-04-23 14:57 ` Samir Bellabes @ 2009-04-23 21:56 ` Tetsuo Handa 0 siblings, 0 replies; 11+ messages in thread From: Tetsuo Handa @ 2009-04-23 21:56 UTC (permalink / raw) To: sam; +Cc: davem, netdev, linux-security-module 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.) ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-04-23 21:56 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-04-18 8:04 What is lock_sock() before skb_free_datagram() for? Tetsuo Handa 2009-04-18 8:09 ` Eric Dumazet 2009-04-18 8:35 ` David Miller 2009-04-18 9:04 ` Tetsuo Handa 2009-04-18 9:08 ` David Miller 2009-04-18 12:23 ` Tetsuo Handa 2009-04-19 4:28 ` David Miller 2009-04-19 5:12 ` Tetsuo Handa 2009-04-19 7:44 ` David Miller 2009-04-23 14:57 ` Samir Bellabes 2009-04-23 21:56 ` Tetsuo Handa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).