Re: [PATCH v3 16/30] liveupdate: luo_ioctl: add userpsace interface

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Aug 07, 2025 at 01:44:22AM +0000, Pasha Tatashin wrote:
> +/**
> + * DOC: General ioctl format
> + *
> + * The ioctl interface follows a general format to allow for extensibility. Each
> + * ioctl is passed in a structure pointer as the argument providing the size of
> + * the structure in the first u32. The kernel checks that any structure space
> + * beyond what it understands is 0. This allows userspace to use the backward
> + * compatible portion while consistently using the newer, larger, structures.
> + *
> + * ioctls use a standard meaning for common errnos:
> + *
> + *  - ENOTTY: The IOCTL number itself is not supported at all
> + *  - E2BIG: The IOCTL number is supported, but the provided structure has
> + *    non-zero in a part the kernel does not understand.
> + *  - EOPNOTSUPP: The IOCTL number is supported, and the structure is
> + *    understood, however a known field has a value the kernel does not
> + *    understand or support.
> + *  - EINVAL: Everything about the IOCTL was understood, but a field is not
> + *    correct.
> + *  - ENOENT: An ID or IOVA provided does not exist.
                    ^^^^^^^^^

Maybe this should be 'token' ?

> + *  - ENOMEM: Out of memory.
> + *  - EOVERFLOW: Mathematics overflowed.
> + *
> + * As well as additional errnos, within specific ioctls.
> + */

Ah if you copy the comment make sure to faithfully follow it in the
implementation :)

> +struct liveupdate_ioctl_fd_unpreserve {
> +       __u32           size;
> +       __aligned_u64   token;
> +};

It is best to explicitly pad, so add a __u32 reserved between size and
token

Then you need to also check that the reserved is 0 when parsing it,
return -EOPNOTSUPP otherwise.

> +static atomic_t luo_device_in_use = ATOMIC_INIT(0);

I suggest you bundle this together into one struct with the misc_dev
and the other globals and largely pretend it is not global, eg refer
to it through container_of, etc

Following practices like this make it harder to abuse the globals.

> +struct luo_ucmd {
> +	void __user *ubuffer;
> +	u32 user_size;
> +	void *cmd;
> +};
> +
> +static int luo_ioctl_fd_preserve(struct luo_ucmd *ucmd)
> +{
> +	struct liveupdate_ioctl_fd_preserve *argp = ucmd->cmd;
> +	int ret;
> +
> +	ret = luo_register_file(argp->token, argp->fd);
> +	if (!ret)
> +		return ret;
> +
> +	if (copy_to_user(ucmd->ubuffer, argp, ucmd->user_size))
> +		return -EFAULT;

This will overflow memory, ucmd->user_size may be > sizeof(*argp)

The respond function is an important part of this scheme:

static inline int iommufd_ucmd_respond(struct iommufd_ucmd *ucmd,
                                       size_t cmd_len)
{
        if (copy_to_user(ucmd->ubuffer, ucmd->cmd,
                         min_t(size_t, ucmd->user_size, cmd_len)))
                return -EFAULT;

The min (sizeof(*argp) in this case) can't be skipped!

> +static int luo_ioctl_fd_restore(struct luo_ucmd *ucmd)
> +{
> +	struct liveupdate_ioctl_fd_restore *argp = ucmd->cmd;
> +	struct file *file;
> +	int ret;
> +
> +	argp->fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (argp->fd < 0) {
> +		pr_err("Failed to allocate new fd: %d\n", argp->fd);

No need

> +		return argp->fd;
> +	}
> +
> +	ret = luo_retrieve_file(argp->token, &file);
> +	if (ret < 0) {
> +		put_unused_fd(argp->fd);
> +
> +		return ret;
> +	}
> +
> +	fd_install(argp->fd, file);
> +
> +	if (copy_to_user(ucmd->ubuffer, argp, ucmd->user_size))
> +		return -EFAULT;

Wrong order, fd_install must be last right before return 0. Failing
system calls should not leave behind installed FDs.

> +static int luo_ioctl_set_event(struct luo_ucmd *ucmd)
> +{
> +	struct liveupdate_ioctl_set_event *argp = ucmd->cmd;
> +	int ret;
> +
> +	switch (argp->event) {
> +	case LIVEUPDATE_PREPARE:
> +		ret = luo_prepare();
> +		break;
> +	case LIVEUPDATE_FINISH:
> +		ret = luo_finish();
> +		break;
> +	case LIVEUPDATE_CANCEL:
> +		ret = luo_cancel();
> +		break;
> +	default:
> +		ret = -EINVAL;

EOPNOTSUPP

> +union ucmd_buffer {
> +	struct liveupdate_ioctl_fd_preserve	preserve;
> +	struct liveupdate_ioctl_fd_unpreserve	unpreserve;
> +	struct liveupdate_ioctl_fd_restore	restore;
> +	struct liveupdate_ioctl_get_state	state;
> +	struct liveupdate_ioctl_set_event	event;
> +};

I discourage the column alignment. Also sort by name.

> +static const struct luo_ioctl_op luo_ioctl_ops[] = {
> +	IOCTL_OP(LIVEUPDATE_IOCTL_FD_PRESERVE, luo_ioctl_fd_preserve,
> +		 struct liveupdate_ioctl_fd_preserve, token),
> +	IOCTL_OP(LIVEUPDATE_IOCTL_FD_UNPRESERVE, luo_ioctl_fd_unpreserve,
> +		 struct liveupdate_ioctl_fd_unpreserve, token),
> +	IOCTL_OP(LIVEUPDATE_IOCTL_FD_RESTORE, luo_ioctl_fd_restore,
> +		 struct liveupdate_ioctl_fd_restore, token),
> +	IOCTL_OP(LIVEUPDATE_IOCTL_GET_STATE, luo_ioctl_get_state,
> +		 struct liveupdate_ioctl_get_state, state),
> +	IOCTL_OP(LIVEUPDATE_IOCTL_SET_EVENT, luo_ioctl_set_event,
> +		 struct liveupdate_ioctl_set_event, event),

Sort by name

Jason