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

quoted

+ *  - 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' ?

Yes, replaced with token. :-)

quoted

+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.

Done.

quoted

+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.

Done, good suggestion.

quoted

+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!

Done, thank you for catching this.

quoted

+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

Removed

quoted

+             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.

Fixed.

quoted

+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

Ack.

quoted

+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.

Done

quoted

+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

Done