On Thu, 07 Aug 2008 19:40:31 +0800
Ian Kent <raven@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
