Messages in this thread | | | From | Andrey Konovalov <> | Date | Mon, 24 Jan 2022 23:33:13 +0100 | Subject | Re: [PATCH v3 1/2] kcov: split ioctl handling into locked and unlocked parts |
| |
.On Mon, Jan 17, 2022 at 4:36 PM Aleksandr Nogikh <nogikh@google.com> wrote: > > Currently all ioctls are de facto processed under a spinlock in order > to serialise them. This, however, prohibits the use of vmalloc and other > memory management functions in the implementations of those ioctls, > unnecessary complicating any further changes to the code. > > Let all ioctls first be processed inside the kcov_ioctl() function > which should execute the ones that are not compatible with spinlock > and then pass control to kcov_ioctl_locked() for all other ones. > KCOV_REMOTE_ENABLE is processed both in kcov_ioctl() and > kcov_ioctl_locked() as the steps are easily separable. > > Although it is still compatible with a spinlock, move KCOV_INIT_TRACE > handling to kcov_ioctl(), so that the changes from the next commit are > easier to follow. > > Signed-off-by: Aleksandr Nogikh <nogikh@google.com> > --- > kernel/kcov.c | 68 ++++++++++++++++++++++++++++----------------------- > 1 file changed, 37 insertions(+), 31 deletions(-) > > diff --git a/kernel/kcov.c b/kernel/kcov.c > index 36ca640c4f8e..e1be7301500b 100644 > --- a/kernel/kcov.c > +++ b/kernel/kcov.c > @@ -564,31 +564,12 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd, > unsigned long arg) > { > struct task_struct *t; > - unsigned long size, unused; > + unsigned long flags, unused; > int mode, i; > struct kcov_remote_arg *remote_arg; > struct kcov_remote *remote; > - unsigned long flags; > > switch (cmd) { > - case KCOV_INIT_TRACE: > - /* > - * Enable kcov in trace mode and setup buffer size. > - * Must happen before anything else. > - */ > - if (kcov->mode != KCOV_MODE_DISABLED) > - return -EBUSY; > - /* > - * Size must be at least 2 to hold current position and one PC. > - * Later we allocate size * sizeof(unsigned long) memory, > - * that must not overflow. > - */ > - size = arg; > - if (size < 2 || size > INT_MAX / sizeof(unsigned long)) > - return -EINVAL; > - kcov->size = size; > - kcov->mode = KCOV_MODE_INIT; > - return 0; > case KCOV_ENABLE: > /* > * Enable coverage for the current task. > @@ -692,9 +673,32 @@ static long kcov_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > struct kcov_remote_arg *remote_arg = NULL; > unsigned int remote_num_handles; > unsigned long remote_arg_size; > - unsigned long flags; > + unsigned long size, flags; > > - if (cmd == KCOV_REMOTE_ENABLE) { > + kcov = filep->private_data; > + switch (cmd) { > + case KCOV_INIT_TRACE: > + /* > + * Enable kcov in trace mode and setup buffer size. > + * Must happen before anything else. > + * > + * First check the size argument - it must be at least 2 > + * to hold the current position and one PC. Later we allocate > + * size * sizeof(unsigned long) memory, that must not overflow. > + */ > + size = arg; > + if (size < 2 || size > INT_MAX / sizeof(unsigned long)) > + return -EINVAL; > + spin_lock_irqsave(&kcov->lock, flags);
Arguably, we could keep the part of the KCOV_INIT_TRACE handler that happens under the lock in kcov_ioctl_locked(). In a similar way as it's done for KCOV_REMOTE_ENABLE. This would get rid of the asymmetric fallthrough usage.
But I'll leave this up to you, either way looks acceptable to me.
> + if (kcov->mode != KCOV_MODE_DISABLED) { > + spin_unlock_irqrestore(&kcov->lock, flags); > + return -EBUSY; > + } > + kcov->size = size; > + kcov->mode = KCOV_MODE_INIT; > + spin_unlock_irqrestore(&kcov->lock, flags); > + return 0; > + case KCOV_REMOTE_ENABLE: > if (get_user(remote_num_handles, (unsigned __user *)(arg + > offsetof(struct kcov_remote_arg, num_handles)))) > return -EFAULT; > @@ -710,16 +714,18 @@ static long kcov_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > return -EINVAL; > } > arg = (unsigned long)remote_arg; > + fallthrough; > + default: > + /* > + * All other commands can be normally executed under a spin lock, so we > + * obtain and release it here in order to simplify kcov_ioctl_locked(). > + */ > + spin_lock_irqsave(&kcov->lock, flags); > + res = kcov_ioctl_locked(kcov, cmd, arg); > + spin_unlock_irqrestore(&kcov->lock, flags); > + kfree(remote_arg); > + return res; > } > - > - kcov = filep->private_data; > - spin_lock_irqsave(&kcov->lock, flags); > - res = kcov_ioctl_locked(kcov, cmd, arg); > - spin_unlock_irqrestore(&kcov->lock, flags); > - > - kfree(remote_arg); > - > - return res; > } > > static const struct file_operations kcov_fops = { > -- > 2.34.1.703.g22d0c6ccf7-goog >
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
| |