* [PATCH] staging: erofs: fix an error handling in erofs_readdir() @ 2019-08-18 1:48 Gao Xiang 2019-08-18 1:56 ` [PATCH v2] " Gao Xiang 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-18 1:48 UTC (permalink / raw) 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 at nod.at/ Reported-by: Richard Weinberger <richard at nod.at> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations") Cc: <stable at vger.kernel.org> # 4.19+ Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com> --- Which is based on the following patch as well https://lore.kernel.org/r/20190816071142.8633-1-gaoxiang25 at huawei.com/ and https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao at 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 1:48 [PATCH] staging: erofs: fix an error handling in erofs_readdir() Gao Xiang @ 2019-08-18 1:56 ` Gao Xiang 2019-08-18 2:20 ` Matthew Wilcox 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-18 1:56 UTC (permalink / raw) 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 at nod.at/ Reported-by: Richard Weinberger <richard at nod.at> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations") Cc: <stable at vger.kernel.org> # 4.19+ Signed-off-by: Gao Xiang <gaoxiang25 at 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 at huawei.com/ and https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao at 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 1:56 ` [PATCH v2] " Gao Xiang @ 2019-08-18 2:20 ` Matthew Wilcox 2019-08-18 2:32 ` Gao Xiang 0 siblings, 1 reply; 27+ messages in thread From: Matthew Wilcox @ 2019-08-18 2:20 UTC (permalink / raw) On Sun, Aug 18, 2019@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/... ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 2:20 ` Matthew Wilcox @ 2019-08-18 2:32 ` Gao Xiang 2019-08-18 2:53 ` Matthew Wilcox 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-18 2:32 UTC (permalink / raw) Hi Willy, On Sat, Aug 17, 2019@07:20:55PM -0700, Matthew Wilcox wrote: > On Sun, Aug 18, 2019@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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 2:32 ` Gao Xiang @ 2019-08-18 2:53 ` Matthew Wilcox 2019-08-18 3:01 ` Gao Xiang 2019-08-18 10:39 ` [PATCH v2] " Chao Yu 0 siblings, 2 replies; 27+ messages in thread From: Matthew Wilcox @ 2019-08-18 2:53 UTC (permalink / raw) On Sun, Aug 18, 2019@10:32:45AM +0800, Gao Xiang wrote: > On Sat, Aug 17, 2019@07:20:55PM -0700, Matthew Wilcox wrote: > > On Sun, Aug 18, 2019@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; ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 2:53 ` Matthew Wilcox @ 2019-08-18 3:01 ` Gao Xiang 2019-08-18 3:18 ` [PATCH] " Gao Xiang 2019-08-18 3:21 ` [PATCH v3 RESEND] " Gao Xiang 2019-08-18 10:39 ` [PATCH v2] " Chao Yu 1 sibling, 2 replies; 27+ messages in thread From: Gao Xiang @ 2019-08-18 3:01 UTC (permalink / raw) On Sat, Aug 17, 2019@07:53:39PM -0700, Matthew Wilcox wrote: > On Sun, Aug 18, 2019@10:32:45AM +0800, Gao Xiang wrote: > > On Sat, Aug 17, 2019@07:20:55PM -0700, Matthew Wilcox wrote: > > > On Sun, Aug 18, 2019@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; ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 3:01 ` Gao Xiang @ 2019-08-18 3:18 ` Gao Xiang 2019-08-18 12:07 ` kbuild test robot 2019-08-18 13:17 ` kbuild test robot 2019-08-18 3:21 ` [PATCH v3 RESEND] " Gao Xiang 1 sibling, 2 replies; 27+ messages in thread From: Gao Xiang @ 2019-08-18 3:18 UTC (permalink / raw) 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 at nod.at/ Reported-by: Richard Weinberger <richard at nod.at> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations") Cc: <stable at vger.kernel.org> # 4.19+ Signed-off-by: Gao Xiang <gaoxiang25 at 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 at huawei.com/ and https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao at 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 3:18 ` [PATCH] " Gao Xiang @ 2019-08-18 12:07 ` kbuild test robot 2019-08-18 13:17 ` kbuild test robot 1 sibling, 0 replies; 27+ messages in thread From: kbuild test robot @ 2019-08-18 12:07 UTC (permalink / raw) To: Gao Xiang Cc: devel, Richard Weinberger, Miao Xie, LKML, Matthew Wilcox, kbuild-all, Greg Kroah-Hartman, linux-fsdevel, stable, linux-erofs [-- 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 --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 3:18 ` [PATCH] " Gao Xiang 2019-08-18 12:07 ` kbuild test robot @ 2019-08-18 13:17 ` kbuild test robot 2019-08-18 13:25 ` Gao Xiang 1 sibling, 1 reply; 27+ messages in thread From: kbuild test robot @ 2019-08-18 13:17 UTC (permalink / raw) To: Gao Xiang Cc: devel, Richard Weinberger, Miao Xie, LKML, Matthew Wilcox, kbuild-all, Greg Kroah-Hartman, linux-fsdevel, stable, linux-erofs [-- 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 --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 13:17 ` kbuild test robot @ 2019-08-18 13:25 ` Gao Xiang 2019-08-20 6:50 ` Philip Li 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-18 13:25 UTC (permalink / raw) On Sun, Aug 18, 2019@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 at 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 13:25 ` Gao Xiang @ 2019-08-20 6:50 ` Philip Li 2019-08-20 6:50 ` Gao Xiang 0 siblings, 1 reply; 27+ messages in thread From: Philip Li @ 2019-08-20 6:50 UTC (permalink / raw) To: Gao Xiang Cc: devel, kbuild test robot, Miao Xie, LKML, stable, kbuild-all, linux-fsdevel, linux-erofs 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 > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir() 2019-08-20 6:50 ` Philip Li @ 2019-08-20 6:50 ` Gao Xiang 2019-08-20 6:58 ` Li, Philip 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-20 6:50 UTC (permalink / raw) To: Philip Li Cc: devel, kbuild test robot, Miao Xie, LKML, stable, kbuild-all, linux-fsdevel, linux-erofs 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 > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH] staging: erofs: fix an error handling in erofs_readdir() 2019-08-20 6:50 ` Gao Xiang @ 2019-08-20 6:58 ` Li, Philip 2019-08-20 7:16 ` Gao Xiang 0 siblings, 1 reply; 27+ messages in thread From: Li, Philip @ 2019-08-20 6:58 UTC (permalink / raw) To: Gao Xiang Cc: devel, lkp, Miao Xie, LKML, stable, kbuild-all, linux-fsdevel, linux-erofs > 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 > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] staging: erofs: fix an error handling in erofs_readdir() 2019-08-20 6:58 ` Li, Philip @ 2019-08-20 7:16 ` Gao Xiang 0 siblings, 0 replies; 27+ messages in thread From: Gao Xiang @ 2019-08-20 7:16 UTC (permalink / raw) To: Li, Philip Cc: devel, lkp, Miao Xie, LKML, stable, kbuild-all, linux-fsdevel, linux-erofs 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 > > > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 3:01 ` Gao Xiang 2019-08-18 3:18 ` [PATCH] " Gao Xiang @ 2019-08-18 3:21 ` Gao Xiang 2019-08-18 8:33 ` Richard Weinberger ` (3 more replies) 1 sibling, 4 replies; 27+ messages in thread From: Gao Xiang @ 2019-08-18 3:21 UTC (permalink / raw) 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 at nod.at/ Reported-by: Richard Weinberger <richard at nod.at> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations") Cc: <stable at vger.kernel.org> # 4.19+ Signed-off-by: Gao Xiang <gaoxiang25 at 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 at huawei.com/ and https://lore.kernel.org/r/20190817082313.21040-1-hsiangkao at 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 3:21 ` [PATCH v3 RESEND] " Gao Xiang @ 2019-08-18 8:33 ` Richard Weinberger 2019-08-18 9:10 ` Gao Xiang 2019-08-18 11:52 ` [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() Sasha Levin ` (2 subsequent siblings) 3 siblings, 1 reply; 27+ messages in thread From: Richard Weinberger @ 2019-08-18 8:33 UTC (permalink / raw) ----- 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 8:33 ` Richard Weinberger @ 2019-08-18 9:10 ` Gao Xiang 2019-08-18 9:18 ` [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()y Gao Xiang 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-18 9:10 UTC (permalink / raw) Hi Richard, On Sun, Aug 18, 2019@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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()y 2019-08-18 9:10 ` Gao Xiang @ 2019-08-18 9:18 ` Gao Xiang 0 siblings, 0 replies; 27+ messages in thread From: Gao Xiang @ 2019-08-18 9:18 UTC (permalink / raw) On Sun, Aug 18, 2019@05:10:38PM +0800, Gao Xiang via Linux-erofs wrote: > Hi Richard, > > On Sun, Aug 18, 2019@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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 3:21 ` [PATCH v3 RESEND] " Gao Xiang 2019-08-18 8:33 ` Richard Weinberger @ 2019-08-18 11:52 ` Sasha Levin 2019-08-18 12:29 ` Chao Yu 2019-08-18 12:33 ` Matthew Wilcox 3 siblings, 0 replies; 27+ messages in thread From: Sasha Levin @ 2019-08-18 11:52 UTC (permalink / raw) Hi, [This is an automated email] This commit has been processed because it contains a "Fixes:" tag, fixing commit: 3aa8ec716e52 staging: erofs: add directory operations. The bot has tested the following trees: v5.2.9, v4.19.67. v5.2.9: Build failed! Errors: drivers/staging/erofs/dir.c:111:11: error: ???EFSCORRUPTED??? undeclared (first use in this function); did you mean ???FS_NRSUPER???? v4.19.67: Build failed! Errors: drivers/staging/erofs/dir.c:111:11: error: ???EFSCORRUPTED??? undeclared (first use in this function); did you mean ???FS_NRSUPER???? NOTE: The patch will not be queued to stable trees until it is upstream. How should we proceed with this patch? -- Thanks, Sasha ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 3:21 ` [PATCH v3 RESEND] " Gao Xiang 2019-08-18 8:33 ` Richard Weinberger 2019-08-18 11:52 ` [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() Sasha Levin @ 2019-08-18 12:29 ` Chao Yu 2019-08-18 12:33 ` Matthew Wilcox 3 siblings, 0 replies; 27+ messages in thread From: Chao Yu @ 2019-08-18 12:29 UTC (permalink / raw) On 2019-8-18 11:21, Gao Xiang wrote: > From: Gao Xiang <gaoxiang25 at 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 at nod.at/ > > Reported-by: Richard Weinberger <richard at nod.at> > Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations") > Cc: <stable at vger.kernel.org> # 4.19+ > Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com> Reviewed-by: Chao Yu <yuchao0 at huawei.com> Thanks, ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 3:21 ` [PATCH v3 RESEND] " Gao Xiang ` (2 preceding siblings ...) 2019-08-18 12:29 ` Chao Yu @ 2019-08-18 12:33 ` Matthew Wilcox 2019-08-18 12:38 ` Gao Xiang 3 siblings, 1 reply; 27+ messages in thread From: Matthew Wilcox @ 2019-08-18 12:33 UTC (permalink / raw) On Sun, Aug 18, 2019@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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 12:33 ` Matthew Wilcox @ 2019-08-18 12:38 ` Gao Xiang 2019-08-18 12:54 ` [PATCH v4] " Gao Xiang 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-18 12:38 UTC (permalink / raw) On Sun, Aug 18, 2019@05:33:14AM -0700, Matthew Wilcox wrote: > On Sun, Aug 18, 2019@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 > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v4] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 12:38 ` Gao Xiang @ 2019-08-18 12:54 ` Gao Xiang 2019-08-19 0:08 ` Sasha Levin 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-18 12:54 UTC (permalink / raw) 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 at nod.at/ Reported-by: Richard Weinberger <richard at nod.at> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations") Cc: <stable at vger.kernel.org> # 4.19+ Reviewed-by: Chao Yu <yuchao0 at huawei.com> Signed-off-by: Gao Xiang <gaoxiang25 at 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 at 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v4] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 12:54 ` [PATCH v4] " Gao Xiang @ 2019-08-19 0:08 ` Sasha Levin 0 siblings, 0 replies; 27+ messages in thread From: Sasha Levin @ 2019-08-19 0:08 UTC (permalink / raw) Hi, [This is an automated email] This commit has been processed because it contains a "Fixes:" tag, fixing commit: 3aa8ec716e52 staging: erofs: add directory operations. The bot has tested the following trees: v5.2.9, v4.19.67. v5.2.9: Build failed! Errors: drivers/staging/erofs/dir.c:109:11: error: ???EFSCORRUPTED??? undeclared (first use in this function); did you mean ???FS_NRSUPER???? v4.19.67: Build failed! Errors: drivers/staging/erofs/dir.c:109:11: error: ???EFSCORRUPTED??? undeclared (first use in this function); did you mean ???FS_NRSUPER???? NOTE: The patch will not be queued to stable trees until it is upstream. How should we proceed with this patch? -- Thanks, Sasha ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 2:53 ` Matthew Wilcox 2019-08-18 3:01 ` Gao Xiang @ 2019-08-18 10:39 ` Chao Yu 2019-08-18 10:52 ` Gao Xiang 1 sibling, 1 reply; 27+ messages in thread From: Chao Yu @ 2019-08-18 10:39 UTC (permalink / raw) On 2019-8-18 10:53, Matthew Wilcox wrote: > On Sun, Aug 18, 2019@10:32:45AM +0800, Gao Xiang wrote: >> On Sat, Aug 17, 2019@07:20:55PM -0700, Matthew Wilcox wrote: >>> On Sun, Aug 18, 2019@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; > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 10:39 ` [PATCH v2] " Chao Yu @ 2019-08-18 10:52 ` Gao Xiang 2019-08-18 12:28 ` Chao Yu 0 siblings, 1 reply; 27+ messages in thread From: Gao Xiang @ 2019-08-18 10:52 UTC (permalink / raw) Hi Chao, On Sun, Aug 18, 2019@06:39:52PM +0800, Chao Yu wrote: > On 2019-8-18 10:53, Matthew Wilcox wrote: > > On Sun, Aug 18, 2019@10:32:45AM +0800, Gao Xiang wrote: > >> On Sat, Aug 17, 2019@07:20:55PM -0700, Matthew Wilcox wrote: > >>> On Sun, Aug 18, 2019@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; > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] staging: erofs: fix an error handling in erofs_readdir() 2019-08-18 10:52 ` Gao Xiang @ 2019-08-18 12:28 ` Chao Yu 0 siblings, 0 replies; 27+ messages in thread From: Chao Yu @ 2019-08-18 12:28 UTC (permalink / raw) Hi Xiang, On 2019-8-18 18:52, Gao Xiang wrote: > Hi Chao, > > On Sun, Aug 18, 2019@06:39:52PM +0800, Chao Yu wrote: >> On 2019-8-18 10:53, Matthew Wilcox wrote: >>> On Sun, Aug 18, 2019@10:32:45AM +0800, Gao Xiang wrote: >>>> On Sat, Aug 17, 2019@07:20:55PM -0700, Matthew Wilcox wrote: >>>>> On Sun, Aug 18, 2019@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; >>> ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2019-08-21 10:45 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-18 1:48 [PATCH] staging: erofs: fix an error handling in erofs_readdir() Gao Xiang 2019-08-18 1:56 ` [PATCH v2] " Gao Xiang 2019-08-18 2:20 ` Matthew Wilcox 2019-08-18 2:32 ` Gao Xiang 2019-08-18 2:53 ` Matthew Wilcox 2019-08-18 3:01 ` Gao Xiang 2019-08-18 3:18 ` [PATCH] " Gao Xiang 2019-08-18 12:07 ` kbuild test robot 2019-08-18 13:17 ` kbuild test robot 2019-08-18 13:25 ` Gao Xiang 2019-08-20 6:50 ` Philip Li 2019-08-20 6:50 ` Gao Xiang 2019-08-20 6:58 ` Li, Philip 2019-08-20 7:16 ` Gao Xiang 2019-08-18 3:21 ` [PATCH v3 RESEND] " Gao Xiang 2019-08-18 8:33 ` Richard Weinberger 2019-08-18 9:10 ` Gao Xiang 2019-08-18 9:18 ` [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir()y Gao Xiang 2019-08-18 11:52 ` [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() Sasha Levin 2019-08-18 12:29 ` Chao Yu 2019-08-18 12:33 ` Matthew Wilcox 2019-08-18 12:38 ` Gao Xiang 2019-08-18 12:54 ` [PATCH v4] " Gao Xiang 2019-08-19 0:08 ` Sasha Levin 2019-08-18 10:39 ` [PATCH v2] " Chao Yu 2019-08-18 10:52 ` Gao Xiang 2019-08-18 12:28 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).