Re: [PATCH] devtmpfs: move NULLing the thread pointer before unregistering fs

[Alexander Atanasov]

On 2.12.22 17:56, Greg Kroah-Hartman wrote:
> On Fri, Dec 02, 2022 at 02:45:01PM +0200, Alexander Atanasov wrote:
> > In commit
> > 31c779f293b3 ("devtmpfs: fix the dangling pointer of global devtmpfsd thread")
> > a dangling pointer on an error condition was fixed. But the fix
> > left the dangling pointer during unregister_filesystem and printk calls.
>
> And how could it be used there?

I don't said it can be used there - they might trigger events that get 
back to it.

> > Improve the fix to clear the pointer before unregistration to close
> > the window  where the dangling pointer can be potentially used.
>
> Again, how can that happen?  And you have an extra ' ' in that line :(

Sorry for the extra ' ' i will check where it came from.

> > Make it clear the pointer at only one place in the function.
> >
> > Signed-off-by: Alexander Atanasov <alexander.atanasov@xxxxxxxxxxxxx>
> > ---
> >   drivers/base/devtmpfs.c | 8 +++-----
> >   1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> > index e4bffeabf344..773e66ef5642 100644
> > --- a/drivers/base/devtmpfs.c
> > +++ b/drivers/base/devtmpfs.c
> > @@ -472,17 +472,15 @@ int __init devtmpfs_init(void)
> >   	}
> >   
> >   	thread = kthread_run(devtmpfsd, &err, "kdevtmpfs");
> > -	if (!IS_ERR(thread)) {
> > +	if (!IS_ERR(thread))
> >   		wait_for_completion(&setup_done);
> > -	} else {
> > +	else
> >   		err = PTR_ERR(thread);
> > -		thread = NULL;
> > -	}
> >   
> >   	if (err) {
> > +		thread = NULL;
> >   		printk(KERN_ERR "devtmpfs: unable to create devtmpfs %i\n", err);
> >   		unregister_filesystem(&dev_fs_type);
> > -		thread = NULL;
> >   		return err;
> >   	}
>
> This all feels wrong and way too complex to have to clean up from a call
> to kthread_run().  Are you sure this is the correct way to do this?

Agree on this but this is the code as it is.

> And how was this "issue" found?  How does the call to kthread_run() ever
> fail for you?

I was after something else and this stuck into my eye:
....
        thread = kthread_run(devtmpfsd, &err, "kdevtmpfs");
        if (!IS_ERR(thread)) {
                wait_for_completion(&setup_done);
        } else {
                err = PTR_ERR(thread);
                thread = NULL;
        }

        if (err) {
                printk(KERN_ERR "devtmpfs: unable to create devtmpfs 
%i\n", err);
                unregister_filesystem(&dev_fs_type);
                thread = NULL;
                return err;
        }
....

Why do we do thread = NULL twice ? One time before unregistration, one 
time after unregistration.

So if it is going to handle the error the same way as the kthread_run 
error (original) then when the thread completes with error we must do 
the same. And do it one time.

...
        thread = kthread_run(devtmpfsd, &err, "kdevtmpfs");
        if (!IS_ERR(thread))
                wait_for_completion(&setup_done);
        else
                err = PTR_ERR(thread);

        if (err) {
                thread = NULL;
                printk(KERN_ERR "devtmpfs: unable to create devtmpfs 
%i\n", err);
                unregister_filesystem(&dev_fs_type);
                return err;
        }
...

Which is more readable ?

I guess  I should have put this as the commit message.

--
Regards,
Alexander Atanasov