Re: [GIT PULL] Preparation for BKL'ed ioctl removal

[Jan Blunck]

On Fri, Apr 16, Frederic Weisbecker wrote:

> Linus,
> 
> In order to stop the bkl contagion in ioctl and push the
> bkl from Vfs to the drivers that don't implement a proper
> unlock_ioctl, this patch proposes to add a new "locked_ioctl"
> field in struct file_operation.
> 
> This field is never called from vfs and requires to assign a
> new "deprecated_ioctl" helper to the unlocked_ioctl field.
> This deprecated_ioctl() helper then calls the locked_ioctl()
> callback with the bkl held.
> 
> The point of doing this is to change every users of fops::ioctl
> to the new scheme. Once there is no more users of the ioctl field,
> we'll then be able to remove it.
> 
> Also this change brings a new config BKL which is always
> enabled for now. Every drivers that use the bkl through
> explicit lock_kernel() calls or by using deprecated_ioctl()
> will have to select CONFIG_BKL.
> 
> Once we get no more uses of the bkl from the core features but
> only on individual drivers, config BKL can be turned off
> by default and selected by those drivers if needed, relegating
> the bkl as an obsolete library for poor old drivers.
> 
> And given the work happening currently (tty mutex, vfs pushdowns,
> procfs bkl removal, llseek janitorials, etc...), this may happen
> sooner than was expected.
> 
> Now the point of having this patch in 2.6.34 is to turn
> the old ioctl implementations to the new scheme in a set
> of patches that relevant maintainers can apply to their
> tree (or apply by ourself for unmaintained areas) for .35
> Otherwise we would need to queue everything in a single
> tree that may bring conflicts in the next merge window.
> 
> This single patch has very few chances to bring any regressions.
> 
> Please pull the bkl/ioctl branch that can be found at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	bkl/ioctl
> 
> Thanks,
> 	Frederic
> ---
> 
> Arnd Bergmann (1):
>       vfs: Introduce CONFIG_BKL and deprecated_ioctl
> 
> 
>  fs/ioctl.c               |   22 ++++++++++++++++++++++
>  include/linux/fs.h       |    3 +++
>  include/linux/smp_lock.h |    4 ++++
>  kernel/Kconfig.locks     |   10 ++++++++++
>  4 files changed, 39 insertions(+), 0 deletions(-)
> 
> ---
> commit 588a2267f7b2919ae73380e0932cc6a202787036
> Author: Arnd Bergmann <arnd@xxxxxxxx>
> Date:   Thu Apr 1 14:42:38 2010 +0200
> 
>     vfs: Introduce CONFIG_BKL and deprecated_ioctl
>     
>     This is a preparation for the removal of the big kernel lock that
>     introduces new interfaces for device drivers still using it.
>     
>     We can start marking those device drivers as 'depends on CONFIG_BKL'
>     now, and make that symbol optional later, when the point has come
>     at which we are able to build a kernel without the BKL.
>     
>     Similarly, device drivers that currently make use of the implicit
>     BKL locking around the ioctl function can now get annotated by
>     changing
>     
>     	.ioctl = foo_ioctl,
>     
>     to
>     
>     	.locked_ioctl = foo_ioctl,
>     	.unlocked_ioctl = deprecated_ioctl,
>     
>     As soon as no driver remains using the old ioctl callback, it can
>     get removed.
>     
>     [fweisbec: move config bkl from Kconfig.debug to Kconfig.locks,
>     rename default_ioctl to deprecated_ioctl]
>     
>     Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
>     Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 6c75110..e6d6a75 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -58,6 +58,28 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd,
>  	return error;
>  }
>  
> +#ifdef CONFIG_BKL
> +/*
> + * deprecated_ioctl - call locked_ioctl with BKL held
> + *
> + * Setting only the ioctl operation but not unlocked_ioctl will become
> + * invalid in the future, all drivers that are not converted to unlocked_ioctl
> + * should set .unlocked_ioctl = deprecated_ioctl now.
> + */
> +long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	int error = -ENOTTY;
> +	if (filp->f_op->locked_ioctl) {
> +		lock_kernel();
> +		error = filp->f_op->locked_ioctl(filp->f_path.dentry->d_inode,
> +						 filp, cmd, arg);
> +		unlock_kernel();
> +	}
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(default_ioctl);
> +#endif
> +
>  static int ioctl_fibmap(struct file *filp, int __user *p)
>  {
>  	struct address_space *mapping = filp->f_mapping;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 39d57bc..6b65b26 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1492,6 +1492,9 @@ struct file_operations {
>  	int (*readdir) (struct file *, void *, filldir_t);
>  	unsigned int (*poll) (struct file *, struct poll_table_struct *);
>  	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
> +#ifdef CONFIG_BKL
> +	int (*locked_ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
> +#endif
>  	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
>  	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
>  	int (*mmap) (struct file *, struct vm_area_struct *);
> diff --git a/include/linux/smp_lock.h b/include/linux/smp_lock.h
> index 2ea1dd1..c8693e1 100644
> --- a/include/linux/smp_lock.h
> +++ b/include/linux/smp_lock.h
> @@ -62,4 +62,8 @@ static inline void cycle_kernel_lock(void)
>  #define kernel_locked()				1
>  
>  #endif /* CONFIG_LOCK_KERNEL */
> +
> +loff_t default_llseek(struct file *file, loff_t offset, int origin);

Any specific reason why the default llseek() is in this patch?

Jan

> +long deprecated_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> +
>  #endif /* __LINUX_SMPLOCK_H */
> diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
> index 88c92fb..9a80413 100644
> --- a/kernel/Kconfig.locks
> +++ b/kernel/Kconfig.locks
> @@ -200,3 +200,13 @@ config INLINE_WRITE_UNLOCK_IRQRESTORE
>  
>  config MUTEX_SPIN_ON_OWNER
>  	def_bool SMP && !DEBUG_MUTEXES && !HAVE_DEFAULT_NO_SPIN_MUTEXES
> +
> +config BKL
> +	def_bool y
> +	help
> +	  This is the traditional lock that is used in old code instead
> +	  of proper locking. All drivers that use the BKL should depend
> +	  on this symbol.
> +	  This configuration option will become user-selectable in the
> +	  future, as soon as it is possible to build a kernel without
> +	  it.
Regards,
	Jan

-- 
Jan Blunck <jblunck@xxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/