On Wed 07-12-22 19:39:54, yebin (H) wrote:
>
>
> On 2022/12/7 19:14, Jan Kara wrote:
> > On Wed 07-12-22 15:40:39, Ye Bin wrote:
> > > From: Ye Bin <yebin10@xxxxxxxxxx>
> > >
> > > Add primary check for extended attribute inode, only do hash check when read
> > > ea_inode's data in ext4_xattr_inode_get().
> > >
> > > Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>
> > ...
> >
> > > +static inline int ext4_xattr_check_extra_inode(struct inode *inode,
> > > + struct ext4_xattr_entry *entry)
> > > +{
> > > + int err;
> > > + struct inode *ea_inode;
> > > +
> > > + err = ext4_xattr_inode_iget(inode, le32_to_cpu(entry->e_value_inum),
> > > + le32_to_cpu(entry->e_hash), &ea_inode);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (i_size_read(ea_inode) != le32_to_cpu(entry->e_value_size)) {
> > > + ext4_warning_inode(ea_inode,
> > > + "ea_inode file size=%llu entry size=%u",
> > > + i_size_read(ea_inode),
> > > + le32_to_cpu(entry->e_value_size));
> > > + err = -EFSCORRUPTED;
> > > + }
> > > + iput(ea_inode);
> > > +
> > > + return err;
> > > +}
> > > +
> > > static int
> > > -ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> > > - void *value_start)
> > > +ext4_xattr_check_entries(struct inode *inode, struct ext4_xattr_entry *entry,
> > > + void *end, void *value_start)
> > > {
> > > struct ext4_xattr_entry *e = entry;
> > > @@ -221,6 +247,10 @@ ext4_xattr_check_entries(struct ext4_xattr_entry *entry, void *end,
> > > size > end - value ||
> > > EXT4_XATTR_SIZE(size) > end - value)
> > > return -EFSCORRUPTED;
> > > + } else if (entry->e_value_inum) {
> > > + int err = ext4_xattr_check_extra_inode(inode, entry);
> > > + if (err)
> > > + return err;
> > > }
> > > entry = EXT4_XATTR_NEXT(entry);
> > > }
> > So I was thinking about this. It is nice to have the inode references
> > checked but OTOH this is rather expensive for a filesystem with EA inodes -
> > we have to lookup and possibly load EA inodes from the disk although they
> > won't be needed for anything else than the check. Also as you have noticed
> > we do check whether i_size and xattr size as recorded in xattr entry match
> > in ext4_xattr_inode_iget() which gets called once we need to do anything
> > with the EA inode.
> >
> > Also I've checked and we do call ext4_xattr_check_block() and
> > xattr_check_inode() in ext4_expand_extra_isize_ea() so Ted's suspicion that
> > the problem comes from not checking the xattr entries before moving them
> > from the inode was not correct.
> >
> > So to summarize, I don't think this and the following patch is actually
> > needed and brings benefit that would outweight the performance cost.
> >
> > Honza
>
> Yes, I agree with you.
> In ext4_ xattr_ check_ Entries () simply verifies the length of the extended
> attribute with
> ea_inode. If the previous patch is not merged, EXT4_ XATTR_ SIZE_ MAX is
> much larger
> than the actual constraint value. Data verification can only be postponed
> until the ea_inode
> is read.
>
> So your suggestion is to modify EXT4_ XATTR_ SIZE_ MAX Or defer data
> verification until the ea_inode is read?
My suggestion would be to take patches 1,4,5,6 from your series. So reduce
EXT4_XATTR_SIZE_MAX (if Ted agrees), use kvmalloc() instead of kmalloc(),
do the cleanup of funtion names, and fix the inode refcount leak.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
