linux-kernel - Re: [RFC PATCH] erofs/zmap.c: Bail out when no further region remains

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Fri, 9 Dec 2022 12:50:47 +0800
From:   Gao Xiang <hsiangkao@...ux.alibaba.com>
To:     Siddh Raman Pant <code@...dh.me>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Yue Hu <huyue2@...lpad.com>,
        linux-erofs <linux-erofs@...ts.ozlabs.org>
Subject: Re: [RFC PATCH] erofs/zmap.c: Bail out when no further region remains

Hi Siddh,

On Tue, Nov 15, 2022 at 06:50:33PM +0800, Gao Xiang wrote:
> On Tue, Nov 15, 2022 at 03:39:38PM +0530, Siddh Raman Pant via Linux-erofs wrote:
> > On Tue, 15 Nov 2022 08:54:47 +0530, Gao Xiang wrote:
> > > I just wonder if we should return -EINVAL for post-EOF cases or
> > > IOMAP_HOLE with arbitrary length?
> > 
> > Since it has been observed that length can be zeroed, and we
> > must stop, I think we should return an error appropriately.
> > 
> > For a read-only filesystem, we probably don't really need to
> > care what's after the EOF or in unmapped regions, nothing can
> > be changed/extended. The definition of IOMAP_HOLE in iomap.h
> > says it stands for "no blocks allocated, need allocation".
> 
> For fiemap implementation, yes.  So it looks fine to me.
> 
> Let's see what other people think.  Anyway, I'd like to apply it later.
>

Very sorry for late response.

I've just confirmed that the reason is that

796                 /*
797                  * No strict rule how to describe extents for post EOF, yet
798                  * we need do like below. Otherwise, iomap itself will get
799                  * into an endless loop on post EOF.
800                  */
801                 if (iomap->offset >= inode->i_size)
802                         iomap->length = length + map.m_la - offset;

Here iomap->length should be length + offset - map.m_la here. Because
the extent start (map.m_la) is always no more than requested `offset'.

We should need this code sub-block since userspace (filefrag -v) could
pass
ioctl(3, FS_IOC_FIEMAP, {fm_start=0, fm_length=18446744073709551615, fm_flags=0, fm_extent_count=292} => {fm_flags=0, fm_mapped_extents=68, ...}) = 0

without this sub-block, fiemap could get into a very long loop as below:
[  574.030856][ T7030] erofs: m_la 70000000 offset 70457397 length 9223372036784318410 m_llen 457398
[  574.031622][ T7030] erofs: m_la 70000000 offset 70457398 length 9223372036784318409 m_llen 457399
[  574.032397][ T7030] erofs: m_la 70000000 offset 70457399 length 9223372036784318408 m_llen 457400

So could you fix this as?
	iomap->length = length + offset - map.m_la;

I've already verified it can properly resolve the issue and do the
correct thing although I'd like to submit this later since we're quite
close to the merge window.

Thanks,
Gao Xiang