lkml.org 
[lkml]   [2021]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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
>

\
 
 \ /
  Last update: 2021-12-21 21:21    [W:0.041 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site