[PATCH] staging: erofs: fix an error handling in erofs_readdir()

[PATCH] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

From: Gao Xiang <gaoxiang25@huawei.com>

Richard observed a forever loop of erofs_read_raw_page() [1]
which can be generated by forcely setting ->u.i_blkaddr
to 0xdeadbeef (as my understanding block layer can
handle access beyond end of device correctly).

After digging into that, it seems the problem is highly
related with directories and then I found the root cause
is an improper error handling in erofs_readdir().

Let's fix it now.

[1] https://lore.kernel.org/r/1746679415.68815.1566076790942.JavaMail.zimbra@nod.at/

Reported-by: Richard Weinberger <richard@nod.at>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <stable@vger.kernel.org> # 4.19+
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---

Which is based on the following patch as well
https://lore.kernel.org/r/20190816071142.8633-1-gaoxiang25@huawei.com/

and
https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao@aol.com/
can still be properly applied after this patch.

 drivers/staging/erofs/dir.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c
index 5f38382637e6..f2d7539589e4 100644
--- a/drivers/staging/erofs/dir.c
+++ b/drivers/staging/erofs/dir.c
@@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 		unsigned int nameoff, maxsize;
 
 		dentry_page = read_mapping_page(mapping, i, NULL);
-		if (IS_ERR(dentry_page))
-			continue;
+		if (IS_ERR(dentry_page)) {
+			errln("fail to readdir of logical block %u of nid %llu",
+			      i, EROFS_V(dir)->nid);
+			err = PTR_ERR(dentry_page);
+			break;
+		}
 
 		de = (struct erofs_dirent *)kmap(dentry_page);
 
-- 
2.17.1

[PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

From: Gao Xiang <gaoxiang25@huawei.com>

Richard observed a forever loop of erofs_read_raw_page() [1]
which can be generated by forcely setting ->u.i_blkaddr
to 0xdeadbeef (as my understanding block layer can
handle access beyond end of device correctly).

After digging into that, it seems the problem is highly
related with directories and then I found the root cause
is an improper error handling in erofs_readdir().

Let's fix it now.

[1] https://lore.kernel.org/r/1163995781.68824.1566084358245.JavaMail.zimbra@nod.at/

Reported-by: Richard Weinberger <richard@nod.at>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <stable@vger.kernel.org> # 4.19+
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---

changelog from v1:
 - fix the incorrect external link in commit message.

This patch is based on the following patch as well
https://lore.kernel.org/r/20190816071142.8633-1-gaoxiang25@huawei.com/

and
https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao@aol.com/
can still be properly applied after this patch.

 drivers/staging/erofs/dir.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c
index 5f38382637e6..f2d7539589e4 100644
--- a/drivers/staging/erofs/dir.c
+++ b/drivers/staging/erofs/dir.c
@@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 		unsigned int nameoff, maxsize;
 
 		dentry_page = read_mapping_page(mapping, i, NULL);
-		if (IS_ERR(dentry_page))
-			continue;
+		if (IS_ERR(dentry_page)) {
+			errln("fail to readdir of logical block %u of nid %llu",
+			      i, EROFS_V(dir)->nid);
+			err = PTR_ERR(dentry_page);
+			break;
+		}
 
 		de = (struct erofs_dirent *)kmap(dentry_page);
 
-- 
2.17.1

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Matthew Wilcox]

On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
> @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>  		unsigned int nameoff, maxsize;
>  
>  		dentry_page = read_mapping_page(mapping, i, NULL);
> -		if (IS_ERR(dentry_page))
> -			continue;
> +		if (IS_ERR(dentry_page)) {
> +			errln("fail to readdir of logical block %u of nid %llu",
> +			      i, EROFS_V(dir)->nid);
> +			err = PTR_ERR(dentry_page);
> +			break;

I don't think you want to use the errno that came back from
read_mapping_page() (which is, I think, always going to be -EIO).
Rather you want -EFSCORRUPTED, at least if I understand the recent
patches to ext2/ext4/f2fs/xfs/...

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

Hi Willy,

On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
> > @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> >  		unsigned int nameoff, maxsize;
> >  
> >  		dentry_page = read_mapping_page(mapping, i, NULL);
> > -		if (IS_ERR(dentry_page))
> > -			continue;
> > +		if (IS_ERR(dentry_page)) {
> > +			errln("fail to readdir of logical block %u of nid %llu",
> > +			      i, EROFS_V(dir)->nid);
> > +			err = PTR_ERR(dentry_page);
> > +			break;
> 
> I don't think you want to use the errno that came back from
> read_mapping_page() (which is, I think, always going to be -EIO).
> Rather you want -EFSCORRUPTED, at least if I understand the recent
> patches to ext2/ext4/f2fs/xfs/...

Thanks for your reply and noticing this. :)

Yes, as I talked with you about read_mapping_page() in a xfs related
topic earlier, I think I fully understand what returns here.

I actually had some concern about that before sending out this patch.
You know the status is
   PG_uptodate is not set and PG_error is set here.

But we cannot know it is actually a disk read error or due to
corrupted images (due to lack of page flags or some status, and
I think it could be a waste of page structure space for such
corrupted image or disk error)...

And some people also like propagate errors from insiders...
(and they could argue about err = -EFSCORRUPTED as well..)

I'd like hear your suggestion about this after my words above?
still return -EFSCORRUPTED?

Thanks,
Gao Xiang

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Matthew Wilcox]

On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
> > On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
> > > @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> > >  		unsigned int nameoff, maxsize;
> > >  
> > >  		dentry_page = read_mapping_page(mapping, i, NULL);
> > > -		if (IS_ERR(dentry_page))
> > > -			continue;
> > > +		if (IS_ERR(dentry_page)) {
> > > +			errln("fail to readdir of logical block %u of nid %llu",
> > > +			      i, EROFS_V(dir)->nid);
> > > +			err = PTR_ERR(dentry_page);
> > > +			break;
> > 
> > I don't think you want to use the errno that came back from
> > read_mapping_page() (which is, I think, always going to be -EIO).
> > Rather you want -EFSCORRUPTED, at least if I understand the recent
> > patches to ext2/ext4/f2fs/xfs/...
> 
> Thanks for your reply and noticing this. :)
> 
> Yes, as I talked with you about read_mapping_page() in a xfs related
> topic earlier, I think I fully understand what returns here.
> 
> I actually had some concern about that before sending out this patch.
> You know the status is
>    PG_uptodate is not set and PG_error is set here.
> 
> But we cannot know it is actually a disk read error or due to
> corrupted images (due to lack of page flags or some status, and
> I think it could be a waste of page structure space for such
> corrupted image or disk error)...
> 
> And some people also like propagate errors from insiders...
> (and they could argue about err = -EFSCORRUPTED as well..)
> 
> I'd like hear your suggestion about this after my words above?
> still return -EFSCORRUPTED?

I don't think it matters whether it's due to a disk error or a corrupted
image.  We can't read the directory entry, so we should probably return
-EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
also return -ENOMEM, so it should probably look something like this:

		err = 0;
		if (dentry_page == ERR_PTR(-ENOMEM))
			err = -ENOMEM;
		else if (IS_ERR(dentry_page)) {
			errln("fail to readdir of logical block %u of nid %llu",
			      i, EROFS_V(dir)->nid);
			err = -EFSCORRUPTED;
		}

		if (err)
			break;

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

