Re: [PATCH 4/4] autofs4 - add miscelaneous device for ioctls

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 07 Aug 2008 19:40:31 +0800
Ian Kent <raven@themaw.net> wrote:
>
> Subject: [PATCH 4/4] autofs4 - add miscelaneous device for ioctls

I fixed that spello

> Patch to add a miscellaneous device to the autofs4 module for routing
> ioctls. This provides the ability to obtain an ioctl file handle for
> an autofs mount point that is possibly covered by another mount.
> 
> The actual problem with autofs is that it can't reconnect to existing
> mounts. Immediately one things of just adding the ability to remount
> autofs file systems would solve it, but alas, that can't work. This is
> because autofs direct mounts and the implementation of "on demand mount
> and expire" of nested mount trees have the file system mounted on top of
> the mount trigger dentry.
> 
> To resolve this a miscellaneous device node for routing ioctl commands
> to these mount points has been implemented in the autofs4 kernel module
> and a library added to autofs. This provides the ability to open a file
> descriptor for these over mounted autofs mount points.
> 
> Please refer to Documentation/filesystems/autofs4-mount-control.txt for
> a discussion of the problem, implementation alternatives considered and
> a description of the interface.
> 
>
> ...
>
> +
> +#define AUTOFS_DEV_IOCTL_SIZE	sizeof(struct autofs_dev_ioctl)
> +
> +typedef int (*ioctl_fn)(struct file *,
> +struct autofs_sb_info *, struct autofs_dev_ioctl *);

Weird layout, which I fixed.

> +static int check_name(const char *name)
> +{
> +	if (!strchr(name, '/'))
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +/*
> + * Check a string doesn't overrun the chunk of
> + * memory we copied from user land.
> + */
> +static int invalid_str(char *str, void *end)
> +{
> +	while ((void *) str <= end)
> +		if (!*str++)
> +			return 0;
> +	return -EINVAL;
> +}

What is this?  gWwe copy strings in from userspace in 10000 different
places without needing checks such as this?

> +/*
> + * Check that the user compiled against correct version of autofs
> + * misc device code.
> + *
> + * As well as checking the version compatibility this always copies
> + * the kernel interface version out.
> + */
> +static int check_dev_ioctl_version(int cmd, struct autofs_dev_ioctl *param)
> +{
> +	int err = 0;
> +
> +	if ((AUTOFS_DEV_IOCTL_VERSION_MAJOR != param->ver_major) ||
> +	    (AUTOFS_DEV_IOCTL_VERSION_MINOR < param->ver_minor)) {
> +		AUTOFS_WARN("ioctl control interface version mismatch: "
> +		     "kernel(%u.%u), user(%u.%u), cmd(%d)",
> +		     AUTOFS_DEV_IOCTL_VERSION_MAJOR,
> +		     AUTOFS_DEV_IOCTL_VERSION_MINOR,
> +		     param->ver_major, param->ver_minor, cmd);
> +		err = -EINVAL;
> +	}
> +
> +	/* Fill in the kernel version. */
> +	param->ver_major = AUTOFS_DEV_IOCTL_VERSION_MAJOR;
> +	param->ver_minor = AUTOFS_DEV_IOCTL_VERSION_MINOR;
> +
> +	return err;
> +}
> +
> +/*
> + * Copy parameter control struct, including a possible path allocated
> + * at the end of the struct.
> + */
> +static struct autofs_dev_ioctl *copy_dev_ioctl(struct autofs_dev_ioctl __user *in)
> +{
> +	struct autofs_dev_ioctl tmp, *ads;

Variables called `tmp' get me upset, but it seems appropriate here.

> +	if (copy_from_user(&tmp, in, sizeof(tmp)))
> +		return ERR_PTR(-EFAULT);
> +
> +	if (tmp.size < sizeof(tmp))
> +		return ERR_PTR(-EINVAL);
> +
> +	ads = kmalloc(tmp.size, GFP_KERNEL);
> +	if (!ads)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (copy_from_user(ads, in, tmp.size)) {
> +		kfree(ads);
> +		return ERR_PTR(-EFAULT);
> +	}
> +
> +	return ads;
> +}
> +
> +static inline void free_dev_ioctl(struct autofs_dev_ioctl *param)
> +{
> +	kfree(param);
> +	return;
> +}
> +
> +/*
> + * Check sanity of parameter control fields and if a path is present
> + * check that it has a "/" and is terminated.
> + */
> +static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param)
> +{
> +	int err = -EINVAL;
> +
> +	if (check_dev_ioctl_version(cmd, param)) {
> +		AUTOFS_WARN("invalid device control module version "
> +		     "supplied for cmd(0x%08x)", cmd);
> +		goto out;

check_dev_ioctl_version() carefully returned a -EFOO value, but this
caller dropped it on the floor.

> +	}
> +
> +	if (param->size > sizeof(*param)) {
> +		err = check_name(param->path);
> +		if (err) {
> +			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
> +				    cmd);
> +			goto out;
> +		}
> +
> +		err = invalid_str(param->path,
> +				 (void *) ((size_t) param + param->size));
> +		if (err) {
> +			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
> +				    cmd);
> +			goto out;
> +		}
> +	}
> +
> +	err = 0;
> +out:
> +	return err;
> +}
> +
>
> ...
>
> +static void autofs_dev_ioctl_fd_install(unsigned int fd, struct file *file)
> +{
> +	struct files_struct *files = current->files;
> +	struct fdtable *fdt;
> +
> +	spin_lock(&files->file_lock);
> +	fdt = files_fdtable(files);
> +	BUG_ON(fdt->fd[fd] != NULL);
> +	rcu_assign_pointer(fdt->fd[fd], file);
> +	FD_SET(fd, fdt->close_on_exec);
> +	spin_unlock(&files->file_lock);
> +}

urgh, it's bad to have done a copy-n-paste on fd_install() here.  It
means that if we go and change the locking in core kernel (quite
possible) then people won't udpate this code.

Do we have alternative here?  Can we set close_on_exec outside the lock
and just call fd_install()?  Probably not.

Can we export set_close_on_exec() then call that after having called
fd_install()?  I think so.

But not this, please.