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

Alexander Atanasov
2022-12-02T19:49:20Z

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@virtuozzo.com>
>> ---
>> 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