Re: [PATCH v2] erofs: fix incorrect symlink detection in fast symlink

[Gao Xiang <hsiangkao@linux.alibaba.com>]

On 2024/9/9 21:58, Colin Walters wrote:
> 
> 
> On Mon, Sep 9, 2024, at 9:21 AM, Gao Xiang wrote:
>>
>> It can be bigger.
>>
>> Like ext4, EROFS supports PAGE_SIZE symlink via page_get_link()
>> (non-fastsymlink cases), but mostly consider this as 4KiB though
>> regardless of on-disk block sizes.

Let me rephrase it more clearer...

For each EROFS logical block read (e.g 4KiB), EROFS will
only generate one filesystem physical block read instead
of two or more physical block read.

Symlink files just like regular files, so it doesn't allow
the inline tail crosses physical block boundary (which
means an 8KiB I/O is needed), see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/erofs/data.c?h=v6.10#n103

In that case,
EROFS_INODE_FLAT_INLINE (2) shouldn't be used and
EROFS_INODE_FLAT_PLAIN(0) need to be used instead in order
to contain full data.

But the fast symlink handling has an issue: it should
fall back to non-fastsymlink path instead of erroring out
directly.


> 
> But symlink targets can't be bigger than PATH_MAX which has always been 4KiB right? (Does Linux support systems with sub-4KiB pages?)

Yes, I think 4KiB is the minimum page size of Linux, so
in practice the maximum size of generic Linux is
PATH_MAX(4KiB).

> 
> I guess let me ask it a different way: Since we're removing a sanity check here I just want to be sure that the constraints are still handled.

It seems page_get_link() doesn't check this, but it
will add trailing `\0` to the end buffer (PAGE_SIZE - 1),
so at least it won't crash the kernel.

> 
> Hmm, skimming through the vfs code from vfs_readlink() I'm not seeing anything constraining the userspace buffer length which seems surprising.
> 
> Ah interesting, XFS has
> #define XFS_SYMLINK_MAXLEN	1024
> and always constrains even its kmalloc invocation to that.


Not quite sure about hard limitation in EROFS
runtime, we could define

#define EROFS_SYMLINK_MAXLEN	4096

But since symlink i_size > 4096 only due to crafted
images (and not generated by mkfs) and not crash, so
either way (to check or not check in kernel) is okay
to me.

Thanks,
Gao Xiang

>