On Sat, Aug 17, 2019 at 07:53:39PM -0700, Matthew Wilcox wrote:
> On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
> > On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
> > > On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
> > > > @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> > > >  		unsigned int nameoff, maxsize;
> > > >  
> > > >  		dentry_page = read_mapping_page(mapping, i, NULL);
> > > > -		if (IS_ERR(dentry_page))
> > > > -			continue;
> > > > +		if (IS_ERR(dentry_page)) {
> > > > +			errln("fail to readdir of logical block %u of nid %llu",
> > > > +			      i, EROFS_V(dir)->nid);
> > > > +			err = PTR_ERR(dentry_page);
> > > > +			break;
> > > 
> > > I don't think you want to use the errno that came back from
> > > read_mapping_page() (which is, I think, always going to be -EIO).
> > > Rather you want -EFSCORRUPTED, at least if I understand the recent
> > > patches to ext2/ext4/f2fs/xfs/...
> > 
> > Thanks for your reply and noticing this. :)
> > 
> > Yes, as I talked with you about read_mapping_page() in a xfs related
> > topic earlier, I think I fully understand what returns here.
> > 
> > I actually had some concern about that before sending out this patch.
> > You know the status is
> >    PG_uptodate is not set and PG_error is set here.
> > 
> > But we cannot know it is actually a disk read error or due to
> > corrupted images (due to lack of page flags or some status, and
> > I think it could be a waste of page structure space for such
> > corrupted image or disk error)...
> > 
> > And some people also like propagate errors from insiders...
> > (and they could argue about err = -EFSCORRUPTED as well..)
> > 
> > I'd like hear your suggestion about this after my words above?
> > still return -EFSCORRUPTED?
> 
> I don't think it matters whether it's due to a disk error or a corrupted
> image.  We can't read the directory entry, so we should probably return
> -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
> also return -ENOMEM, so it should probably look something like this:

OK, I will send the next version like what you described below.
I realized that at first but I have no tendency to return
EFSCORRUPTED or EIO here.

Thanks,
Gao Xiang

> 
> 		err = 0;
> 		if (dentry_page == ERR_PTR(-ENOMEM))
> 			err = -ENOMEM;
> 		else if (IS_ERR(dentry_page)) {
> 			errln("fail to readdir of logical block %u of nid %llu",
> 			      i, EROFS_V(dir)->nid);
> 			err = -EFSCORRUPTED;
> 		}
> 
> 		if (err)
> 			break;

[PATCH] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

From: Gao Xiang <gaoxiang25@huawei.com>

Richard observed a forever loop of erofs_read_raw_page() [1]
which can be generated by forcely setting ->u.i_blkaddr
to 0xdeadbeef (as my understanding block layer can
handle access beyond end of device correctly).

After digging into that, it seems the problem is highly
related with directories and then I found the root cause
is an improper error handling in erofs_readdir().

Let's fix it now.

[1] https://lore.kernel.org/r/1163995781.68824.1566084358245.JavaMail.zimbra@nod.at/

Reported-by: Richard Weinberger <richard@nod.at>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <stable@vger.kernel.org> # 4.19+
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
changelog from v2:
 - transform EIO to EFSCORRUPTED as suggested by Matthew;

changelog from v1:
 - fix the incorrect external link in commit message.

This patch is based on the following patch as well
https://lore.kernel.org/r/20190816071142.8633-1-gaoxiang25@huawei.com/

and
https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao@aol.com/
can still be properly applied after this patch.

 drivers/staging/erofs/dir.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c
index 5f38382637e6..eb430a031b20 100644
--- a/drivers/staging/erofs/dir.c
+++ b/drivers/staging/erofs/dir.c
@@ -82,8 +82,17 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 		unsigned int nameoff, maxsize;
 
 		dentry_page = read_mapping_page(mapping, i, NULL);
-		if (IS_ERR(dentry_page))
-			continue;
+		if (dentry_page == ERR_PTR(-ENOMEM)) {
+			errln("no memory to readdir of logical block %u of nid %llu",
+			      i, EROFS_V(dir)->nid);
+			err = -ENOMEM;
+			break;
+		} else if (IS_ERR(dentry_page)) {
+			errln("fail to readdir of logical block %u of nid %llu",
+			      i, EROFS_V(dir)->nid);
+			err = -EFSCORRUPTED;
+			break;
+		}
 
 		de = (struct erofs_dirent *)kmap(dentry_page);
 
-- 
2.17.1

Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3539 bytes --]

Hi Gao,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc4 next-20190816]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging//erofs/dir.c: In function 'erofs_readdir':
>> drivers/staging//erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first use in this function)
       err = -EFSCORRUPTED;
              ^~~~~~~~~~~~
   drivers/staging//erofs/dir.c:110:11: note: each undeclared identifier is reported only once for each function it appears in

vim +/EFSCORRUPTED +110 drivers/staging//erofs/dir.c

    85	
    86	static int erofs_readdir(struct file *f, struct dir_context *ctx)
    87	{
    88		struct inode *dir = file_inode(f);
    89		struct address_space *mapping = dir->i_mapping;
    90		const size_t dirsize = i_size_read(dir);
    91		unsigned int i = ctx->pos / EROFS_BLKSIZ;
    92		unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
    93		int err = 0;
    94		bool initial = true;
    95	
    96		while (ctx->pos < dirsize) {
    97			struct page *dentry_page;
    98			struct erofs_dirent *de;
    99			unsigned int nameoff, maxsize;
   100	
   101			dentry_page = read_mapping_page(mapping, i, NULL);
   102			if (dentry_page == ERR_PTR(-ENOMEM)) {
   103				errln("no memory to readdir of logical block %u of nid %llu",
   104				      i, EROFS_V(dir)->nid);
   105				err = -ENOMEM;
   106				break;
   107			} else if (IS_ERR(dentry_page)) {
   108				errln("fail to readdir of logical block %u of nid %llu",
   109				      i, EROFS_V(dir)->nid);
 > 110				err = -EFSCORRUPTED;
   111				break;
   112			}
   113	
   114			de = (struct erofs_dirent *)kmap(dentry_page);
   115	
   116			nameoff = le16_to_cpu(de->nameoff);
   117	
   118			if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
   119				     nameoff >= PAGE_SIZE)) {
   120				errln("%s, invalid de[0].nameoff %u",
   121				      __func__, nameoff);
   122	
   123				err = -EIO;
   124				goto skip_this;
   125			}
   126	
   127			maxsize = min_t(unsigned int,
   128					dirsize - ctx->pos + ofs, PAGE_SIZE);
   129	
   130			/* search dirents at the arbitrary position */
   131			if (unlikely(initial)) {
   132				initial = false;
   133	
   134				ofs = roundup(ofs, sizeof(struct erofs_dirent));
   135				if (unlikely(ofs >= nameoff))
   136					goto skip_this;
   137			}
   138	
   139			err = erofs_fill_dentries(ctx, de, &ofs, nameoff, maxsize);
   140	skip_this:
   141			kunmap(dentry_page);
   142	
   143			put_page(dentry_page);
   144	
   145			ctx->pos = blknr_to_addr(i) + ofs;
   146	
   147			if (unlikely(err))
   148				break;
   149			++i;
   150			ofs = 0;
   151		}
   152		return err < 0 ? err : 0;
   153	}
   154	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58668 bytes --]

Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3569 bytes --]

Hi Gao,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc4 next-20190816]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/erofs/dir.c: In function 'erofs_readdir':
>> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first use in this function); did you mean 'FS_NRSUPER'?
       err = -EFSCORRUPTED;
              ^~~~~~~~~~~~
              FS_NRSUPER
   drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is reported only once for each function it appears in

