[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <63907B8A.9030800@huawei.com>
Date: Wed, 7 Dec 2022 19:39:54 +0800
From: "yebin (H)" <yebin10@...wei.com>
To: Jan Kara <jack@...e.cz>, Ye Bin <yebin@...weicloud.com>
CC: <tytso@....edu>, <adilger.kernel@...ger.ca>,
<linux-ext4@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/6] ext4: add primary check extended attribute inode
in ext4_xattr_check_entries()
On 2022/12/7 19:14, Jan Kara wrote:
> On Wed 07-12-22 15:40:39, Ye Bin wrote:
>> From: Ye Bin <yebin10@...wei.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@...wei.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?
Powered by blists - more mailing lists