Re: [PATCH v5 3/4] fs: handle potential filesystems which use I_DONTCACHE and drop the lock in ->drop_inode

[Mateusz Guzik]

On Wed, Apr 1, 2026 at 7:45 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Tue 31-03-26 18:08:50, Mateusz Guzik wrote:
> > f2fs and ntfs play games where they transitioning the refcount 0->1 and release
>
> I don't see ntfs providing .drop_inode at all in current Linu's kernel? The
> f2fs case is indeed ugly and a gross hack. But I don't see easy way how to
> fix that for now.
>

This was generated against next-20260327, then:
fs/ntfs/super.c:        .drop_inode     = ntfs_drop_big_inode,

[snip]
                                 /* To avoid evict_inode call
simultaneously */
                                 atomic_inc(&inode->i_count);
                                 spin_unlock(&inode->i_lock);

> > the inode spinlock, allowing other threads to grab a ref of their own.
> >
> > They also return 0 in that case, making this problem harmless.
> >
> > However, should they start using the I_DONTCACHE machinery down the road
> > while retaining the above, iput_final() will get a race where it can
> > proceed to teardown an inode with references.
> >
> > The easiest way out for the time being is to future-proof it by
> > predicating caching on the count.
> >
> > Developing better ->drop_inode semantics and sanitizing all users is
> > left as en exercise for the reader.
> >
> > Signed-off-by: Mateusz Guzik <mjguzik@xxxxxxxxx>
> > ---
> >  fs/inode.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index e397a4b56671..013470e6d144 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -1935,20 +1935,29 @@ static void iput_final(struct inode *inode)
> >       else
> >               drop = inode_generic_drop(inode);
> >
> > -     if (!drop &&
> > -         !(inode_state_read(inode) & I_DONTCACHE) &&
> > -         (sb->s_flags & SB_ACTIVE)) {
> > +     /*
> > +      * FIXME: there are ->drop_inode hooks playing nasty games releasing the
> > +      * spinlock and temporarily grabbing refs, in turn opening a possibility
> > +      * someone else will sneak in and grab a ref while it happens.
> > +      *
> > +      * If such a hook returns 0 (== don't drop) it ends being up harmless as long
> > +      * as the inode is not marked with I_DONTCACHE. Otherwise we are proceeding
> > +      * with teardown despite references being present.
> > +      *
> > +      * Damage-control the problem by including the count in the decision. However,
> > +      * assert no refs showed up if the hook decided to drop the inode.
> > +      */
>
> Would be better to format the comment to fit 80 columns...
>
> > +     if (drop)
> > +             VFS_BUG_ON_INODE(icount_read(inode) != 0, inode);
> > +
> > +     if (unlikely(icount_read(inode) > 0) ||
> > +         (!drop && !(inode_state_read(inode) & I_DONTCACHE) &&
> > +         (sb->s_flags & SB_ACTIVE))) {
>
> Any reason why you want to gracefully handle the buggy filesystem code? If
> a filesystem playing games with .drop_inode starts using I_DONTCACHE, it
> will BUG_ON below which is good enough I'd say. I don't think we need to
> handle buggy fs drivers any better...
>

That depends on what you consider buggy. is what they are doing now a
bug or at least "not supported"?

*technically* these is no bug at the moment in that both filesystems
don't want to whack the inode in this case.

Ultimately the goal here is to catch the problematic combination early
instead of getting weird crashes later.

That said, I have no strong opinion on handling this vs straight up BUG() out.