vim +110 drivers/staging/erofs/dir.c

    85	
    86	static int erofs_readdir(struct file *f, struct dir_context *ctx)
    87	{
    88		struct inode *dir = file_inode(f);
    89		struct address_space *mapping = dir->i_mapping;
    90		const size_t dirsize = i_size_read(dir);
    91		unsigned int i = ctx->pos / EROFS_BLKSIZ;
    92		unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
    93		int err = 0;
    94		bool initial = true;
    95	
    96		while (ctx->pos < dirsize) {
    97			struct page *dentry_page;
    98			struct erofs_dirent *de;
    99			unsigned int nameoff, maxsize;
   100	
   101			dentry_page = read_mapping_page(mapping, i, NULL);
   102			if (dentry_page == ERR_PTR(-ENOMEM)) {
   103				errln("no memory to readdir of logical block %u of nid %llu",
   104				      i, EROFS_V(dir)->nid);
   105				err = -ENOMEM;
   106				break;
   107			} else if (IS_ERR(dentry_page)) {
   108				errln("fail to readdir of logical block %u of nid %llu",
   109				      i, EROFS_V(dir)->nid);
 > 110				err = -EFSCORRUPTED;
   111				break;
   112			}
   113	
   114			de = (struct erofs_dirent *)kmap(dentry_page);
   115	
   116			nameoff = le16_to_cpu(de->nameoff);
   117	
   118			if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
   119				     nameoff >= PAGE_SIZE)) {
   120				errln("%s, invalid de[0].nameoff %u",
   121				      __func__, nameoff);
   122	
   123				err = -EIO;
   124				goto skip_this;
   125			}
   126	
   127			maxsize = min_t(unsigned int,
   128					dirsize - ctx->pos + ofs, PAGE_SIZE);
   129	
   130			/* search dirents at the arbitrary position */
   131			if (unlikely(initial)) {
   132				initial = false;
   133	
   134				ofs = roundup(ofs, sizeof(struct erofs_dirent));
   135				if (unlikely(ofs >= nameoff))
   136					goto skip_this;
   137			}
   138	
   139			err = erofs_fill_dentries(ctx, de, &ofs, nameoff, maxsize);
   140	skip_this:
   141			kunmap(dentry_page);
   142	
   143			put_page(dentry_page);
   144	
   145			ctx->pos = blknr_to_addr(i) + ofs;
   146	
   147			if (unlikely(err))
   148				break;
   149			++i;
   150			ofs = 0;
   151		}
   152		return err < 0 ? err : 0;
   153	}
   154	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66441 bytes --]

Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

On Sun, Aug 18, 2019 at 09:17:52PM +0800, kbuild test robot wrote:
> Hi Gao,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [cannot apply to v5.3-rc4 next-20190816]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

... those patches should be applied to staging tree
since linux-next has not been updated yet...

Thanks,
Gao Xiang

> 
> url:    https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344
> config: arm64-allyesconfig (attached as .config)
> compiler: aarch64-linux-gcc (GCC) 7.4.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.4.0 make.cross ARCH=arm64 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/staging/erofs/dir.c: In function 'erofs_readdir':
> >> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first use in this function); did you mean 'FS_NRSUPER'?
>        err = -EFSCORRUPTED;
>               ^~~~~~~~~~~~
>               FS_NRSUPER
>    drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is reported only once for each function it appears in
> 
> vim +110 drivers/staging/erofs/dir.c
> 
>     85	
>     86	static int erofs_readdir(struct file *f, struct dir_context *ctx)
>     87	{
>     88		struct inode *dir = file_inode(f);
>     89		struct address_space *mapping = dir->i_mapping;
>     90		const size_t dirsize = i_size_read(dir);
>     91		unsigned int i = ctx->pos / EROFS_BLKSIZ;
>     92		unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
>     93		int err = 0;
>     94		bool initial = true;
>     95	
>     96		while (ctx->pos < dirsize) {
>     97			struct page *dentry_page;
>     98			struct erofs_dirent *de;
>     99			unsigned int nameoff, maxsize;
>    100	
>    101			dentry_page = read_mapping_page(mapping, i, NULL);
>    102			if (dentry_page == ERR_PTR(-ENOMEM)) {
>    103				errln("no memory to readdir of logical block %u of nid %llu",
>    104				      i, EROFS_V(dir)->nid);
>    105				err = -ENOMEM;
>    106				break;
>    107			} else if (IS_ERR(dentry_page)) {
>    108				errln("fail to readdir of logical block %u of nid %llu",
>    109				      i, EROFS_V(dir)->nid);
>  > 110				err = -EFSCORRUPTED;
>    111				break;
>    112			}
>    113	
>    114			de = (struct erofs_dirent *)kmap(dentry_page);
>    115	
>    116			nameoff = le16_to_cpu(de->nameoff);
>    117	
>    118			if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
>    119				     nameoff >= PAGE_SIZE)) {
>    120				errln("%s, invalid de[0].nameoff %u",
>    121				      __func__, nameoff);
>    122	
>    123				err = -EIO;
>    124				goto skip_this;
>    125			}
>    126	
>    127			maxsize = min_t(unsigned int,
>    128					dirsize - ctx->pos + ofs, PAGE_SIZE);
>    129	
>    130			/* search dirents at the arbitrary position */
>    131			if (unlikely(initial)) {
>    132				initial = false;
>    133	
>    134				ofs = roundup(ofs, sizeof(struct erofs_dirent));
>    135				if (unlikely(ofs >= nameoff))
>    136					goto skip_this;
>    137			}
>    138	
>    139			err = erofs_fill_dentries(ctx, de, &ofs, nameoff, maxsize);
>    140	skip_this:
>    141			kunmap(dentry_page);
>    142	
>    143			put_page(dentry_page);
>    144	
>    145			ctx->pos = blknr_to_addr(i) + ofs;
>    146	
>    147			if (unlikely(err))
>    148				break;
>    149			++i;
>    150			ofs = 0;
>    151		}
>    152		return err < 0 ? err : 0;
>    153	}
>    154	
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

[Philip Li]

On Sun, Aug 18, 2019 at 09:25:04PM +0800, Gao Xiang wrote:
> On Sun, Aug 18, 2019 at 09:17:52PM +0800, kbuild test robot wrote:
> > Hi Gao,
> > 
> > I love your patch! Yet something to improve:
> > 
> > [auto build test ERROR on linus/master]
> > [cannot apply to v5.3-rc4 next-20190816]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> ... those patches should be applied to staging tree
> since linux-next has not been updated yet...
thanks for the feedback, we will consider this to our todo list.

> 
> Thanks,
> Gao Xiang
> 
> > 
> > url:    https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344
> > config: arm64-allyesconfig (attached as .config)
> > compiler: aarch64-linux-gcc (GCC) 7.4.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         GCC_VERSION=7.4.0 make.cross ARCH=arm64 
> > 
> > If you fix the issue, kindly add following tag
> > Reported-by: kbuild test robot <lkp@intel.com>
> > 
> > All errors (new ones prefixed by >>):
> > 
> >    drivers/staging/erofs/dir.c: In function 'erofs_readdir':
> > >> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first use in this function); did you mean 'FS_NRSUPER'?
> >        err = -EFSCORRUPTED;
> >               ^~~~~~~~~~~~
> >               FS_NRSUPER
> >    drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is reported only once for each function it appears in
> > 
> > vim +110 drivers/staging/erofs/dir.c
> > 
> >     85	
> >     86	static int erofs_readdir(struct file *f, struct dir_context *ctx)
> >     87	{
> >     88		struct inode *dir = file_inode(f);
> >     89		struct address_space *mapping = dir->i_mapping;
> >     90		const size_t dirsize = i_size_read(dir);
> >     91		unsigned int i = ctx->pos / EROFS_BLKSIZ;
> >     92		unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
> >     93		int err = 0;
> >     94		bool initial = true;
> >     95	
> >     96		while (ctx->pos < dirsize) {
> >     97			struct page *dentry_page;
> >     98			struct erofs_dirent *de;
> >     99			unsigned int nameoff, maxsize;
> >    100	
> >    101			dentry_page = read_mapping_page(mapping, i, NULL);
> >    102			if (dentry_page == ERR_PTR(-ENOMEM)) {
> >    103				errln("no memory to readdir of logical block %u of nid %llu",
> >    104				      i, EROFS_V(dir)->nid);
> >    105				err = -ENOMEM;
> >    106				break;
> >    107			} else if (IS_ERR(dentry_page)) {
> >    108				errln("fail to readdir of logical block %u of nid %llu",
> >    109				      i, EROFS_V(dir)->nid);
> >  > 110				err = -EFSCORRUPTED;
> >    111				break;
> >    112			}
> >    113	
> >    114			de = (struct erofs_dirent *)kmap(dentry_page);
> >    115	
> >    116			nameoff = le16_to_cpu(de->nameoff);
> >    117	
> >    118			if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
> >    119				     nameoff >= PAGE_SIZE)) {
> >    120				errln("%s, invalid de[0].nameoff %u",
> >    121				      __func__, nameoff);
> >    122	
> >    123				err = -EIO;
> >    124				goto skip_this;
> >    125			}
> >    126	
> >    127			maxsize = min_t(unsigned int,
> >    128					dirsize - ctx->pos + ofs, PAGE_SIZE);
> >    129	
> >    130			/* search dirents at the arbitrary position */
> >    131			if (unlikely(initial)) {
> >    132				initial = false;
> >    133	
> >    134				ofs = roundup(ofs, sizeof(struct erofs_dirent));
> >    135				if (unlikely(ofs >= nameoff))
> >    136					goto skip_this;
> >    137			}
> >    138	
> >    139			err = erofs_fill_dentries(ctx, de, &ofs, nameoff, maxsize);
> >    140	skip_this:
> >    141			kunmap(dentry_page);
> >    142	
> >    143			put_page(dentry_page);
> >    144	
> >    145			ctx->pos = blknr_to_addr(i) + ofs;
> >    146	
> >    147			if (unlikely(err))
> >    148				break;
> >    149			++i;
> >    150			ofs = 0;
> >    151		}
> >    152		return err < 0 ? err : 0;
> >    153	}
> >    154	
> > 
> > ---
> > 0-DAY kernel test infrastructure                Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 
> 

Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

