From: Gao Xiang <[email protected]>
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/[email protected]/
Reported-by: Richard Weinberger <[email protected]>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <[email protected]> # 4.19+
Signed-off-by: Gao Xiang <[email protected]>
---
Which is based on the following patch as well
https://lore.kernel.org/r/[email protected]/
and
https://lore.kernel.org/r/[email protected]/
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
From: Gao Xiang <[email protected]>
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/[email protected]/
Reported-by: Richard Weinberger <[email protected]>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <[email protected]> # 4.19+
Signed-off-by: Gao Xiang <[email protected]>
---
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/[email protected]/
and
https://lore.kernel.org/r/[email protected]/
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
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 <[email protected]>
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/[email protected]/
Reported-by: Richard Weinberger <[email protected]>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <[email protected]> # 4.19+
Signed-off-by: Gao Xiang <[email protected]>
---
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/[email protected]/
and
https://lore.kernel.org/r/[email protected]/
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
From: Gao Xiang <[email protected]>
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/[email protected]/
Reported-by: Richard Weinberger <[email protected]>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <[email protected]> # 4.19+
Signed-off-by: Gao Xiang <[email protected]>
---
[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/[email protected]/
and
https://lore.kernel.org/r/[email protected]/
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
----- 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=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
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
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 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 <[email protected]>
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 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;
>>>
On 2019-8-18 11:21, Gao Xiang wrote:
> From: Gao Xiang <[email protected]>
>
> 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/[email protected]/
>
> Reported-by: Richard Weinberger <[email protected]>
> Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
> Cc: <[email protected]> # 4.19+
> Signed-off-by: Gao Xiang <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
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 <[email protected]>
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/[email protected]/
Reported-by: Richard Weinberger <[email protected]>
Fixes: 3aa8ec716e52 ("staging: erofs: add directory operations")
Cc: <[email protected]> # 4.19+
Reviewed-by: Chao Yu <[email protected]>
Signed-off-by: Gao Xiang <[email protected]>
---
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/[email protected]/
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
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 <[email protected]>
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-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 <[email protected]>
>
> 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: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 <[email protected]>
> >
> > 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
>
>
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 <[email protected]>
> > >
> > > 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
> >
> >
> 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 <[email protected]>
> > > >
> > > > 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 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 <[email protected]>
> > > > >
> > > > > 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
> > > >
> > > >