On Tue, 2024-02-27 at 06:51 -0800, Ben Greear wrote:
> On 2/27/24 06:32, Johannes Berg wrote:
> > On Tue, 2024-02-27 at 06:29 -0800, Ben Greear wrote:
> > > > --- a/fs/debugfs/inode.c
> > > > +++ b/fs/debugfs/inode.c
> > > > @@ -751,13 +751,19 @@ static void __debugfs_file_removed(struct dentry *dentry)
> > > > if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
> > > > return;
> > > >
> > > > - /* if we hit zero, just wait for all to finish */
> > > > - if (!refcount_dec_and_test(&fsd->active_users)) {
> > > > - wait_for_completion(&fsd->active_users_drained);
> > > > - return;
> > > > - }
> > > > + /*
> > > > + * Now that debugfs_file_get() no longer sees a valid entry,
> > > > + * decrement the refcount to remove the initial reference.
> > > > + */
> > > > + refcount_dec(&fsd->active_users);
> >
> > >
> > > [ 94.576688] ------------[ cut here ]------------
> > > [ 94.576699] refcount_t: decrement hit 0; leaking memory.
> > >
> >
> > Ah ... right, refcount_dec() doesn't like to hit 0, it's not meant for
> > this path.
> >
> > I guess we can
> >
> > if (refcount_dec_and_test(...))
> > return;
> >
> > while (refcount_read(...)) { ... }
> >
> > johannes
> >
>
> Like this?
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 034a617cb1a5..166053095610 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -751,13 +751,20 @@ static void __debugfs_file_removed(struct dentry *dentry)
> if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
> return;
>
> - /* if we hit zero, just wait for all to finish */
> - if (!refcount_dec_and_test(&fsd->active_users)) {
> - wait_for_completion(&fsd->active_users_drained);
> + /*
> + * Now that debugfs_file_get() no longer sees a valid entry,
> + * decrement the refcount to remove the initial reference.
> + */
> + if (!refcount_dec_and_test(&fsd->active_users))
> return;
> - }
>
Which, btw, ignoring comments, braces, whitespace - then really just
removes the line you're getting stuck on.
So actually no ... invert the test?
if (refcount_dec_and_test(...))
return;
If it hit zero here, there's guaranteed to be no user, so we can return.
If it's not zero yet, we might yet go into a new cancellation, so we
need the rest of the function.
johannes
