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 <rpete...@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;
> }
>
>
