[RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Carlos Maiolino]

Hi,

this is supposed to fix the issue while trying to print a non attr3
block using attr3 type, which ends up on an ASSERT, and crashing xfs_db.

This is based on Eric's suggestion, which an attempt to print the
possible magic number from the current block.

As Eric mentioned, there are other types which need such handling too,
but, this is supposed to be a RFC to get comments on the approach. Once
we agree how to fix it, I'll fix the remaining types accordingly.

This is the following output of an attempt to print a non attr3 type block,
with this patch:

xfs_db> fsblock 2
xfs_db> type attr3
Unknown attribute buffer type!
Unknown attribute buffer type!
xfs_db> p
Unrecognized attr3 block, possible magic number:
Unrecognized attr3 block, possible magic number:
hdr.magic = 0x41423343

While, printing a correct attr3 block, nothing is changed:

xfs_db> fsblock 11
xfs_db> type attr3
xfs_db> p
hdr.info.hdr.forw = 0
hdr.info.hdr.back = 0
hdr.info.hdr.magic = 0x3ebe
hdr.info.crc = 0x6f7911f7 (correct)
hdr.info.bno = 88
.
.
.

Comments?

Cheers

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 db/attr.c  | 22 ++++++++++++++++++++++
 db/attr.h  |  1 +
 db/field.c |  2 ++
 db/field.h |  1 +
 4 files changed, 26 insertions(+)

diff --git a/db/attr.c b/db/attr.c
index 9cbb20d3..b7692e9e 100644
--- a/db/attr.c
+++ b/db/attr.c
@@ -523,6 +523,21 @@ attr3_remote_hdr_count(
 	return be32_to_cpu(node->rm_magic) == XFS_ATTR3_RMT_MAGIC;
 }
 
