[Cluster-devel] [GFS2 Patch] GFS2: Add kobject release method

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

Now in the -nmw git tree. Thanks,

Steve.

On Wed, 2012-06-13 at 10:27 -0400, Bob Peterson wrote:
> Hi,
> 
> This patch adds a kobject release function that properly maintains
> the kobject use count, so that accesses to the sysfs files do not
> cause an access to freed kernel memory after an unmount.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
> ---
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index c5871ae..378c90f 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -1123,20 +1123,33 @@ static int fill_super(struct super_block *sb, struct gfs2_args *args, int silent
>  	}
>  
>  	error = init_names(sdp, silent);
> -	if (error)
> -		goto fail;
> +	if (error) {
> +		/* In this case, we haven't initialized sysfs, so we have to
> +		   manually free the sdp. */
> +		free_percpu(sdp->sd_lkstats);
> +		kfree(sdp);
> +		sb->s_fs_info = NULL;
> +		return error;
> +	}
>  
>  	snprintf(sdp->sd_fsname, GFS2_FSNAME_LEN, "%s", sdp->sd_table_name);
>  
> -	gfs2_create_debugfs_file(sdp);
> -
>  	error = gfs2_sys_fs_add(sdp);
> +	/*
> +	 * If we hit an error here, gfs2_sys_fs_add will have called function
> +	 * kobject_put which causes the sysfs usage count to go to zero, which
> +	 * causes sysfs to call function gfs2_sbd_release, which frees sdp.
> +	 * Subsequent error paths here will call gfs2_sys_fs_del, which also
> +	 * kobject_put to free sdp.
> +	 */
>  	if (error)
> -		goto fail;
> +		return error;
> +
> +	gfs2_create_debugfs_file(sdp);
>  
>  	error = gfs2_lm_mount(sdp, silent);
>  	if (error)
> -		goto fail_sys;
> +		goto fail_debug;
>  
>  	error = init_locking(sdp, &mount_gh, DO);
>  	if (error)
> @@ -1220,12 +1233,12 @@ fail_locking:
>  fail_lm:
>  	gfs2_gl_hash_clear(sdp);
>  	gfs2_lm_unmount(sdp);
> -fail_sys:
> -	gfs2_sys_fs_del(sdp);
> -fail:
> +fail_debug:
>  	gfs2_delete_debugfs_file(sdp);
>  	free_percpu(sdp->sd_lkstats);
> -	kfree(sdp);
> +	/* gfs2_sys_fs_del must be the last thing we do, since it causes
> +	 * sysfs to call function gfs2_sbd_release, which frees sdp. */
> +	gfs2_sys_fs_del(sdp);
>  	sb->s_fs_info = NULL;
>  	return error;
>  }
> @@ -1395,10 +1408,9 @@ static void gfs2_kill_sb(struct super_block *sb)
>  	sdp->sd_root_dir = NULL;
>  	sdp->sd_master_dir = NULL;
>  	shrink_dcache_sb(sb);
> -	kill_block_super(sb);
>  	gfs2_delete_debugfs_file(sdp);
>  	free_percpu(sdp->sd_lkstats);
> -	kfree(sdp);
> +	kill_block_super(sb);
>  }
>  
>  struct file_system_type gfs2_fs_type = {
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index d33172c..1a0fad9 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -276,7 +276,15 @@ static struct attribute *gfs2_attrs[] = {
>  	NULL,
>  };
>  
> +static void gfs2_sbd_release(struct kobject *kobj)
> +{
> +	struct gfs2_sbd *sdp = container_of(kobj, struct gfs2_sbd, sd_kobj);
> +
> +	kfree(sdp);
> +}
> +
>  static struct kobj_type gfs2_ktype = {
> +	.release = gfs2_sbd_release,
>  	.default_attrs = gfs2_attrs,
>  	.sysfs_ops     = &gfs2_attr_ops,
>  };
> @@ -581,6 +589,7 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
>  	char ro[20];
>  	char spectator[20];
>  	char *envp[] = { ro, spectator, NULL };
> +	int sysfs_frees_sdp = 0;
>  
>  	sprintf(ro, "RDONLY=%d", (sb->s_flags & MS_RDONLY) ? 1 : 0);
>  	sprintf(spectator, "SPECTATOR=%d", sdp->sd_args.ar_spectator ? 1 : 0);
> @@ -589,8 +598,10 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
>  	error = kobject_init_and_add(&sdp->sd_kobj, &gfs2_ktype, NULL,
>  				     "%s", sdp->sd_table_name);
>  	if (error)
> -		goto fail;
> +		goto fail_reg;
>  
> +	sysfs_frees_sdp = 1; /* Freeing sdp is now done by sysfs calling
> +				function gfs2_sbd_release. */
>  	error = sysfs_create_group(&sdp->sd_kobj, &tune_group);
>  	if (error)
>  		goto fail_reg;
> @@ -613,9 +624,13 @@ fail_lock_module:
>  fail_tune:
>  	sysfs_remove_group(&sdp->sd_kobj, &tune_group);
>  fail_reg:
> -	kobject_put(&sdp->sd_kobj);
> -fail:
> +	free_percpu(sdp->sd_lkstats);
>  	fs_err(sdp, "error %d adding sysfs files", error);
> +	if (sysfs_frees_sdp)
> +		kobject_put(&sdp->sd_kobj);
> +	else
> +		kfree(sdp);
> +	sb->s_fs_info = NULL;
>  	return error;
>  }
>  
>