[RFC,2/8] fs: always get the file size in _fs_read()

For _fs_read(), @len == 0 means we read the whole file.
And we just pass @len == 0 to make each filesystem to handle it.

In fact we have info->size() call to properly get the filesize.

So we can not only call info->size() to grab the file_size for len == 0
case, but also detect invalid @len (e.g. @len > file_size) in advance or
truncate @len.

This behavior also allows us to handle unaligned better in the incoming
patches.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/fs.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Hi, wenruo,

在 2022/6/28 15:28, Qu Wenruo 写道:
> For _fs_read(), @len == 0 means we read the whole file.
> And we just pass @len == 0 to make each filesystem to handle it.
>
> In fact we have info->size() call to properly get the filesize.
>
> So we can not only call info->size() to grab the file_size for len == 0
> case, but also detect invalid @len (e.g. @len > file_size) in advance or
> truncate @len.
>
> This behavior also allows us to handle unaligned better in the incoming
> patches.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/fs.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index 6de1a3eb6d5d..d992cdd6d650 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -579,6 +579,7 @@ static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
>   {
>   	struct fstype_info *info = fs_get_info(fs_type);
>   	void *buf;
> +	loff_t file_size;
>   	int ret;
>   
>   #ifdef CONFIG_LMB
> @@ -589,10 +590,26 @@ static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
>   	}
>   #endif
>   
> -	/*
> -	 * We don't actually know how many bytes are being read, since len==0
> -	 * means read the whole file.
> -	 */
> +	ret = info->size(filename, &file_size);

I get an error when running the erofs test cases. The return value isn't 
as expected
when reading symlink file.
For symlink file, erofs_size will return the size of the symlink itself 
here.
> +	if (ret < 0) {
> +		log_err("** Unable to get file size for %s, %d **\n",
> +				filename, ret);
> +		return ret;
> +	}
> +	if (offset >= file_size) {
> +		log_err(
> +		"** Invalid offset, offset (%llu) >= file size (%llu)\n",
> +			offset, file_size);
> +		return -EINVAL;
> +
> +	}
> +	if (len == 0 || offset + len > file_size) {
> +		if (len > file_size)
> +			log_info(
> +	"** Truncate read length from %llu to %llu, as file size is %llu **\n",
> +				 len, file_size, file_size);
> +		len = file_size - offset;
Then, we will get a wrong len in the case of len==0. So I think we need 
to do something
extra with the symlink file?

Thanks,
Jianan
> +	}
>   	buf = map_sysmem(addr, len);
>   	ret = info->read(filename, buf, offset, len, actread);
>   	unmap_sysmem(buf);

On 2022/6/28 20:36, Huang Jianan wrote:
> Hi, wenruo,
>
> 在 2022/6/28 15:28, Qu Wenruo 写道:
>> For _fs_read(), @len == 0 means we read the whole file.
>> And we just pass @len == 0 to make each filesystem to handle it.
>>
>> In fact we have info->size() call to properly get the filesize.
>>
>> So we can not only call info->size() to grab the file_size for len == 0
>> case, but also detect invalid @len (e.g. @len > file_size) in advance or
>> truncate @len.
>>
>> This behavior also allows us to handle unaligned better in the incoming
>> patches.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/fs.c | 25 +++++++++++++++++++++----
>>   1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/fs.c b/fs/fs.c
>> index 6de1a3eb6d5d..d992cdd6d650 100644
>> --- a/fs/fs.c
>> +++ b/fs/fs.c
>> @@ -579,6 +579,7 @@ static int _fs_read(const char *filename, ulong
>> addr, loff_t offset, loff_t len,
>>   {
>>       struct fstype_info *info = fs_get_info(fs_type);
>>       void *buf;
>> +    loff_t file_size;
>>       int ret;
>>   #ifdef CONFIG_LMB
>> @@ -589,10 +590,26 @@ static int _fs_read(const char *filename, ulong
>> addr, loff_t offset, loff_t len,
>>       }
>>   #endif
>> -    /*
>> -     * We don't actually know how many bytes are being read, since
>> len==0
>> -     * means read the whole file.
>> -     */
>> +    ret = info->size(filename, &file_size);
>
> I get an error when running the erofs test cases. The return value isn't
> as expected
> when reading symlink file.
> For symlink file, erofs_size will return the size of the symlink itself
> here.

Indeed, this is a problem, in fact I also checked other fses, it looks
like we all just return the inode size for the softlink, thus size()
call can not be relied on for those cases.

While for the read() calls, every fs will do extra resolving for soft
links, thus it doesn't cause problems.


This can still be solved by not calling size() calls at all, and only do
the unaligned read handling for the leading block.

Thank you very much for pointing this bug out, would update the patchset
for that.

Thanks,
Qu
>> +    if (ret < 0) {
>> +        log_err("** Unable to get file size for %s, %d **\n",
>> +                filename, ret);
>> +        return ret;
>> +    }
>> +    if (offset >= file_size) {
>> +        log_err(
>> +        "** Invalid offset, offset (%llu) >= file size (%llu)\n",
>> +            offset, file_size);
>> +        return -EINVAL;
>> +
>> +    }
>> +    if (len == 0 || offset + len > file_size) {
>> +        if (len > file_size)
>> +            log_info(
>> +    "** Truncate read length from %llu to %llu, as file size is %llu
>> **\n",
>> +                 len, file_size, file_size);
>> +        len = file_size - offset;
> Then, we will get a wrong len in the case of len==0. So I think we need
> to do something
> extra with the symlink file?
>
> Thanks,
> Jianan
>> +    }
>>       buf = map_sysmem(addr, len);
>>       ret = info->read(filename, buf, offset, len, actread);
>>       unmap_sysmem(buf);
>