Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e676694f-6f6e-661d-d776-75d357ba510d@virtuozzo.com>
Date:   Fri, 2 Dec 2022 21:49:03 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.5.0
Subject: Re: [PATCH] devtmpfs: move NULLing the thread pointer before
 unregistering fs
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>, kernel@openvz.org,
        linux-kernel@vger.kernel.org
References: <20221202124502.217462-1-alexander.atanasov@virtuozzo.com>
 <Y4ogFSOaQtZ+5Xqy@kroah.com>
From:   Alexander Atanasov <alexander.atanasov@virtuozzo.com>
In-Reply-To: <Y4ogFSOaQtZ+5Xqy@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bFR3bm9aYi9rL2J0K1FYeWZzRlhXckhXMXBuWnFHR0NGaVU5aUhGSjZ2eldZ?=
 =?utf-8?B?ZGxzejE3M2ZaOW5JdFdhc2FKVVlka0toRjVlUHR4NjByOGlnc1pScTN0SnZ3?=
 =?utf-8?B?b1Nzb0ZiaXRNU2p0aHBGeTFaUlZKN1F2Tm51VkR2cS9Ud3hLangzQzlNbVkz?=
 =?utf-8?B?Q1NlS05RWEdrNFhpZUtub29RTTJ3TkZRVzFxR1dBSnh6T3VpVlI1amxtRlh3?=
 =?utf-8?B?d040aWZYQWhEWHdWYVVnZGlvRGkrT3BUOHErVHptKy9qQzdzb016UjQ0U2Yy?=
 =?utf-8?B?WTR4eWhQL1ZOWGNWZ2MwZlNDbW10VEF1SENYNWd6YXEvTGl0eHZtVk1TdFNz?=
 =?utf-8?B?K1VLVS9wb3I5cC9qNWtjc1lYMnZxd3Q0cS9GbjdMVjdiNzlPdW9VdGppRWxz?=
 =?utf-8?B?NndYRFFJTkZLUTdHb2Y0aFJqbzFLL1JvaWVaWU4reGltSjFwNGp5dE1RdUZi?=
 =?utf-8?B?dVpFUENkN1RScFE5Rk1mTThWeVJtTzNlV2tCTU5VWU84ZEUzQzJ6VW95UWxM?=
 =?utf-8?B?S3FxUEdiUGZTTGtnM1lxajFmVnJHaFBtS2VNM2JZV2FkMnNhb2JpRk5yMFVu?=
 =?utf-8?B?eXVWZExXRi9ZUUhGZDlnSmdMQXpHS3hVclh0WXFQL0d2VU1YMWxHdnJ0TXlm?=
 =?utf-8?B?bWtZSlJEa2dKa0R3bmhMd3hmUk9LM3VZRlZidXRsbHVuTU12d3lZWnVOU3VE?=
 =?utf-8?B?TlVlTnFPVVFTRkNNNFJ3aUJoK2tNeFZRWWtwK0s4S05QcFdhUnlSSFcvOEVI?=
 =?utf-8?B?a0c2YnBLUXo3dWxYeCs5MVdCTGQrZzRXd3pxTXA0aFZjdHVleFliUkxRbWNQ?=
 =?utf-8?B?aldQTDdkS3cwTjFKSDY3L0gyNU9XMXVqNXRVTGZML2ZSMENWZ1JicGZiSHN5?=
 =?utf-8?B?aTVNMkV4cTJGUVE2UmhkakF2VERmV2FkYkJ5OUdhZ21CWDE1OTNpb0JZb1ps?=
 =?utf-8?B?Q3ZubUJ4VUlyVzlRdE1DK2tPSWl6SEZkUFMyenkwQ1hJOTBQaS8wZjFPd2hR?=
 =?utf-8?B?QUxmWTJJd2MwY2Y3bk0rb0RLMTM5V0dVenZjYy85QWh3VUM3cTdNcldBZHY5?=
 =?utf-8?B?c21yYXdXc3dId0NzSHBLS1pSbEZsVDJvRm81NmVXMmQvZC9nYnRXMUJvaHZh?=
 =?utf-8?B?QmFDNUdPOVI1RGVuNllOS1dGK0g4c2dHcmEwKytOSG84aUgwaFkvRGJxVUJ3?=
 =?utf-8?B?VkZ3MnJZRkxacnp6SUt0L0lJQnE5bm9wdzVyeUJCMGY5clFRZ3g1YlpWbThL?=
 =?utf-8?B?UlB6Vk5ZLzREYmNOVzhZY01ZaDNkeGZTOUl0YUoxR0hQcnBZRWQyMnVQaldt?=
 =?utf-8?B?TGh3dHVpeTZDWTF0Zk5RbXBadnBBYmZha0JtNHRuYzF3dHdrYnVRVHMrMnVP?=
 =?utf-8?B?Q3hIb1EwUE1pd0QyelR3QUpCdThOK3JZZUVlenFuNHdRRElRWlUxNUNwNHFK?=
 =?utf-8?B?U2J4LzJUUnNwNW1ucmxRb2hQQ2QxR1M2RlFVdWZJSFpYNXZEQkJFaE5XTktq?=
 =?utf-8?B?MlUzamZBQzdscnlpV2RTQ05pTlJsbG8vOHVVb1NRT252d1U2cko2SUxRcFhF?=
 =?utf-8?B?dXJzdWlyMmEzK3M5RkUyMG9LU2oyeUtTVkxwUmNvU2ZRbXNmY3k4L3NBcXAr?=
 =?utf-8?B?UnlIOWhWcGZSQ1puVE42dHduM21CWWRDRGgzMUoxTEZ1cTdDMFV4THRDRU9w?=
 =?utf-8?B?S3Y1RXQxN3FpK2RrU2x1ZFpuOUhDWTdyK1pFSWxuaEtkelp4UHVsZDZRcktp?=
 =?utf-8?B?N0ZtQmdTOWRIaWNVNWVDRzB5VTYwT0tVSzJudTdsN2VPTHR2czF3MFBnaHJ1?=
 =?utf-8?B?TVJTMkxIa3pMV01ZUFBjQmJldlcxTFVKeFZTS3dmbld2U1hUb1k4c2FZRElP?=
 =?utf-8?B?eklDR2o3NVJ3cUNvU2d0YU1KRGw0TE5BU1BTenQ4SWlabnFxSGNPenpqMDVt?=
 =?utf-8?B?TDExSm1OT0k0WDVwc3RTTy9pRUpmbkRpemVXY2dsYmV6ODVhcE9MSSt4N0t5?=
 =?utf-8?B?ZEMwd0JmRmlOL3lyemZwR0VWV0h3QjM4dTk0b01nanFYaWR5MEF4WVhxR0li?=
 =?utf-8?B?UGxyZXpJN1U3WkZwZTdaVUpacmxHWWFzaWduMGh4K1lscUVkYjdyZmtjakV4?=
 =?utf-8?B?MDJRMnpRejlTdG5vbndYZzNsSys3V01VSHh2ZkZnVU9sYTVBNnBoYUZlbU9w?=
 =?utf-8?B?bWc9PQ==?=
X-OriginatorOrg: virtuozzo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a3db7642-0e09-4818-5542-08dad49e4466
X-MS-Exchange-CrossTenant-AuthSource: VE1PR08MB4765.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Dec 2022 19:49:04.5387
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: GUlqvNWknWmPa92xjMiJWwAjaOmtLPkVMWrezKpUuLCzBqB0J86gcd2Y6iB9t037IPe6bDl/scDD+9EdHCtnJ/Q/03FeE2N2x8YTMXreVBr0lnJCtjyrvSx87qLodevG
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR08MB6286
X-Spam-Status: No, score=-1.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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