On Sun, Dec 24, 2006 at 04:25:11PM -0800, Andrew Morton wrote:
On Mon, 25 Dec 2006 05:21:24 +0800
"Adam J. Richter" [off-list ref] wrote:
quoted
Under 2.6.20-rc1 and 2.6.20-rc2, I get the following complaint
for several network programs running on my system:
[ 156.381868] BUG: sleeping function called from invalid context at net/core/sock.c:1523
[...]
There's a glaring bug in selinux_netlbl_inode_permission() - taking
lock_sock() inside rcu_read_lock().
I would again draw attention to Documentation/SubmitChecklist. In
particular please always always always enable all kernel debugging options
when developing and testing new kernel code. And everything else in that
file, too.
<guesses that this was tested on ia64>
I have not yet performed the 21 steps of
linux-2.6.20-rc3/Documentation/SubmitChecklist, which I think is a
great objectives list for future automation or some kind of community
web site. I hope to find time to make progress through that
checklist, but, in the meantime, I think the world may nevertheless be
infinitesmally better off if I post the patch that I'm currently
using that seems to fix the problem, seeing as how rc3 has passed
with no fix incorporated.
I think the intent of the offending code was to avoid doing
a lock_sock() in a presumably common case where there was no need to
take the lock. So, I have kept the presumably fast test to exit
early.
When it turns out to be necessary to take lock_sock(), RCU is
unlocked, then lock_sock is taken, the RCU is locked again, and
the test is repeated.
If I am wrong about lock_sock being expensive, I can
delete the lines that do the early return.
By the way, in a change not included in this patch,
I also tried consolidating the RCU locking in this file into a macro
IF_NLBL_REQUIRE(sksec, action), where "action" is the code
fragment to be executed with rcu_read_lock() held, although this
required splitting a couple of functions in half.
Anyhow, here is my current patch as MIME attachment.
Comments and labor in getting it through SubmitChecklist would
both be welcome.
Adam Richter
On Tuesday, January 2 2007 2:58 am, Adam J. Richter wrote:
I have not yet performed the 21 steps of
linux-2.6.20-rc3/Documentation/SubmitChecklist, which I think is a
great objectives list for future automation or some kind of community
web site. I hope to find time to make progress through that
checklist, but, in the meantime, I think the world may nevertheless be
infinitesmally better off if I post the patch that I'm currently
using that seems to fix the problem, seeing as how rc3 has passed
with no fix incorporated.
I think the intent of the offending code was to avoid doing
a lock_sock() in a presumably common case where there was no need to
take the lock. So, I have kept the presumably fast test to exit
early.
When it turns out to be necessary to take lock_sock(), RCU is
unlocked, then lock_sock is taken, the RCU is locked again, and
the test is repeated.
Hi Adam,
I'm sorry I just saw this mail (mail not sent directly to me get shuffled off
to a folder). I agree with your patch, I think dropping and then re-taking
the RCU lock is the best way to go, although I'm curious to see what others
have to say.
The only real comment I have with the patch is that there is some extra
whitespace which could probably be removed, but that is more of a style nit
than anything substantial.
By the way, in a change not included in this patch,
I also tried consolidating the RCU locking in this file into a macro
IF_NLBL_REQUIRE(sksec, action), where "action" is the code
fragment to be executed with rcu_read_lock() held, although this
required splitting a couple of functions in half.
From your description above I'm not sure I like that approach so much,
however, I could be misunderstanding something. Do you have a small example
you could send?
--
paul moore
linux security @ hp
From: Paul Moore <redacted>
Date: Tue, 2 Jan 2007 16:25:24 -0500
I'm sorry I just saw this mail (mail not sent directly to me get
shuffled off to a folder). I agree with your patch, I think
dropping and then re-taking the RCU lock is the best way to go,
although I'm curious to see what others have to say.
I think this is fine too.
On Tuesday, January 2 2007 6:37 pm, David Miller wrote:
From: Paul Moore <redacted>
Date: Tue, 2 Jan 2007 16:25:24 -0500
quoted
I'm sorry I just saw this mail (mail not sent directly to me get
shuffled off to a folder). I agree with your patch, I think
dropping and then re-taking the RCU lock is the best way to go,
although I'm curious to see what others have to say.
I think this is fine too.
[NOTE: dropped linux-kernel as I think this discussion is now strictly related
to socket locking so netdev is probably the best list]
I've been looking some more at Adam's and Ingo's patches for this as well as a
recent bug against a FC test kernel:
* https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=220966
For those who don't follow the link here is the meat of the bug report:
****
[ INFO: soft-safe -> soft-unsafe lock order detected ]
2.6.19-1.2891.fc7 #1
------------------------------------------------------
cupsd/1884 [HC0[0]:SC0[1]:HE1:SE0] is trying to acquire:
(&ssec->nlbl_lock){--..}, at: [<c04cec37>]
selinux_netlbl_socket_setsid+0xbb/0x123
and this task is already holding:
(af_callback_keys + sk->sk_family#3){-.-+}, at: [<c05daa1c>]
inet_accept+0x70/0xb5
which would create a new lock dependency:
(af_callback_keys + sk->sk_family#3){-.-+} -> (&ssec->nlbl_lock){--..}
but this new dependency connects a soft-irq-safe lock:
(af_callback_keys + sk->sk_family#3){-.-+}
... which became soft-irq-safe at:
[<c043fff1>] __lock_acquire+0x37d/0x9f8
[<c044094d>] lock_acquire+0x56/0x6f
[<c05fbdb6>] _read_lock_bh+0x30/0x3d
[<c04c687e>] selinux_socket_sock_rcv_skb+0xbd/0x252
[<c05d0645>] tcp_v4_rcv+0x37a/0x909
[<c05b7593>] ip_local_deliver+0x185/0x22e
[<c05b73d6>] ip_rcv+0x418/0x450
[<c059ae9c>] netif_receive_skb+0x2db/0x35a
[<c059c85f>] process_backlog+0x95/0xf6
[<c059ca46>] net_rx_action+0xa1/0x1a8
[<c042bf5a>] __do_softirq+0x6f/0xe2
[<c04063a1>] do_softirq+0x61/0xc7
[<ffffffff>] 0xffffffff
to a soft-irq-unsafe lock:
(&ssec->nlbl_lock){--..}
... which became soft-irq-unsafe at:
... [<c044007d>] __lock_acquire+0x409/0x9f8
[<c044094d>] lock_acquire+0x56/0x6f
[<c05fbc89>] _spin_lock+0x2b/0x38
[<c04cec37>] selinux_netlbl_socket_setsid+0xbb/0x123
[<c04d0c92>] selinux_netlbl_socket_post_create+0x2d/0x2f
[<c04c807b>] selinux_socket_post_create+0x156/0x15c
[<c059213c>] __sock_create+0x179/0x1b2
[<c05921ae>] sock_create+0x1a/0x1f
[<c0592435>] sys_socket+0x1b/0x3c
[<c0592cba>] sys_socketcall+0x77/0x241
[<c0404050>] syscall_call+0x7/0xb
[<ffffffff>] 0xffffffff
****
This makes me believe that Ingo's patch (which I see is now in Linus' and
Andrew's trees) is the way to go and not the lock re-order approach in Adam's
patch. I'm going to continue to look into this, almost more for my own
education than anything else, but I thought I would mention this lock
dependency message as it seemed relevant to the discussion.
--
paul moore
linux security @ hp