[PATCH] kernfs: Relax constraint in draining guard

[PATCH] kernfs: Relax constraint in draining guard

[Michal Koutný]

The active reference lifecycle provides the break/unbreak mechanism but
the active reference is not truly active after unbreak -- callers don't
use it afterwards but it's important for proper pairing of kn->active
counting. Assuming this mechanism is in place, the WARN check in
kernfs_should_drain_open_files() is too sensitive -- it may transiently
catch those (rightful) callers between
kernfs_unbreak_active_protection() and kernfs_put_active() as found out by Chen
Ridong:

	kernfs_remove_by_name_ns	kernfs_get_active // active=1
	__kernfs_remove					  // active=0x80000002
	kernfs_drain			...
	wait_event
	//waiting (active == 0x80000001)
					kernfs_break_active_protection
					// active = 0x80000001
	// continue
					kernfs_unbreak_active_protection
					// active = 0x80000002
	...
	kernfs_should_drain_open_files
	// warning occurs
					kernfs_put_active

To avoid the false positives (mind panic_on_warn) remove the check altogether.
(This is meant as quick fix, I think active reference break/unbreak may be
simplified with larger rework.)

Fixes: bdb2fd7fc56e1 ("kernfs: Skip kernfs_drain_open_files() more aggressively")
Link: https://lore.kernel.org/r/kmmrseckjctb4gxcx2rdminrjnq2b4ipf7562nvfd432ld5v5m@2byj5eedkb2o/

Cc: Chen Ridong <chenridong@huawei.com>
Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
 fs/kernfs/dir.c  | 5 +++--
 fs/kernfs/file.c | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index fc70d72c3fe80..43487fa83eaea 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1580,8 +1580,9 @@ void kernfs_break_active_protection(struct kernfs_node *kn)
  * invoked before finishing the kernfs operation.  Note that while this
  * function restores the active reference, it doesn't and can't actually
  * restore the active protection - @kn may already or be in the process of
- * being removed.  Once kernfs_break_active_protection() is invoked, that
- * protection is irreversibly gone for the kernfs operation instance.
+ * being drained and removed.  Once kernfs_break_active_protection() is
+ * invoked, that protection is irreversibly gone for the kernfs operation
+ * instance.
  *
  * While this function may be called at any point after
  * kernfs_break_active_protection() is invoked, its most useful location
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 66fe8fe41f060..a6c692cac6165 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -778,8 +778,9 @@ bool kernfs_should_drain_open_files(struct kernfs_node *kn)
 	/*
 	 * @kn being deactivated guarantees that @kn->attr.open can't change
 	 * beneath us making the lockless test below safe.
+	 * Callers post kernfs_unbreak_active_protection may be counted in
+	 * kn->active by now, do not WARN_ON because of them.
 	 */
-	WARN_ON_ONCE(atomic_read(&kn->active) != KN_DEACTIVATED_BIAS);
 
 	rcu_read_lock();
 	on = rcu_dereference(kn->attr.open);
-- 
2.49.0

Re: [PATCH] kernfs: Relax constraint in draining guard

[Tejun Heo]

On Mon, May 05, 2025 at 02:12:00PM +0200, Michal Koutný wrote:
> The active reference lifecycle provides the break/unbreak mechanism but
> the active reference is not truly active after unbreak -- callers don't
> use it afterwards but it's important for proper pairing of kn->active
> counting. Assuming this mechanism is in place, the WARN check in
> kernfs_should_drain_open_files() is too sensitive -- it may transiently
> catch those (rightful) callers between
> kernfs_unbreak_active_protection() and kernfs_put_active() as found out by Chen
> Ridong:
> 
> 	kernfs_remove_by_name_ns	kernfs_get_active // active=1
> 	__kernfs_remove					  // active=0x80000002
> 	kernfs_drain			...
> 	wait_event
> 	//waiting (active == 0x80000001)
> 					kernfs_break_active_protection
> 					// active = 0x80000001
> 	// continue
> 					kernfs_unbreak_active_protection
> 					// active = 0x80000002
> 	...
> 	kernfs_should_drain_open_files
> 	// warning occurs
> 					kernfs_put_active
> 
> To avoid the false positives (mind panic_on_warn) remove the check altogether.
> (This is meant as quick fix, I think active reference break/unbreak may be
> simplified with larger rework.)
> 
> Fixes: bdb2fd7fc56e1 ("kernfs: Skip kernfs_drain_open_files() more aggressively")
> Link: https://lore.kernel.org/r/kmmrseckjctb4gxcx2rdminrjnq2b4ipf7562nvfd432ld5v5m@2byj5eedkb2o/
> 
> Cc: Chen Ridong <chenridong@huawei.com>
> Signed-off-by: Michal Koutný <mkoutny@suse.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] kernfs: Relax constraint in draining guard

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 192 bytes --]

On Mon, May 05, 2025 at 01:28:22PM -1000, Tejun Heo <tj@kernel.org> wrote:
...
> Acked-by: Tejun Heo <tj@kernel.org>

Is it correct to expect Greg picking it through his repo?

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] kernfs: Relax constraint in draining guard

[Tejun Heo]

On Tue, May 13, 2025 at 11:06:40AM +0200, Michal Koutný wrote:
> On Mon, May 05, 2025 at 01:28:22PM -1000, Tejun Heo <tj@kernel.org> wrote:
> ...
> > Acked-by: Tejun Heo <tj@kernel.org>
> 
> Is it correct to expect Greg picking it through his repo?

Yes, I think so.

Thanks.

-- 
tejun

Re: [PATCH] kernfs: Relax constraint in draining guard

[Greg Kroah-Hartman]

On Tue, May 13, 2025 at 11:06:40AM +0200, Michal Koutný wrote:
> On Mon, May 05, 2025 at 01:28:22PM -1000, Tejun Heo <tj@kernel.org> wrote:
> ...
> > Acked-by: Tejun Heo <tj@kernel.org>
> 
> Is it correct to expect Greg picking it through his repo?

Yes, I will take this when I catch up with pending patches, thanks.

greg k-h