fuzz tested user mode linux core dumps in fs/lockd/clntproc.c:131

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Quoted 10 lines · Toralf Förster

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

Quoted 7 lines

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Thanks.

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used?

In this case the memory it points to
can be already freed.

Oleg.

kernel/exit.c +1 -1

@@ -783,8 +783,8 @@ void do_exit(long code)
 	exit_shm(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);
-	exit_task_namespaces(tsk);
 	exit_task_work(tsk);
+	exit_task_namespaces(tsk);
 	check_stack_usage();
 	exit_thread();
 

Quoted 10 lines · Toralf Förster

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

Quoted 7 lines

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Thanks.

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used? In this case the memory it points to can be already freed.

Oleg.

kernel/exit.c +1 -1

@@ -783,8 +783,8 @@ void do_exit(long code)
 	exit_shm(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);
-	exit_task_namespaces(tsk);
 	exit_task_work(tsk);
+	exit_task_namespaces(tsk);
 	check_stack_usage();
 	exit_thread();
 

The attached patch works - applied on top of current git - at least the issue cannot be reproduced then.

Quoted 51 lines · Toralf Förster

Quoted 10 lines

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

Quoted 7 lines

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Thanks.

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used? In this case the memory it points to can be already freed.

Oleg.

kernel/exit.c +1 -1

@@ -783,8 +783,8 @@ void do_exit(long code)
 	exit_shm(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);
-	exit_task_namespaces(tsk);
 	exit_task_work(tsk);
+	exit_task_namespaces(tsk);
 	check_stack_usage();
 	exit_thread();
 

Signature · 3 lines

MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

The attached patch works - applied on top of current git - at least the issue cannot be reproduced then.

Quoted 51 lines · Toralf Förster

Quoted 10 lines

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

Quoted 7 lines

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Thanks.

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used? In this case the memory it points to can be already freed.

Oleg.

kernel/exit.c +1 -1

@@ -783,8 +783,8 @@ void do_exit(long code)
 	exit_shm(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);
-	exit_task_namespaces(tsk);
 	exit_task_work(tsk);
+	exit_task_namespaces(tsk);
 	check_stack_usage();
 	exit_thread();
 

Signature · 3 lines

MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

Quoted 3 lines · Toralf Förster

The attached patch works - applied on top of current git - at least the issue cannot be reproduced then.

Thanks Toralf.

I'll write the changelog and send the patch tomorrow.

Andrey, any chance you can check that with this patch free_ipc_ns() doesn't have any problem with ->shm_file ?

e7b2c406 should be enough to fix that leak, but it would be nice if you can confirm.

Quoted 58 lines · Oleg Nesterov

Quoted 51 lines · Toralf Förster

Quoted 10 lines

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

Quoted 7 lines

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Thanks.

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used? In this case the memory it points to can be already freed.

Oleg.

kernel/exit.c +1 -1

@@ -783,8 +783,8 @@ void do_exit(long code)
 	exit_shm(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);
-	exit_task_namespaces(tsk);
 	exit_task_work(tsk);
+	exit_task_namespaces(tsk);
 	check_stack_usage();
 	exit_thread();
 

Signature · 3 lines

MfG/Sincerely
Toralf Förster
pgp finger print: 7B1A 07F4 EC82 0F90 D4C2 8936 872A E508 7DB6 9DA3

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Toralf Förster

The attached patch works - applied on top of current git - at least the issue cannot be reproduced then.

Quoted 6 lines

Thanks Toralf.

I'll write the changelog and send the patch tomorrow.

Andrey, any chance you can check that with this patch free_ipc_ns() doesn't have any problem with ->shm_file ?

kmemleak doesn't detect any leak,

but I think this patch is incorrect.

