Re: [PATCH] Btrfs: fix the corruption by reading stale btree blocks

[Nikolay Borisov]

On 18.05.2018 17:06, David Sterba wrote:
> On Fri, May 18, 2018 at 09:15:16PM +0800, Liu Bo wrote:
 Doesn't this warrant a stable tag and
 Fixes: 5bdd3536cbbe ("Btrfs: Fix block generation verification race")
>>>
>>> The patch will not apply to < 4.16 as it depends on the addition of
>>> _key check from 581c1760415c4, but yes we need it for stable.
>>
>> True, we can easily fix the conflicts by removing @first_key, can't
>> we?
> 
> Yes the backport is easy but still needs to be done manually and we
> can't and don't expect the stable maintainers to resolve the conflicts
> but rather send a adapter version ourselves. Nikolay promised to do
> that.

Once the patch gets sent to Linus I will backport it for 4.4/4.9 and 4.14

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix the corruption by reading stale btree blocks

[David Sterba]

On Fri, May 18, 2018 at 09:15:16PM +0800, Liu Bo wrote:
> > > Doesn't this warrant a stable tag and
> > > Fixes: 5bdd3536cbbe ("Btrfs: Fix block generation verification race")
> > 
> > The patch will not apply to < 4.16 as it depends on the addition of
> > _key check from 581c1760415c4, but yes we need it for stable.
> 
> True, we can easily fix the conflicts by removing @first_key, can't
> we?

Yes the backport is easy but still needs to be done manually and we
can't and don't expect the stable maintainers to resolve the conflicts
but rather send a adapter version ourselves. Nikolay promised to do
that.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix the corruption by reading stale btree blocks

[Liu Bo]

On Wed, May 16, 2018 at 04:12:21PM +0200, David Sterba wrote:
> On Wed, May 16, 2018 at 11:00:14AM +0300, Nikolay Borisov wrote:
> > 
> > 
> > On 15.05.2018 20:37, Liu Bo wrote:
> > > If a btree block, aka. extent buffer, is not available in the extent
> > > buffer cache, it'll be read out from the disk instead, i.e.
> > > 
> > > btrfs_search_slot()
> > >   read_block_for_search()  # hold parent and its lock, go to read child
> > > btrfs_release_path()
> > > read_tree_block()  # read child
> > > 
> > > Unfortunately, the parent lock got released before reading child, so
> > > commit 5bdd3536cbbe ("Btrfs: Fix block generation verification race") had
> > > used 0 as parent transid to read the child block.  It forces
> > > read_tree_block() not to check if parent transid is different with the
> > > generation id of the child that it reads out from disk.
> > > 
> > > A simple PoC is included in btrfs/124,
> > > 
> > > 0. A two-disk raid1 btrfs,
> > > 
> > > 1. Right after mkfs.btrfs, block A is allocated to be device tree's root.
> > > 
> > > 2. Mount this filesystem and put it in use, after a while, device tree's
> > >root got COW but block A hasn't been allocated/overwritten yet.
> > > 
> > > 3. Umount it and reload the btrfs module to remove both disks from the
> > >global @fs_devices list.
> > > 
> > > 4. mount -odegraded dev1 and write some data, so now block A is allocated
> > >to be a leaf in checksum tree.  Note that only dev1 has the latest
> > >metadata of this filesystem.
> > > 
> > > 5. Umount it and mount it again normally (with both disks), since raid1
> > >can pick up one disk by the writer task's pid, if btrfs_search_slot()
> > >needs to read block A, dev2 which does NOT have the latest metadata
> > >might be read for block A, then we got a stale block A.
> > > 
> > > 6. As parent transid is not checked, block A is marked as uptodate and
> > >put into the extent buffer cache, so the future search won't bother
> > >to read disk again, which means it'll make changes on this stale
> > >one and make it dirty and flush it onto disk.
> > > 
> > > To avoid the problem, parent transid needs to be passed to
> > > read_tree_block().
> > > 
> > > In order to get a valid parent transid, we need to hold the parent's
> > > lock until finish reading child.
> > > 
> > > Signed-off-by: Liu Bo 
> > 
> > Doesn't this warrant a stable tag and
> > Fixes: 5bdd3536cbbe ("Btrfs: Fix block generation verification race")
> 
> The patch will not apply to < 4.16 as it depends on the addition of
> _key check from 581c1760415c4, but yes we need it for stable.

