* [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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 1:48 UTC (permalink / raw) To: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel Cc: LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 ^ permalink raw reply related [flat|nested] 25+ 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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 1:56 UTC (permalink / raw) To: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel Cc: LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Matthew Wilcox @ 2019-08-18 2:20 UTC (permalink / raw) To: Gao Xiang Cc: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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/... ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 2:32 UTC (permalink / raw) To: Matthew Wilcox Cc: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Matthew Wilcox @ 2019-08-18 2:53 UTC (permalink / raw) To: Gao Xiang Cc: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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; ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 3:01 UTC (permalink / raw) To: Matthew Wilcox Cc: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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; ^ permalink raw reply [flat|nested] 25+ 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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 3:18 UTC (permalink / raw) To: Chao Yu, Richard Weinberger, Matthew Wilcox, Greg Kroah-Hartman, devel, linux-fsdevel Cc: LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 ^ permalink raw reply related [flat|nested] 25+ 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; 25+ messages in thread From: kbuild test robot @ 2019-08-18 12:07 UTC (permalink / raw) To: Gao Xiang Cc: kbuild-all, Chao Yu, Richard Weinberger, Matthew Wilcox, Greg Kroah-Hartman, devel, linux-fsdevel, linux-erofs, Chao Yu, LKML, stable, Fang Wei, Miao Xie [-- 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] 25+ 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; 25+ messages in thread From: kbuild test robot @ 2019-08-18 13:17 UTC (permalink / raw) To: Gao Xiang Cc: kbuild-all, Chao Yu, Richard Weinberger, Matthew Wilcox, Greg Kroah-Hartman, devel, linux-fsdevel, linux-erofs, Chao Yu, LKML, stable, Fang Wei, Miao Xie [-- 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] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 13:25 UTC (permalink / raw) To: kbuild test robot Cc: kbuild-all, devel, linux-fsdevel, linux-erofs, LKML, stable, Fang Wei, Miao Xie 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 ^ permalink raw reply [flat|nested] 25+ 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; 25+ messages in thread From: Philip Li @ 2019-08-20 6:50 UTC (permalink / raw) To: Gao Xiang Cc: kbuild test robot, kbuild-all, devel, linux-fsdevel, linux-erofs, LKML, stable, Fang Wei, Miao Xie 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] 25+ 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; 25+ messages in thread From: Gao Xiang @ 2019-08-20 6:50 UTC (permalink / raw) To: Philip Li Cc: Gao Xiang, kbuild test robot, kbuild-all, devel, linux-fsdevel, linux-erofs, LKML, stable, Fang Wei, Miao Xie 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] 25+ 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; 25+ messages in thread From: Li, Philip @ 2019-08-20 6:58 UTC (permalink / raw) To: Gao Xiang Cc: Gao Xiang, lkp, kbuild-all, devel, linux-fsdevel, linux-erofs, LKML, stable, Fang Wei, Miao Xie > 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] 25+ 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; 25+ messages in thread From: Gao Xiang @ 2019-08-20 7:16 UTC (permalink / raw) To: Li, Philip Cc: Gao Xiang, lkp, kbuild-all, devel, linux-fsdevel, linux-erofs, LKML, stable, Fang Wei, Miao Xie 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] 25+ 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 ` (2 more replies) 1 sibling, 3 replies; 25+ messages in thread From: Gao Xiang @ 2019-08-18 3:21 UTC (permalink / raw) To: Chao Yu, Richard Weinberger, Matthew Wilcox, Greg Kroah-Hartman, devel, linux-fsdevel Cc: LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [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 12:29 ` [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() Chao Yu 2019-08-18 12:33 ` Matthew Wilcox 2 siblings, 1 reply; 25+ messages in thread From: Richard Weinberger @ 2019-08-18 8:33 UTC (permalink / raw) To: Gao Xiang Cc: Chao Yu, Matthew Wilcox, Greg Kroah-Hartman, devel, linux-fsdevel, linux-kernel, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable ----- 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] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 9:10 UTC (permalink / raw) To: Richard Weinberger Cc: Chao Yu, Matthew Wilcox, Greg Kroah-Hartman, devel, linux-fsdevel, linux-kernel, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 9:18 UTC (permalink / raw) To: Richard Weinberger, devel, Greg Kroah-Hartman, Miao Xie, linux-kernel, Matthew Wilcox, linux-fsdevel, stable, linux-erofs 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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 12:29 ` Chao Yu 2019-08-18 12:33 ` Matthew Wilcox 2 siblings, 0 replies; 25+ messages in thread From: Chao Yu @ 2019-08-18 12:29 UTC (permalink / raw) To: Gao Xiang, Chao Yu, Richard Weinberger, Matthew Wilcox, Greg Kroah-Hartman, devel, linux-fsdevel Cc: LKML, linux-erofs, Miao Xie, Fang Wei, Gao Xiang, stable 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, ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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 12:29 ` [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() Chao Yu @ 2019-08-18 12:33 ` Matthew Wilcox 2019-08-18 12:38 ` Gao Xiang 2 siblings, 1 reply; 25+ messages in thread From: Matthew Wilcox @ 2019-08-18 12:33 UTC (permalink / raw) To: Gao Xiang Cc: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 12:38 UTC (permalink / raw) To: Matthew Wilcox Cc: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 > > ^ permalink raw reply [flat|nested] 25+ 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 0 siblings, 0 replies; 25+ messages in thread From: Gao Xiang @ 2019-08-18 12:54 UTC (permalink / raw) To: Chao Yu, Richard Weinberger, Matthew Wilcox, Greg Kroah-Hartman, devel, linux-fsdevel Cc: LKML, linux-erofs, Chao Yu, Miao Xie, Fang Wei, Gao Xiang, stable 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Chao Yu @ 2019-08-18 10:39 UTC (permalink / raw) To: Matthew Wilcox, Gao Xiang Cc: Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Miao Xie, Fang Wei, Gao Xiang, stable 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; > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Gao Xiang @ 2019-08-18 10:52 UTC (permalink / raw) To: Chao Yu Cc: Matthew Wilcox, Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Miao Xie, Fang Wei, Gao Xiang, stable 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; > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [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; 25+ messages in thread From: Chao Yu @ 2019-08-18 12:28 UTC (permalink / raw) To: Gao Xiang Cc: Matthew Wilcox, Chao Yu, Richard Weinberger, Greg Kroah-Hartman, devel, linux-fsdevel, LKML, linux-erofs, Miao Xie, Fang Wei, Gao Xiang, stable 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; >>> ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2019-08-20 7:17 UTC | newest] Thread overview: 25+ 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 12:29 ` [PATCH v3 RESEND] staging: erofs: fix an error handling in erofs_readdir() 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-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).