* [PATCH 2/4] habanalabs: disable IRQ in user interrupts spinlock
2021-08-20 15:07 [PATCH 1/4] habanalabs: add "in device creation" status Oded Gabbay
@ 2021-08-20 15:07 ` Oded Gabbay
2021-08-20 15:07 ` [PATCH 3/4] habanalabs: remove unnecessary device status check Oded Gabbay
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Oded Gabbay @ 2021-08-20 15:07 UTC (permalink / raw)
To: linux-kernel
Because this spinlock is taken in an interrupt handler, we must use
the spin_lock_irqsave/irqrestore version to disable the interrupts
on the local CPU. Otherwise, we can have a potential deadlock (if
the interrupt handler is scheduled to run on the same cpu that the
code who took the lock was running on).
Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
---
.../habanalabs/common/command_submission.c | 25 ++++++++++---------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/misc/habanalabs/common/command_submission.c b/drivers/misc/habanalabs/common/command_submission.c
index 3a67265312ee..8a2f75de6df8 100644
--- a/drivers/misc/habanalabs/common/command_submission.c
+++ b/drivers/misc/habanalabs/common/command_submission.c
@@ -923,13 +923,14 @@ static void
wake_pending_user_interrupt_threads(struct hl_user_interrupt *interrupt)
{
struct hl_user_pending_interrupt *pend;
+ unsigned long flags;
- spin_lock(&interrupt->wait_list_lock);
+ spin_lock_irqsave(&interrupt->wait_list_lock, flags);
list_for_each_entry(pend, &interrupt->wait_list_head, wait_list_node) {
pend->fence.error = -EIO;
complete_all(&pend->fence.completion);
}
- spin_unlock(&interrupt->wait_list_lock);
+ spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
}
void hl_release_pending_user_interrupts(struct hl_device *hdev)
@@ -2714,9 +2715,9 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
{
struct hl_user_pending_interrupt *pend;
struct hl_user_interrupt *interrupt;
- unsigned long timeout;
- long completion_rc;
+ unsigned long timeout, flags;
u32 completion_value;
+ long completion_rc;
int rc = 0;
if (timeout_us == U32_MAX)
@@ -2739,7 +2740,7 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
else
interrupt = &hdev->user_interrupt[interrupt_offset];
- spin_lock(&interrupt->wait_list_lock);
+ spin_lock_irqsave(&interrupt->wait_list_lock, flags);
if (!hl_device_operational(hdev, NULL)) {
rc = -EPERM;
goto unlock_and_free_fence;
@@ -2765,7 +2766,7 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
* handler to monitor
*/
list_add_tail(&pend->wait_list_node, &interrupt->wait_list_head);
- spin_unlock(&interrupt->wait_list_lock);
+ spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
wait_again:
/* Wait for interrupt handler to signal completion */
@@ -2777,12 +2778,12 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
* If comparison fails, keep waiting until timeout expires
*/
if (completion_rc > 0) {
- spin_lock(&interrupt->wait_list_lock);
+ spin_lock_irqsave(&interrupt->wait_list_lock, flags);
if (copy_from_user(&completion_value,
u64_to_user_ptr(user_address), 4)) {
- spin_unlock(&interrupt->wait_list_lock);
+ spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
dev_err(hdev->dev,
"Failed to copy completion value from user\n");
@@ -2792,13 +2793,13 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
}
if (completion_value >= target_value) {
- spin_unlock(&interrupt->wait_list_lock);
+ spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
*status = CS_WAIT_STATUS_COMPLETED;
} else {
reinit_completion(&pend->fence.completion);
timeout = completion_rc;
- spin_unlock(&interrupt->wait_list_lock);
+ spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
goto wait_again;
}
} else if (completion_rc == -ERESTARTSYS) {
@@ -2812,11 +2813,11 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
}
remove_pending_user_interrupt:
- spin_lock(&interrupt->wait_list_lock);
+ spin_lock_irqsave(&interrupt->wait_list_lock, flags);
list_del(&pend->wait_list_node);
unlock_and_free_fence:
- spin_unlock(&interrupt->wait_list_lock);
+ spin_unlock_irqrestore(&interrupt->wait_list_lock, flags);
kfree(pend);
hl_ctx_put(ctx);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/4] habanalabs: remove unnecessary device status check
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 ` Oded Gabbay
2021-08-20 15:07 ` [PATCH 4/4] habanalabs: never copy_from_user inside spinlock Oded Gabbay
2021-08-20 19:36 ` [PATCH 1/4] habanalabs: add "in device creation" status kernel test robot
3 siblings, 0 replies; 6+ messages in thread
From: Oded Gabbay @ 2021-08-20 15:07 UTC (permalink / raw)
To: linux-kernel
Checking if the device is operational when entering the function to
wait for user interrupt is not something that is useful or necessary.
It is not done in any other wait_for_cs ioctl path.
If the device becomes non-operational during the wait, the reset
function will make sure the process wait is interrupted.
Instead, move the check to the beginning of hl_wait_ioctl(). It will
block any attempt to wait on CS or user interrupt once the device
is already marked as non-operational.
Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
---
drivers/misc/habanalabs/common/command_submission.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/habanalabs/common/command_submission.c b/drivers/misc/habanalabs/common/command_submission.c
index 8a2f75de6df8..a97bb27ebb90 100644
--- a/drivers/misc/habanalabs/common/command_submission.c
+++ b/drivers/misc/habanalabs/common/command_submission.c
@@ -2741,10 +2741,6 @@ static int _hl_interrupt_wait_ioctl(struct hl_device *hdev, struct hl_ctx *ctx,
interrupt = &hdev->user_interrupt[interrupt_offset];
spin_lock_irqsave(&interrupt->wait_list_lock, flags);
- if (!hl_device_operational(hdev, NULL)) {
- rc = -EPERM;
- goto unlock_and_free_fence;
- }
if (copy_from_user(&completion_value, u64_to_user_ptr(user_address),
4)) {
@@ -2891,6 +2887,12 @@ int hl_wait_ioctl(struct hl_fpriv *hpriv, void *data)
u32 flags = args->in.flags;
int rc;
+ /* If the device is not operational, no point in waiting for any command submission or
+ * user interrupt
+ */
+ if (!hl_device_operational(hpriv->hdev, NULL))
+ return -EPERM;
+
if (flags & HL_WAIT_CS_FLAGS_INTERRUPT)
rc = hl_interrupt_wait_ioctl(hpriv, data);
else if (flags & HL_WAIT_CS_FLAGS_MULTI_CS)
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/4] habanalabs: never copy_from_user inside spinlock
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 ` Oded Gabbay
2021-08-21 11:04 ` Oded Gabbay
2021-08-20 19:36 ` [PATCH 1/4] habanalabs: add "in device creation" status kernel test robot
3 siblings, 1 reply; 6+ messages in thread
From: Oded Gabbay @ 2021-08-20 15:07 UTC (permalink / raw)
To: linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 4/4] habanalabs: never copy_from_user inside spinlock
2021-08-20 15:07 ` [PATCH 4/4] habanalabs: never copy_from_user inside spinlock Oded Gabbay
@ 2021-08-21 11:04 ` Oded Gabbay
0 siblings, 0 replies; 6+ messages in thread
From: Oded Gabbay @ 2021-08-21 11:04 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Linux-Kernel@Vger. 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/4] habanalabs: add "in device creation" status
2021-08-20 15:07 [PATCH 1/4] habanalabs: add "in device creation" status Oded Gabbay
` (2 preceding siblings ...)
2021-08-20 15:07 ` [PATCH 4/4] habanalabs: never copy_from_user inside spinlock Oded Gabbay
@ 2021-08-20 19:36 ` kernel test robot
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2021-08-20 19:36 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]
Hi Oded,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on linux/master linus/master v5.14-rc6 next-20210820]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Oded-Gabbay/habanalabs-add-in-device-creation-status/20210820-230930
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 1bd35e77ac2aadc1c22ca7335d4157eeae4a2fb8
config: sparc-randconfig-r014-20210816 (attached as .config)
compiler: sparc-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/c88e36085a6e21fde7e1ebcea36144a7d6df8d95
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Oded-Gabbay/habanalabs-add-in-device-creation-status/20210820-230930
git checkout c88e36085a6e21fde7e1ebcea36144a7d6df8d95
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/misc/habanalabs/common/sysfs.c: In function 'status_show':
>> drivers/misc/habanalabs/common/sysfs.c:290:9: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation]
290 | strncpy(str, hdev->status[hl_device_status(hdev)], HL_STR_MAX);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/strncpy +290 drivers/misc/habanalabs/common/sysfs.c
283
284 static ssize_t status_show(struct device *dev, struct device_attribute *attr,
285 char *buf)
286 {
287 struct hl_device *hdev = dev_get_drvdata(dev);
288 char str[HL_STR_MAX];
289
> 290 strncpy(str, hdev->status[hl_device_status(hdev)], HL_STR_MAX);
291
292 /* use uppercase for backward compatibility */
293 str[0] = 'A' + (str[0] - 'a');
294
295 return sprintf(buf, "%s\n", str);
296 }
297
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32998 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread