Re: [RFC 1/2] proc: Return if nothing to unmount

[Rik van Riel]

On Sat, 2017-09-09 at 19:31 +0100, Al Viro wrote:
> On Sat, Sep 09, 2017 at 06:03:16PM +0530, Gargi Sharma wrote:
> > If a task exits before procfs is mounted, proc_flush_task_mnt will
> > be called with a NULL mnt parameter. In that case, not only is
> > there
> > nothing to unhash, but trying to do so will oops the kernel with a
> > null pointer dereference.
> 
> You are misreading that sucker.  It's about userland mounts, it's
> about
> the internal ones in pidns, for each pidns the process belongs to.
> 
> IOW, what you are adding is dead code.  The very first alloc_pid() in
> that pidns should've called pid_ns_prepare_proc(), which creates that
> vfsmount.

Looking at the code (now that I am home, and no longer
reading this email on my phone), I see the cause of this
problem.

A previous version of Gargi's code had RESERVED_PIDS as
the lower bound for idr_alloc_cyclic, even on the very
first PID allocation cycle in a PID namespace.

With the code changed to have 1 as the lower bound during
the first allocation cycle, pid_ns_prepare_proc() should
be called correctly, and things should work as expected.

Gargi, can you drop this patch 1/2, and make sure the code
still works fine?

-- 
All Rights Reversed.

signature.asc
Description: This is a digitally signed message part

Re: [RFC 1/2] proc: Return if nothing to unmount

[Rik van Riel]

On Sat, 2017-09-09 at 19:31 +0100, Al Viro wrote:
> On Sat, Sep 09, 2017 at 06:03:16PM +0530, Gargi Sharma wrote:
> > If a task exits before procfs is mounted, proc_flush_task_mnt will
> > be called with a NULL mnt parameter. In that case, not only is
> > there
> > nothing to unhash, but trying to do so will oops the kernel with a
> > null pointer dereference.
> 
> You are misreading that sucker.  It's about userland mounts, it's
> about
> the internal ones in pidns, for each pidns the process belongs to.
> 
> IOW, what you are adding is dead code.  The very first alloc_pid() in
> that pidns should've called pid_ns_prepare_proc(), which creates that
> vfsmount.

Looking at the code (now that I am home, and no longer
reading this email on my phone), I see the cause of this
problem.

A previous version of Gargi's code had RESERVED_PIDS as
the lower bound for idr_alloc_cyclic, even on the very
first PID allocation cycle in a PID namespace.

With the code changed to have 1 as the lower bound during
the first allocation cycle, pid_ns_prepare_proc() should
be called correctly, and things should work as expected.

Gargi, can you drop this patch 1/2, and make sure the code
still works fine?

-- 
All Rights Reversed.

signature.asc
Description: This is a digitally signed message part

Re: [RFC 1/2] proc: Return if nothing to unmount

[Rik van Riel]

On September 9, 2017 2:31:35 PM EDT, Al Viro  wrote:
>On Sat, Sep 09, 2017 at 06:03:16PM +0530, Gargi Sharma wrote:
>> If a task exits before procfs is mounted, proc_flush_task_mnt will
>> be called with a NULL mnt parameter. In that case, not only is there
>> nothing to unhash, but trying to do so will oops the kernel with a
>> null pointer dereference.
>
>You are misreading that sucker.  It's about userland mounts, it's about
>the internal ones in pidns, for each pidns the process belongs to.
>
>IOW, what you are adding is dead code.  The very first alloc_pid() in
>that pidns should've called pid_ns_prepare_proc(), which creates that
>vfsmount.

Huh, my bad. I wonder why Gargi's code ran into a null pointer dereference on a 
null mnt pointer, then...
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [RFC 1/2] proc: Return if nothing to unmount

[Rik van Riel]

On September 9, 2017 2:31:35 PM EDT, Al Viro  wrote:
>On Sat, Sep 09, 2017 at 06:03:16PM +0530, Gargi Sharma wrote:
>> If a task exits before procfs is mounted, proc_flush_task_mnt will
>> be called with a NULL mnt parameter. In that case, not only is there
>> nothing to unhash, but trying to do so will oops the kernel with a
>> null pointer dereference.
>
>You are misreading that sucker.  It's about userland mounts, it's about
>the internal ones in pidns, for each pidns the process belongs to.
>
>IOW, what you are adding is dead code.  The very first alloc_pid() in
>that pidns should've called pid_ns_prepare_proc(), which creates that
>vfsmount.

Huh, my bad. I wonder why Gargi's code ran into a null pointer dereference on a 
null mnt pointer, then...
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [RFC 1/2] proc: Return if nothing to unmount

[Al Viro]

On Sat, Sep 09, 2017 at 06:03:16PM +0530, Gargi Sharma wrote:
> If a task exits before procfs is mounted, proc_flush_task_mnt will
> be called with a NULL mnt parameter. In that case, not only is there
> nothing to unhash, but trying to do so will oops the kernel with a
> null pointer dereference.

You are misreading that sucker.  It's about userland mounts, it's about
the internal ones in pidns, for each pidns the process belongs to.

IOW, what you are adding is dead code.  The very first alloc_pid() in
that pidns should've called pid_ns_prepare_proc(), which creates that
vfsmount.

Re: [RFC 1/2] proc: Return if nothing to unmount

[Al Viro]

On Sat, Sep 09, 2017 at 06:03:16PM +0530, Gargi Sharma wrote:
> If a task exits before procfs is mounted, proc_flush_task_mnt will
> be called with a NULL mnt parameter. In that case, not only is there
> nothing to unhash, but trying to do so will oops the kernel with a
> null pointer dereference.

You are misreading that sucker.  It's about userland mounts, it's about
the internal ones in pidns, for each pidns the process belongs to.

IOW, what you are adding is dead code.  The very first alloc_pid() in
that pidns should've called pid_ns_prepare_proc(), which creates that
vfsmount.

[RFC 1/2] proc: Return if nothing to unmount

[Gargi Sharma]

If a task exits before procfs is mounted, proc_flush_task_mnt will
be called with a NULL mnt parameter. In that case, not only is there
nothing to unhash, but trying to do so will oops the kernel with a
null pointer dereference.

Signed-off-by: Gargi Sharma 
---
 fs/proc/base.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index e5d89a0..7b83c21 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3021,6 +3021,10 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, 
pid_t pid, pid_t tgid)
char buf[PROC_NUMBUF];
struct qstr name;
 
+   /* procfs is not mounted. There is nothing to unhash. */
+   if (!mnt)
+   return;
+
name.name = buf;
name.len = snprintf(buf, sizeof(buf), "%d", pid);
/* no ->d_hash() rejects on procfs */
-- 
2.7.4

[RFC 1/2] proc: Return if nothing to unmount

[Gargi Sharma]

If a task exits before procfs is mounted, proc_flush_task_mnt will
be called with a NULL mnt parameter. In that case, not only is there
nothing to unhash, but trying to do so will oops the kernel with a
null pointer dereference.

Signed-off-by: Gargi Sharma 
---
 fs/proc/base.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index e5d89a0..7b83c21 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3021,6 +3021,10 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, 
pid_t pid, pid_t tgid)
char buf[PROC_NUMBUF];
struct qstr name;
 
+   /* procfs is not mounted. There is nothing to unhash. */
+   if (!mnt)
+   return;
+
name.name = buf;
name.len = snprintf(buf, sizeof(buf), "%d", pid);
/* no ->d_hash() rejects on procfs */
-- 
2.7.4