According to my previous investigations exit_task_work should be called after exit task namespaces
(http://comments.gmane.org/gmane.linux.kernel/1475123)

I applied the following patch:

(quoted diff) +4 -1

@@ -11,8 +11,11 @@ task_work_add(struct task_struct *task, struct
 callback_head *work, bool notify)
 
        do {
                head = ACCESS_ONCE(task->task_works);
-               if (unlikely(head == &work_exited))
+               if (unlikely(head == &work_exited)) {
+                       printk("%s:%d\n", __func__, __LINE__);
+                       dump_stack();
                        return -ESRCH;
+               }
                work->next = head;
        } while (cmpxchg(&task->task_works, head, work) != head);

and I got a few backtraces in a kernel log

[  151.513725] task_work_add:15
[  151.514860] CPU: 1 PID: 15303 Comm: ipc Not tainted 3.11.0-rc2+ #75
[  151.516743] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[  151.518558]  ffff880067bf0000 ffff88006922fba0 ffffffff81630dd5 ffff88006d9b2280
[  151.521767]  ffff88006922fbb0 ffffffff8107b478 ffff88006922fbd0 ffffffff8119ad43
[  151.524587]  ffff880079e81740 ffff88007a9035c8 ffff88006922fbe8 ffffffff81281ebd
[  151.527785] Call Trace:
[  151.528811]  [<ffffffff81630dd5>] dump_stack+0x45/0x56
[  151.530378]  [<ffffffff8107b478>] task_work_add+0x78/0x80
[  151.533219]  [<ffffffff8119ad43>] fput+0x63/0xa0

Quoted 7 lines · Andrew Vagin

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Toralf Förster

The attached patch works - applied on top of current git - at least the issue cannot be reproduced then.

Quoted 8 lines

Quoted 6 lines

Thanks Toralf.

I'll write the changelog and send the patch tomorrow.

Andrey, any chance you can check that with this patch free_ipc_ns() doesn't have any problem with ->shm_file ?

kmemleak doesn't detect any leak,

Good.

Quoted 25 lines

but I think this patch is incorrect.

According to my previous investigations exit_task_work should be called after exit task namespaces
(http://comments.gmane.org/gmane.linux.kernel/1475123)

I applied the following patch:

(quoted diff) +4 -1

@@ -11,8 +11,11 @@ task_work_add(struct task_struct *task, struct
 callback_head *work, bool notify)
 
        do {
                head = ACCESS_ONCE(task->task_works);
-               if (unlikely(head == &work_exited))
+               if (unlikely(head == &work_exited)) {
+                       printk("%s:%d\n", __func__, __LINE__);
+                       dump_stack();
                        return -ESRCH;
+               }
                work->next = head;
        } while (cmpxchg(&task->task_works, head, work) != head);

and I got a few backtraces in a kernel log

Quoted 10 lines

[  151.513725] task_work_add:15
[  151.514860] CPU: 1 PID: 15303 Comm: ipc Not tainted 3.11.0-rc2+ #75
[  151.516743] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[  151.518558]  ffff880067bf0000 ffff88006922fba0 ffffffff81630dd5 ffff88006d9b2280
[  151.521767]  ffff88006922fbb0 ffffffff8107b478 ffff88006922fbd0 ffffffff8119ad43
[  151.524587]  ffff880079e81740 ffff88007a9035c8 ffff88006922fbe8 ffffffff81281ebd
[  151.527785] Call Trace:
[  151.528811]  [<ffffffff81630dd5>] dump_stack+0x45/0x56
[  151.530378]  [<ffffffff8107b478>] task_work_add+0x78/0x80
[  151.533219]  [<ffffffff8119ad43>] fput+0x63/0xa0

But this is fine?

Once again, we also have e7b2c406 "fput: task_work_add() can fail if the caller has passed exit_task_work()" commit which should also fix this particulat problem.

I don't have objections against this patch. All my tests work find.

Quoted 2 lines

I don't have objections against this patch. All my tests work find.

Great, thanks for confirmation. Can I translate this into your acked-by or tested-by if I send this patch?

(there is another fix from Eric, it is not clear to me if I
should send it right now).

Oleg.

Quoted 10 lines · Toralf Förster

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

Quoted 7 lines

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Thanks.

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used? In this case the memory it points to can be already freed.

Quoted 27 lines · Oleg Nesterov

Quoted 10 lines · Toralf Förster

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

Quoted 7 lines

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Thanks.

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

Quoted 7 lines

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used? In this case the memory it points to can be already freed.

I don't think anyone has ever looked into that. This was a flyby conversion by Serge in 2006 when he originally did the uts namespace.

from commit e9ff3990f08e9a0c2839cc22808b01732ea5b3e4
[PATCH] namespaces: utsname: switch to using uts namespaces

Replace references to system_utsname to the per-process uts namespace where appropriate. This includes things like uname.

Changes: Per Eric Biederman's comments, use the per-process uts namespace
for ELF_PLATFORM, sunrpc, and parts of net/ipv4/ipconfig.c

Hmm. That credits with me with this mess. What was I thinking? Perhaps I just said you missed a couple of spots.

This untested patch should fix it without any need to worry about dynamic behavior. Although I am wondering if we have a few other spots where the dynamic behavior might be iffy.

Serge do you remember any of this?

On a good day I can follow the nfs code but it takes quite a while. I feel the same way about filesystems locks so I am not really certain what is going on.

Eric

fs/lockd/clntproc.c +2 -2

index 9760ecb..6643cfc 100644
@@ -128,11 +128,11 @@ static void nlmclnt_setlockargs(struct nlm_rqst *req, struct file_lock *fl)
 
        nlmclnt_next_cookie(&argp->cookie);
        memcpy(&lock->fh, NFS_FH(file_inode(fl->fl_file)), sizeof(struct nfs_fh));
-       lock->caller  = utsname()->nodename;
+       lock->caller  = init_utsname()->nodename;
        lock->oh.data = req->a_owner;
        lock->oh.len  = snprintf(req->a_owner, sizeof(req->a_owner), "%u@%s",
                                (unsigned int)fl->fl_u.nfs_fl.owner->pid,
-                               utsname()->nodename);
+                               init_utsname()->nodename);
        lock->svid = fl->fl_u.nfs_fl.owner->pid;
        lock->fl.fl_start = fl->fl_start;
        lock->fl.fl_end = fl->fl_end;

Eric

Quoted 3 lines

This untested patch should fix it without any need to worry about dynamic behavior.

Although I am wondering if we have a few other spots

Quoted 1 line

where the dynamic behavior might be iffy.

Quoted 28 lines

Serge do you remember any of this?

On a good day I can follow the nfs code but it takes quite a while. I feel the same way about filesystems locks so I am not really certain what is going on.

Eric

fs/lockd/clntproc.c +2 -2

index 9760ecb..6643cfc 100644
@@ -128,11 +128,11 @@ static void nlmclnt_setlockargs(struct nlm_rqst *req, struct file_lock *fl)
 
        nlmclnt_next_cookie(&argp->cookie);
        memcpy(&lock->fh, NFS_FH(file_inode(fl->fl_file)), sizeof(struct nfs_fh));
-       lock->caller  = utsname()->nodename;
+       lock->caller  = init_utsname()->nodename;
        lock->oh.data = req->a_owner;
        lock->oh.len  = snprintf(req->a_owner, sizeof(req->a_owner), "%u@%s",
                                (unsigned int)fl->fl_u.nfs_fl.owner->pid,
-                               utsname()->nodename);
+                               init_utsname()->nodename);
        lock->svid = fl->fl_u.nfs_fl.owner->pid;
        lock->fl.fl_start = fl->fl_start;
        lock->fl.fl_end = fl->fl_end;

