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
prev parent 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).