/**
* struct iommu_destroy - ioctl(IOMMU_DESTROY)
* @size: sizeof(struct iommu_destroy)
* @id: iommufd object ID to destroy. Can be any destroyable object type.
*
* Destroy any object held within iommufd.
*/
struct iommu_destroy {
__u32 size;
__u32 id;
};
#define IOMMU_DESTROY _IO(IOMMUFD_TYPE, IOMMUFD_CMD_DESTROY)
Generates this kdoc:
https://docs.kernel.org/userspace-api/iommufd.html#c.iommu_destroy
You should also make sure to link the uapi header into the kdoc build
under the "userspace API" chaper.
The structs should also be self-describing. I am fairly strongly
against using the size mechanism in the _IOW macro, it is instantly
ABI incompatible and basically impossible to deal with from userspace.
Hence why the IOMMFD version is _IO().
This means stick a size member in the first 4 bytes of every
struct. More on this later..
> +/**
> + * LIVEUPDATE_IOCTL_FD_UNPRESERVE - Remove a file descriptor from the
> + * preservation list.
> + *
> + * Argument: Pointer to __u64 token.
Every ioctl should have a struct, with the size header. If you want to
do more down the road you can not using this structure.
> +#define LIVEUPDATE_IOCTL_FD_RESTORE \
> + _IOWR(LIVEUPDATE_IOCTL_TYPE, 0x02, struct liveupdate_fd)
Strongly recommend that every ioctl have a unique struct. Sharing
structs makes future extend-ability harder.
> +/**
> + * LIVEUPDATE_IOCTL_PREPARE - Initiate preparation phase and trigger state
> + * saving.
Perhaps these just want to be a single 'set state' ioctl with an enum
input argument?
> @@ -7,4 +7,5 @@ obj-$(CONFIG_KEXEC_HANDOVER) += kexec_handover.o
> obj-$(CONFIG_KEXEC_HANDOVER_DEBUG) += kexec_handover_debug.o
> obj-$(CONFIG_LIVEUPDATE) += luo_core.o
> obj-$(CONFIG_LIVEUPDATE) += luo_files.o
> +obj-$(CONFIG_LIVEUPDATE) += luo_ioctl.o
> obj-$(CONFIG_LIVEUPDATE) += luo_subsystems.o
I don't think luo is modular, but I think it is generally better to
write the kbuilds as though it was anyhow if it has a lot of files:
iommufd-y := \
device.o \
eventq.o \
hw_pagetable.o \
io_pagetable.o \
ioas.o \
main.o \
pages.o \
vfio_compat.o \
viommu.o
obj-$(CONFIG_IOMMUFD) += iommufd.o
Basically don't repeat obj-$(CONFIG_LIVEUPDATE), every one of those
lines creates a new module (if it was modular)
> +static int luo_open(struct inode *inodep, struct file *filep)
> +{
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
IMHO file system permissions should control permission to open. No
capable check.
> + if (filep->f_flags & O_EXCL)
> + return -EINVAL;
O_EXCL doesn't really do anything for cdev, I'd drop this.
The open should have an atomic to check for single open though.
> +static long luo_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> +{
> + void __user *argp = (void __user *)arg;
> + struct liveupdate_fd luo_fd;
> + enum liveupdate_state state;
> + int ret = 0;
> + u64 token;
> +
> + if (_IOC_TYPE(cmd) != LIVEUPDATE_IOCTL_TYPE)
> + return -ENOTTY;
The generic parse/disptach from fwctl is a really good idea here, you
can cut and paste it, change the names. It makes it really easy to manage future extensibility:
List the ops and their structs:
static const struct fwctl_ioctl_op fwctl_ioctl_ops[] = {
IOCTL_OP(FWCTL_INFO, fwctl_cmd_info, struct fwctl_info, out_device_data),
IOCTL_OP(FWCTL_RPC, fwctl_cmd_rpc, struct fwctl_rpc, out),
};
Index the list and copy_from_user the struct desribing the opt:
static long fwctl_fops_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
{
struct fwctl_uctx *uctx = filp->private_data;
const struct fwctl_ioctl_op *op;
struct fwctl_ucmd ucmd = {};
union fwctl_ucmd_buffer buf;
unsigned int nr;
int ret;
nr = _IOC_NR(cmd);
if ((nr - FWCTL_CMD_BASE) >= ARRAY_SIZE(fwctl_ioctl_ops))
return -ENOIOCTLCMD;
op = &fwctl_ioctl_ops[nr - FWCTL_CMD_BASE];
if (op->ioctl_num != cmd)
return -ENOIOCTLCMD;
ucmd.uctx = uctx;
ucmd.cmd = &buf;
ucmd.ubuffer = (void __user *)arg;
// This is reading/checking the standard 4 byte size header:
ret = get_user(ucmd.user_size, (u32 __user *)ucmd.ubuffer);
if (ret)
return ret;
if (ucmd.user_size < op->min_size)
return -EINVAL;
ret = copy_struct_from_user(ucmd.cmd, op->size, ucmd.ubuffer,
ucmd.user_size);
Removes a bunch of boiler plate and easy to make wrong copy_from_users
in the ioctls. Centralizes size validation, zero padding checking/etc.
> + ret = luo_register_file(luo_fd.token, luo_fd.fd);
> + if (!ret && copy_to_user(argp, &luo_fd, sizeof(luo_fd))) {
> + WARN_ON_ONCE(luo_unregister_file(luo_fd.token));
> + ret = -EFAULT;
Then for extensibility you'd copy back the struct:
static int ucmd_respond(struct fwctl_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;
return 0;
}
Which truncates it/etc according to some ABI extensibility rules.
> +static int __init liveupdate_init(void)
> +{
> + int err;
> +
> + if (!liveupdate_enabled())
> + return 0;
> +
> + err = misc_register(&liveupdate_miscdev);
> + if (err < 0) {
> + pr_err("Failed to register misc device '%s': %d\n",
> + liveupdate_miscdev.name, err);
Should remove most of the pr_err's, here too IMHO..
Jason
