Re: [PATCH 6/6] Btrfs: add sanity check of extent item in scrub

[David Sterba]

On Mon, May 29, 2017 at 09:48:19AM +0800, Qu Wenruo wrote:
> 
> 
> At 05/27/2017 04:20 AM, Liu Bo wrote:
> > On Fri, May 26, 2017 at 08:33:16PM +0200, David Sterba wrote:
> >> On Thu, May 25, 2017 at 06:26:31PM -0600, Liu Bo wrote:
> >>> Currently scrub only verify checksum of both metadata and data and
> >>> couldn't detect an invalid extent_item.
> >>
> >> This is a different kind of check that scrub was never designed to do.
> >> Scrub just verifies the checksums, not the sructural integrity. This has
> >> been referred to as an "on-line fsck". AFAIK xfs does not use 'scrub' in
> >> the same sense as btrfs, so things are going to be confusing for the
> >> users.
> >>
> >> The on-line fsck is still desired, but I'd like to see a discussion how
> >> exactly it should work, whether to extend scrub or add a new ioctl etc.
> >>
> > 
> > I was hesitating about scrub vs online fsck when posting, just thought
> > it'd be good when users got this error while doing a regular scrub
> > ahead of really getting into trouble.  If we have a plan for online
> > fsck, I'm happy to let fsck do the job.
> 
> IIRC on-line fsck will be too challenging for kernel.

Ok, we should define what's considered a sane 'fsck' subset that can be
done in kernel.

> Just check how current btrfs check does, it's doing tons of cross 
> checking for lowmem mode or uses tons of memory to record forward refs 
> and backward refs.

Full cross-ref validation is obviously quite heavy in terms of memory
resources and potential impact on the filesystem work.

> Doing it in kernel will be a disaster.
> 
> > 
> > Or we can do this kind of sanity check at the time when reading the
> > eb.
> 
> This seems more reasonable, just like what Su Yue did for 
> inode_ref/dir_item/dir_index.

Yeah that's what I had in mind, local sanity checks from information
available at the time. We can add such check independently and not
caling them on-line fsck anyway, so we can avoid overloading the scrub
stats.
--
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 6/6] Btrfs: add sanity check of extent item in scrub

[Qu Wenruo]

At 05/27/2017 04:20 AM, Liu Bo wrote:

On Fri, May 26, 2017 at 08:33:16PM +0200, David Sterba wrote:

On Thu, May 25, 2017 at 06:26:31PM -0600, Liu Bo wrote:

Currently scrub only verify checksum of both metadata and data and
couldn't detect an invalid extent_item.


This is a different kind of check that scrub was never designed to do.
Scrub just verifies the checksums, not the sructural integrity. This has
been referred to as an "on-line fsck". AFAIK xfs does not use 'scrub' in
the same sense as btrfs, so things are going to be confusing for the
users.

The on-line fsck is still desired, but I'd like to see a discussion how
exactly it should work, whether to extend scrub or add a new ioctl etc.



I was hesitating about scrub vs online fsck when posting, just thought
it'd be good when users got this error while doing a regular scrub
ahead of really getting into trouble.  If we have a plan for online
fsck, I'm happy to let fsck do the job.


IIRC on-line fsck will be too challenging for kernel.

Just check how current btrfs check does, it's doing tons of cross 
checking for lowmem mode or uses tons of memory to record forward refs 
and backward refs.


Doing it in kernel will be a disaster.



Or we can do this kind of sanity check at the time when reading the
eb.


This seems more reasonable, just like what Su Yue did for 
inode_ref/dir_item/dir_index.


Thanks,
Qu


Thanks,
-liubo


This adds sanity check for extent item, now it can check if
extent_inline_ref_type is valid.

Signed-off-by: Liu Bo 
---
  fs/btrfs/scrub.c | 43 +++
  1 file changed, 43 insertions(+)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index b0251eb..e87b752 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3058,6 +3058,39 @@ static noinline_for_stack int scrub_raid56_parity(struct 
scrub_ctx *sctx,
return ret < 0 ? ret : 0;
  }
  
+static int check_extent_item(struct extent_buffer *l, int slot,


Please use 'eb' for the extent_buffer.


+struct btrfs_extent_item *ei, int key_type)


