LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Oded Gabbay <oded.gabbay@gmail.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/4] habanalabs: never copy_from_user inside spinlock
Date: Sat, 21 Aug 2021 14:04:37 +0300	[thread overview]
Message-ID: <CAFCwf12vkav1WcxEs0YnjA4t_h5uN13-roEK4OF-5DmQQzjHxg@mail.gmail.com> (raw)
In-Reply-To: <20210820150719.67934-4-ogabbay@kernel.org>

On Fri, Aug 20, 2021 at 6:09 PM Oded Gabbay <ogabbay@kernel.org> wrote:
>
> copy_from_user might sleep so we can never call it when we have
> a spinlock.
>
> Moreover, it is not necessary in waiting for user interrupt, because
> if multiple threads will call this function on the same interrupt,
> each one will have it's own fence object inside the driver. The
> user address might be the same, but it doesn't really matter to us,
> as we only read from it.
>
> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> ---
>  .../habanalabs/common/command_submission.c    | 35 +++++++------------
>  1 file changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/misc/habanalabs/common/command_submission.c b/drivers/misc/habanalabs/common/command_submission.c
> index a97bb27ebb90..7b0516cf808b 100644
> --- a/drivers/misc/habanalabs/common/command_submission.c
> +++ b/drivers/misc/habanalabs/common/command_submission.c
> @@ -2740,14 +2740,10 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
>         else
>                 interrupt = &hdev->user_interrupt[interrupt_offset];
>
> -       spin_lock_irqsave(&interrupt->wait_list_lock, flags);
> -
> -       if (copy_from_user(&completion_value, u64_to_user_ptr(user_address),
> -                                                                       4)) {
> -               dev_err(hdev->dev,
> -                       "Failed to copy completion value from user\n");
> +       if (copy_from_user(&completion_value, u64_to_user_ptr(user_address), 4)) {
> +               dev_err(hdev->dev, "Failed to copy completion value from user\n");
>                 rc = -EFAULT;
> -               goto unlock_and_free_fence;
> +               goto free_fence;
>         }
>
>         if (completion_value >= target_value)
> @@ -2756,42 +2752,35 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
>                 *status = CS_WAIT_STATUS_BUSY;
>
>         if (!timeout_us || (*status == CS_WAIT_STATUS_COMPLETED))
> -               goto unlock_and_free_fence;
> +               goto free_fence;
>
>         /* Add pending user interrupt to relevant list for the interrupt
>          * handler to monitor
>          */
> +       spin_lock_irqsave(&interrupt->wait_list_lock, flags);
>         list_add_tail(&pend->wait_list_node, &interrupt->wait_list_head);
>         spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
>
>  wait_again:
>         /* Wait for interrupt handler to signal completion */
> -       completion_rc =
> -               wait_for_completion_interruptible_timeout(
> -                               &pend->fence.completion, timeout);
> +       completion_rc = wait_for_completion_interruptible_timeout(&pend->fence.completion,
> +                                                                               timeout);
>
>         /* If timeout did not expire we need to perform the comparison.
>          * If comparison fails, keep waiting until timeout expires
>          */
>         if (completion_rc > 0) {
> -               spin_lock_irqsave(&interrupt->wait_list_lock, flags);
> -
> -               if (copy_from_user(&completion_value,
> -                               u64_to_user_ptr(user_address), 4)) {
> -
> -                       spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
> -
> -                       dev_err(hdev->dev,
> -                               "Failed to copy completion value from user\n");
> +               if (copy_from_user(&completion_value, u64_to_user_ptr(user_address), 4)) {
> +                       dev_err(hdev->dev, "Failed to copy completion value from user\n");
>                         rc = -EFAULT;
>
>                         goto remove_pending_user_interrupt;
>                 }
>
>                 if (completion_value >= target_value) {
> -                       spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
>                         *status = CS_WAIT_STATUS_COMPLETED;
>                 } else {
> +                       spin_lock_irqsave(&interrupt->wait_list_lock, flags);
>                         reinit_completion(&pend->fence.completion);
>                         timeout = completion_rc;
>
> @@ -2811,9 +2800,9 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
>  remove_pending_user_interrupt:
>         spin_lock_irqsave(&interrupt->wait_list_lock, flags);
>         list_del(&pend->wait_list_node);
> -
> -unlock_and_free_fence:
>         spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
> +
> +free_fence:
>         kfree(pend);
>         hl_ctx_put(ctx);
>
> --
> 2.17.1
>

Hi Greg,
Thanks for pointing this issue out. It slipped my CR (my bad).
I believe this fixes the problem and I've gone over the entire driver
and didn't see any other occurrence of this bug.

Oded

      reply	other threads:[~2021-08-21 11:05 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 15:07 [PATCH 1/4] habanalabs: add "in device creation" status Oded Gabbay
2021-08-20 15:07 ` [PATCH 2/4] habanalabs: disable IRQ in user interrupts spinlock Oded Gabbay
2021-08-20 15:07 ` [PATCH 3/4] habanalabs: remove unnecessary device status check Oded Gabbay
2021-08-20 15:07 ` [PATCH 4/4] habanalabs: never copy_from_user inside spinlock Oded Gabbay
2021-08-21 11:04   ` Oded Gabbay [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFCwf12vkav1WcxEs0YnjA4t_h5uN13-roEK4OF-5DmQQzjHxg@mail.gmail.com \
    --to=oded.gabbay@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH 4/4] habanalabs: never copy_from_user inside spinlock' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).