Eric

Quoted 3 lines · Eric W. Biederman

Quoted 3 lines

This untested patch should fix it without any need to worry about dynamic behavior.

Yeees ;) I was thinking about this change too, but I have no idea what this ->nodename actually means for nfs.

If you are going to send this patch - great!

Quoted 2 lines

Although I am wondering if we have a few other spots

Quoted 1 line

where the dynamic behavior might be iffy.

Yes. And perhaps the patch which moves exit_task_namespaces() after exit_task_work() makes sense anyway (the patch I showed).

(but if you change nlmclnt_setlockargs() then it is not 3.11 material).

The original motivation for 8aac62706 was the leak reported by Andrey, but that leak should be also fixed by e7b2c406. "Move exit_task_namespaces() from exit_notify() to do_exit()" is still fine imho, the reason for exit_task_namespaces() from the middle of exit_notify() has gone away.

But perhaps it would be better if work->func() could use ->nsproxy even if the task is PF_EXITING.

Quoted 28 lines

Quoted 28 lines

Serge do you remember any of this?

On a good day I can follow the nfs code but it takes quite a while. I feel the same way about filesystems locks so I am not really certain what is going on.

Eric

fs/lockd/clntproc.c +2 -2

index 9760ecb..6643cfc 100644
@@ -128,11 +128,11 @@ static void nlmclnt_setlockargs(struct nlm_rqst *req, struct file_lock *fl)
 
        nlmclnt_next_cookie(&argp->cookie);
        memcpy(&lock->fh, NFS_FH(file_inode(fl->fl_file)), sizeof(struct nfs_fh));