Hi Philip,

On Tue, Aug 20, 2019 at 02:50:38PM +0800, Philip Li wrote:
> On Sun, Aug 18, 2019 at 09:25:04PM +0800, Gao Xiang wrote:
> > On Sun, Aug 18, 2019 at 09:17:52PM +0800, kbuild test robot wrote:
> > > Hi Gao,
> > > 
> > > I love your patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on linus/master]
> > > [cannot apply to v5.3-rc4 next-20190816]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > ... those patches should be applied to staging tree
> > since linux-next has not been updated yet...
> thanks for the feedback, we will consider this to our todo list.

Yes, many confusing reports anyway...
(Just my personal suggestion, maybe we can add some hints on the patch email
to indicate which tree can be applied successfully for ci in the future...)

Thanks,
Gao Xiang

> 
> > 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > url:    https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-an-error-handling-in-erofs_readdir/20190818-191344
> > > config: arm64-allyesconfig (attached as .config)
> > > compiler: aarch64-linux-gcc (GCC) 7.4.0
> > > reproduce:
> > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >         chmod +x ~/bin/make.cross
> > >         # save the attached .config to linux build tree
> > >         GCC_VERSION=7.4.0 make.cross ARCH=arm64 
> > > 
> > > If you fix the issue, kindly add following tag
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > 
> > > All errors (new ones prefixed by >>):
> > > 
> > >    drivers/staging/erofs/dir.c: In function 'erofs_readdir':
> > > >> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared (first use in this function); did you mean 'FS_NRSUPER'?
> > >        err = -EFSCORRUPTED;
> > >               ^~~~~~~~~~~~
> > >               FS_NRSUPER
> > >    drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is reported only once for each function it appears in
> > > 
> > > vim +110 drivers/staging/erofs/dir.c
> > > 
> > >     85	
> > >     86	static int erofs_readdir(struct file *f, struct dir_context *ctx)
> > >     87	{
> > >     88		struct inode *dir = file_inode(f);
> > >     89		struct address_space *mapping = dir->i_mapping;
> > >     90		const size_t dirsize = i_size_read(dir);
> > >     91		unsigned int i = ctx->pos / EROFS_BLKSIZ;
> > >     92		unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
> > >     93		int err = 0;
> > >     94		bool initial = true;
> > >     95	
> > >     96		while (ctx->pos < dirsize) {
> > >     97			struct page *dentry_page;
> > >     98			struct erofs_dirent *de;
> > >     99			unsigned int nameoff, maxsize;
> > >    100	
> > >    101			dentry_page = read_mapping_page(mapping, i, NULL);
> > >    102			if (dentry_page == ERR_PTR(-ENOMEM)) {
> > >    103				errln("no memory to readdir of logical block %u of nid %llu",
> > >    104				      i, EROFS_V(dir)->nid);
> > >    105				err = -ENOMEM;
> > >    106				break;
> > >    107			} else if (IS_ERR(dentry_page)) {
> > >    108				errln("fail to readdir of logical block %u of nid %llu",
> > >    109				      i, EROFS_V(dir)->nid);
> > >  > 110				err = -EFSCORRUPTED;
> > >    111				break;
> > >    112			}
> > >    113	
> > >    114			de = (struct erofs_dirent *)kmap(dentry_page);
> > >    115	
> > >    116			nameoff = le16_to_cpu(de->nameoff);
> > >    117	
> > >    118			if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
> > >    119				     nameoff >= PAGE_SIZE)) {
> > >    120				errln("%s, invalid de[0].nameoff %u",
> > >    121				      __func__, nameoff);
> > >    122	
> > >    123				err = -EIO;
> > >    124				goto skip_this;
> > >    125			}
> > >    126	
> > >    127			maxsize = min_t(unsigned int,
> > >    128					dirsize - ctx->pos + ofs, PAGE_SIZE);
> > >    129	
> > >    130			/* search dirents at the arbitrary position */
> > >    131			if (unlikely(initial)) {
> > >    132				initial = false;
> > >    133	
> > >    134				ofs = roundup(ofs, sizeof(struct erofs_dirent));
> > >    135				if (unlikely(ofs >= nameoff))
> > >    136					goto skip_this;
> > >    137			}
> > >    138	
> > >    139			err = erofs_fill_dentries(ctx, de, &ofs, nameoff, maxsize);
> > >    140	skip_this:
> > >    141			kunmap(dentry_page);
> > >    142	
> > >    143			put_page(dentry_page);
> > >    144	
> > >    145			ctx->pos = blknr_to_addr(i) + ofs;
> > >    146	
> > >    147			if (unlikely(err))
> > >    148				break;
> > >    149			++i;
> > >    150			ofs = 0;
> > >    151		}
> > >    152		return err < 0 ? err : 0;
> > >    153	}
> > >    154	
> > > 
> > > ---
> > > 0-DAY kernel test infrastructure                Open Source Technology Center
> > > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> > 
> > 

RE: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

[Li, Philip]

> Subject: Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()
> 
> Hi Philip,
> 
> On Tue, Aug 20, 2019 at 02:50:38PM +0800, Philip Li wrote:
> > On Sun, Aug 18, 2019 at 09:25:04PM +0800, Gao Xiang wrote:
> > > On Sun, Aug 18, 2019 at 09:17:52PM +0800, kbuild test robot wrote:
> > > > Hi Gao,
> > > >
> > > > I love your patch! Yet something to improve:
> > > >
> > > > [auto build test ERROR on linus/master]
> > > > [cannot apply to v5.3-rc4 next-20190816]
> > > > [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system]
> > >
> > > ... those patches should be applied to staging tree
> > > since linux-next has not been updated yet...
> > thanks for the feedback, we will consider this to our todo list.
> 
> Yes, many confusing reports anyway...
> (Just my personal suggestion, maybe we can add some hints on the patch email
> to indicate which tree can be applied successfully for ci in the future...)
thanks, this is good idea. On the other side, we support to add --base option to git format-patch
to automatically suggest the base, refer to https://stackoverflow.com/a/37406982 for detail.
Meanwhile, we will enhance the internal logic to find suitable base if possible.

> 
> Thanks,
> Gao Xiang
> 
> >
> > >
> > > Thanks,
> > > Gao Xiang
> > >
> > > >
> > > > url:    https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-
> an-error-handling-in-erofs_readdir/20190818-191344
> > > > config: arm64-allyesconfig (attached as .config)
> > > > compiler: aarch64-linux-gcc (GCC) 7.4.0
> > > > reproduce:
> > > >         wget https://raw.githubusercontent.com/intel/lkp-
> tests/master/sbin/make.cross -O ~/bin/make.cross
> > > >         chmod +x ~/bin/make.cross
> > > >         # save the attached .config to linux build tree
> > > >         GCC_VERSION=7.4.0 make.cross ARCH=arm64
> > > >
> > > > If you fix the issue, kindly add following tag
> > > > Reported-by: kbuild test robot <lkp@intel.com>
> > > >
> > > > All errors (new ones prefixed by >>):
> > > >
> > > >    drivers/staging/erofs/dir.c: In function 'erofs_readdir':
> > > > >> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared
> (first use in this function); did you mean 'FS_NRSUPER'?
> > > >        err = -EFSCORRUPTED;
> > > >               ^~~~~~~~~~~~
> > > >               FS_NRSUPER
> > > >    drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is
> reported only once for each function it appears in
> > > >
> > > > vim +110 drivers/staging/erofs/dir.c
> > > >
> > > >     85
> > > >     86	static int erofs_readdir(struct file *f, struct dir_context *ctx)
> > > >     87	{
> > > >     88		struct inode *dir = file_inode(f);
> > > >     89		struct address_space *mapping = dir->i_mapping;
> > > >     90		const size_t dirsize = i_size_read(dir);
> > > >     91		unsigned int i = ctx->pos / EROFS_BLKSIZ;
> > > >     92		unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
> > > >     93		int err = 0;
> > > >     94		bool initial = true;
> > > >     95
> > > >     96		while (ctx->pos < dirsize) {
> > > >     97			struct page *dentry_page;
> > > >     98			struct erofs_dirent *de;
> > > >     99			unsigned int nameoff, maxsize;
> > > >    100
> > > >    101			dentry_page = read_mapping_page(mapping, i,
> NULL);
> > > >    102			if (dentry_page == ERR_PTR(-ENOMEM)) {
> > > >    103				errln("no memory to readdir of logical
> block %u of nid %llu",
> > > >    104				      i, EROFS_V(dir)->nid);
> > > >    105				err = -ENOMEM;
> > > >    106				break;
> > > >    107			} else if (IS_ERR(dentry_page)) {
> > > >    108				errln("fail to readdir of logical block %u of
> nid %llu",
> > > >    109				      i, EROFS_V(dir)->nid);
> > > >  > 110				err = -EFSCORRUPTED;
> > > >    111				break;
> > > >    112			}
> > > >    113
> > > >    114			de = (struct erofs_dirent *)kmap(dentry_page);
> > > >    115
> > > >    116			nameoff = le16_to_cpu(de->nameoff);
> > > >    117
> > > >    118			if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
> > > >    119				     nameoff >= PAGE_SIZE)) {
> > > >    120				errln("%s, invalid de[0].nameoff %u",
> > > >    121				      __func__, nameoff);
> > > >    122
> > > >    123				err = -EIO;
> > > >    124				goto skip_this;
> > > >    125			}
> > > >    126
> > > >    127			maxsize = min_t(unsigned int,
> > > >    128					dirsize - ctx->pos + ofs,
> PAGE_SIZE);
> > > >    129
> > > >    130			/* search dirents at the arbitrary position */
> > > >    131			if (unlikely(initial)) {
> > > >    132				initial = false;
> > > >    133
> > > >    134				ofs = roundup(ofs, sizeof(struct
> erofs_dirent));
> > > >    135				if (unlikely(ofs >= nameoff))
> > > >    136					goto skip_this;
> > > >    137			}
> > > >    138
> > > >    139			err = erofs_fill_dentries(ctx, de, &ofs, nameoff,
> maxsize);
> > > >    140	skip_this:
> > > >    141			kunmap(dentry_page);
> > > >    142
> > > >    143			put_page(dentry_page);
> > > >    144
> > > >    145			ctx->pos = blknr_to_addr(i) + ofs;
> > > >    146
> > > >    147			if (unlikely(err))
> > > >    148				break;
> > > >    149			++i;
> > > >    150			ofs = 0;
> > > >    151		}
> > > >    152		return err < 0 ? err : 0;
> > > >    153	}
> > > >    154
> > > >
> > > > ---
> > > > 0-DAY kernel test infrastructure                Open Source Technology Center
> > > > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> > >
> > >

Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

On Tue, Aug 20, 2019 at 06:58:00AM +0000, Li, Philip wrote:
> > Subject: Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir()
> > 
> > Hi Philip,
> > 
> > On Tue, Aug 20, 2019 at 02:50:38PM +0800, Philip Li wrote:
> > > On Sun, Aug 18, 2019 at 09:25:04PM +0800, Gao Xiang wrote:
> > > > On Sun, Aug 18, 2019 at 09:17:52PM +0800, kbuild test robot wrote:
> > > > > Hi Gao,
> > > > >
> > > > > I love your patch! Yet something to improve:
> > > > >
> > > > > [auto build test ERROR on linus/master]
> > > > > [cannot apply to v5.3-rc4 next-20190816]
> > > > > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system]
> > > >
> > > > ... those patches should be applied to staging tree
> > > > since linux-next has not been updated yet...
> > > thanks for the feedback, we will consider this to our todo list.
> > 
> > Yes, many confusing reports anyway...
> > (Just my personal suggestion, maybe we can add some hints on the patch email
> > to indicate which tree can be applied successfully for ci in the future...)
> thanks, this is good idea. On the other side, we support to add --base option to git format-patch
> to automatically suggest the base, refer to https://stackoverflow.com/a/37406982 for detail.

Thanks for your information :) It seems a new patch format,
I will take a try later.

Thanks,
Gao Xiang

