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

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

[Alexander Atanasov]

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.
Improve the fix to clear the pointer before unregistration to close
the window  where the dangling pointer can be potentially used.
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;
 	}
 

base-commit: b7b275e60bcd5f89771e865a8239325f86d9927d
-- 
2.31.1

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

[Greg Kroah-Hartman]

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?

> 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 :(

> 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?

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

thanks,

greg k-h

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@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