key_type should be u8


+{
+   unsigned long ptr;
+   unsigned long end;
+   struct btrfs_extent_inline_ref *iref;
+   u64 flags = btrfs_extent_flags(l, ei);
+   int is_data = !!(flags & BTRFS_EXTENT_FLAG_DATA);
+   int type;
+
+   ptr = (unsigned long)(ei + 1);
+   if (!is_data &&
+   key_type != BTRFS_METADATA_ITEM_KEY)
+   ptr += sizeof(struct btrfs_tree_block_info);
+   end = (unsigned long)ei +
+   btrfs_item_size_nr(l, slot);


This will fit to one line


+
+   while (1) {
+   if (ptr >= end) {
+   WARN_ON(ptr > end);


Please add some verbose error message or get rid of the WARN_ON
completely if possible.


+   break;
+   }
+
+   iref = (struct btrfs_extent_inline_ref *)ptr;
+   type = btrfs_get_extent_inline_ref_type(l, iref, is_data);
+   if (type < 0)
+   return type;
+
+   ptr += btrfs_extent_inline_ref_size(type);
+   }
+   return 0;
+}
+
  static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
   struct map_lookup *map,
   struct btrfs_device *scrub_dev,
@@ -3318,6 +3351,16 @@ static noinline_for_stack int scrub_stripe(struct 
scrub_ctx *sctx,
goto next;
}
  
+			/* sanity check for extent inline ref type */

+   if (check_extent_item(l, slot, extent, key.type)) {
+   btrfs_err(fs_info,
+ "scrub: extent %llu(0x%llx) has an invalid 
extent inline ref type, ignored.",


Strings can be un-indented to the left.


+ key.objectid, key.objectid);
+   spin_lock(>stat_lock);
+   sctx->stat.uncorrectable_errors++;


Yeah, this is an example where the uncorrectable_error would gain a new
meaning. We'd need to extend the scrub error reporting so the various
metadata errors get reported properly and not mangled with the rest.


+   spin_unlock(>stat_lock);
+   goto next;
+   }
  again:
extent_logical = key.objectid;
extent_len = bytes;

--
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





--
To unsubscribe from this list: send the line "unsubscribe 

Re: [PATCH 6/6] Btrfs: add sanity check of extent item in scrub

[Liu Bo]

On Fri, May 26, 2017 at 08:33:16PM +0200, David Sterba wrote:
> On Thu, May 25, 2017 at 06:26:31PM -0600, Liu Bo wrote:
> > Currently scrub only verify checksum of both metadata and data and
> > couldn't detect an invalid extent_item.
> 
> This is a different kind of check that scrub was never designed to do.
> Scrub just verifies the checksums, not the sructural integrity. This has
> been referred to as an "on-line fsck". AFAIK xfs does not use 'scrub' in
> the same sense as btrfs, so things are going to be confusing for the
> users.
> 
> The on-line fsck is still desired, but I'd like to see a discussion how
> exactly it should work, whether to extend scrub or add a new ioctl etc.
>

I was hesitating about scrub vs online fsck when posting, just thought
it'd be good when users got this error while doing a regular scrub
ahead of really getting into trouble.  If we have a plan for online
fsck, I'm happy to let fsck do the job.

Or we can do this kind of sanity check at the time when reading the
eb.

Thanks,
-liubo