> Meanwhile, we will enhance the internal logic to find suitable base if possible.
> 
> > 
> > Thanks,
> > Gao Xiang
> > 
> > >
> > > >
> > > > Thanks,
> > > > Gao Xiang
> > > >
> > > > >
> > > > > url:    https://github.com/0day-ci/linux/commits/Gao-Xiang/staging-erofs-fix-
> > an-error-handling-in-erofs_readdir/20190818-191344
> > > > > config: arm64-allyesconfig (attached as .config)
> > > > > compiler: aarch64-linux-gcc (GCC) 7.4.0
> > > > > reproduce:
> > > > >         wget https://raw.githubusercontent.com/intel/lkp-
> > tests/master/sbin/make.cross -O ~/bin/make.cross
> > > > >         chmod +x ~/bin/make.cross
> > > > >         # save the attached .config to linux build tree
> > > > >         GCC_VERSION=7.4.0 make.cross ARCH=arm64
> > > > >
> > > > > If you fix the issue, kindly add following tag
> > > > > Reported-by: kbuild test robot <lkp@intel.com>
> > > > >
> > > > > All errors (new ones prefixed by >>):
> > > > >
> > > > >    drivers/staging/erofs/dir.c: In function 'erofs_readdir':
> > > > > >> drivers/staging/erofs/dir.c:110:11: error: 'EFSCORRUPTED' undeclared
> > (first use in this function); did you mean 'FS_NRSUPER'?
> > > > >        err = -EFSCORRUPTED;
> > > > >               ^~~~~~~~~~~~
> > > > >               FS_NRSUPER
> > > > >    drivers/staging/erofs/dir.c:110:11: note: each undeclared identifier is
> > reported only once for each function it appears in
> > > > >
> > > > > vim +110 drivers/staging/erofs/dir.c
> > > > >
> > > > >     85
> > > > >     86	static int erofs_readdir(struct file *f, struct dir_context *ctx)
> > > > >     87	{
> > > > >     88		struct inode *dir = file_inode(f);
> > > > >     89		struct address_space *mapping = dir->i_mapping;
> > > > >     90		const size_t dirsize = i_size_read(dir);
> > > > >     91		unsigned int i = ctx->pos / EROFS_BLKSIZ;
> > > > >     92		unsigned int ofs = ctx->pos % EROFS_BLKSIZ;
> > > > >     93		int err = 0;
> > > > >     94		bool initial = true;
> > > > >     95
> > > > >     96		while (ctx->pos < dirsize) {
> > > > >     97			struct page *dentry_page;
> > > > >     98			struct erofs_dirent *de;
> > > > >     99			unsigned int nameoff, maxsize;
> > > > >    100
> > > > >    101			dentry_page = read_mapping_page(mapping, i,
> > NULL);
> > > > >    102			if (dentry_page == ERR_PTR(-ENOMEM)) {
> > > > >    103				errln("no memory to readdir of logical
> > block %u of nid %llu",
> > > > >    104				      i, EROFS_V(dir)->nid);
> > > > >    105				err = -ENOMEM;
> > > > >    106				break;
> > > > >    107			} else if (IS_ERR(dentry_page)) {
> > > > >    108				errln("fail to readdir of logical block %u of
> > nid %llu",
> > > > >    109				      i, EROFS_V(dir)->nid);
> > > > >  > 110				err = -EFSCORRUPTED;
> > > > >    111				break;
> > > > >    112			}
> > > > >    113
> > > > >    114			de = (struct erofs_dirent *)kmap(dentry_page);
> > > > >    115
> > > > >    116			nameoff = le16_to_cpu(de->nameoff);
> > > > >    117
> > > > >    118			if (unlikely(nameoff < sizeof(struct erofs_dirent) ||
> > > > >    119				     nameoff >= PAGE_SIZE)) {
> > > > >    120				errln("%s, invalid de[0].nameoff %u",
> > > > >    121				      __func__, nameoff);
> > > > >    122
> > > > >    123				err = -EIO;
> > > > >    124				goto skip_this;
> > > > >    125			}
> > > > >    126
> > > > >    127			maxsize = min_t(unsigned int,
> > > > >    128					dirsize - ctx->pos + ofs,
> > PAGE_SIZE);
> > > > >    129
> > > > >    130			/* search dirents at the arbitrary position */
> > > > >    131			if (unlikely(initial)) {
> > > > >    132				initial = false;
> > > > >    133
> > > > >    134				ofs = roundup(ofs, sizeof(struct
> > erofs_dirent));
> > > > >    135				if (unlikely(ofs >= nameoff))
> > > > >    136					goto skip_this;
> > > > >    137			}
> > > > >    138
> > > > >    139			err = erofs_fill_dentries(ctx, de, &ofs, nameoff,
> > maxsize);
> > > > >    140	skip_this:
> > > > >    141			kunmap(dentry_page);
> > > > >    142
> > > > >    143			put_page(dentry_page);
> > > > >    144
> > > > >    145			ctx->pos = blknr_to_addr(i) + ofs;
> > > > >    146
> > > > >    147			if (unlikely(err))
> > > > >    148				break;
> > > > >    149			++i;
> > > > >    150			ofs = 0;
> > > > >    151		}
> > > > >    152		return err < 0 ? err : 0;
> > > > >    153	}
> > > > >    154
> > > > >
> > > > > ---
> > > > > 0-DAY kernel test infrastructure                Open Source Technology Center
> > > > > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> > > >
> > > >

[PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

From: Gao Xiang <gaoxiang25@huawei.com>

Richard observed a forever loop of erofs_read_raw_page() [1]
which can be generated by forcely setting ->u.i_blkaddr
to 0xdeadbeef (as my understanding block layer can
handle access beyond end of device correctly).

After digging into that, it seems the problem is highly
related with directories and then I found the root cause
is an improper error handling in erofs_readdir().

Let's fix it now.

[1] https://lore.kernel.org/r/1163995781.68824.1566084358245.JavaMail.zimbra@nod.at/

Reported-by: Richard Weinberger <richard@nod.at>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <stable@vger.kernel.org> # 4.19+
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
[RESEND] --> add the missing v3 version in subject, no logic change.

changelog from v2:
 - transform EIO to EFSCORRUPTED as suggested by Matthew;

changelog from v1:
 - fix the incorrect external link in commit message.

This patch is based on the following patch as well
https://lore.kernel.org/r/20190816071142.8633-1-gaoxiang25@huawei.com/

and
https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao@aol.com/
can still be properly applied after this patch.

 drivers/staging/erofs/dir.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c
index 5f38382637e6..eb430a031b20 100644
--- a/drivers/staging/erofs/dir.c
+++ b/drivers/staging/erofs/dir.c
@@ -82,8 +82,17 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 		unsigned int nameoff, maxsize;
 
 		dentry_page = read_mapping_page(mapping, i, NULL);
-		if (IS_ERR(dentry_page))
-			continue;
+		if (dentry_page == ERR_PTR(-ENOMEM)) {
+			errln("no memory to readdir of logical block %u of nid %llu",
+			      i, EROFS_V(dir)->nid);
+			err = -ENOMEM;
+			break;
+		} else if (IS_ERR(dentry_page)) {
+			errln("fail to readdir of logical block %u of nid %llu",
+			      i, EROFS_V(dir)->nid);
+			err = -EFSCORRUPTED;
+			break;
+		}
 
 		de = (struct erofs_dirent *)kmap(dentry_page);
 
-- 
2.17.1

Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

[Richard Weinberger]

----- Ursprüngliche Mail -----
> changelog from v2:
> - transform EIO to EFSCORRUPTED as suggested by Matthew;

erofs does not define EFSCORRUPTED, so the build fails.
At least on Linus' tree as of today.

Thanks,
//richard

Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

Hi Richard,

On Sun, Aug 18, 2019 at 10:33:33AM +0200, Richard Weinberger wrote:
> ----- Urspr?ngliche Mail -----
> > changelog from v2:
> > - transform EIO to EFSCORRUPTED as suggested by Matthew;
> 
> erofs does not define EFSCORRUPTED, so the build fails.
> At least on Linus' tree as of today.

Thanks for your reply :)

I write all patches based on staging tree and do more cleanups further
than Linus' tree, EFSCORRUPTED was already introduced by Pavel days before...
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=a6b9b1d5eae61a68085030e50d56265dec5baa94

which can be fetched from
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git -b staging-next

Thanks,
Gao Xiang

> 
> Thanks,
> //richard

Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()y

[Gao Xiang]

On Sun, Aug 18, 2019 at 05:10:38PM +0800, Gao Xiang via Linux-erofs wrote:
> Hi Richard,
> 
> On Sun, Aug 18, 2019 at 10:33:33AM +0200, Richard Weinberger wrote:
> > ----- Urspr?ngliche Mail -----
> > > changelog from v2:
> > > - transform EIO to EFSCORRUPTED as suggested by Matthew;
> > 
> > erofs does not define EFSCORRUPTED, so the build fails.
> > At least on Linus' tree as of today.
> 
> Thanks for your reply :)
> 
> I write all patches based on staging tree and do more cleanups further
> than Linus' tree, EFSCORRUPTED was already introduced by Pavel days before...

Sorry, I mean "introduced which was suggested by Paval"...

> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=a6b9b1d5eae61a68085030e50d56265dec5baa94
> 
> which can be fetched from
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git -b staging-next
> 
> Thanks,
> Gao Xiang
> 
> > 
> > Thanks,
> > //richard

Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

[Chao Yu]

On 2019-8-18 11:21, Gao Xiang wrote:
> From: Gao Xiang <gaoxiang25@huawei.com>
> 
> Richard observed a forever loop of erofs_read_raw_page() [1]
> which can be generated by forcely setting ->u.i_blkaddr
> to 0xdeadbeef (as my understanding block layer can
> handle access beyond end of device correctly).
> 
> After digging into that, it seems the problem is highly
> related with directories and then I found the root cause
> is an improper error handling in erofs_readdir().
> 
> Let's fix it now.
> 
> [1] https://lore.kernel.org/r/1163995781.68824.1566084358245.JavaMail.zimbra@nod.at/
> 
> Reported-by: Richard Weinberger <richard@nod.at>
> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
> Cc: <stable@vger.kernel.org> # 4.19+
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

