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...
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);
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...
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);
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/...
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
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;
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;
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...
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);
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-erro... 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
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-erro... 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
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-erro... 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_NRSUPERdrivers/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
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-erro... 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_NRSUPERdrivers/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
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-erro... 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_NRSUPERdrivers/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
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=arm64If 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_NRSUPERdrivers/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
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=arm64If 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_NRSUPERdrivers/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
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...
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);
----- 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
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...
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
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...
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
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...
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,
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
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
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...
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);
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;
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;
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;
linux-stable-mirror@lists.linaro.org
-
Chao Yu -
Gao Xiang -
Gao Xiang -
kbuild test robot -
Li, Philip -
Matthew Wilcox -
Philip Li
-
Richard Weinberger