Message ID | 20221202124502.217462-1-alexander.atanasov@virtuozzo.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp827091wrr; Fri, 2 Dec 2022 04:54:58 -0800 (PST) X-Google-Smtp-Source: AA0mqf7nGgzRrk/QB3N4t02u2lyMGQxq9jOVT/vPYW4yrYsgGuMtiqzaRRuJXDaMzn0tZ6WDWHnc X-Received: by 2002:a17:90a:fc92:b0:219:1545:bc57 with SMTP id ci18-20020a17090afc9200b002191545bc57mr8448682pjb.133.1669985697885; Fri, 02 Dec 2022 04:54:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669985697; cv=none; d=google.com; s=arc-20160816; b=f58nDtSr0kInj+xFUG8u0mrkUJM25h9BoCJ4IjhXTpFZfjciMlXCFzS+u/uGiaq6LY NYWXBw2+/nPLyyVTbe/niGhuvbyNXRvbUXQVz1koBkWOX6f1e51q2x6IRUtHIEk+bLYk UMVmEUQMx2bUgqzUJAp6PhA32teQ2T+I7SO9KL7F9aXHMvSC+XDEiXJaMtzrNyC8qpQO Vw77oBM5DqCDPCpYWxJcVSvX3eA27XQZUplMOS/W7oIBE5dkjvGnQAfc2wHiT1glN19Q q/IfkwANNFF86smXwu8dbxEoYA2rNDuabpFD0ERL7BK5R9KDyZPaZZW3jcWwGV97hBkN wcyw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=b2IRnrm+I2gOA6t6Is3guuC27DUZk9YV3rbhZ/DI6B0=; b=mw+Ru/yqu23uAD5wB3E6vJ7NAIM2sjC1x0pEhKT1HKK3FZBy2YqICTcU18T1jD58J1 gCxzvJscOTsJxDs0u6FwMeNsEYzsIYsSQyml84uG+ogt6iGN8wVut9nsbkuCVPIQ2tEz 50ViRx0WMeE8PB5z7P0uS5bS0GHiuDPH5LY4u7Nb/ZTiKKYMij6Amvs03cFtcCro+HAR Q8wTLvilg1TEAT4MaXfG49L4sM+GUfN0PVla2YrNg+D5+f+SmSHfq4ZVQ8xFCJwdMedB PgQ2r6+kghEOyiK6QS2VuQu4eiuABXKfCoWkJl9j501W04TyEnMZXsrHrgQn7SxT1xAc yjqQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=virtuozzo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id cm9-20020a056a00338900b00572ef957b35si7032695pfb.210.2022.12.02.04.54.44; Fri, 02 Dec 2022 04:54:57 -0800 (PST) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=virtuozzo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233557AbiLBMpl (ORCPT <rfc822;lhua1029@gmail.com> + 99 others); Fri, 2 Dec 2022 07:45:41 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42618 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232801AbiLBMpj (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 2 Dec 2022 07:45:39 -0500 Received: from relay.virtuozzo.com (relay.virtuozzo.com [130.117.225.111]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 216F5DCBD9 for <linux-kernel@vger.kernel.org>; Fri, 2 Dec 2022 04:45:34 -0800 (PST) Received: from dev011.ch-qa.sw.ru ([172.29.1.16]) by relay.virtuozzo.com with esmtp (Exim 4.95) (envelope-from <alexander.atanasov@virtuozzo.com>) id 1p15Pp-005MRe-O6; Fri, 02 Dec 2022 13:45:17 +0100 From: Alexander Atanasov <alexander.atanasov@virtuozzo.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org> Cc: kernel@openvz.org, Alexander Atanasov <alexander.atanasov@virtuozzo.com>, linux-kernel@vger.kernel.org Subject: [PATCH] devtmpfs: move NULLing the thread pointer before unregistering fs Date: Fri, 2 Dec 2022 14:45:01 +0200 Message-Id: <20221202124502.217462-1-alexander.atanasov@virtuozzo.com> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_NONE autolearn=ham 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1751106923148596508?= X-GMAIL-MSGID: =?utf-8?q?1751106923148596508?= |
Series |
devtmpfs: move NULLing the thread pointer before unregistering fs
|
|
Commit Message
Alexander Atanasov
Dec. 2, 2022, 12:45 p.m. UTC
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(-)
base-commit: b7b275e60bcd5f89771e865a8239325f86d9927d
Comments
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
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.
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; }