Re: [GIT PULL] pidfd updates

[Al Viro]

On Thu, Apr 27, 2023 at 10:33:38AM +0200, Christian Brauner wrote:

> File descriptor installation is not core functionality for drivers. It's
> just something that they have to do and so it's not that people usually
> put a lot of thought into it. So that's why I think an API has to be
> dumb enough. A three call api may still be simpler to use than an overly
> clever single call api.

Grep and you will see...  Seriously, for real mess take a look at e.g.
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c.  See those close_fd() calls in
there?  That's completely wrong - you can't undo the insertion into
descriptor table.

I'm not suggesting that for the core kernel, but there's a plenty of
drivers that do descriptor allocation.

Take a look at e.g. drivers/media/mc/mc-request.c:media_request_alloc().
OK, we open, insert, etc. and we pass the descriptor to caller in
*alloc_fd.  Caller is in drivers/media/mc/mc-device.c:
static long media_device_request_alloc(struct media_device *mdev, void *arg)
and descriptor goes into *(int *)arg there.  That is reached via
static const struct media_ioctl_info ioctl_info[] = {
        MEDIA_IOC(DEVICE_INFO, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
	MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
	MEDIA_IOC(ENUM_LINKS, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
	MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
	MEDIA_IOC(REQUEST_ALLOC, media_device_request_alloc, 0),
};      
used in
static long media_device_ioctl(struct file *filp, unsigned int cmd,
                               unsigned long __arg)
There we have
        ret = info->fn(dev, karg);

	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
		mutex_unlock(&dev->graph_mutex);

	if (!ret && info->arg_to_user)
		ret = info->arg_to_user(arg, karg, cmd);

array elements are set by
#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)              \
        [_IOC_NR(MEDIA_IOC_##__cmd)] = {                                \
                .cmd = MEDIA_IOC_##__cmd,                               \
                .fn = func,                                             \
                .flags = fl,                                            \
                .arg_from_user = from_user,                             \
                .arg_to_user = to_user,                                 \
        }

#define MEDIA_IOC(__cmd, func, fl)                                      \
        MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)

so this ->arg_to_user() is copy_arg_to_user(), which is
static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
{
        if ((_IOC_DIR(cmd) & _IOC_READ) &&
            copy_to_user(uarg, karg, _IOC_SIZE(cmd)))
                return -EFAULT;

        return 0;
}

That copy_to_user() is not attempted until media_device_request_alloc()
returns.  And I don't see any way to make it unroll the insertion into
descriptor table without massive restructuring of the entire thing;
if you do, I'd love to hear it.

This is actually not the worst case - again, drm stuff has a bunch of
such crap, and I don't believe it's feasible to move drm folks from
their "we do copyin and copyout in generic ioctl code, passing the
copy to handlers supplied by drivers and copying whatever they modified
back to userland" approach.