True, we can easily fix the conflicts by removing @first_key, can't
we?

The problem does exist before it.

thanks,
-liubo
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix the corruption by reading stale btree blocks

[David Sterba]

On Wed, May 16, 2018 at 11:00:14AM +0300, Nikolay Borisov wrote:
> 
> 
> On 15.05.2018 20:37, Liu Bo wrote:
> > If a btree block, aka. extent buffer, is not available in the extent
> > buffer cache, it'll be read out from the disk instead, i.e.
> > 
> > btrfs_search_slot()
> >   read_block_for_search()  # hold parent and its lock, go to read child
> > btrfs_release_path()
> > read_tree_block()  # read child
> > 
> > Unfortunately, the parent lock got released before reading child, so
> > commit 5bdd3536cbbe ("Btrfs: Fix block generation verification race") had
> > used 0 as parent transid to read the child block.  It forces
> > read_tree_block() not to check if parent transid is different with the
> > generation id of the child that it reads out from disk.
> > 
> > A simple PoC is included in btrfs/124,
> > 
> > 0. A two-disk raid1 btrfs,
> > 
> > 1. Right after mkfs.btrfs, block A is allocated to be device tree's root.
> > 
> > 2. Mount this filesystem and put it in use, after a while, device tree's
> >root got COW but block A hasn't been allocated/overwritten yet.
> > 
> > 3. Umount it and reload the btrfs module to remove both disks from the
> >global @fs_devices list.
> > 
> > 4. mount -odegraded dev1 and write some data, so now block A is allocated
> >to be a leaf in checksum tree.  Note that only dev1 has the latest
> >metadata of this filesystem.
> > 
> > 5. Umount it and mount it again normally (with both disks), since raid1
> >can pick up one disk by the writer task's pid, if btrfs_search_slot()
> >needs to read block A, dev2 which does NOT have the latest metadata
> >might be read for block A, then we got a stale block A.
> > 
> > 6. As parent transid is not checked, block A is marked as uptodate and
> >put into the extent buffer cache, so the future search won't bother
> >to read disk again, which means it'll make changes on this stale
> >one and make it dirty and flush it onto disk.
> > 
> > To avoid the problem, parent transid needs to be passed to
> > read_tree_block().
> > 
> > In order to get a valid parent transid, we need to hold the parent's
> > lock until finish reading child.
> > 
> > Signed-off-by: Liu Bo 
> 
> Doesn't this warrant a stable tag and
> Fixes: 5bdd3536cbbe ("Btrfs: Fix block generation verification race")

The patch will not apply to < 4.16 as it depends on the addition of
_key check from 581c1760415c4, but yes we need it for stable.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix the corruption by reading stale btree blocks

[Anand Jain]

On 05/16/2018 01:37 AM, Liu Bo wrote:

If a btree block, aka. extent buffer, is not available in the extent
buffer cache, it'll be read out from the disk instead, i.e.

btrfs_search_slot()
   read_block_for_search()  # hold parent and its lock, go to read child
 btrfs_release_path()
 read_tree_block()  # read child

Unfortunately, the parent lock got released before reading child, so
commit 5bdd3536cbbe ("Btrfs: Fix block generation verification race") had
used 0 as parent transid to read the child block.  It forces
read_tree_block() not to check if parent transid is different with the
generation id of the child that it reads out from disk.

A simple PoC is included in btrfs/124,

0. A two-disk raid1 btrfs,

1. Right after mkfs.btrfs, block A is allocated to be device tree's root.

2. Mount this filesystem and put it in use, after a while, device tree's
root got COW but block A hasn't been allocated/overwritten yet.

3. Umount it and reload the btrfs module to remove both disks from the
global @fs_devices list.

4. mount -odegraded dev1 and write some data, so now block A is allocated
to be a leaf in checksum tree.  Note that only dev1 has the latest
metadata of this filesystem.

