Messages in this thread | | | Subject | Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode in ext4_xattr_check_entries() | From | "yebin (H)" <> | Date | Wed, 7 Dec 2022 19:39:54 +0800 |
| |
On 2022/12/7 19:14, Jan Kara wrote: > On Wed 07-12-22 15:40:39, Ye Bin wrote: >> From: Ye Bin <yebin10@huawei.com> >> >> 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@huawei.com> > ... > >> +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?
| |