Re: [PATCH v4] ext4: validate EA inode i_nlink in ext4_xattr_inode_iget

[Jan Kara]

On Thu 18-06-26 09:02:10, Zhou, Yun wrote:
> 
> 
> On 6/18/26 04:25, Jan Kara wrote:
> > On Mon 15-06-26 13:35:12, Yun Zhou wrote:
> > 
> > --- a/fs/ext4/xattr.c
> > +++ b/fs/ext4/xattr.c
> > @@ -464,6 +464,33 @@ static int ext4_xattr_inode_iget(struct inode *parent, unsigned long ea_ino,
> >                inode_unlock(inode);
> >        }
> > 
> > +     /*
> > +      * Since this function resolves references from active xattr entries,
> > +      * the EA inode must be in active state (i_nlink=1, ref_count>0).
> > +      * i_nlink > 1, i_nlink == 0 (dangling reference), or ref_count == 0
> > +      * (inconsistent with an active entry) all indicate corruption or
> > +      * a concurrent last-reference drop.
> > +      */
> > +     if (inode->i_nlink != 1 || !ext4_xattr_inode_get_ref(inode)) {
> > +             ext4_error(parent->i_sb,
> > +                        "EA inode %lu has unexpected i_nlink=%u ref_count=%llu",
> > +                        ea_ino, inode->i_nlink,
> > +                        ext4_xattr_inode_get_ref(inode));
> > Hum, given motivation of this is syzbot corrupted fs image, I'd just put
> > check in ext4_iget() verifying ext4_xattr_inode_get_ref() is > 0 and be
> > done with it. Much simpler and catches at least the obvious cases. The
> > consistency of xattr refcount and i_nlink is otherwise guarded by
> > ext4_xattr_inode_update_ref() and it can never be perfect as in the kernel
> > we don't have the full view of the filesystem and so cannot ascertain that
> > xattr ref count matches reality...
> > 
> Thanks very much for your review and suggestions, The issue should be easily
> fixed after "introduce ext4_put_ea_inode() for safe deferred iput" merged.
> Just need to defer iput() and no longer to make inode bad.

OK, just to make it clear, calling make_bad_inode() is never a right thing
to do once the inode has been exposed to other users. It is only usable
within iget functions when loading inode into memory and finding out the
content isn't valid. This function is one of the traps we have in VFS...

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR