diff mbox series

debugfs: initialize cancellations earlier

Message ID 20231221150444.1e47a0377f80.If7e8ba721ba2956f12c6e8405e7d61e154aa7ae7@changeid
State New
Headers
Series debugfs: initialize cancellations earlier |

Commit Message

Johannes Berg Dec. 21, 2023, 2:04 p.m. UTC 
  From: Johannes Berg <johannes.berg@intel.com>

Tetsuo Handa pointed out that in the (now reverted)
lockdep commit I initialized the data too late. The
same is true for the cancellation data, it must be
initialized before the cmpxchg(), otherwise it may
be done twice and possibly even overwriting data in
there already when there's a race. Fix that, which
also requires destroying the mutex in case we lost
the race.

Fixes: 8c88a474357e ("debugfs: add API to allow debugfs operations cancellation")
Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 fs/debugfs/file.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Greg KH Dec. 21, 2023, 5:05 p.m. UTC | #1 
On Thu, Dec 21, 2023 at 03:04:45PM +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Tetsuo Handa pointed out that in the (now reverted)
> lockdep commit I initialized the data too late.

As the patch isn't in any tree, what is this against?

confused,

greg k-h
Johannes Berg Dec. 21, 2023, 5:10 p.m. UTC | #2 
On Thu, 2023-12-21 at 18:05 +0100, Greg Kroah-Hartman wrote:
> On Thu, Dec 21, 2023 at 03:04:45PM +0100, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > Tetsuo Handa pointed out that in the (now reverted)
> > lockdep commit I initialized the data too late.
> 
> As the patch isn't in any tree, what is this against?

Hm? You mean the lockdep patch? It's not relevant, but I then
continued and wrote:

> > The same is true for the cancellation data, [...]

and then the patch goes and changes the cancellation data
initialization?

Or do you mean the patch mentioned in the fixes?

> > Fixes: 8c88a474357e ("debugfs: add API to allow debugfs operations cancellation")

That *is* in Linus's tree, as of -rc4.

Not sure I understand the question.

johannes
Greg KH Dec. 21, 2023, 5:17 p.m. UTC | #3 
On Thu, Dec 21, 2023 at 06:10:17PM +0100, Johannes Berg wrote:
> On Thu, 2023-12-21 at 18:05 +0100, Greg Kroah-Hartman wrote:
> > On Thu, Dec 21, 2023 at 03:04:45PM +0100, Johannes Berg wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > Tetsuo Handa pointed out that in the (now reverted)
> > > lockdep commit I initialized the data too late.
> > 
> > As the patch isn't in any tree, what is this against?
> 
> Hm? You mean the lockdep patch? It's not relevant, but I then
> continued and wrote:
> 
> > > The same is true for the cancellation data, [...]
> 
> and then the patch goes and changes the cancellation data
> initialization?
> 
> Or do you mean the patch mentioned in the fixes?
> 
> > > Fixes: 8c88a474357e ("debugfs: add API to allow debugfs operations cancellation")
> 
> That *is* in Linus's tree, as of -rc4.
> 
> Not sure I understand the question.

But this doesn't apply against Linus's tree, or my driver-core-next
branch now, where should it go?

still confused,

greg k-h
Johannes Berg Dec. 21, 2023, 6:55 p.m. UTC | #4 
On Thu, 2023-12-21 at 18:17 +0100, Greg Kroah-Hartman wrote:
> 
> But this doesn't apply against Linus's tree, 

Hmm. It does for me?

$ git checkout linux/master
...
$ curl -s https://lore.kernel.org/lkml/20231221150444.1e47a0377f80.If7e8ba721ba2956f12c6e8405e7d61e154aa7ae7@changeid/raw | git am -
Applying: debugfs: initialize cancellations earlier
$ 

> or my driver-core-next branch now,

Right, it doesn't apply there, that was branched out from v6.7-rc3.

> where should it go?

I think/hope to 6.7 still, since it fixes something that only got there.

Looks like you routed that other debugfs patch (the lockdep revert I was
talking about) through char-misc?

johannes
Greg KH Dec. 21, 2023, 7:43 p.m. UTC | #5 
On Thu, Dec 21, 2023 at 07:55:13PM +0100, Johannes Berg wrote:
> On Thu, 2023-12-21 at 18:17 +0100, Greg Kroah-Hartman wrote:
> > 
> > But this doesn't apply against Linus's tree, 
> 
> Hmm. It does for me?
> 
> $ git checkout linux/master
> ...
> $ curl -s https://lore.kernel.org/lkml/20231221150444.1e47a0377f80.If7e8ba721ba2956f12c6e8405e7d61e154aa7ae7@changeid/raw | git am -
> Applying: debugfs: initialize cancellations earlier
> $ 
> 
> > or my driver-core-next branch now,
> 
> Right, it doesn't apply there, that was branched out from v6.7-rc3.

Ah, yes.

> > where should it go?
> 
> I think/hope to 6.7 still, since it fixes something that only got there.
> 
> Looks like you routed that other debugfs patch (the lockdep revert I was
> talking about) through char-misc?

I did, sorry for the confusion, too many branches/trees...

I'll queue this up in the morning, thanks.

greg k-h
diff mbox series

Patch

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 5063434be0fc..6d7c1a49581f 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -104,12 +104,14 @@  int debugfs_file_get(struct dentry *dentry)
 					~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
 		refcount_set(&fsd->active_users, 1);
 		init_completion(&fsd->active_users_drained);
+		INIT_LIST_HEAD(&fsd->cancellations);
+		mutex_init(&fsd->cancellations_mtx);
+
 		if (cmpxchg(&dentry->d_fsdata, d_fsd, fsd) != d_fsd) {
+			mutex_destroy(&fsd->cancellations_mtx);
 			kfree(fsd);
 			fsd = READ_ONCE(dentry->d_fsdata);
 		}
-		INIT_LIST_HEAD(&fsd->cancellations);
-		mutex_init(&fsd->cancellations_mtx);
 	}
 
 	/*