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);