Messages in this thread | | | From | Marco Elver <> | Date | Tue, 21 Dec 2021 21:19:41 +0100 | Subject | Re: [PATCH v2 1/2] kcov: split ioctl handling into locked and unlocked parts |
| |
On Tue, 21 Dec 2021 at 18:04, Aleksandr Nogikh <nogikh@google.com> wrote: > > Currently all ioctls are de facto processed under a spin lock in order > to serialise them. This, however, prohibits the use of vmalloc and other > memory management functions in the implementation of those ioctls, > unnecessary complicating any further changes. > > Let all ioctls first be processed inside the kcov_ioctl_unlocked() > function which should execute the ones that are not compatible with > spinlock and pass control to kcov_ioctl_locked() for all other ones. > > Although it is still compatible with a spinlock, move KCOV_INIT_TRACE > handling to kcov_ioctl_unlocked(), so that its planned change is easier > to follow. > > Signed-off-by: Aleksandr Nogikh <nogikh@google.com> > --- > kernel/kcov.c | 64 +++++++++++++++++++++++++++++++-------------------- > 1 file changed, 39 insertions(+), 25 deletions(-) > > diff --git a/kernel/kcov.c b/kernel/kcov.c > index 36ca640c4f8e..5d87b4e0126f 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. > @@ -685,6 +666,43 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd, > } > } > > +static int kcov_ioctl_unlocked(struct kcov *kcov, unsigned int cmd, > + unsigned long arg) > +{ > + unsigned long size, flags; > + int res; > + > + 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;
This patch should be a non-functional change, but it is not.
To do that, you'd have to add the locking around KCOV_INIT_TRACE here, and then do whatever else you're doing in patch 2/2.
> + default: > + /* > + * All other commands can be fully executed under a spin lock, so we > + * obtain and release it here to simplify the code of kcov_ioctl_locked(). > + */ > + spin_lock_irqsave(&kcov->lock, flags); > + res = kcov_ioctl_locked(kcov, cmd, arg); > + spin_unlock_irqrestore(&kcov->lock, flags); > + return res; > + } > +} > + > static long kcov_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > { > struct kcov *kcov; > @@ -692,7 +710,6 @@ 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; > > if (cmd == KCOV_REMOTE_ENABLE) { > if (get_user(remote_num_handles, (unsigned __user *)(arg + > @@ -713,10 +730,7 @@ static long kcov_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > } > > kcov = filep->private_data; > - spin_lock_irqsave(&kcov->lock, flags); > - res = kcov_ioctl_locked(kcov, cmd, arg); > - spin_unlock_irqrestore(&kcov->lock, flags); > - > + res = kcov_ioctl_unlocked(kcov, cmd, arg);
Also, I find that kcov_ioctl_unlocked() isn't a very descriptive name, since now we have both locked and unlocked variants. What is it actually doing?
Perhaps kcov_ioctl_with_context()? Assuming that 'struct kcov' is some sort of context.
> kfree(remote_arg); > > return res; > -- > 2.34.1.307.g9b7440fafd-goog >
| |