-       lock->caller  = utsname()->nodename;
+       lock->caller  = init_utsname()->nodename;
        lock->oh.data = req->a_owner;
        lock->oh.len  = snprintf(req->a_owner, sizeof(req->a_owner), "%u@%s",
                                (unsigned int)fl->fl_u.nfs_fl.owner->pid,
-                               utsname()->nodename);
+                               init_utsname()->nodename);
        lock->svid = fl->fl_u.nfs_fl.owner->pid;
        lock->fl.fl_start = fl->fl_start;
        lock->fl.fl_end = fl->fl_end;

Eric

So I really don't think using utsname() aka current->nsproxy->uts_ns makes sense in nlmclnt_setlockargs.

We most definitely have an inconsistency in nfs.

Quoted 10 lines

Yes. And perhaps the patch which moves exit_task_namespaces() after exit_task_work() makes sense anyway (the patch I showed).

(but if you change nlmclnt_setlockargs() then it is not 3.11 material).

The original motivation for 8aac62706 was the leak reported by Andrey, but that leak should be also fixed by e7b2c406. "Move exit_task_namespaces() from exit_notify() to do_exit()" is still fine imho, the reason for exit_task_namespaces() from the middle of exit_notify() has gone away.

Quoted 2 lines

But perhaps it would be better if work->func() could use ->nsproxy even if the task is PF_EXITING.

So far there is nothing in the nfs code that would suggest allowing work->func() being able to use ->nsproxy would make this code any better. I think that would just paper over the problem we are seeing right now.

Quoted 5 lines · Eric W. Biederman

So I really don't think using utsname() aka current->nsproxy->uts_ns makes sense in nlmclnt_setlockargs.

We most definitely have an inconsistency in nfs.

I tend to agree, but can't really comment.

Quoted 10 lines

Quoted 10 lines

Yes. And perhaps the patch which moves exit_task_namespaces() after exit_task_work() makes sense anyway (the patch I showed).

(but if you change nlmclnt_setlockargs() then it is not 3.11 material).

The original motivation for 8aac62706 was the leak reported by Andrey, but that leak should be also fixed by e7b2c406. "Move exit_task_namespaces() from exit_notify() to do_exit()" is still fine imho, the reason for exit_task_namespaces() from the middle of exit_notify() has gone away.

Quoted 7 lines

Quoted 2 lines

But perhaps it would be better if work->func() could use ->nsproxy even if the task is PF_EXITING.

So far there is nothing in the nfs code that would suggest allowing work->func() being able to use ->nsproxy would make this code any better. I think that would just paper over the problem we are seeing right now.

I think you misunderstood my point.

I fully agree if you change nlmclnt_setlockargs(). I am suggesting to move exit_task_namespaces() down after exit_task_work() as a separate change which perhaps makes sense by itself. Not to fix this problem, not for nfs, not for fput().

Just to allow work->func() to play with ->nsproxy if needed. task_work has other users, not only fput().

Adding some people who pay more attention to nfs in network namespaces than I usually do.

Quoted 8 lines · Oleg Nesterov

Quoted 3 lines · Eric W. Biederman

Quoted 3 lines

This untested patch should fix it without any need to worry about dynamic behavior.

Yeees ;) I was thinking about this change too, but I have no idea what this ->nodename actually means for nfs.

Quoted 1 line

If you are going to send this patch - great!

Just batting it around for now, and hoping we have the right combination of eyeballs look at it. There are more places in nfs that have questionable uses of utsname() instead of init_utsname(). So I think we probably need a more comprehensive patch at the very least.

nfsclnt_reclaim is never called from userspace.

nfsclnt_cancel is called from a callback that seems to have no guarantee
of an approprate userspace context.
nfsclnt_proc is called from rpc_ops and I did not spend the time to see
if that could be a userspace context.

So I really don't think using utsname() aka current->nsproxy->uts_ns makes sense in nlmclnt_setlockargs.

We most definitely have an inconsistency in nfs. I am a little hesitant to suggest my patch because it is likely to have strange effects on nfs in a network namespace. On the flip side the code is broken anyway, we might as well at least use a version that is guarnateed not to cause a null pointer dereference in the kernel.