[Matthew Wilcox]

On Sun, Aug 18, 2019 at 11:21:11AM +0800, Gao Xiang wrote:
> +		if (dentry_page == ERR_PTR(-ENOMEM)) {
> +			errln("no memory to readdir of logical block %u of nid %llu",
> +			      i, EROFS_V(dir)->nid);

I don't think you need the error message.  If we get a memory allocation
failure, there's already going to be a lot of spew in the logs from the
mm system.  And if we do fail to allocate memory, we don't need to know
the logical block number or the nid -- it has nothiing to do with those;
the system simply ran out of memory.

> +			err = -ENOMEM;
> +			break;
> +		} else if (IS_ERR(dentry_page)) {
> +			errln("fail to readdir of logical block %u of nid %llu",
> +			      i, EROFS_V(dir)->nid);
> +			err = -EFSCORRUPTED;
> +			break;
> +		}
>  
>  		de = (struct erofs_dirent *)kmap(dentry_page);
>  
> -- 
> 2.17.1
> 

Re: [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

On Sun, Aug 18, 2019 at 05:33:14AM -0700, Matthew Wilcox wrote:
> On Sun, Aug 18, 2019 at 11:21:11AM +0800, Gao Xiang wrote:
> > +		if (dentry_page == ERR_PTR(-ENOMEM)) {
> > +			errln("no memory to readdir of logical block %u of nid %llu",
> > +			      i, EROFS_V(dir)->nid);
> 
> I don't think you need the error message.  If we get a memory allocation
> failure, there's already going to be a lot of spew in the logs from the
> mm system.  And if we do fail to allocate memory, we don't need to know
> the logical block number or the nid -- it has nothiing to do with those;
> the system simply ran out of memory.

OK, I agree with you. There is a messy of messages when
memory allocation fail.

Since I don't really care apart from crashing or hanging
the kernel, I will resend the patch to make you and Chao
happy... :)

Thanks,
Gao Xiang

> 
> > +			err = -ENOMEM;
> > +			break;
> > +		} else if (IS_ERR(dentry_page)) {
> > +			errln("fail to readdir of logical block %u of nid %llu",
> > +			      i, EROFS_V(dir)->nid);
> > +			err = -EFSCORRUPTED;
> > +			break;
> > +		}
> >  
> >  		de = (struct erofs_dirent *)kmap(dentry_page);
> >  
> > -- 
> > 2.17.1
> > 

[PATCH v4] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

From: Gao Xiang <gaoxiang25@huawei.com>

Richard observed a forever loop of erofs_read_raw_page() [1]
which can be generated by forcely setting ->u.i_blkaddr
to 0xdeadbeef (as my understanding block layer can
handle access beyond end of device correctly).

After digging into that, it seems the problem is highly
related with directories and then I found the root cause
is an improper error handling in erofs_readdir().

Let's fix it now.

[1] https://lore.kernel.org/r/1163995781.68824.1566084358245.JavaMail.zimbra@nod.at/

Reported-by: Richard Weinberger <richard@nod.at>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <stable@vger.kernel.org> # 4.19+
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
changelog from v3:
 - kill message when memory allocation fails as suggested by Matthew;

[RESEND] --> add the missing v3 version in subject, no logic change.

changelog from v2:
 - transform EIO to EFSCORRUPTED as suggested by Matthew;

changelog from v1:
 - fix the incorrect external link in commit message.

This patch is based on staging-testing tree and
https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao@aol.com/
can still be properly applied after this patch.

 drivers/staging/erofs/dir.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/dir.c b/drivers/staging/erofs/dir.c
index 5f38382637e6..77ef856df9f3 100644
--- a/drivers/staging/erofs/dir.c
+++ b/drivers/staging/erofs/dir.c
@@ -82,8 +82,15 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
 		unsigned int nameoff, maxsize;
 
 		dentry_page = read_mapping_page(mapping, i, NULL);
-		if (IS_ERR(dentry_page))
-			continue;
+		if (dentry_page == ERR_PTR(-ENOMEM)) {
+			err = -ENOMEM;
+			break;
+		} else if (IS_ERR(dentry_page)) {
+			errln("fail to readdir of logical block %u of nid %llu",
+			      i, EROFS_V(dir)->nid);
+			err = -EFSCORRUPTED;
+			break;
+		}
 
 		de = (struct erofs_dirent *)kmap(dentry_page);
 
-- 
2.17.1

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Chao Yu]

On 2019-8-18 10:53, Matthew Wilcox wrote:
> On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
>> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
>>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
>>>> @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>>>>  		unsigned int nameoff, maxsize;
>>>>  
>>>>  		dentry_page = read_mapping_page(mapping, i, NULL);
>>>> -		if (IS_ERR(dentry_page))
>>>> -			continue;
>>>> +		if (IS_ERR(dentry_page)) {
>>>> +			errln("fail to readdir of logical block %u of nid %llu",
>>>> +			      i, EROFS_V(dir)->nid);
>>>> +			err = PTR_ERR(dentry_page);
>>>> +			break;
>>>
>>> I don't think you want to use the errno that came back from
>>> read_mapping_page() (which is, I think, always going to be -EIO).
>>> Rather you want -EFSCORRUPTED, at least if I understand the recent
>>> patches to ext2/ext4/f2fs/xfs/...
>>
>> Thanks for your reply and noticing this. :)
>>
>> Yes, as I talked with you about read_mapping_page() in a xfs related
>> topic earlier, I think I fully understand what returns here.
>>
>> I actually had some concern about that before sending out this patch.
>> You know the status is
>>    PG_uptodate is not set and PG_error is set here.
>>
>> But we cannot know it is actually a disk read error or due to
>> corrupted images (due to lack of page flags or some status, and
>> I think it could be a waste of page structure space for such
>> corrupted image or disk error)...
>>
>> And some people also like propagate errors from insiders...
>> (and they could argue about err = -EFSCORRUPTED as well..)
>>
>> I'd like hear your suggestion about this after my words above?
>> still return -EFSCORRUPTED?
> 
> I don't think it matters whether it's due to a disk error or a corrupted
> image.  We can't read the directory entry, so we should probably return
> -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
> also return -ENOMEM, so it should probably look something like this:
> 
> 		err = 0;
> 		if (dentry_page == ERR_PTR(-ENOMEM))
> 			err = -ENOMEM;
> 		else if (IS_ERR(dentry_page)) {
> 			errln("fail to readdir of logical block %u of nid %llu",
> 			      i, EROFS_V(dir)->nid);
> 			err = -EFSCORRUPTED;

Well, if there is real IO error happen under filesystem, we should return -EIO
instead of EFSCORRUPTED?

The right fix may be that doing sanity check on on-disk blkaddr, and return
-EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller
erofs_readdir(), IIUC below error info.

> [36297.354090] attempt to access beyond end of device
> [36297.354098] loop17: rw=0, want=29887428984, limit=1953128
> [36297.354107] attempt to access beyond end of device
> [36297.354109] loop17: rw=0, want=29887428480, limit=1953128
> [36301.827234] attempt to access beyond end of device
> [36301.827243] loop17: rw=0, want=29887428480, limit=1953128

Thanks,

> 		}
> 
> 		if (err)
> 			break;
> 

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Gao Xiang]

Hi Chao,

