[Cluster-devel] [PATCH] gfs2: Null-check gl in gfs2_rgrp_dump()

[Andrew Price <anprice@redhat.com>]

On 14/10/2020 15:30, Andreas Gruenbacher wrote:
> Hi Andy,
> 
>> When an rindex entry is found to be corrupt, compute_bitstructs() calls
>> gfs2_consist_rgrpd() which calls gfs2_rgrp_dump() like this:
>>
>>      gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
>>
>> gfs2_rgrp_dump then dereferences the gl without checking it and we get
>>
>>      BUG: KASAN: null-ptr-deref in gfs2_rgrp_dump+0x28/0x280
>>
>> because there's no rgrp glock involved while reading the rindex on mount.
>>
>> Add a NULL check for gl in gfs2_rgrp_dump() to allow the withdraw to
>> continue cleanly.
> 
> thanks for looking into this.  Shouldn't gfs2_rgrp_dump just take an
> rgrp argument as below though?
> 

Yes, that's a much better arrangement. Built and tested against the 
reproducer. Looks good.

Thanks,
Andy

> ---
>   fs/gfs2/glops.c | 11 ++++++++++-
>   fs/gfs2/rgrp.c  |  9 +++------
>   fs/gfs2/rgrp.h  |  2 +-
>   fs/gfs2/util.c  |  2 +-
>   4 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index de1d5f1d9ff8..c2c90747d79b 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -227,6 +227,15 @@ static void rgrp_go_inval(struct gfs2_glock *gl, int flags)
>   		rgd->rd_flags &= ~GFS2_RDF_UPTODATE;
>   }
>   
> +static void gfs2_rgrp_go_dump(struct seq_file *seq, struct gfs2_glock *gl,
> +			      const char *fs_id_buf)
> +{
> +	struct gfs2_rgrpd *rgd = gfs2_glock2rgrp(gl);
> +
> +	if (rgd)
> +		gfs2_rgrp_dump(seq, rgd, fs_id_buf);
> +}
> +
>   static struct gfs2_inode *gfs2_glock2inode(struct gfs2_glock *gl)
>   {
>   	struct gfs2_inode *ip;
> @@ -712,7 +721,7 @@ const struct gfs2_glock_operations gfs2_rgrp_glops = {
>   	.go_sync = rgrp_go_sync,
>   	.go_inval = rgrp_go_inval,
>   	.go_lock = gfs2_rgrp_go_lock,
> -	.go_dump = gfs2_rgrp_dump,
> +	.go_dump = gfs2_rgrp_go_dump,
>   	.go_type = LM_TYPE_RGRP,
>   	.go_flags = GLOF_LVB,
>   };
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 074f228ea839..1bba5a9d45fa 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -2209,20 +2209,17 @@ static void rgblk_free(struct gfs2_sbd *sdp, struct gfs2_rgrpd *rgd,
>   /**
>    * gfs2_rgrp_dump - print out an rgrp
>    * @seq: The iterator
> - * @gl: The glock in question
> + * @rgd: The rgrp in question
>    * @fs_id_buf: pointer to file system id (if requested)
>    *
>    */
>   
> -void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_glock *gl,
> +void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_rgrpd *rgd,
>   		    const char *fs_id_buf)
>   {
> -	struct gfs2_rgrpd *rgd = gl->gl_object;
>   	struct gfs2_blkreserv *trs;
>   	const struct rb_node *n;
>   
> -	if (rgd == NULL)
> -		return;
>   	gfs2_print_dbg(seq, "%s R: n:%llu f:%02x b:%u/%u i:%u r:%u e:%u\n",
>   		       fs_id_buf,
>   		       (unsigned long long)rgd->rd_addr, rgd->rd_flags,
> @@ -2253,7 +2250,7 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
>   		(unsigned long long)rgd->rd_addr);
>   	fs_warn(sdp, "umount on all nodes and run fsck.gfs2 to fix the error\n");
>   	sprintf(fs_id_buf, "fsid=%s: ", sdp->sd_fsname);
> -	gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
> +	gfs2_rgrp_dump(NULL, rgd, fs_id_buf);
>   	rgd->rd_flags |= GFS2_RDF_ERROR;
>   }
>   
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index a1d7e14fc55b..9a587ada51ed 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -67,7 +67,7 @@ extern void gfs2_rlist_add(struct gfs2_inode *ip, struct gfs2_rgrp_list *rlist,
>   extern void gfs2_rlist_alloc(struct gfs2_rgrp_list *rlist);
>   extern void gfs2_rlist_free(struct gfs2_rgrp_list *rlist);
>   extern u64 gfs2_ri_total(struct gfs2_sbd *sdp);
> -extern void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_glock *gl,
> +extern void gfs2_rgrp_dump(struct seq_file *seq, struct gfs2_rgrpd *rgd,
>   			   const char *fs_id_buf);
>   extern int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
>   				   struct buffer_head *bh,
> diff --git a/fs/gfs2/util.c b/fs/gfs2/util.c
> index 1cd0328cae20..0fba3bf64189 100644
> --- a/fs/gfs2/util.c
> +++ b/fs/gfs2/util.c
> @@ -419,7 +419,7 @@ void gfs2_consist_rgrpd_i(struct gfs2_rgrpd *rgd,
>   	char fs_id_buf[sizeof(sdp->sd_fsname) + 7];
>   
>   	sprintf(fs_id_buf, "fsid=%s: ", sdp->sd_fsname);
> -	gfs2_rgrp_dump(NULL, rgd->rd_gl, fs_id_buf);
> +	gfs2_rgrp_dump(NULL, rgd, fs_id_buf);
>   	gfs2_lm(sdp,
>   		"fatal: filesystem consistency error\n"
>   		"  RG = %llu\n"
>