| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANpmjNMAWuE0Y20ZuBUSRXkvWZd8NC1d=DDYYrEZytJz9ndxeA@mail.gmail.com>
Date: Tue, 21 Dec 2021 21:19:41 +0100
From: Marco Elver <elver@...gle.com>
To: Aleksandr Nogikh <nogikh@...gle.com>
Cc: kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org,
akpm@...ux-foundation.org, dvyukov@...gle.com,
andreyknvl@...il.com, glider@...gle.com, tarasmadan@...gle.com,
bigeasy@...utronix.de
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@...gle.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@...gle.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
>
Powered by blists - more mailing lists