> > This adds sanity check for extent item, now it can check if
> > extent_inline_ref_type is valid.
> > 
> > Signed-off-by: Liu Bo 
> > ---
> >  fs/btrfs/scrub.c | 43 +++
> >  1 file changed, 43 insertions(+)
> > 
> > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> > index b0251eb..e87b752 100644
> > --- a/fs/btrfs/scrub.c
> > +++ b/fs/btrfs/scrub.c
> > @@ -3058,6 +3058,39 @@ static noinline_for_stack int 
> > scrub_raid56_parity(struct scrub_ctx *sctx,
> > return ret < 0 ? ret : 0;
> >  }
> >  
> > +static int check_extent_item(struct extent_buffer *l, int slot,
> 
> Please use 'eb' for the extent_buffer.
> 
> > +struct btrfs_extent_item *ei, int key_type)
> 
> key_type should be u8
> 
> > +{
> > +   unsigned long ptr;
> > +   unsigned long end;
> > +   struct btrfs_extent_inline_ref *iref;
> > +   u64 flags = btrfs_extent_flags(l, ei);
> > +   int is_data = !!(flags & BTRFS_EXTENT_FLAG_DATA);
> > +   int type;
> > +
> > +   ptr = (unsigned long)(ei + 1);
> > +   if (!is_data &&
> > +   key_type != BTRFS_METADATA_ITEM_KEY)
> > +   ptr += sizeof(struct btrfs_tree_block_info);
> > +   end = (unsigned long)ei +
> > +   btrfs_item_size_nr(l, slot);
> 
> This will fit to one line
> 
> > +
> > +   while (1) {
> > +   if (ptr >= end) {
> > +   WARN_ON(ptr > end);
> 
> Please add some verbose error message or get rid of the WARN_ON
> completely if possible.
> 
> > +   break;
> > +   }
> > +
> > +   iref = (struct btrfs_extent_inline_ref *)ptr;
> > +   type = btrfs_get_extent_inline_ref_type(l, iref, is_data);
> > +   if (type < 0)
> > +   return type;
> > +
> > +   ptr += btrfs_extent_inline_ref_size(type);
> > +   }
> > +   return 0;
> > +}
> > +
> >  static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
> >struct map_lookup *map,
> >struct btrfs_device *scrub_dev,
> > @@ -3318,6 +3351,16 @@ static noinline_for_stack int scrub_stripe(struct 
> > scrub_ctx *sctx,
> > goto next;
> > }
> >  
> > +   /* sanity check for extent inline ref type */
> > +   if (check_extent_item(l, slot, extent, key.type)) {
> > +   btrfs_err(fs_info,
> > + "scrub: extent %llu(0x%llx) has an 
> > invalid extent inline ref type, ignored.",
> 
> Strings can be un-indented to the left.
> 
> > + key.objectid, key.objectid);
> > +   spin_lock(>stat_lock);
> > +   sctx->stat.uncorrectable_errors++;
> 
> Yeah, this is an example where the uncorrectable_error would gain a new
> meaning. We'd need to extend the scrub error reporting so the various
> metadata errors get reported properly and not mangled with the rest.
> 
> > +   spin_unlock(>stat_lock);
> > +   goto next;
> > +   }
> >  again:
> > extent_logical = key.objectid;
> > extent_len = bytes;
> --
> 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 6/6] Btrfs: add sanity check of extent item in scrub

[David Sterba]

On Thu, May 25, 2017 at 06:26:31PM -0600, Liu Bo wrote:
> Currently scrub only verify checksum of both metadata and data and
> couldn't detect an invalid extent_item.

This is a different kind of check that scrub was never designed to do.
Scrub just verifies the checksums, not the sructural integrity. This has
been referred to as an "on-line fsck". AFAIK xfs does not use 'scrub' in
the same sense as btrfs, so things are going to be confusing for the
users.

The on-line fsck is still desired, but I'd like to see a discussion how
exactly it should work, whether to extend scrub or add a new ioctl etc.

> This adds sanity check for extent item, now it can check if
> extent_inline_ref_type is valid.
> 
> Signed-off-by: Liu Bo 
> ---
>  fs/btrfs/scrub.c | 43 +++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index b0251eb..e87b752 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3058,6 +3058,39 @@ static noinline_for_stack int 
> scrub_raid56_parity(struct scrub_ctx *sctx,
>   return ret < 0 ? ret : 0;
>  }
>  
> +static int check_extent_item(struct extent_buffer *l, int slot,

Please use 'eb' for the extent_buffer.

> +  struct btrfs_extent_item *ei, int key_type)

key_type should be u8