On Sun, Aug 18, 2019 at 06:39:52PM +0800, Chao Yu wrote:
> On 2019-8-18 10:53, Matthew Wilcox wrote:
> > On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
> >> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
> >>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
> >>>> @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
> >>>>  		unsigned int nameoff, maxsize;
> >>>>  
> >>>>  		dentry_page = read_mapping_page(mapping, i, NULL);
> >>>> -		if (IS_ERR(dentry_page))
> >>>> -			continue;
> >>>> +		if (IS_ERR(dentry_page)) {
> >>>> +			errln("fail to readdir of logical block %u of nid %llu",
> >>>> +			      i, EROFS_V(dir)->nid);
> >>>> +			err = PTR_ERR(dentry_page);
> >>>> +			break;
> >>>
> >>> I don't think you want to use the errno that came back from
> >>> read_mapping_page() (which is, I think, always going to be -EIO).
> >>> Rather you want -EFSCORRUPTED, at least if I understand the recent
> >>> patches to ext2/ext4/f2fs/xfs/...
> >>
> >> Thanks for your reply and noticing this. :)
> >>
> >> Yes, as I talked with you about read_mapping_page() in a xfs related
> >> topic earlier, I think I fully understand what returns here.
> >>
> >> I actually had some concern about that before sending out this patch.
> >> You know the status is
> >>    PG_uptodate is not set and PG_error is set here.
> >>
> >> But we cannot know it is actually a disk read error or due to
> >> corrupted images (due to lack of page flags or some status, and
> >> I think it could be a waste of page structure space for such
> >> corrupted image or disk error)...
> >>
> >> And some people also like propagate errors from insiders...
> >> (and they could argue about err = -EFSCORRUPTED as well..)
> >>
> >> I'd like hear your suggestion about this after my words above?
> >> still return -EFSCORRUPTED?
> > 
> > I don't think it matters whether it's due to a disk error or a corrupted
> > image.  We can't read the directory entry, so we should probably return
> > -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
> > also return -ENOMEM, so it should probably look something like this:
> > 
> > 		err = 0;
> > 		if (dentry_page == ERR_PTR(-ENOMEM))
> > 			err = -ENOMEM;
> > 		else if (IS_ERR(dentry_page)) {
> > 			errln("fail to readdir of logical block %u of nid %llu",
> > 			      i, EROFS_V(dir)->nid);
> > 			err = -EFSCORRUPTED;
> 
> Well, if there is real IO error happen under filesystem, we should return -EIO
> instead of EFSCORRUPTED?
> 
> The right fix may be that doing sanity check on on-disk blkaddr, and return
> -EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller
> erofs_readdir(), IIUC below error info.

In my thought, I actually don't care what is actually returned
(In other words, I have no tendency about EFSCORRUPTED / EIO)
as long as it behaves normal for corrupted image.

A little concern is that I have no idea whether all user problems
can handle EUCLEAN properly.

I don't want to limit blkaddr as what ->blocks recorded in
erofs_super_block since it's already used for our hotpatching
approach and that field is only used for statfs() for users
to know its visible size, and block layer will block such
invalid block access.

All in all, that is minor. I think we can fix as what Matthew said.

Thanks,
Gao Xiang

> 
> > [36297.354090] attempt to access beyond end of device
> > [36297.354098] loop17: rw=0, want=29887428984, limit=1953128
> > [36297.354107] attempt to access beyond end of device
> > [36297.354109] loop17: rw=0, want=29887428480, limit=1953128
> > [36301.827234] attempt to access beyond end of device
> > [36301.827243] loop17: rw=0, want=29887428480, limit=1953128
> 
> Thanks,
> 
> > 		}
> > 
> > 		if (err)
> > 			break;
> > 

Re: [PATCH v2] staging: erofs: fix an error handling in erofs_readdir()

[Chao Yu]

Hi Xiang,

On 2019-8-18 18:52, Gao Xiang wrote:
> Hi Chao,
> 
> On Sun, Aug 18, 2019 at 06:39:52PM +0800, Chao Yu wrote:
>> On 2019-8-18 10:53, Matthew Wilcox wrote:
>>> On Sun, Aug 18, 2019 at 10:32:45AM +0800, Gao Xiang wrote:
>>>> On Sat, Aug 17, 2019 at 07:20:55PM -0700, Matthew Wilcox wrote:
>>>>> On Sun, Aug 18, 2019 at 09:56:31AM +0800, Gao Xiang wrote:
>>>>>> @@ -82,8 +82,12 @@ static int erofs_readdir(struct file *f, struct dir_context *ctx)
>>>>>>  		unsigned int nameoff, maxsize;
>>>>>>  
>>>>>>  		dentry_page = read_mapping_page(mapping, i, NULL);
>>>>>> -		if (IS_ERR(dentry_page))
>>>>>> -			continue;
>>>>>> +		if (IS_ERR(dentry_page)) {
>>>>>> +			errln("fail to readdir of logical block %u of nid %llu",
>>>>>> +			      i, EROFS_V(dir)->nid);
>>>>>> +			err = PTR_ERR(dentry_page);
>>>>>> +			break;
>>>>>
>>>>> I don't think you want to use the errno that came back from
>>>>> read_mapping_page() (which is, I think, always going to be -EIO).
>>>>> Rather you want -EFSCORRUPTED, at least if I understand the recent
>>>>> patches to ext2/ext4/f2fs/xfs/...
>>>>
>>>> Thanks for your reply and noticing this. :)
>>>>
>>>> Yes, as I talked with you about read_mapping_page() in a xfs related
>>>> topic earlier, I think I fully understand what returns here.
>>>>
>>>> I actually had some concern about that before sending out this patch.
>>>> You know the status is
>>>>    PG_uptodate is not set and PG_error is set here.
>>>>
>>>> But we cannot know it is actually a disk read error or due to
>>>> corrupted images (due to lack of page flags or some status, and
>>>> I think it could be a waste of page structure space for such
>>>> corrupted image or disk error)...
>>>>
>>>> And some people also like propagate errors from insiders...
>>>> (and they could argue about err = -EFSCORRUPTED as well..)
>>>>
>>>> I'd like hear your suggestion about this after my words above?
>>>> still return -EFSCORRUPTED?
>>>
>>> I don't think it matters whether it's due to a disk error or a corrupted
>>> image.  We can't read the directory entry, so we should probably return
>>> -EFSCORRUPTED.  Thinking about it some more, read_mapping_page() can
>>> also return -ENOMEM, so it should probably look something like this:
>>>
>>> 		err = 0;
>>> 		if (dentry_page == ERR_PTR(-ENOMEM))
>>> 			err = -ENOMEM;
>>> 		else if (IS_ERR(dentry_page)) {
>>> 			errln("fail to readdir of logical block %u of nid %llu",
>>> 			      i, EROFS_V(dir)->nid);
>>> 			err = -EFSCORRUPTED;
>>
>> Well, if there is real IO error happen under filesystem, we should return -EIO
>> instead of EFSCORRUPTED?
>>
>> The right fix may be that doing sanity check on on-disk blkaddr, and return
>> -EFSCORRUPTED if the blkaddr is invalid and propagate the error to its caller
>> erofs_readdir(), IIUC below error info.
> 
> In my thought, I actually don't care what is actually returned
> (In other words, I have no tendency about EFSCORRUPTED / EIO)
> as long as it behaves normal for corrupted image.

I doubt that user can and be willing to handle different error code in there
error path.

> 
> A little concern is that I have no idea whether all user problems
> can handle EUCLEAN properly.

Yes, I can see it's widely used in fileystem, I guess it would be better to
update manual of common fs interfaces to let user be aware of the meaning of it.

> 
> I don't want to limit blkaddr as what ->blocks recorded in
> erofs_super_block since it's already used for our hotpatching
> approach and that field is only used for statfs() for users
> to know its visible size, and block layer will block such
> invalid block access.
> 
> All in all, that is minor. I think we can fix as what Matthew said.

Yeah, as we discuss offline, we need keep flexibility on current version for
android, and maybe we can add a feature to check blkaddr validation later.

Thanks,

> 
> Thanks,
> Gao Xiang
> 
>>
>>> [36297.354090] attempt to access beyond end of device
>>> [36297.354098] loop17: rw=0, want=29887428984, limit=1953128
>>> [36297.354107] attempt to access beyond end of device
>>> [36297.354109] loop17: rw=0, want=29887428480, limit=1953128
>>> [36301.827234] attempt to access beyond end of device
>>> [36301.827243] loop17: rw=0, want=29887428480, limit=1953128
>>
>> Thanks,
>>
>>> 		}
>>>
>>> 		if (err)
>>> 			break;
>>>