linux-kernel - Re: [PATCH -tip] remove the BKL: Replace BKL in mount/umount syscalls with a mutex

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: <20090416143626.GA17683@infradead.org>
Date:	Thu, 16 Apr 2009 10:36:26 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Alessio Igor Bogani <abogani@...ware.it>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	LKML <linux-kernel@...r.kernel.org>,
	Jonathan Corbet <corbet@....net>, viro@...iv.linux.org.uk
Subject: Re: [PATCH -tip] remove the BKL: Replace BKL in mount/umount
	syscalls with a mutex

On Thu, Apr 16, 2009 at 04:27:58PM +0200, Alessio Igor Bogani wrote:
> Replace ths BKL in sys_mount()/sys_umount() syscalls with a regular mutex.

Could you try to explain what these actuall try to protect?

>  	unsigned long tmp = ((unsigned long)mnt / L1_CACHE_BYTES);
> @@ -1073,9 +1075,9 @@ static int do_umount(struct vfsmount *mnt, int flags)
>  	 */
>  
>  	if (flags & MNT_FORCE && sb->s_op->umount_begin) {
> -		lock_kernel();
> +		mutex_lock(&mount_lock);
>  		sb->s_op->umount_begin(sb);
> -		unlock_kernel();
> +		mutex_unlock(&mount_lock);

This is a very easy case, just move the lock into ->umount_begin.  And
then ping the maintainers of the 5 instances actually making use of it -
I suspect none of them actually require it.

> @@ -1094,9 +1096,9 @@ static int do_umount(struct vfsmount *mnt, int flags)
>  		 */
>  		down_write(&sb->s_umount);
>  		if (!(sb->s_flags & MS_RDONLY)) {
> -			lock_kernel();
> +			mutex_lock(&mount_lock);
>  			retval = do_remount_sb(sb, MS_RDONLY, NULL, 0);
> -			unlock_kernel();
> +			mutex_unlock(&mount_lock);

I suspect moving lock_kernel down into ->remount_fs is the much better
option.  Will require some audit of do_remount_sb, though.

> -	lock_kernel();
> +	mutex_lock(&mount_lock);
>  	retval = do_mount((char *)dev_page, dir_page, (char *)type_page,
>  			  flags, (void *)data_page);
> -	unlock_kernel();
> +	mutex_unlock(&mount_lock);

Again, much better to push it down and probably eliminate it completely
for all sane filesystems.

--
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/