Now that we have pidfs_{get,register}_pid() that needs to be paired with
pidfs_put_pid() it's possible that someone pairs them with put_pid().
Thus freeing struct pid while it's still used by pidfs. Notice when that
happens. I'll also add a scheme to detect invalid uses of
pidfs_get_pid() and pidfs_put_pid() later.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 kernel/pid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/pid.c b/kernel/pid.c
index 26f1e136f017..8317bcbc7cf7 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -100,6 +100,7 @@ void put_pid(struct pid *pid)
 
 	ns = pid->numbers[pid->level].ns;
 	if (refcount_dec_and_test(&pid->count)) {
+		WARN_ON_ONCE(pid->stashed);
 		kmem_cache_free(ns->pid_cachep, pid);
 		put_pid_ns(ns);
 	}
-- 
2.47.2

On Tue, May 06, 2025 at 01:55:54PM +0200, Christian Brauner wrote:
> Now that we have pidfs_{get,register}_pid() that needs to be paired with
> pidfs_put_pid() it's possible that someone pairs them with put_pid().
> Thus freeing struct pid while it's still used by pidfs. Notice when that
> happens. I'll also add a scheme to detect invalid uses of
> pidfs_get_pid() and pidfs_put_pid() later.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  kernel/pid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 26f1e136f017..8317bcbc7cf7 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -100,6 +100,7 @@ void put_pid(struct pid *pid)
>  
>  	ns = pid->numbers[pid->level].ns;
>  	if (refcount_dec_and_test(&pid->count)) {
> +		WARN_ON_ONCE(pid->stashed);
>  		kmem_cache_free(ns->pid_cachep, pid);
>  		put_pid_ns(ns);
>  	}
> -- 
> 2.47.2
> 

With the patch as proposed you are only catching the misuse if this is
the last ref though.

iow, the check should be hoisted above unrefing?

On Tue, May 06, 2025 at 04:43:56PM +0200, Mateusz Guzik wrote:
> On Tue, May 06, 2025 at 01:55:54PM +0200, Christian Brauner wrote:
> > Now that we have pidfs_{get,register}_pid() that needs to be paired with
> > pidfs_put_pid() it's possible that someone pairs them with put_pid().
> > Thus freeing struct pid while it's still used by pidfs. Notice when that
> > happens. I'll also add a scheme to detect invalid uses of
> > pidfs_get_pid() and pidfs_put_pid() later.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  kernel/pid.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 26f1e136f017..8317bcbc7cf7 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -100,6 +100,7 @@ void put_pid(struct pid *pid)
> >  
> >  	ns = pid->numbers[pid->level].ns;
> >  	if (refcount_dec_and_test(&pid->count)) {
> > +		WARN_ON_ONCE(pid->stashed);
> >  		kmem_cache_free(ns->pid_cachep, pid);
> >  		put_pid_ns(ns);
> >  	}
> > -- 
> > 2.47.2
> > 
> 
> With the patch as proposed you are only catching the misuse if this is
> the last ref though.
> 
> iow, the check should be hoisted above unrefing?

No, not really. If there's more than one reference then pid->stashed can
be legimitately != NULL.

I am traveling until May 15, can't read the code, but FWIW this change
looks good to me.

Oleg.

On 05/06, Christian Brauner wrote:
>
> Now that we have pidfs_{get,register}_pid() that needs to be paired with
> pidfs_put_pid() it's possible that someone pairs them with put_pid().
> Thus freeing struct pid while it's still used by pidfs. Notice when that
> happens. I'll also add a scheme to detect invalid uses of
> pidfs_get_pid() and pidfs_put_pid() later.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  kernel/pid.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 26f1e136f017..8317bcbc7cf7 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -100,6 +100,7 @@ void put_pid(struct pid *pid)
>  
>  	ns = pid->numbers[pid->level].ns;
>  	if (refcount_dec_and_test(&pid->count)) {
> +		WARN_ON_ONCE(pid->stashed);
>  		kmem_cache_free(ns->pid_cachep, pid);
>  		put_pid_ns(ns);
>  	}
> -- 
> 2.47.2
>