5. Umount it and mount it again normally (with both disks), since raid1
can pick up one disk by the writer task's pid, if btrfs_search_slot()
needs to read block A, dev2 which does NOT have the latest metadata
might be read for block A, then we got a stale block A.

6. As parent transid is not checked, block A is marked as uptodate and
put into the extent buffer cache, so the future search won't bother
to read disk again, which means it'll make changes on this stale
one and make it dirty and flush it onto disk.

To avoid the problem, parent transid needs to be passed to
read_tree_block().

In order to get a valid parent transid, we need to hold the parent's
lock until finish reading child.


 Thanks for the patch. I was trying to fix this as well.  I have
 a test case for this btrfs/161 which I just posted to the mailing
 list. And I find it still fails.

Thanks, Anand


Signed-off-by: Liu Bo 
---
  fs/btrfs/ctree.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3fd44835b386..b3f6f300e492 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2436,10 +2436,8 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path 
*path, int level)
if (p->reada != READA_NONE)
reada_for_search(fs_info, p, level, slot, key->objectid);
  
-	btrfs_release_path(p);

-
ret = -EAGAIN;
-   tmp = read_tree_block(fs_info, blocknr, 0, parent_level - 1,
+   tmp = read_tree_block(fs_info, blocknr, gen, parent_level - 1,
  _key);
if (!IS_ERR(tmp)) {
/*
@@ -2454,6 +2452,8 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path 
*path, int level)
} else {
ret = PTR_ERR(tmp);
}
+
+   btrfs_release_path(p);
return ret;
  }
  


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix the corruption by reading stale btree blocks

[Nikolay Borisov]

On 15.05.2018 20:37, Liu Bo wrote:
> If a btree block, aka. extent buffer, is not available in the extent
> buffer cache, it'll be read out from the disk instead, i.e.
> 
> btrfs_search_slot()
>   read_block_for_search()  # hold parent and its lock, go to read child
> btrfs_release_path()
> read_tree_block()  # read child
> 
> Unfortunately, the parent lock got released before reading child, so
> commit 5bdd3536cbbe ("Btrfs: Fix block generation verification race") had
> used 0 as parent transid to read the child block.  It forces
> read_tree_block() not to check if parent transid is different with the
> generation id of the child that it reads out from disk.
> 
> A simple PoC is included in btrfs/124,
> 
> 0. A two-disk raid1 btrfs,
> 
> 1. Right after mkfs.btrfs, block A is allocated to be device tree's root.
> 
> 2. Mount this filesystem and put it in use, after a while, device tree's
>root got COW but block A hasn't been allocated/overwritten yet.
> 
> 3. Umount it and reload the btrfs module to remove both disks from the
>global @fs_devices list.
> 
> 4. mount -odegraded dev1 and write some data, so now block A is allocated
>to be a leaf in checksum tree.  Note that only dev1 has the latest
>metadata of this filesystem.
> 
> 5. Umount it and mount it again normally (with both disks), since raid1
>can pick up one disk by the writer task's pid, if btrfs_search_slot()
>needs to read block A, dev2 which does NOT have the latest metadata
>might be read for block A, then we got a stale block A.
> 
> 6. As parent transid is not checked, block A is marked as uptodate and
>put into the extent buffer cache, so the future search won't bother
>to read disk again, which means it'll make changes on this stale
>one and make it dirty and flush it onto disk.
> 
> To avoid the problem, parent transid needs to be passed to
> read_tree_block().
> 
> In order to get a valid parent transid, we need to hold the parent's
> lock until finish reading child.
> 
> Signed-off-by: Liu Bo 

Doesn't this warrant a stable tag and
Fixes: 5bdd3536cbbe ("Btrfs: Fix block generation verification race")
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix the corruption by reading stale btree blocks

[Qu Wenruo]

On 2018年05月16日 01:37, Liu Bo wrote:
> If a btree block, aka. extent buffer, is not available in the extent
> buffer cache, it'll be read out from the disk instead, i.e.
> 
> btrfs_search_slot()
>   read_block_for_search()  # hold parent and its lock, go to read child
> btrfs_release_path()
> read_tree_block()  # read child
> 
> Unfortunately, the parent lock got released before reading child, so
> commit 5bdd3536cbbe ("Btrfs: Fix block generation verification race") had
> used 0 as parent transid to read the child block.  It forces
> read_tree_block() not to check if parent transid is different with the
> generation id of the child that it reads out from disk.
> 
> A simple PoC is included in btrfs/124,
> 
> 0. A two-disk raid1 btrfs,
> 
> 1. Right after mkfs.btrfs, block A is allocated to be device tree's root.
> 
> 2. Mount this filesystem and put it in use, after a while, device tree's
>root got COW but block A hasn't been allocated/overwritten yet.
> 
> 3. Umount it and reload the btrfs module to remove both disks from the
>global @fs_devices list.
> 
> 4. mount -odegraded dev1 and write some data, so now block A is allocated
>to be a leaf in checksum tree.  Note that only dev1 has the latest
>metadata of this filesystem.
> 
> 5. Umount it and mount it again normally (with both disks), since raid1
>can pick up one disk by the writer task's pid, if btrfs_search_slot()
>needs to read block A, dev2 which does NOT have the latest metadata
>might be read for block A, then we got a stale block A.
> 
> 6. As parent transid is not checked, block A is marked as uptodate and
>put into the extent buffer cache, so the future search won't bother
>to read disk again, which means it'll make changes on this stale
>one and make it dirty and flush it onto disk.
> 
> To avoid the problem, parent transid needs to be passed to
> read_tree_block().
> 
> In order to get a valid parent transid, we need to hold the parent's
> lock until finish reading child.

Thanks for the detailed explanation.

It explains the first_key check error reported, thanks a lot!

Reviewed-by: Qu Wenruo 

Thanks,
Qu

> 
> Signed-off-by: Liu Bo 
> ---
>  fs/btrfs/ctree.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 3fd44835b386..b3f6f300e492 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2436,10 +2436,8 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path 
> *path, int level)
>   if (p->reada != READA_NONE)
>   reada_for_search(fs_info, p, level, slot, key->objectid);
>  
> - btrfs_release_path(p);
> -
>   ret = -EAGAIN;
> - tmp = read_tree_block(fs_info, blocknr, 0, parent_level - 1,
> + tmp = read_tree_block(fs_info, blocknr, gen, parent_level - 1,
> _key);
>   if (!IS_ERR(tmp)) {
>   /*
> @@ -2454,6 +2452,8 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path 
> *path, int level)
>   } else {
>   ret = PTR_ERR(tmp);
>   }
> +
> + btrfs_release_path(p);
>   return ret;
>  }
>  
> 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Btrfs: fix the corruption by reading stale btree blocks

[Filipe Manana]

On Tue, May 15, 2018 at 6:37 PM, Liu Bo  wrote:
> If a btree block, aka. extent buffer, is not available in the extent
> buffer cache, it'll be read out from the disk instead, i.e.
>
> btrfs_search_slot()
>   read_block_for_search()  # hold parent and its lock, go to read child
> btrfs_release_path()
> read_tree_block()  # read child
>
> Unfortunately, the parent lock got released before reading child, so
> commit 5bdd3536cbbe ("Btrfs: Fix block generation verification race") had
> used 0 as parent transid to read the child block.  It forces
> read_tree_block() not to check if parent transid is different with the
> generation id of the child that it reads out from disk.
>
> A simple PoC is included in btrfs/124,
>
> 0. A two-disk raid1 btrfs,
>
> 1. Right after mkfs.btrfs, block A is allocated to be device tree's root.
>
> 2. Mount this filesystem and put it in use, after a while, device tree's
>root got COW but block A hasn't been allocated/overwritten yet.
>
> 3. Umount it and reload the btrfs module to remove both disks from the
>global @fs_devices list.
>
> 4. mount -odegraded dev1 and write some data, so now block A is allocated
>to be a leaf in checksum tree.  Note that only dev1 has the latest
>metadata of this filesystem.
>
> 5. Umount it and mount it again normally (with both disks), since raid1
>can pick up one disk by the writer task's pid, if btrfs_search_slot()
>needs to read block A, dev2 which does NOT have the latest metadata
>might be read for block A, then we got a stale block A.
>
> 6. As parent transid is not checked, block A is marked as uptodate and
>put into the extent buffer cache, so the future search won't bother
>to read disk again, which means it'll make changes on this stale
>one and make it dirty and flush it onto disk.
>
> To avoid the problem, parent transid needs to be passed to
> read_tree_block().
>
> In order to get a valid parent transid, we need to hold the parent's
> lock until finish reading child.
>
> Signed-off-by: Liu Bo 
Reviewed-by: Filipe Manana 

Looks good, great finding and explanation.

> ---
>  fs/btrfs/ctree.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 3fd44835b386..b3f6f300e492 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2436,10 +2436,8 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path 
> *path, int level)
> if (p->reada != READA_NONE)
> reada_for_search(fs_info, p, level, slot, key->objectid);
>
> -   btrfs_release_path(p);
> -
> ret = -EAGAIN;
> -   tmp = read_tree_block(fs_info, blocknr, 0, parent_level - 1,
> +   tmp = read_tree_block(fs_info, blocknr, gen, parent_level - 1,
>   _key);
> if (!IS_ERR(tmp)) {
> /*
> @@ -2454,6 +2452,8 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path 
> *path, int level)
> } else {
> ret = PTR_ERR(tmp);
> }
> +
> +   btrfs_release_path(p);
> return ret;
>  }
>
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Btrfs: fix the corruption by reading stale btree blocks

[Liu Bo]

If a btree block, aka. extent buffer, is not available in the extent
buffer cache, it'll be read out from the disk instead, i.e.

btrfs_search_slot()
  read_block_for_search()  # hold parent and its lock, go to read child
btrfs_release_path()
read_tree_block()  # read child

Unfortunately, the parent lock got released before reading child, so
commit 5bdd3536cbbe ("Btrfs: Fix block generation verification race") had
used 0 as parent transid to read the child block.  It forces
read_tree_block() not to check if parent transid is different with the
generation id of the child that it reads out from disk.

A simple PoC is included in btrfs/124,

0. A two-disk raid1 btrfs,

1. Right after mkfs.btrfs, block A is allocated to be device tree's root.

2. Mount this filesystem and put it in use, after a while, device tree's
   root got COW but block A hasn't been allocated/overwritten yet.

3. Umount it and reload the btrfs module to remove both disks from the
   global @fs_devices list.

4. mount -odegraded dev1 and write some data, so now block A is allocated
   to be a leaf in checksum tree.  Note that only dev1 has the latest
   metadata of this filesystem.

5. Umount it and mount it again normally (with both disks), since raid1
   can pick up one disk by the writer task's pid, if btrfs_search_slot()
   needs to read block A, dev2 which does NOT have the latest metadata
   might be read for block A, then we got a stale block A.

6. As parent transid is not checked, block A is marked as uptodate and
   put into the extent buffer cache, so the future search won't bother
   to read disk again, which means it'll make changes on this stale
   one and make it dirty and flush it onto disk.

To avoid the problem, parent transid needs to be passed to
read_tree_block().

In order to get a valid parent transid, we need to hold the parent's
lock until finish reading child.

Signed-off-by: Liu Bo 
---
 fs/btrfs/ctree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3fd44835b386..b3f6f300e492 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2436,10 +2436,8 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path 
*path, int level)
if (p->reada != READA_NONE)
reada_for_search(fs_info, p, level, slot, key->objectid);
 
-   btrfs_release_path(p);
-
ret = -EAGAIN;
-   tmp = read_tree_block(fs_info, blocknr, 0, parent_level - 1,
+   tmp = read_tree_block(fs_info, blocknr, gen, parent_level - 1,
  _key);
if (!IS_ERR(tmp)) {
/*
@@ -2454,6 +2452,8 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path 
*path, int level)
} else {
ret = PTR_ERR(tmp);
}
+
+   btrfs_release_path(p);
return ret;
 }
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html