[Cluster-devel] [PATCH 8/8] libgfs2: Add support for rg_crc

[Andrew Price]

On 07/12/17 13:53, Bob Peterson wrote:
> Hi,
> 
> ----- Original Message -----
> | This new field in the resource group headers is set on write and checked
> | on read so that fsck.gfs2 now considers a resource group with a bad,
> | non-zero crc to be corrupted. gfs2_grow, gfs2_edit, and mkfs.gfs2 all
> | now support the rg_crc field, including save/restoremeta.
> |
> | Signed-off-by: Andrew Price <anprice at redhat.com>
> | ---
> (snip)
> | diff --git a/gfs2/libgfs2/rgrp.c b/gfs2/libgfs2/rgrp.c
> | index 17178bd9..03861d14 100644
> | --- a/gfs2/libgfs2/rgrp.c
> | +++ b/gfs2/libgfs2/rgrp.c
> | @@ -158,6 +158,42 @@ void lgfs2_rgrp_bitbuf_free(lgfs2_rgrp_t rg)
> |  }
> |
> |  /**
> | + * Check a resource group's crc
> | + * Returns 0 on success, non-zero if crc is bad
> | + */
> | +int lgfs2_rgrp_crc_check(const struct gfs2_rgrp *rg, char *buf)
> | +{
> | +	int ret = 0;
> | +#ifdef GFS2_HAS_RG_RI_FIELDS
> | +	uint32_t crc = rg->rg_crc;
> | +
> | +	if (crc == 0)
> | +		return 0;
> | +
> | +	((struct gfs2_rgrp *)buf)->rg_crc = 0;
> | +	if (crc != gfs2_disk_hash(buf, sizeof(struct gfs2_rgrp)))
> | +		ret = 1;
> | +	((struct gfs2_rgrp *)buf)->rg_crc = cpu_to_be32(crc);
> 
> I didn't trace the code all the way back, so I might be wrong. But:
> 
> My concern here is that lgfs2_rgrp_crc_check can modify the contents
> of the buffer here. That is exactly what you want in most cases, but
> it might be a problem for fsck.gfs2 cases other than -y. For example,
> The -n option instructs fsck to not make any changes, and without -y,
> it's supposed to ask before making any changes. So hopefully this
> buffer is never rewritten without permission, at some other point in
> fsck. It's worth checking. It also may be worth testing to make sure
> specifying -n detects crc errors, reports them, but does not fix them
> with -n.

So fsck.gfs2 opens the device read-only with -n but I agree with your 
core concern. Since we can't be sure that the contents of rg and buf 
mirror each other, it would be better to operate on just the buffer and 
make sure the original value is restored once the check is done. I've 
updated it to:

int lgfs2_rgrp_crc_check(char *buf)
{
         int ret = 0;
#ifdef GFS2_HAS_RG_RI_FIELDS
         struct gfs2_rgrp *rg = (struct gfs2_rgrp *)buf;
         uint32_t crc = rg->rg_crc;

         if (crc == 0)
                 return 0;

         rg->rg_crc = 0;
         if (be32_to_cpu(crc) != gfs2_disk_hash(buf, sizeof(struct 
gfs2_rgrp)))
                 ret = 1;
         rg->rg_crc = crc;
#endif
         return ret;
}

> 
> (snip)
> | +/**
> | + * Set the crc of an on-disk resource group
> | + */
> | +void lgfs2_rgrp_crc_set(char *buf)
> | +{
> | +#ifdef GFS2_HAS_RG_RI_FIELDS
> | +	struct gfs2_rgrp *rg = (struct gfs2_rgrp *)buf;
> | +	uint32_t crc;
> | +
> | +	rg->rg_crc = 0;
> | +	crc = gfs2_disk_hash(buf, sizeof(struct gfs2_rgrp));
> | +	rg->rg_crc = cpu_to_be32(crc);
> | +#endif
> | +}
> 
> You may want to move the initialization of rg_crc (and supporting
> code, obviously) outside the #ifdef so it gets initialized to 0.

rg_crc doesn't exist outside of the #ifdef and that rg_reserved chunk 
gets zeroed in libgfs2 so this should be ok too.

Thanks for your reviews, I'll repost the updated patches after some 
extra testing.

Cheers,
Andy