linux-kernel - Re: [PATCH] fs: record task name which froze superblock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150216093852.GB4749@quack.suse.cz>
Date:	Mon, 16 Feb 2015 10:38:52 +0100
From:	Jan Kara <jack@...e.cz>
To:	Alexey Dobriyan <adobriyan@...il.com>
Cc:	akpm@...ux-foundation.org, viro@...iv.linux.org.uk,
	linux-kernel@...r.kernel.org, jack@...e.cz,
	linux-fsdevel@...r.kernel.org, swhiteho@...hat.com,
	cluster-devel@...hat.com
Subject: Re: [PATCH] fs: record task name which froze superblock

On Sat 14-02-15 21:55:24, Alexey Dobriyan wrote:
> Freezing and thawing are separate system calls, task which is supposed
> to thaw filesystem/superblock can disappear due to crash or not thaw
> due to a bug. Record at least task name (we can't take task_struct
> reference) to make support engineer's life easier.
> 
> Hopefully 16 bytes per superblock isn't much.
> 
> P.S.: Cc'ing GFS2 people just in case they want to correct
> my understanding of GFS2 having async freeze code.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@...il.com>
  Hum, and when do you show the task name? Or do you expect that customer
takes a crashdump and support just finds it in memory?

> ---
> 
>  fs/ioctl.c         |   22 ++++++++++++++++------
>  fs/super.c         |    2 ++
>  include/linux/fs.h |    2 ++
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -518,6 +518,7 @@ static int ioctl_fioasync(unsigned int fd, struct file *filp,
>  static int ioctl_fsfreeze(struct file *filp)
>  {
>  	struct super_block *sb = file_inode(filp)->i_sb;
> +	int rv;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -527,22 +528,31 @@ static int ioctl_fsfreeze(struct file *filp)
>  		return -EOPNOTSUPP;
>  
>  	/* Freeze */
> -	if (sb->s_op->freeze_super)
> -		return sb->s_op->freeze_super(sb);
> -	return freeze_super(sb);
> +	if (sb->s_op->freeze_super) {
> +		rv = sb->s_op->freeze_super(sb);
> +		if (rv == 0)
> +			get_task_comm(sb->s_writers.freeze_comm, current);
> +	} else
> +		rv = freeze_super(sb);
> +	return rv;
  Why don't you just set the name in ioctl_fsfreeze() in both cases?
Also you seem to be missing freezing / thawing in freeze/thaw_bdev()
functions.

>  }
>  
>  static int ioctl_fsthaw(struct file *filp)
>  {
>  	struct super_block *sb = file_inode(filp)->i_sb;
> +	int rv;
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
>  	/* Thaw */
> -	if (sb->s_op->thaw_super)
> -		return sb->s_op->thaw_super(sb);
> -	return thaw_super(sb);
> +	if (sb->s_op->thaw_super) {
> +		rv = sb->s_op->thaw_super(sb);
> +		if (rv == 0)
> +			memset(sb->s_writers.freeze_comm, 0, TASK_COMM_LEN);
> +	} else
> +		rv = thaw_super(sb);
> +	return rv;
>  }
>  
>  /*
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1355,6 +1355,7 @@ int freeze_super(struct super_block *sb)
>  	 * sees write activity when frozen is set to SB_FREEZE_COMPLETE.
>  	 */
>  	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
> +	get_task_comm(sb->s_writers.freeze_comm, current);
>  	up_write(&sb->s_umount);
>  	return 0;
>  }
> @@ -1391,6 +1392,7 @@ int thaw_super(struct super_block *sb)
>  
>  out:
>  	sb->s_writers.frozen = SB_UNFROZEN;
> +	memset(sb->s_writers.freeze_comm, 0, TASK_COMM_LEN);
>  	smp_wmb();
>  	wake_up(&sb->s_writers.wait_unfrozen);
>  	deactivate_locked_super(sb);
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1221,6 +1221,8 @@ struct sb_writers {
>  	int			frozen;		/* Is sb frozen? */
>  	wait_queue_head_t	wait_unfrozen;	/* queue for waiting for
>  						   sb to be thawed */
> +	/* who froze superblock */
> +	char			freeze_comm[16];
  Here should be TASK_COMM_LEN, shouldn't it?

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/