Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Mon, Jan 14, 2008 at 09:24:54AM -0500, Jeff Layton wrote: > Thanks Christoph. I incorporated this into my latest patchset. It does > seem to fix the issue (tested by bouncing NFS up and down for 30 mins > or so). Let me know if you want me to add a signed-off-by line for > you... No need to add anything, this was just a two-liner trivial fix.. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Sun, 13 Jan 2008 18:17:43 + Christoph Hellwig <[EMAIL PROTECTED]> wrote: > On Sun, Jan 13, 2008 at 08:27:18AM -0500, Jeff Layton wrote: > >I've been hitting an intermittent null pointer dereference ever > > since I've made this change: > > The first thing lockd does is to call lock_kernel(). This may either > block (or spin) when it is contended and thus delay updating > nlmsvc_serv. Now lockd_up checks for nlmsvc_task which already is > non-NULL and happily dereference nlmsvc_serv. The patch below > updates nlmsvc_serv in lockd_up where it is protected by nlmsvc_mutex > and also checks for nlmsvc_serv beeing set instead of nlmsvc_task to > fix this problem. > > The patch hasn't actually been tested but I'm sure it will fix this > issue. > Thanks Christoph. I incorporated this into my latest patchset. It does seem to fix the issue (tested by bouncing NFS up and down for 30 mins or so). Let me know if you want me to add a signed-off-by line for you... > Btw, lockd() takes BKL just after starting up and only implicitly > drops it when blocking. This seems very dangerous to me and badly > wants updating to some real locking scheme.. > Yep -- It's ugly. I took a look a while back at what it would take to change that. The problem is that it's very difficult to tell exactly what the BKL is intended to protect in. I assume it does it for the same reason that fs/locks.c uses it, but there may be other things that need protection if it's removed. It might be best to try to change this incrementally -- gradually audit and move pieces of lockd() outside of the BKL, until it's clear that it's no longer needed. -- Jeff Layton <[EMAIL PROTECTED]> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Sun, 13 Jan 2008 18:17:43 + Christoph Hellwig [EMAIL PROTECTED] wrote: On Sun, Jan 13, 2008 at 08:27:18AM -0500, Jeff Layton wrote: I've been hitting an intermittent null pointer dereference ever since I've made this change: The first thing lockd does is to call lock_kernel(). This may either block (or spin) when it is contended and thus delay updating nlmsvc_serv. Now lockd_up checks for nlmsvc_task which already is non-NULL and happily dereference nlmsvc_serv. The patch below updates nlmsvc_serv in lockd_up where it is protected by nlmsvc_mutex and also checks for nlmsvc_serv beeing set instead of nlmsvc_task to fix this problem. The patch hasn't actually been tested but I'm sure it will fix this issue. Thanks Christoph. I incorporated this into my latest patchset. It does seem to fix the issue (tested by bouncing NFS up and down for 30 mins or so). Let me know if you want me to add a signed-off-by line for you... Btw, lockd() takes BKL just after starting up and only implicitly drops it when blocking. This seems very dangerous to me and badly wants updating to some real locking scheme.. Yep -- It's ugly. I took a look a while back at what it would take to change that. The problem is that it's very difficult to tell exactly what the BKL is intended to protect in. I assume it does it for the same reason that fs/locks.c uses it, but there may be other things that need protection if it's removed. It might be best to try to change this incrementally -- gradually audit and move pieces of lockd() outside of the BKL, until it's clear that it's no longer needed. -- Jeff Layton [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Mon, Jan 14, 2008 at 09:24:54AM -0500, Jeff Layton wrote: Thanks Christoph. I incorporated this into my latest patchset. It does seem to fix the issue (tested by bouncing NFS up and down for 30 mins or so). Let me know if you want me to add a signed-off-by line for you... No need to add anything, this was just a two-liner trivial fix.. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Sun, Jan 13, 2008 at 06:17:43PM +, Christoph Hellwig wrote: > Btw, lockd() takes BKL just after starting up and only implicitly drops > it when blocking. This seems very dangerous to me and badly wants > updating to some real locking scheme.. Yep. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Sun, Jan 13, 2008 at 08:27:18AM -0500, Jeff Layton wrote:
>I've been hitting an intermittent null pointer dereference ever
> since I've made this change:
The first thing lockd does is to call lock_kernel(). This may either
block (or spin) when it is contended and thus delay updating
nlmsvc_serv. Now lockd_up checks for nlmsvc_task which already is
non-NULL and happily dereference nlmsvc_serv. The patch below
updates nlmsvc_serv in lockd_up where it is protected by nlmsvc_mutex
and also checks for nlmsvc_serv beeing set instead of nlmsvc_task to
fix this problem.
The patch hasn't actually been tested but I'm sure it will fix this
issue.
Btw, lockd() takes BKL just after starting up and only implicitly drops
it when blocking. This seems very dangerous to me and badly wants
updating to some real locking scheme..
Signed-off-by: Christoph Hellwig <[EMAIL PROTECTED]>
Index: linux-2.6/fs/lockd/svc.c
===
--- linux-2.6.orig/fs/lockd/svc.c 2008-01-13 19:07:17.0 +0100
+++ linux-2.6/fs/lockd/svc.c2008-01-13 19:13:23.0 +0100
@@ -118,7 +118,6 @@ lockd(void *vrqstp)
/* set up kernel thread */
lock_kernel();
- nlmsvc_serv = rqstp->rq_server;
set_freezable();
/* Allow SIGKILL to tell lockd to drop all of its locks */
@@ -253,7 +252,7 @@ lockd_up(int proto) /* Maybe add a 'fami
/*
* Check whether we're already up and running.
*/
- if (nlmsvc_task) {
+ if (nlmsvc_serv) {
if (proto)
error = make_socks(nlmsvc_serv, proto);
goto out;
@@ -290,6 +289,9 @@ lockd_up(int proto) /* Maybe add a 'fami
}
svc_sock_update_bufs(serv);
+
+ nlmsvc_serv = rqstp->rq_server;
+
nlmsvc_task = kthread_run(lockd, rqstp, serv->sv_name);
if (IS_ERR(nlmsvc_task)) {
error = PTR_ERR(nlmsvc_task);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Wed, 9 Jan 2008 17:35:42 +
Christoph Hellwig <[EMAIL PROTECTED]> wrote:
> On Tue, Jan 08, 2008 at 02:33:15PM -0500, Jeff Layton wrote:
> > lockd_start_done is a global var that can be reused if lockd is
> > restarted, but it's never reinitialized. On all but the first use,
> > wait_for_completion isn't actually waiting on it since it has
> > already completed once.
>
> I don't think we'll need lockd_start_done anymore after the kthread
> conversion. When kthread_run returns the thread it created is
> guaranteed to have run until it scheduled away.
>
Christoph,
I've been hitting an intermittent null pointer dereference ever
since I've made this change:
BUG: unable to handle kernel NULL pointer dereference at virtual address
0038
printing eip: e09ddee1 *pde = 1f377067 *pte =
Oops: [#1] SMP
Modules linked in: nfsd nfs_acl auth_rpcgss exportfs rfcomm l2cap bluetooth
autofs4 lockd sunrpc nf_conntrack_ipv6 xt_state nf_conntrack xt_tcpudp
ip6t_ipv6header ip6t_REJECT ip6table_filter ip6_tables x_tables ipv6 loop
dm_multipath pcspkr 8139cp 8139too mii joydev i2c_piix4 i2c_core sr_mod sg
cdrom dm_snapshot dm_zero dm_mirror dm_mod ata_piix pata_acpi ata_generic
libata sd_mod scsi_mod ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd
Pid: 1946, comm: rpc.nfsd Not tainted (2.6.24-0.138.rc7.kthread.2.fc9 #1)
EIP: 0060:[] EFLAGS: 00010202 CPU: 0
EIP is at find_socket+0xa/0x3f [lockd]
EAX: EBX: 0006 ECX: EDX: 0011
ESI: EDI: 0011 EBP: df358ec4 ESP: df358eb8
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process rpc.nfsd (pid: 1946, ti=df358000 task=df38ae10 task.ti=df358000)
Stack: 0006 0006 df358ee0 e09de0cb 0006
0008 0801 df358f04 e09de337
0801 0008 0801 df358f1c e0aa05ac df356200 0008
Call Trace:
[] show_trace_log_lvl+0x1a/0x2f
[] show_stack_log_lvl+0x9b/0xa3
[] show_registers+0xa7/0x178
[] die+0x135/0x220
[] do_page_fault+0x553/0x631
[] error_code+0x72/0x78
[] make_socks+0x27/0xbe [lockd]
[] lockd_up+0x3b/0x148 [lockd]
[] nfsd_svc+0xf2/0x107 [nfsd]
[] write_svc+0x1a/0x20 [nfsd]
[] nfsctl_transaction_write+0x39/0x63 [nfsd]
[] sys_nfsservctl+0x11f/0x160
[] syscall_call+0x7/0xb
===
Code: 89 d8 5b 5d c3 55 89 e5 c7 05 48 a4 9e e0 01 00 00 00 e8 a7 ff ff ff 8b
15 00 0c 76 c0 5d 01 d0 c3 55 89 e5 57 89 d7 56 89 c6 53 <8b> 48 38 83 e9 08 eb
15 8b 41 14 0f b6 40 29 39 f8 75 07 b8 01
EIP: [] find_socket+0xa/0x3f [lockd] SS:ESP 0068:df358eb8
---[ end trace 7d509b4c18b144aa ]---
The problem is that make_socks is occasionally getting called with a
NULL nlmsvc_serv pointer. I think the problem occurs here in lockd_up().
if (nlmsvc_task) {
if (proto)
error = make_socks(nlmsvc_serv, proto);
goto out;
}
You pointed out earlier that this should really be checking that
nlmsvc_serv is non-NULL. I can and will make this change, and that will
likely fix this particular oops, but the fact that I'm hitting it here
suggests that kthread_run is returning before lockd has a chance
to set nlmsvc_serv. It shouldn't be according to your statement above.
Are you sure that kthread_run is working correctly? It seems like it
might not be doing the right thing here...
--
Jeff Layton <[EMAIL PROTECTED]>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Wed, 9 Jan 2008 17:35:42 +
Christoph Hellwig [EMAIL PROTECTED] wrote:
On Tue, Jan 08, 2008 at 02:33:15PM -0500, Jeff Layton wrote:
lockd_start_done is a global var that can be reused if lockd is
restarted, but it's never reinitialized. On all but the first use,
wait_for_completion isn't actually waiting on it since it has
already completed once.
I don't think we'll need lockd_start_done anymore after the kthread
conversion. When kthread_run returns the thread it created is
guaranteed to have run until it scheduled away.
Christoph,
I've been hitting an intermittent null pointer dereference ever
since I've made this change:
BUG: unable to handle kernel NULL pointer dereference at virtual address
0038
printing eip: e09ddee1 *pde = 1f377067 *pte =
Oops: [#1] SMP
Modules linked in: nfsd nfs_acl auth_rpcgss exportfs rfcomm l2cap bluetooth
autofs4 lockd sunrpc nf_conntrack_ipv6 xt_state nf_conntrack xt_tcpudp
ip6t_ipv6header ip6t_REJECT ip6table_filter ip6_tables x_tables ipv6 loop
dm_multipath pcspkr 8139cp 8139too mii joydev i2c_piix4 i2c_core sr_mod sg
cdrom dm_snapshot dm_zero dm_mirror dm_mod ata_piix pata_acpi ata_generic
libata sd_mod scsi_mod ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd
Pid: 1946, comm: rpc.nfsd Not tainted (2.6.24-0.138.rc7.kthread.2.fc9 #1)
EIP: 0060:[e09ddee1] EFLAGS: 00010202 CPU: 0
EIP is at find_socket+0xa/0x3f [lockd]
EAX: EBX: 0006 ECX: EDX: 0011
ESI: EDI: 0011 EBP: df358ec4 ESP: df358eb8
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process rpc.nfsd (pid: 1946, ti=df358000 task=df38ae10 task.ti=df358000)
Stack: 0006 0006 df358ee0 e09de0cb 0006
0008 0801 df358f04 e09de337
0801 0008 0801 df358f1c e0aa05ac df356200 0008
Call Trace:
[c040649a] show_trace_log_lvl+0x1a/0x2f
[c040654a] show_stack_log_lvl+0x9b/0xa3
[c04065f9] show_registers+0xa7/0x178
[c04067ff] die+0x135/0x220
[c063ff1b] do_page_fault+0x553/0x631
[c063e5a2] error_code+0x72/0x78
[e09de0cb] make_socks+0x27/0xbe [lockd]
[e09de337] lockd_up+0x3b/0x148 [lockd]
[e0aa05ac] nfsd_svc+0xf2/0x107 [nfsd]
[e0aa0b19] write_svc+0x1a/0x20 [nfsd]
[e0aa0d10] nfsctl_transaction_write+0x39/0x63 [nfsd]
[c04b97cf] sys_nfsservctl+0x11f/0x160
[c0405252] syscall_call+0x7/0xb
===
Code: 89 d8 5b 5d c3 55 89 e5 c7 05 48 a4 9e e0 01 00 00 00 e8 a7 ff ff ff 8b
15 00 0c 76 c0 5d 01 d0 c3 55 89 e5 57 89 d7 56 89 c6 53 8b 48 38 83 e9 08 eb
15 8b 41 14 0f b6 40 29 39 f8 75 07 b8 01
EIP: [e09ddee1] find_socket+0xa/0x3f [lockd] SS:ESP 0068:df358eb8
---[ end trace 7d509b4c18b144aa ]---
The problem is that make_socks is occasionally getting called with a
NULL nlmsvc_serv pointer. I think the problem occurs here in lockd_up().
if (nlmsvc_task) {
if (proto)
error = make_socks(nlmsvc_serv, proto);
goto out;
}
You pointed out earlier that this should really be checking that
nlmsvc_serv is non-NULL. I can and will make this change, and that will
likely fix this particular oops, but the fact that I'm hitting it here
suggests that kthread_run is returning before lockd has a chance
to set nlmsvc_serv. It shouldn't be according to your statement above.
Are you sure that kthread_run is working correctly? It seems like it
might not be doing the right thing here...
--
Jeff Layton [EMAIL PROTECTED]
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Sun, Jan 13, 2008 at 08:27:18AM -0500, Jeff Layton wrote:
I've been hitting an intermittent null pointer dereference ever
since I've made this change:
The first thing lockd does is to call lock_kernel(). This may either
block (or spin) when it is contended and thus delay updating
nlmsvc_serv. Now lockd_up checks for nlmsvc_task which already is
non-NULL and happily dereference nlmsvc_serv. The patch below
updates nlmsvc_serv in lockd_up where it is protected by nlmsvc_mutex
and also checks for nlmsvc_serv beeing set instead of nlmsvc_task to
fix this problem.
The patch hasn't actually been tested but I'm sure it will fix this
issue.
Btw, lockd() takes BKL just after starting up and only implicitly drops
it when blocking. This seems very dangerous to me and badly wants
updating to some real locking scheme..
Signed-off-by: Christoph Hellwig [EMAIL PROTECTED]
Index: linux-2.6/fs/lockd/svc.c
===
--- linux-2.6.orig/fs/lockd/svc.c 2008-01-13 19:07:17.0 +0100
+++ linux-2.6/fs/lockd/svc.c2008-01-13 19:13:23.0 +0100
@@ -118,7 +118,6 @@ lockd(void *vrqstp)
/* set up kernel thread */
lock_kernel();
- nlmsvc_serv = rqstp-rq_server;
set_freezable();
/* Allow SIGKILL to tell lockd to drop all of its locks */
@@ -253,7 +252,7 @@ lockd_up(int proto) /* Maybe add a 'fami
/*
* Check whether we're already up and running.
*/
- if (nlmsvc_task) {
+ if (nlmsvc_serv) {
if (proto)
error = make_socks(nlmsvc_serv, proto);
goto out;
@@ -290,6 +289,9 @@ lockd_up(int proto) /* Maybe add a 'fami
}
svc_sock_update_bufs(serv);
+
+ nlmsvc_serv = rqstp-rq_server;
+
nlmsvc_task = kthread_run(lockd, rqstp, serv-sv_name);
if (IS_ERR(nlmsvc_task)) {
error = PTR_ERR(nlmsvc_task);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Sun, Jan 13, 2008 at 06:17:43PM +, Christoph Hellwig wrote: Btw, lockd() takes BKL just after starting up and only implicitly drops it when blocking. This seems very dangerous to me and badly wants updating to some real locking scheme.. Yep. --b. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Wed, Jan 09, 2008 at 01:05:54PM -0500, Jeff Layton wrote: > Makes sense. My only concern is that we make sure this is behavior we > can count on in the future and not just an artifact of the current > kthread implementation. If that's the case, then I'll plan to remove it > on the next respin. It's absolutely intentional and one of the reasons why the kthread infrastructure is so much nicer than plain kthread_create :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Wed, 9 Jan 2008 17:35:42 + Christoph Hellwig <[EMAIL PROTECTED]> wrote: > I don't think we'll need lockd_start_done anymore after the kthread > conversion. When kthread_run returns the thread it created is > guaranteed to have run until it scheduled away. > Makes sense. My only concern is that we make sure this is behavior we can count on in the future and not just an artifact of the current kthread implementation. If that's the case, then I'll plan to remove it on the next respin. -- Jeff Layton <[EMAIL PROTECTED]> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Tue, Jan 08, 2008 at 02:33:15PM -0500, Jeff Layton wrote: > lockd_start_done is a global var that can be reused if lockd is > restarted, but it's never reinitialized. On all but the first use, > wait_for_completion isn't actually waiting on it since it has > already completed once. I don't think we'll need lockd_start_done anymore after the kthread conversion. When kthread_run returns the thread it created is guaranteed to have run until it scheduled away. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Tue, Jan 08, 2008 at 02:33:15PM -0500, Jeff Layton wrote: lockd_start_done is a global var that can be reused if lockd is restarted, but it's never reinitialized. On all but the first use, wait_for_completion isn't actually waiting on it since it has already completed once. I don't think we'll need lockd_start_done anymore after the kthread conversion. When kthread_run returns the thread it created is guaranteed to have run until it scheduled away. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Wed, 9 Jan 2008 17:35:42 + Christoph Hellwig [EMAIL PROTECTED] wrote: I don't think we'll need lockd_start_done anymore after the kthread conversion. When kthread_run returns the thread it created is guaranteed to have run until it scheduled away. Makes sense. My only concern is that we make sure this is behavior we can count on in the future and not just an artifact of the current kthread implementation. If that's the case, then I'll plan to remove it on the next respin. -- Jeff Layton [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] NLM: Initialize completion variable in lockd_up
On Wed, Jan 09, 2008 at 01:05:54PM -0500, Jeff Layton wrote: Makes sense. My only concern is that we make sure this is behavior we can count on in the future and not just an artifact of the current kthread implementation. If that's the case, then I'll plan to remove it on the next respin. It's absolutely intentional and one of the reasons why the kthread infrastructure is so much nicer than plain kthread_create :) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