This is the first time someone has seen a problem since late 2006 since Serge updated most of the references to use system_utsname but I think we have just been lucky.

Quoted 8 lines

Quoted 2 lines

Although I am wondering if we have a few other spots

Quoted 1 line

where the dynamic behavior might be iffy.

Yes. And perhaps the patch which moves exit_task_namespaces() after exit_task_work() makes sense anyway (the patch I showed).

(but if you change nlmclnt_setlockargs() then it is not 3.11 material).

Quoted 7 lines

The original motivation for 8aac62706 was the leak reported by Andrey, but that leak should be also fixed by e7b2c406. "Move exit_task_namespaces() from exit_notify() to do_exit()" is still fine imho, the reason for exit_task_namespaces() from the middle of exit_notify() has gone away.

But perhaps it would be better if work->func() could use ->nsproxy even if the task is PF_EXITING.

So far there is nothing in the nfs code that would suggest allowing work->func() being able to use ->nsproxy would make this code any better. I think that would just paper over the problem we are seeing right now.

Quoted 27 lines

Quoted 27 lines

Quoted 27 lines

Serge do you remember any of this?

On a good day I can follow the nfs code but it takes quite a while. I feel the same way about filesystems locks so I am not really certain what is going on.

Eric

fs/lockd/clntproc.c +2 -2

index 9760ecb..6643cfc 100644
@@ -128,11 +128,11 @@ static void nlmclnt_setlockargs(struct nlm_rqst *req, struct file_lock *fl)
 
        nlmclnt_next_cookie(&argp->cookie);
        memcpy(&lock->fh, NFS_FH(file_inode(fl->fl_file)), sizeof(struct nfs_fh));
-       lock->caller  = utsname()->nodename;
+       lock->caller  = init_utsname()->nodename;
        lock->oh.data = req->a_owner;
        lock->oh.len  = snprintf(req->a_owner, sizeof(req->a_owner), "%u@%s",
                                (unsigned int)fl->fl_u.nfs_fl.owner->pid,
-                               utsname()->nodename);
+                               init_utsname()->nodename);
        lock->svid = fl->fl_u.nfs_fl.owner->pid;
        lock->fl.fl_start = fl->fl_start;
        lock->fl.fl_end = fl->fl_end;

Quoted 1 line

Quoted 1 line

Quoted 1 line

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Toralf Förster

I do have a user mode linux image (stable 32 bit Gentoo Linux ) which erratically crashes while fuzz tested with trinity if the victim files are located on a NFS share.

The back trace of the core dumps always looks like the attached.

To bisect it is hard. However after few attempts in the last weeks the following commit is either the first bad commit or at least the upper limit (less likely).

Quoted 7 lines

commit 8aac62706adaaf0fab02c4327761561c8bda9448

Author: Oleg Nesterov <[email protected]>
Date:   Fri Jun 14 21:09:49 2013 +0200

move exit_task_namespaces() outside of exit_notify()

#15 nlmclnt_setlockargs (req=0x48e18860, fl=0x48f27c8c) at fs/lockd/clntproc.c:131

Thanks.

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used? In this case the memory it points to can be already freed.

Oleg.

Quoted 17 lines · Oleg Nesterov

So nlmclnt_setlockargs()->utsname() crashes and we probably need the patch below.

But is it correct? I know _absolutely_ nothing about nfs/sunrpc/etc and I never looked into this code before, most probably I am wrong.

But it seems that __nlm_async_call() relies on workqueues. nlmclnt_async_call() does rpc_wait_for_completion_task(), but what if the caller is killed?

nlm_rqst can't go away, ->a_count was incremented. But can't the caller exit before call->name is used? In this case the memory it points to can be already freed.

Oleg.

Quoted 5 lines

kernel/exit.c void do_exit(long code) · line 790

Quoted 9 lines

kernel/exit.c +1 -1

@@ -783,8 +783,8 @@ void do_exit(long code)
 	exit_shm(tsk);
 	exit_files(tsk);
 	exit_fs(tsk);
-	exit_task_namespaces(tsk);
 	exit_task_work(tsk);
+	exit_task_namespaces(tsk);
 	check_stack_usage();
 	exit_thread();
 

/me wonders if/when this will go in the main kernel ?