linux-ext4 - Re: [PATCH v2 09/10] ext4: factor out the common checking part of all fallocate operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <88bf7d21-2058-47fd-99f8-4266f87fcfd9@huaweicloud.com>
Date: Tue, 24 Sep 2024 15:52:53 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
 ritesh.list@...il.com, yi.zhang@...wei.com, chengzhihao1@...wei.com,
 yukuai3@...wei.com
Subject: Re: [PATCH v2 09/10] ext4: factor out the common checking part of all
 fallocate operations

On 2024/9/23 16:31, Jan Kara wrote:
> On Wed 04-09-24 14:29:24, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@...wei.com>
>>
>> Now the beginning of all the five functions in ext4_fallocate() (punch
>> hole, zero range, insert range, collapse range and normal fallocate) are
>> almost the same, they need to hold i_rwsem and check the validity of
>> input parameters, so move the holding of i_rwsem to ext4_fallocate()
>> and factor out a common helper to check the input parameters can make
>> the code more clear.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> ...
>> +static int ext4_fallocate_check(struct inode *inode, int mode,
>> +				loff_t offset, loff_t len)
>> +{
>> +	/* Currently except punch_hole, just for extent based files. */
>> +	if (!(mode & FALLOC_FL_PUNCH_HOLE) &&
>> +	    !ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
>> +		return -EOPNOTSUPP;
>> +
>> +	/*
>> +	 * Insert range and collapse range works only on fs cluster size
>> +	 * aligned regions.
>> +	 */
>> +	if (mode & (FALLOC_FL_INSERT_RANGE | FALLOC_FL_COLLAPSE_RANGE) &&
>> +	    !IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(inode->i_sb)))
>> +		return -EINVAL;
>> +
>> +	if (mode & FALLOC_FL_INSERT_RANGE) {
>> +		/* Collapse range, offset must be less than i_size */
>> +		if (offset >= inode->i_size)
>> +			return -EINVAL;
>> +		/* Check whether the maximum file size would be exceeded */
>> +		if (len > inode->i_sb->s_maxbytes - inode->i_size)
>> +			return -EFBIG;
>> +	} else if (mode & FALLOC_FL_COLLAPSE_RANGE) {
>> +		/*
>> +		 * Insert range, there is no need to overlap collapse
>> +		 * range with EOF, in which case it is effectively a
>> +		 * truncate operation.
>> +		 */
>> +		if (offset + len >= inode->i_size)
>> +			return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> I don't think this helps. If the code is really shared, then the
> factorization is good but here you have to do various checks what operation
> we perform and in that case I don't think it really helps readability to
> factor out checks into a common function.
> 

Yeah, I think you are right, this is just move out the checks and
may increase the reading difficulty, it should be easier to understand
if they're still in their original places.

Thanks,
Yi.