> +{
> + unsigned long ptr;
> + unsigned long end;
> + struct btrfs_extent_inline_ref *iref;
> + u64 flags = btrfs_extent_flags(l, ei);
> + int is_data = !!(flags & BTRFS_EXTENT_FLAG_DATA);
> + int type;
> +
> + ptr = (unsigned long)(ei + 1);
> + if (!is_data &&
> + key_type != BTRFS_METADATA_ITEM_KEY)
> + ptr += sizeof(struct btrfs_tree_block_info);
> + end = (unsigned long)ei +
> + btrfs_item_size_nr(l, slot);

This will fit to one line

> +
> + while (1) {
> + if (ptr >= end) {
> + WARN_ON(ptr > end);

Please add some verbose error message or get rid of the WARN_ON
completely if possible.

> + break;
> + }
> +
> + iref = (struct btrfs_extent_inline_ref *)ptr;
> + type = btrfs_get_extent_inline_ref_type(l, iref, is_data);
> + if (type < 0)
> + return type;
> +
> + ptr += btrfs_extent_inline_ref_size(type);
> + }
> + return 0;
> +}
> +
>  static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>  struct map_lookup *map,
>  struct btrfs_device *scrub_dev,
> @@ -3318,6 +3351,16 @@ static noinline_for_stack int scrub_stripe(struct 
> scrub_ctx *sctx,
>   goto next;
>   }
>  
> + /* sanity check for extent inline ref type */
> + if (check_extent_item(l, slot, extent, key.type)) {
> + btrfs_err(fs_info,
> +   "scrub: extent %llu(0x%llx) has an 
> invalid extent inline ref type, ignored.",

Strings can be un-indented to the left.

> +   key.objectid, key.objectid);
> + spin_lock(>stat_lock);
> + sctx->stat.uncorrectable_errors++;

Yeah, this is an example where the uncorrectable_error would gain a new
meaning. We'd need to extend the scrub error reporting so the various
metadata errors get reported properly and not mangled with the rest.

> + spin_unlock(>stat_lock);
> + goto next;
> + }
>  again:
>   extent_logical = key.objectid;
>   extent_len = bytes;
--
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 6/6] Btrfs: add sanity check of extent item in scrub

[Liu Bo]

Currently scrub only verify checksum of both metadata and data and
couldn't detect an invalid extent_item.

This adds sanity check for extent item, now it can check if
extent_inline_ref_type is valid.

Signed-off-by: Liu Bo 
---
 fs/btrfs/scrub.c | 43 +++
 1 file changed, 43 insertions(+)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index b0251eb..e87b752 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3058,6 +3058,39 @@ static noinline_for_stack int scrub_raid56_parity(struct 
scrub_ctx *sctx,
return ret < 0 ? ret : 0;
 }
 
+static int check_extent_item(struct extent_buffer *l, int slot,
+struct btrfs_extent_item *ei, int key_type)
+{
+   unsigned long ptr;
+   unsigned long end;
+   struct btrfs_extent_inline_ref *iref;
+   u64 flags = btrfs_extent_flags(l, ei);
+   int is_data = !!(flags & BTRFS_EXTENT_FLAG_DATA);
+   int type;
+
+   ptr = (unsigned long)(ei + 1);
+   if (!is_data &&
+   key_type != BTRFS_METADATA_ITEM_KEY)
+   ptr += sizeof(struct btrfs_tree_block_info);
+   end = (unsigned long)ei +
+   btrfs_item_size_nr(l, slot);
+
+   while (1) {
+   if (ptr >= end) {
+   WARN_ON(ptr > end);
+   break;
+   }
+
+   iref = (struct btrfs_extent_inline_ref *)ptr;
+   type = btrfs_get_extent_inline_ref_type(l, iref, is_data);
+   if (type < 0)
+   return type;
+
+   ptr += btrfs_extent_inline_ref_size(type);
+   }
+   return 0;
+}
+
 static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
   struct map_lookup *map,
   struct btrfs_device *scrub_dev,
@@ -3318,6 +3351,16 @@ static noinline_for_stack int scrub_stripe(struct 
scrub_ctx *sctx,
goto next;
}
 
+   /* sanity check for extent inline ref type */
+   if (check_extent_item(l, slot, extent, key.type)) {
+   btrfs_err(fs_info,
+ "scrub: extent %llu(0x%llx) has an 
invalid extent inline ref type, ignored.",
+ key.objectid, key.objectid);
+   spin_lock(>stat_lock);
+   sctx->stat.uncorrectable_errors++;
+   spin_unlock(>stat_lock);
+   goto next;
+   }
 again:
extent_logical = key.objectid;
extent_len = bytes;
-- 
2.9.4

--
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