+static int
+attr3_unkown_hdr_count(
+	void			*obj,
+	int			startoff)
+{
+	if (!attr3_leaf_hdr_count(obj, startoff) &&
+	    !attr3_node_hdr_count(obj, startoff) &&
+	    !attr3_remote_hdr_count(obj,startoff)) {
+		dbprintf(_("Unrecognized attr3 block, possible magic number:\n"));
+		return 1;
+	}
+
+	return 0;
+}
+
 int
 attr_size(
 	void	*obj,
@@ -549,6 +564,8 @@ const field_t	attr3_flds[] = {
 	  FLD_COUNT, TYP_NONE },
 	{ "hdr", FLDT_ATTR3_REMOTE_HDR, OI(0), attr3_remote_hdr_count,
 	  FLD_COUNT, TYP_NONE },
+	{ "hdr", FLDT_ATTR3_UNKOWN_HDR, OI(0), attr3_unkown_hdr_count,
+	  FLD_COUNT, TYP_NONE },
 	{ "data", FLDT_CHARNS, OI(bitize(sizeof(struct xfs_attr3_rmt_hdr))),
 	  attr3_remote_data_count, FLD_COUNT, TYP_NONE },
 	{ "entries", FLDT_ATTR_LEAF_ENTRY, OI(L3OFF(entries)),
@@ -606,6 +623,11 @@ const struct field	attr3_remote_crc_flds[] = {
 	{ NULL }
 };
 
+const struct field	attr3_unkown_flds[] = {
+	{ "magic", FLDT_UINT32X, 0, C1, 0, TYP_NONE},
+	{ NULL }
+};
+
 /* Set the CRC. */
 void
 xfs_attr3_set_crc(
diff --git a/db/attr.h b/db/attr.h
index ba23480c..a8566a8c 100644
--- a/db/attr.h
+++ b/db/attr.h
@@ -33,6 +33,7 @@ extern const field_t	attr3_node_hdr_flds[];
 extern const field_t	attr3_blkinfo_flds[];
 extern const field_t	attr3_node_hdr_flds[];
 extern const field_t	attr3_remote_crc_flds[];
+extern const field_t	attr3_unkown_flds[];
 
 extern int	attr_leaf_name_size(void *obj, int startoff, int idx);
 extern int	attr_size(void *obj, int startoff, int idx);
diff --git a/db/field.c b/db/field.c
index ae4c8057..388bcbe1 100644
--- a/db/field.c
+++ b/db/field.c
@@ -102,6 +102,8 @@ const ftattr_t	ftattrtab[] = {
 	{ FLDT_ATTR3_REMOTE_HDR, "attr3_remote_hdr", NULL,
 	  (char *)attr3_remote_crc_flds, attr_size, FTARG_SIZE, NULL,
 	  attr3_remote_crc_flds },
+	{ FLDT_ATTR3_UNKOWN_HDR, "attr3_unkown_hdr", NULL,
+	  NULL, NULL, FTARG_SIZE, NULL, attr3_unkown_flds},
 
 	{ FLDT_BMAPBTA, "bmapbta", NULL, (char *)bmapbta_flds, btblock_size,
 	  FTARG_SIZE, NULL, bmapbta_flds },
diff --git a/db/field.h b/db/field.h
index a8df29bc..291ef2e5 100644
--- a/db/field.h
+++ b/db/field.h
@@ -48,6 +48,7 @@ typedef enum fldt	{
 	FLDT_ATTR3_LEAF_HDR,
 	FLDT_ATTR3_NODE_HDR,
 	FLDT_ATTR3_REMOTE_HDR,
+	FLDT_ATTR3_UNKOWN_HDR,
 
 	FLDT_BMAPBTA,
 	FLDT_BMAPBTA_CRC,
-- 
2.14.3

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Darrick J. Wong]

On Wed, Apr 18, 2018 at 11:49:35AM +0200, Carlos Maiolino wrote:
> Hi,
> 
> this is supposed to fix the issue while trying to print a non attr3
> block using attr3 type, which ends up on an ASSERT, and crashing xfs_db.
> 
> This is based on Eric's suggestion, which an attempt to print the
> possible magic number from the current block.
> 
> As Eric mentioned, there are other types which need such handling too,
> but, this is supposed to be a RFC to get comments on the approach. Once
> we agree how to fix it, I'll fix the remaining types accordingly.

I'm assuming you mean attr, dir, and dir3?

> This is the following output of an attempt to print a non attr3 type block,
> with this patch:
> 
> xfs_db> fsblock 2
> xfs_db> type attr3
> Unknown attribute buffer type!
> Unknown attribute buffer type!
> xfs_db> p
> Unrecognized attr3 block, possible magic number:
> Unrecognized attr3 block, possible magic number:
> hdr.magic = 0x41423343
> 
> While, printing a correct attr3 block, nothing is changed:
> 
> xfs_db> fsblock 11
> xfs_db> type attr3
> xfs_db> p
> hdr.info.hdr.forw = 0
> hdr.info.hdr.back = 0
> hdr.info.hdr.magic = 0x3ebe
> hdr.info.crc = 0x6f7911f7 (correct)
> hdr.info.bno = 88
> .
> .
> .
> 
> Comments?
> 
> Cheers
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  db/attr.c  | 22 ++++++++++++++++++++++
>  db/attr.h  |  1 +
>  db/field.c |  2 ++
>  db/field.h |  1 +
>  4 files changed, 26 insertions(+)
> 
> diff --git a/db/attr.c b/db/attr.c
> index 9cbb20d3..b7692e9e 100644
> --- a/db/attr.c
> +++ b/db/attr.c
> @@ -523,6 +523,21 @@ attr3_remote_hdr_count(
>  	return be32_to_cpu(node->rm_magic) == XFS_ATTR3_RMT_MAGIC;
>  }
>  
> +static int
> +attr3_unkown_hdr_count(
> +	void			*obj,
> +	int			startoff)
> +{
> +	if (!attr3_leaf_hdr_count(obj, startoff) &&
> +	    !attr3_node_hdr_count(obj, startoff) &&
> +	    !attr3_remote_hdr_count(obj,startoff)) {
> +		dbprintf(_("Unrecognized attr3 block, possible magic number:\n"));
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  int
>  attr_size(
>  	void	*obj,
> @@ -549,6 +564,8 @@ const field_t	attr3_flds[] = {
>  	  FLD_COUNT, TYP_NONE },
>  	{ "hdr", FLDT_ATTR3_REMOTE_HDR, OI(0), attr3_remote_hdr_count,
>  	  FLD_COUNT, TYP_NONE },
> +	{ "hdr", FLDT_ATTR3_UNKOWN_HDR, OI(0), attr3_unkown_hdr_count,

"FLDT_ATTR3_UNKNOWN_HDR" (need extra 'N') here and elsewhere.

> +	  FLD_COUNT, TYP_NONE },
>  	{ "data", FLDT_CHARNS, OI(bitize(sizeof(struct xfs_attr3_rmt_hdr))),
>  	  attr3_remote_data_count, FLD_COUNT, TYP_NONE },
>  	{ "entries", FLDT_ATTR_LEAF_ENTRY, OI(L3OFF(entries)),
> @@ -606,6 +623,11 @@ const struct field	attr3_remote_crc_flds[] = {
>  	{ NULL }
>  };
>  
> +const struct field	attr3_unkown_flds[] = {
> +	{ "magic", FLDT_UINT32X, 0, C1, 0, TYP_NONE},

There are two possible magics for attr3 blocks -- this one, which is for
remote attr value blocks, and the xfs_da3_blkinfo magic for the attr
leaf and da node blocks.  Can you please fish out the second magic and
print that too?

Looks otherwise reasonable, and certainly better than the ASSERT.

--D

> +	{ NULL }
> +};
> +
>  /* Set the CRC. */
>  void
>  xfs_attr3_set_crc(
> diff --git a/db/attr.h b/db/attr.h
> index ba23480c..a8566a8c 100644
> --- a/db/attr.h
> +++ b/db/attr.h
> @@ -33,6 +33,7 @@ extern const field_t	attr3_node_hdr_flds[];
>  extern const field_t	attr3_blkinfo_flds[];
>  extern const field_t	attr3_node_hdr_flds[];
>  extern const field_t	attr3_remote_crc_flds[];
> +extern const field_t	attr3_unkown_flds[];
>  
>  extern int	attr_leaf_name_size(void *obj, int startoff, int idx);
>  extern int	attr_size(void *obj, int startoff, int idx);
> diff --git a/db/field.c b/db/field.c
> index ae4c8057..388bcbe1 100644
> --- a/db/field.c
> +++ b/db/field.c
> @@ -102,6 +102,8 @@ const ftattr_t	ftattrtab[] = {
>  	{ FLDT_ATTR3_REMOTE_HDR, "attr3_remote_hdr", NULL,
>  	  (char *)attr3_remote_crc_flds, attr_size, FTARG_SIZE, NULL,
>  	  attr3_remote_crc_flds },
> +	{ FLDT_ATTR3_UNKOWN_HDR, "attr3_unkown_hdr", NULL,
> +	  NULL, NULL, FTARG_SIZE, NULL, attr3_unkown_flds},
>  
>  	{ FLDT_BMAPBTA, "bmapbta", NULL, (char *)bmapbta_flds, btblock_size,
>  	  FTARG_SIZE, NULL, bmapbta_flds },
> diff --git a/db/field.h b/db/field.h
> index a8df29bc..291ef2e5 100644
> --- a/db/field.h
> +++ b/db/field.h
> @@ -48,6 +48,7 @@ typedef enum fldt	{
>  	FLDT_ATTR3_LEAF_HDR,
>  	FLDT_ATTR3_NODE_HDR,
>  	FLDT_ATTR3_REMOTE_HDR,
> +	FLDT_ATTR3_UNKOWN_HDR,
>  
>  	FLDT_BMAPBTA,
>  	FLDT_BMAPBTA_CRC,
> -- 
> 2.14.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Carlos Maiolino]

On Wed, Apr 18, 2018 at 08:44:35AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 18, 2018 at 11:49:35AM +0200, Carlos Maiolino wrote:
> > Hi,
> > 
> > this is supposed to fix the issue while trying to print a non attr3
> > block using attr3 type, which ends up on an ASSERT, and crashing xfs_db.
> > 
> > This is based on Eric's suggestion, which an attempt to print the
> > possible magic number from the current block.
> > 
> > As Eric mentioned, there are other types which need such handling too,
> > but, this is supposed to be a RFC to get comments on the approach. Once
> > we agree how to fix it, I'll fix the remaining types accordingly.
> 
> I'm assuming you mean attr, dir, and dir3?

Yes.

> 
> > This is the following output of an attempt to print a non attr3 type block,
> > with this patch:
> > 
> > xfs_db> fsblock 2
> > xfs_db> type attr3
> > Unknown attribute buffer type!
> > Unknown attribute buffer type!
> > xfs_db> p
> > Unrecognized attr3 block, possible magic number:
> > Unrecognized attr3 block, possible magic number:
> > hdr.magic = 0x41423343
> > 
> > While, printing a correct attr3 block, nothing is changed:
> > 
> > xfs_db> fsblock 11
> > xfs_db> type attr3
> > xfs_db> p
> > hdr.info.hdr.forw = 0
> > hdr.info.hdr.back = 0
> > hdr.info.hdr.magic = 0x3ebe
> > hdr.info.crc = 0x6f7911f7 (correct)
> > hdr.info.bno = 88
> > .
> > .
> > .
> > 
> > Comments?
> > 
> > Cheers
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  db/attr.c  | 22 ++++++++++++++++++++++
> >  db/attr.h  |  1 +
> >  db/field.c |  2 ++
> >  db/field.h |  1 +
> >  4 files changed, 26 insertions(+)
> > 
> > diff --git a/db/attr.c b/db/attr.c
> > index 9cbb20d3..b7692e9e 100644
> > --- a/db/attr.c
> > +++ b/db/attr.c
> > @@ -523,6 +523,21 @@ attr3_remote_hdr_count(
> >  	return be32_to_cpu(node->rm_magic) == XFS_ATTR3_RMT_MAGIC;
> >  }
> >  
> > +static int
> > +attr3_unkown_hdr_count(
> > +	void			*obj,
> > +	int			startoff)
> > +{
> > +	if (!attr3_leaf_hdr_count(obj, startoff) &&
> > +	    !attr3_node_hdr_count(obj, startoff) &&
> > +	    !attr3_remote_hdr_count(obj,startoff)) {
> > +		dbprintf(_("Unrecognized attr3 block, possible magic number:\n"));
> > +		return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int
> >  attr_size(
> >  	void	*obj,
> > @@ -549,6 +564,8 @@ const field_t	attr3_flds[] = {
> >  	  FLD_COUNT, TYP_NONE },
> >  	{ "hdr", FLDT_ATTR3_REMOTE_HDR, OI(0), attr3_remote_hdr_count,
> >  	  FLD_COUNT, TYP_NONE },
> > +	{ "hdr", FLDT_ATTR3_UNKOWN_HDR, OI(0), attr3_unkown_hdr_count,
> 
> "FLDT_ATTR3_UNKNOWN_HDR" (need extra 'N') here and elsewhere.
> 

Ops, my typo, will fix.

> > +	  FLD_COUNT, TYP_NONE },
> >  	{ "data", FLDT_CHARNS, OI(bitize(sizeof(struct xfs_attr3_rmt_hdr))),
> >  	  attr3_remote_data_count, FLD_COUNT, TYP_NONE },
> >  	{ "entries", FLDT_ATTR_LEAF_ENTRY, OI(L3OFF(entries)),
> > @@ -606,6 +623,11 @@ const struct field	attr3_remote_crc_flds[] = {
> >  	{ NULL }
> >  };
> >  
> > +const struct field	attr3_unkown_flds[] = {
> > +	{ "magic", FLDT_UINT32X, 0, C1, 0, TYP_NONE},
> 
> There are two possible magics for attr3 blocks -- this one, which is for
> remote attr value blocks, and the xfs_da3_blkinfo magic for the attr
> leaf and da node blocks.  Can you please fish out the second magic and
> print that too?
> 
> Looks otherwise reasonable, and certainly better than the ASSERT.

Yes, my idea though, was to try to print the magic number of whatever block we
fall into, and maybe give the user some 'hint' of whatever block he/she was
trying to print using the attr3 type.

In my example above, notice the magic num corresponds to the count indexed bno
btree, not an attr3 type. This might be a dumb question, but, why care about the
blkinfo magic for the leaf and danode blocks, if they would be properly printed
using attr3 type? We wouldn't fall into the UNKNOWN case if we were trying to
read any of the attr3 blocks.

Despite of that, I agree that printing a possible magic number using the offset
from xfs_da3_blkinfo might be useful as well if we fall into some leaf/node
block.

I'll rework the patch and resubmit it as a proper patch.

Cheers
> 
> --D
> 
> > +	{ NULL }
> > +};
> > +
> >  /* Set the CRC. */
> >  void
> >  xfs_attr3_set_crc(
> > diff --git a/db/attr.h b/db/attr.h
> > index ba23480c..a8566a8c 100644
> > --- a/db/attr.h
> > +++ b/db/attr.h
> > @@ -33,6 +33,7 @@ extern const field_t	attr3_node_hdr_flds[];
> >  extern const field_t	attr3_blkinfo_flds[];
> >  extern const field_t	attr3_node_hdr_flds[];
> >  extern const field_t	attr3_remote_crc_flds[];
> > +extern const field_t	attr3_unkown_flds[];
> >  
> >  extern int	attr_leaf_name_size(void *obj, int startoff, int idx);
> >  extern int	attr_size(void *obj, int startoff, int idx);
> > diff --git a/db/field.c b/db/field.c
> > index ae4c8057..388bcbe1 100644
> > --- a/db/field.c
> > +++ b/db/field.c
> > @@ -102,6 +102,8 @@ const ftattr_t	ftattrtab[] = {
> >  	{ FLDT_ATTR3_REMOTE_HDR, "attr3_remote_hdr", NULL,
> >  	  (char *)attr3_remote_crc_flds, attr_size, FTARG_SIZE, NULL,
> >  	  attr3_remote_crc_flds },
> > +	{ FLDT_ATTR3_UNKOWN_HDR, "attr3_unkown_hdr", NULL,
> > +	  NULL, NULL, FTARG_SIZE, NULL, attr3_unkown_flds},
> >  
> >  	{ FLDT_BMAPBTA, "bmapbta", NULL, (char *)bmapbta_flds, btblock_size,
> >  	  FTARG_SIZE, NULL, bmapbta_flds },
> > diff --git a/db/field.h b/db/field.h
> > index a8df29bc..291ef2e5 100644
> > --- a/db/field.h
> > +++ b/db/field.h
> > @@ -48,6 +48,7 @@ typedef enum fldt	{
> >  	FLDT_ATTR3_LEAF_HDR,
> >  	FLDT_ATTR3_NODE_HDR,
> >  	FLDT_ATTR3_REMOTE_HDR,
> > +	FLDT_ATTR3_UNKOWN_HDR,
> >  
> >  	FLDT_BMAPBTA,
> >  	FLDT_BMAPBTA_CRC,
> > -- 
> > 2.14.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Carlos Maiolino]

On Wed, Apr 18, 2018 at 08:44:35AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 18, 2018 at 11:49:35AM +0200, Carlos Maiolino wrote:
> 
> There are two possible magics for attr3 blocks -- this one, which is for
> remote attr value blocks, and the xfs_da3_blkinfo magic for the attr
> leaf and da node blocks.  Can you please fish out the second magic and
> print that too?
> 
> Looks otherwise reasonable, and certainly better than the ASSERT.
> 
> --D
> 

What do you think about this output. Using attr3 type trying to print a
directory's leaf block.

xfs_db> type attr3
Unknown attribute buffer type!
xfs_db> p
Unrecognized attr3 block, attempting to print magic numbers and/or blkinfo:
Unrecognized attr3 block, attempting to print magic numbers and/or blkinfo:
hdr.magic = 0
hdr.info.hdr.forw = 0
hdr.info.hdr.back = 0
hdr.info.hdr.magic = 0x3df1
hdr.info.crc = 0x72e2e910 (correct)
hdr.info.bno = 64
hdr.info.lsn = 0x300002833
hdr.info.uuid = 2b21796f-7f81-4cec-b583-968c55b7f4bb
hdr.info.owner = 99
xfs_db> 

-- 
Carlos

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Darrick J. Wong]

On Thu, Apr 19, 2018 at 11:13:02AM +0200, Carlos Maiolino wrote:
> On Wed, Apr 18, 2018 at 08:44:35AM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 18, 2018 at 11:49:35AM +0200, Carlos Maiolino wrote:
> > 
> > There are two possible magics for attr3 blocks -- this one, which is for
> > remote attr value blocks, and the xfs_da3_blkinfo magic for the attr
> > leaf and da node blocks.  Can you please fish out the second magic and
> > print that too?
> > 
> > Looks otherwise reasonable, and certainly better than the ASSERT.
> > 
> > --D
> > 
> 
> What do you think about this output. Using attr3 type trying to print a
> directory's leaf block.
> 
> xfs_db> type attr3
> Unknown attribute buffer type!
> xfs_db> p
> Unrecognized attr3 block, attempting to print magic numbers and/or blkinfo:
> Unrecognized attr3 block, attempting to print magic numbers and/or blkinfo:
> hdr.magic = 0
> hdr.info.hdr.forw = 0
> hdr.info.hdr.back = 0
> hdr.info.hdr.magic = 0x3df1

I like it better, though on further thought I think I like better the
idea of printing the contents of all potential magic numbers:

For a block starting with:

0xDE 0xAD 0xBE 0xEF 0xCA 0xFE 0xF0 0x0D 0xBA 0xAD...

xfs_db> p
Unrecognized attr3 block, attempting to print magic numbers and/or blkinfo:
unknown.blockhdr.magic = 0xDEADBEEF
unknown.da_blkinfo.magic = 0xBAAD
unknown.inode.magic = 0xDEAD

--D

> hdr.info.crc = 0x72e2e910 (correct)
> hdr.info.bno = 64
> hdr.info.lsn = 0x300002833
> hdr.info.uuid = 2b21796f-7f81-4cec-b583-968c55b7f4bb
> hdr.info.owner = 99
> xfs_db> 
> 
> -- 
> Carlos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Eric Sandeen]

On 4/19/18 3:01 PM, Darrick J. Wong wrote:

 
> I like it better, though on further thought I think I like better the
> idea of printing the contents of all potential magic numbers:
> 
> For a block starting with:
> 
> 0xDE 0xAD 0xBE 0xEF 0xCA 0xFE 0xF0 0x0D 0xBA 0xAD...
> 
> xfs_db> p
> Unrecognized attr3 block, attempting to print magic numbers and/or blkinfo:
> unknown.blockhdr.magic = 0xDEADBEEF
> unknown.da_blkinfo.magic = 0xBAAD
> unknown.inode.magic = 0xDEAD

If we asked for an attr3 type, I don't see the value in printing out structures
that are unrelated to an attr3, like an inode....?

The whole problem here is that "attr3" is non-specific, and could be several
formats.  (bad design decision, IMHO, but bridge, water under, etc).  So I'd
prefer that we only print structures which might be relevant to the thing(s)
the user asked for.

-Eric

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Eric Sandeen]

On 4/19/18 3:18 PM, Eric Sandeen wrote:
> On 4/19/18 3:01 PM, Darrick J. Wong wrote:
> 
>  
>> I like it better, though on further thought I think I like better the
>> idea of printing the contents of all potential magic numbers:
>>
>> For a block starting with:
>>
>> 0xDE 0xAD 0xBE 0xEF 0xCA 0xFE 0xF0 0x0D 0xBA 0xAD...
>>
>> xfs_db> p
>> Unrecognized attr3 block, attempting to print magic numbers and/or blkinfo:
>> unknown.blockhdr.magic = 0xDEADBEEF
>> unknown.da_blkinfo.magic = 0xBAAD
>> unknown.inode.magic = 0xDEAD
> 
> If we asked for an attr3 type, I don't see the value in printing out structures
> that are unrelated to an attr3, like an inode....?
> 
> The whole problem here is that "attr3" is non-specific, and could be several
> formats.  (bad design decision, IMHO, but bridge, water under, etc).  So I'd
> prefer that we only print structures which might be relevant to the thing(s)
> the user asked for.

or (per Darrick's irc suggestion) maybe we should just bite the bullet and create
proper explicit types for attr3_leaf, _node, and _remote, and if the magic attr3
type fails, just tell the user to try the others manually?

-Eric

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Carlos Maiolino]

On Thu, Apr 19, 2018 at 03:18:12PM -0500, Eric Sandeen wrote:
> On 4/19/18 3:01 PM, Darrick J. Wong wrote:
> 
>  
> > I like it better, though on further thought I think I like better the
> > idea of printing the contents of all potential magic numbers:
> > 
> > For a block starting with:
> > 
> > 0xDE 0xAD 0xBE 0xEF 0xCA 0xFE 0xF0 0x0D 0xBA 0xAD...
> > 
> > xfs_db> p
> > Unrecognized attr3 block, attempting to print magic numbers and/or blkinfo:
> > unknown.blockhdr.magic = 0xDEADBEEF
> > unknown.da_blkinfo.magic = 0xBAAD
> > unknown.inode.magic = 0xDEAD
> 
> If we asked for an attr3 type, I don't see the value in printing out structures
> that are unrelated to an attr3, like an inode....?
> 

Well, if we fall into this case, the block is supposed to have no attr3 relevant
fields at all, so, I don't see how we would benefit from printing 'relevant'
attr3 data from a non attr3 block.

Although I also didn't like the idea of being 'specific' on the magic number
type, because we don't know what we would be printing, unless we do some checks
and see if we could match the block with a known type, which well, although I
think it's doable, I don't see it being too useful at all.

Best case scenario, we try to print a non-attr3 type using attr3, and a
'generic' attempt to print a magic number using both blkinfo style offset, and
remote style offset, would be enough. As the main reason here is to tell the
user "hey, you are using attr3 on the wrong place", and let the user correct it.
After all, xfs_db users are supposed to know what they are doing.

Worst case scenario is trying to print a corrupted block, which well, no matter
what we print, it's not gonna make sense anyway.

/me starts to think if just printing an "unrecognized format" message isn't the
best approach after all :P


> The whole problem here is that "attr3" is non-specific, and could be several
> formats.  (bad design decision, IMHO, but bridge, water under, etc).  So I'd
> prefer that we only print structures which might be relevant to the thing(s)
> the user asked for.
> 
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Eric Sandeen]

On 4/18/18 4:49 AM, Carlos Maiolino wrote:
> Hi,
> 
> this is supposed to fix the issue while trying to print a non attr3
> block using attr3 type, which ends up on an ASSERT, and crashing xfs_db.
> 
> This is based on Eric's suggestion, which an attempt to print the
> possible magic number from the current block.
> 
> As Eric mentioned, there are other types which need such handling too,
> but, this is supposed to be a RFC to get comments on the approach. Once
> we agree how to fix it, I'll fix the remaining types accordingly.
> 
> This is the following output of an attempt to print a non attr3 type block,
> with this patch:
> 
> xfs_db> fsblock 2
> xfs_db> type attr3
> Unknown attribute buffer type!
> Unknown attribute buffer type!
> xfs_db> p
> Unrecognized attr3 block, possible magic number:
> Unrecognized attr3 block, possible magic number:
> hdr.magic = 0x41423343

I think that if we are going to print anything, we should print it as an
entire, known structure so that the user can decide if it even looks close or
if they just missed... printing only the "magic" which may not even be the
magic for the actual type of block it /should/ be might not be as helpful,
especially if we don't even know what type of "header" this magic lives in.

So it seems like casting it into something the user might expect would
be better?

let's see.

leaf and node blocks both start with a xfs_da3_blkinfo struct, remote is
different :( it starts with xfs_attr3_rmt_hdr, and of course they have
the magic in different places.


What if we had two "unknown" counter functions, and they each printed 
their own possible struct after a printf that explains what they're showing,
something like:

+       { "unk.blkinfo", FLDT_ATTR3_BLKINFO, OI(0), attr3_unknown_hdr_blkinfo_count,
+         FLD_COUNT, TYP_NONE },
+       { "unk.remote", FLDT_ATTR3_REMOTE_HDR, OI(0), attr3_unknown_hdr_remote_count,
+         FLD_COUNT, TYP_NONE },

xfs_db> p
Uknown attr3 type, printing as blkinfo
Uknown attr3 type, printing as remote
Uknown attr3 type, printing as blkinfo  <---  dunno why these gets printed twice
Uknown attr3 type, printing as remote
unk.blkinfo.hdr.forw = 0
unk.blkinfo.hdr.back = 0
unk.blkinfo.hdr.magic = 0
unk.blkinfo.crc = 0 (correct)
unk.blkinfo.bno = 0
unk.blkinfo.lsn = 0
unk.blkinfo.uuid = 00000000-0000-0000-0000-000000000000
unk.blkinfo.owner = 0
unk.remote.magic = 0
unk.remote.offset = 0
unk.remote.bytes = 0
unk.remote.crc = 0 (correct)
unk.remote.uuid = 00000000-0000-0000-0000-000000000000
unk.remote.owner = 0
unk.remote.bno = 0
unk.remote.lsn = 0


?

Is this getting too tricksy?

> While, printing a correct attr3 block, nothing is changed:
> 
> xfs_db> fsblock 11
> xfs_db> type attr3
> xfs_db> p
> hdr.info.hdr.forw = 0
> hdr.info.hdr.back = 0
> hdr.info.hdr.magic = 0x3ebe
> hdr.info.crc = 0x6f7911f7 (correct)
> hdr.info.bno = 88
> .
> .
> .
> 
> Comments?
> 
> Cheers
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  db/attr.c  | 22 ++++++++++++++++++++++
>  db/attr.h  |  1 +
>  db/field.c |  2 ++
>  db/field.h |  1 +
>  4 files changed, 26 insertions(+)
> 
> diff --git a/db/attr.c b/db/attr.c
> index 9cbb20d3..b7692e9e 100644
> --- a/db/attr.c
> +++ b/db/attr.c
> @@ -523,6 +523,21 @@ attr3_remote_hdr_count(
>  	return be32_to_cpu(node->rm_magic) == XFS_ATTR3_RMT_MAGIC;
>  }
>  
> +static int
> +attr3_unkown_hdr_count(
> +	void			*obj,
> +	int			startoff)
> +{
> +	if (!attr3_leaf_hdr_count(obj, startoff) &&
> +	    !attr3_node_hdr_count(obj, startoff) &&
> +	    !attr3_remote_hdr_count(obj,startoff)) {
> +		dbprintf(_("Unrecognized attr3 block, possible magic number:\n"));
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  int
>  attr_size(
>  	void	*obj,
> @@ -549,6 +564,8 @@ const field_t	attr3_flds[] = {
>  	  FLD_COUNT, TYP_NONE },
>  	{ "hdr", FLDT_ATTR3_REMOTE_HDR, OI(0), attr3_remote_hdr_count,
>  	  FLD_COUNT, TYP_NONE },
> +	{ "hdr", FLDT_ATTR3_UNKOWN_HDR, OI(0), attr3_unkown_hdr_count,
> +	  FLD_COUNT, TYP_NONE },
>  	{ "data", FLDT_CHARNS, OI(bitize(sizeof(struct xfs_attr3_rmt_hdr))),
>  	  attr3_remote_data_count, FLD_COUNT, TYP_NONE },
>  	{ "entries", FLDT_ATTR_LEAF_ENTRY, OI(L3OFF(entries)),
> @@ -606,6 +623,11 @@ const struct field	attr3_remote_crc_flds[] = {
>  	{ NULL }
>  };
>  
> +const struct field	attr3_unkown_flds[] = {
> +	{ "magic", FLDT_UINT32X, 0, C1, 0, TYP_NONE},
> +	{ NULL }
> +};
> +
>  /* Set the CRC. */
>  void
>  xfs_attr3_set_crc(
> diff --git a/db/attr.h b/db/attr.h
> index ba23480c..a8566a8c 100644
> --- a/db/attr.h
> +++ b/db/attr.h
> @@ -33,6 +33,7 @@ extern const field_t	attr3_node_hdr_flds[];
>  extern const field_t	attr3_blkinfo_flds[];
>  extern const field_t	attr3_node_hdr_flds[];
>  extern const field_t	attr3_remote_crc_flds[];
> +extern const field_t	attr3_unkown_flds[];
>  
>  extern int	attr_leaf_name_size(void *obj, int startoff, int idx);
>  extern int	attr_size(void *obj, int startoff, int idx);
> diff --git a/db/field.c b/db/field.c
> index ae4c8057..388bcbe1 100644
> --- a/db/field.c
> +++ b/db/field.c
> @@ -102,6 +102,8 @@ const ftattr_t	ftattrtab[] = {
>  	{ FLDT_ATTR3_REMOTE_HDR, "attr3_remote_hdr", NULL,
>  	  (char *)attr3_remote_crc_flds, attr_size, FTARG_SIZE, NULL,
>  	  attr3_remote_crc_flds },
> +	{ FLDT_ATTR3_UNKOWN_HDR, "attr3_unkown_hdr", NULL,
> +	  NULL, NULL, FTARG_SIZE, NULL, attr3_unkown_flds},
>  
>  	{ FLDT_BMAPBTA, "bmapbta", NULL, (char *)bmapbta_flds, btblock_size,
>  	  FTARG_SIZE, NULL, bmapbta_flds },
> diff --git a/db/field.h b/db/field.h
> index a8df29bc..291ef2e5 100644
> --- a/db/field.h
> +++ b/db/field.h
> @@ -48,6 +48,7 @@ typedef enum fldt	{
>  	FLDT_ATTR3_LEAF_HDR,
>  	FLDT_ATTR3_NODE_HDR,
>  	FLDT_ATTR3_REMOTE_HDR,
> +	FLDT_ATTR3_UNKOWN_HDR,
>  
>  	FLDT_BMAPBTA,
>  	FLDT_BMAPBTA_CRC,
> 

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Carlos Maiolino]

Hi,

> > xfs_db> fsblock 2
> > xfs_db> type attr3
> > Unknown attribute buffer type!
> > Unknown attribute buffer type!
> > xfs_db> p
> > Unrecognized attr3 block, possible magic number:
> > Unrecognized attr3 block, possible magic number:
> > hdr.magic = 0x41423343
> 
> I think that if we are going to print anything, we should print it as an
> entire, known structure so that the user can decide if it even looks close or
> if they just missed... printing only the "magic" which may not even be the
> magic for the actual type of block it /should/ be might not be as helpful,
> especially if we don't even know what type of "header" this magic lives in.
> 
> So it seems like casting it into something the user might expect would
> be better?
> 

Not sure, in this case, the 'user' is expaecting an attr3 type, while xfs_db is
reading some other random, non attr3 type block. So, I wonder, what you meant by
'something the user might expect'? The way I view it is: "The user is expecting
attr3 type", but there is no attr3 data in the block being read, so, the way I
understand what you meant, was trying to print the current block using attr3
format, which, IMHO, might fool the xfs_db user. I think saying the block
doesn't match attr3 format, and printing a possible magic number found in the
block works better. After all, xfs_db is a developer tool, not an user tool. If
I use myself and my xfs_db usage as an example:
If I try to print some block as attr3, and I see a message saying no attr3
format has been found, and I got a dump of a possible magic number, I can check
if that magic number matches some metadata format, or if it doesn't look as a
magic at all, I can simply consider the block corrupted. But well, this is just
my way to view it.

> let's see.
> 
> leaf and node blocks both start with a xfs_da3_blkinfo struct, remote is
> different :( it starts with xfs_attr3_rmt_hdr, and of course they have
> the magic in different places.
> 
> 
> What if we had two "unknown" counter functions, and they each printed 
> their own possible struct after a printf that explains what they're showing,
> something like:
> 
> +       { "unk.blkinfo", FLDT_ATTR3_BLKINFO, OI(0), attr3_unknown_hdr_blkinfo_count,
> +         FLD_COUNT, TYP_NONE },
> +       { "unk.remote", FLDT_ATTR3_REMOTE_HDR, OI(0), attr3_unknown_hdr_remote_count,
> +         FLD_COUNT, TYP_NONE },
> 
> xfs_db> p
> Uknown attr3 type, printing as blkinfo
> Uknown attr3 type, printing as remote
> Uknown attr3 type, printing as blkinfo  <---  dunno why these gets printed twice
> Uknown attr3 type, printing as remote

Probably due a print of the 'parent' and another print on 'child', but it's just
a guess :)

> unk.blkinfo.hdr.forw = 0
> unk.blkinfo.hdr.back = 0
> unk.blkinfo.hdr.magic = 0
> unk.blkinfo.crc = 0 (correct)
> unk.blkinfo.bno = 0
> unk.blkinfo.lsn = 0
> unk.blkinfo.uuid = 00000000-0000-0000-0000-000000000000
> unk.blkinfo.owner = 0
> unk.remote.magic = 0
> unk.remote.offset = 0
> unk.remote.bytes = 0
> unk.remote.crc = 0 (correct)
> unk.remote.uuid = 00000000-0000-0000-0000-000000000000
> unk.remote.owner = 0
> unk.remote.bno = 0
> unk.remote.lsn = 0
> 
> 
> ?
> 
> Is this getting too tricksy?

Ok, but, what's the difference in doing this, and simply printing out an "unknown
type" message? All the fields are zeroed, showing no clue if the block is from a
different type, or corrupted in some way, mainly because, if we ever hit this
'problem', we are likely to have no idea if the current block would be a leaf or
a node, so, IMHO, it is just no informative at all, but again, this is just my
humble opinion :)


-- 
Carlos

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Eric Sandeen]

On 4/23/18 2:26 AM, Carlos Maiolino wrote:
>> So it seems like casting it into something the user might expect would
>> be better?
>>
> Not sure, in this case, the 'user' is expaecting an attr3 type, while xfs_db is
> reading some other random, non attr3 type block. So, I wonder, what you meant by
> 'something the user might expect'? The way I view it is: "The user is expecting
> attr3 type", but there is no attr3 data in the block being read, so, the way I
> understand what you meant, was trying to print the current block using attr3
> format, which, IMHO, might fool the xfs_db user. I think saying the block
> doesn't match attr3 format, and printing a possible magic number found in the
> block works better. After all, xfs_db is a developer tool, not an user tool. If
> I use myself and my xfs_db usage as an example:
> If I try to print some block as attr3, and I see a message saying no attr3
> format has been found, and I got a dump of a possible magic number, I can check
> if that magic number matches some metadata format, or if it doesn't look as a
> magic at all, I can simply consider the block corrupted. But well, this is just
> my way to view it.
> 

So, don't get too hung up on your "it's not actually an attr3 block" testcase;
imagine the scenario where it /is/ an attr3 block, but the magic is
corrupted.  Now imagine that you want to examine the block (to see what the magic
was, and what the other fields were, and if it even looks like an attr3 block,
or something else), and possibly even fix a field manually.

If all you get is "unknown" that's not very helpful.  You /could/ hexdump it
and count bytes, but that sucks.

If you tried the same thing on i.e. an agf header (with bitflipped magic) you
would still be able to print the whole structure, and see what was wrong.

Because attr3 only prints semi-validated blocks today, and refuses to show you
anything that doesn't match, it's a far more difficult situation.  The root
cause is that "attr3" is a multiplexer that will really decide between 3 different
formats, but will /only/ show you one of the 3 if the magic is undamaged.

That's why I was wondering about explicit types for the leaf/block etc so you
can force xfs_db to show it to you in that format (like you could with i.e. agf).

Meh, for now maybe I will just merge the "don't segfault" patch, it's at least an
improvement.  A nicer outcome isn't our most pressing problem I guess.

-Eric

Re: [RFC PATCH] db: Stop core dumping on attr3 if block header is not recognized

[Carlos Maiolino]

On Mon, Apr 23, 2018 at 08:54:37AM -0600, Eric Sandeen wrote:
> 
> 
> On 4/23/18 2:26 AM, Carlos Maiolino wrote:
> >> So it seems like casting it into something the user might expect would
> >> be better?
> >>
> > Not sure, in this case, the 'user' is expaecting an attr3 type, while xfs_db is
> > reading some other random, non attr3 type block. So, I wonder, what you meant by
> > 'something the user might expect'? The way I view it is: "The user is expecting
> > attr3 type", but there is no attr3 data in the block being read, so, the way I
> > understand what you meant, was trying to print the current block using attr3
> > format, which, IMHO, might fool the xfs_db user. I think saying the block
> > doesn't match attr3 format, and printing a possible magic number found in the
> > block works better. After all, xfs_db is a developer tool, not an user tool. If
> > I use myself and my xfs_db usage as an example:
> > If I try to print some block as attr3, and I see a message saying no attr3
> > format has been found, and I got a dump of a possible magic number, I can check
> > if that magic number matches some metadata format, or if it doesn't look as a
> > magic at all, I can simply consider the block corrupted. But well, this is just
> > my way to view it.
> > 
> 
> So, don't get too hung up on your "it's not actually an attr3 block" testcase;
> imagine the scenario where it /is/ an attr3 block, but the magic is
> corrupted.  Now imagine that you want to examine the block (to see what the magic
> was, and what the other fields were, and if it even looks like an attr3 block,
> or something else), and possibly even fix a field manually.
> 
> If all you get is "unknown" that's not very helpful.  You /could/ hexdump it
> and count bytes, but that sucks.
> 
> If you tried the same thing on i.e. an agf header (with bitflipped magic) you
> would still be able to print the whole structure, and see what was wrong.
> 
> Because attr3 only prints semi-validated blocks today, and refuses to show you
> anything that doesn't match, it's a far more difficult situation.  The root
> cause is that "attr3" is a multiplexer that will really decide between 3 different
> formats, but will /only/ show you one of the 3 if the magic is undamaged.
> 
> That's why I was wondering about explicit types for the leaf/block etc so you
> can force xfs_db to show it to you in that format (like you could with i.e. agf).
> 

Got your concern, and I agree, I can work on it if you are not in a hurry to get
it merged.

> Meh, for now maybe I will just merge the "don't segfault" patch, it's at least an
> improvement.  A nicer outcome isn't our most pressing problem I guess.
>

That would be great, I can work on the leaf/block separation in the meantime
without having my xfs_db crashing on it :)

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

-- 
Carlos