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

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

[Bob Peterson]

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

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

[Steven Whitehouse]

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