* [PATCH] Btrfs: fix segment fault when doing dio read
@ 2017-04-07 20:11 Liu Bo
2017-04-09 17:08 ` Nikolay Borisov
0 siblings, 1 reply; 4+ messages in thread
From: Liu Bo @ 2017-04-07 20:11 UTC (permalink / raw)
To: linux-btrfs
Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
introduced this bug during iterating bio pages in dio read's endio hook,
and it could end up with segment fault of the dio reading task.
So the reason is 'if (nr_sectors--)', and it makes the code assume that
there is one more block in the same page, so page offset is increased and
the bio which is created to repair the bad block then has an incorrect
bvec.bv_offset, and a later access of the page content would throw a
segment fault.
This also adds ASSERT to check page offset against page size.
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
fs/btrfs/inode.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c875e68..5e71f1e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
start += sectorsize;
- if (nr_sectors--) {
+ nr_sectors--;
+ if (nr_sectors) {
pgoff += sectorsize;
+ ASSERT(pgoff < PAGE_SIZE);
goto next_block_or_try_again;
}
}
@@ -8074,8 +8076,10 @@ static int __btrfs_subio_endio_read(struct inode *inode,
ASSERT(nr_sectors);
- if (--nr_sectors) {
+ nr_sectors--;
+ if (nr_sectors) {
pgoff += sectorsize;
+ ASSERT(pgoff < PAGE_SIZE);
goto next_block;
}
}
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: fix segment fault when doing dio read
2017-04-07 20:11 [PATCH] Btrfs: fix segment fault when doing dio read Liu Bo
@ 2017-04-09 17:08 ` Nikolay Borisov
2017-04-10 16:26 ` David Sterba
2017-04-10 18:32 ` Liu Bo
0 siblings, 2 replies; 4+ messages in thread
From: Nikolay Borisov @ 2017-04-09 17:08 UTC (permalink / raw)
To: Liu Bo, linux-btrfs
On 7.04.2017 23:11, Liu Bo wrote:
> Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
> introduced this bug during iterating bio pages in dio read's endio hook,
> and it could end up with segment fault of the dio reading task.
>
> So the reason is 'if (nr_sectors--)', and it makes the code assume that
> there is one more block in the same page, so page offset is increased and
> the bio which is created to repair the bad block then has an incorrect
> bvec.bv_offset, and a later access of the page content would throw a
> segment fault.
>
> This also adds ASSERT to check page offset against page size.
>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
> fs/btrfs/inode.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c875e68..5e71f1e 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
>
> start += sectorsize;
>
> - if (nr_sectors--) {
> + nr_sectors--;
Why not if(--nr_sectors)? I know it's more of a style issue but it will
reduce the size of the diff ?
> + if (nr_sectors) {
> pgoff += sectorsize;
> + ASSERT(pgoff < PAGE_SIZE);
> goto next_block_or_try_again;
> }
> }
> @@ -8074,8 +8076,10 @@ static int __btrfs_subio_endio_read(struct inode *inode,
>
> ASSERT(nr_sectors);
>
> - if (--nr_sectors) {
> + nr_sectors--;
> + if (nr_sectors) {
> pgoff += sectorsize;
> + ASSERT(pgoff < PAGE_SIZE);
> goto next_block;
> }
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: fix segment fault when doing dio read
2017-04-09 17:08 ` Nikolay Borisov
@ 2017-04-10 16:26 ` David Sterba
2017-04-10 18:32 ` Liu Bo
1 sibling, 0 replies; 4+ messages in thread
From: David Sterba @ 2017-04-10 16:26 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: Liu Bo, linux-btrfs
On Sun, Apr 09, 2017 at 08:08:05PM +0300, Nikolay Borisov wrote:
>
>
> On 7.04.2017 23:11, Liu Bo wrote:
> > Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
> > introduced this bug during iterating bio pages in dio read's endio hook,
> > and it could end up with segment fault of the dio reading task.
> >
> > So the reason is 'if (nr_sectors--)', and it makes the code assume that
> > there is one more block in the same page, so page offset is increased and
> > the bio which is created to repair the bad block then has an incorrect
> > bvec.bv_offset, and a later access of the page content would throw a
> > segment fault.
> >
> > This also adds ASSERT to check page offset against page size.
> >
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> > fs/btrfs/inode.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index c875e68..5e71f1e 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
> >
> > start += sectorsize;
> >
> > - if (nr_sectors--) {
> > + nr_sectors--;
> Why not if(--nr_sectors)? I know it's more of a style issue but it will
> reduce the size of the diff ?
I don't think diff size is a problem here. Actually, this instance is
the only one I can find [1] that uses the decrement inside if, so the
change makes it consistent
<smpl>
@@
expression E;
@@
(
* if (E--)
{ ... }
|
* if (E++)
{ ... }
)
</smp>
(run as: spatch -sp_file smpl.cocci -dir fs/btrfs/)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Btrfs: fix segment fault when doing dio read
2017-04-09 17:08 ` Nikolay Borisov
2017-04-10 16:26 ` David Sterba
@ 2017-04-10 18:32 ` Liu Bo
1 sibling, 0 replies; 4+ messages in thread
From: Liu Bo @ 2017-04-10 18:32 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
On Sun, Apr 09, 2017 at 08:08:05PM +0300, Nikolay Borisov wrote:
>
>
> On 7.04.2017 23:11, Liu Bo wrote:
> > Commit 2dabb3248453 ("Btrfs: Direct I/O read: Work on sectorsized blocks")
> > introduced this bug during iterating bio pages in dio read's endio hook,
> > and it could end up with segment fault of the dio reading task.
> >
> > So the reason is 'if (nr_sectors--)', and it makes the code assume that
> > there is one more block in the same page, so page offset is increased and
> > the bio which is created to repair the bad block then has an incorrect
> > bvec.bv_offset, and a later access of the page content would throw a
> > segment fault.
> >
> > This also adds ASSERT to check page offset against page size.
> >
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> > fs/btrfs/inode.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index c875e68..5e71f1e 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -7972,8 +7972,10 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
> >
> > start += sectorsize;
> >
> > - if (nr_sectors--) {
> > + nr_sectors--;
> Why not if(--nr_sectors)? I know it's more of a style issue but it will
> reduce the size of the diff ?
It's error-prone, as shown by how the bug in the original commit was introduced.
Thanks,
-liubo
> > + if (nr_sectors) {
> > pgoff += sectorsize;
> > + ASSERT(pgoff < PAGE_SIZE);
> > goto next_block_or_try_again;
> > }
> > }
> > @@ -8074,8 +8076,10 @@ static int __btrfs_subio_endio_read(struct inode *inode,
> >
> > ASSERT(nr_sectors);
> >
> > - if (--nr_sectors) {
> > + nr_sectors--;
> > + if (nr_sectors) {
> > pgoff += sectorsize;
> > + ASSERT(pgoff < PAGE_SIZE);
> > goto next_block;
> > }
> > }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-10 18:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-07 20:11 [PATCH] Btrfs: fix segment fault when doing dio read Liu Bo
2017-04-09 17:08 ` Nikolay Borisov
2017-04-10 16:26 ` David Sterba
2017-04-10 18:32 ` Liu Bo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).