2023-09-26 12:14:43

by Chengfeng Ye

Subject: [PATCH] media: s5p-mfc: Fix potential deadlock on condlock

As &dev->condlock is acquired under irq context along the following
call chain from s5p_mfc_irq(), other acquisition of the same lock
inside process context or softirq context should disable irq avoid double
lock. enc_post_frame_start() seems to be one such function that execute
under process context or softirq context.

<deadlock #1>

enc_post_frame_start()
--> clear_work_bit()
--> spin_loc(&dev->condlock)
<interrupt>
--> s5p_mfc_irq()
--> s5p_mfc_handle_frame()
--> clear_work_bit()
--> spin_lock(&dev->condlock)

This flaw was found by an experimental static analysis tool I am
developing for irq-related deadlock.

To prevent the potential deadlock, the patch change clear_work_bit()
inside enc_post_frame_start() to clear_work_bit_irqsave().

Signed-off-by: Chengfeng Ye <[email protected]>
---
drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
index f62703cebb77..4b4c129c09e7 100644
--- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
@@ -1297,7 +1297,7 @@ static int enc_post_frame_start(struct s5p_mfc_ctx *ctx)
if (ctx->state == MFCINST_FINISHING && ctx->ref_queue_cnt == 0)
src_ready = false;
if (!src_ready || ctx->dst_queue_cnt == 0)
- clear_work_bit(ctx);
+ clear_work_bit_irqsave(ctx);

return 0;
}
--
2.17.1

2023-09-29 23:25:46

by Marek Szyprowski

Subject: Re: [PATCH] media: s5p-mfc: Fix potential deadlock on condlock

On 26.09.2023 12:53, Chengfeng Ye wrote:
> As &dev->condlock is acquired under irq context along the following
> call chain from s5p_mfc_irq(), other acquisition of the same lock
> inside process context or softirq context should disable irq avoid double
> lock. enc_post_frame_start() seems to be one such function that execute
> under process context or softirq context.
>
> <deadlock #1>
>
> enc_post_frame_start()
> --> clear_work_bit()
> --> spin_loc(&dev->condlock)
> <interrupt>
> --> s5p_mfc_irq()
> --> s5p_mfc_handle_frame()
> --> clear_work_bit()
> --> spin_lock(&dev->condlock)
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
>
> To prevent the potential deadlock, the patch change clear_work_bit()
> inside enc_post_frame_start() to clear_work_bit_irqsave().
>
> Signed-off-by: Chengfeng Ye <[email protected]>
Acked-by: Marek Szyprowski <[email protected]>
> ---
> drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
> index f62703cebb77..4b4c129c09e7 100644
> --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
> +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_enc.c
> @@ -1297,7 +1297,7 @@ static int enc_post_frame_start(struct s5p_mfc_ctx *ctx)
> if (ctx->state == MFCINST_FINISHING && ctx->ref_queue_cnt == 0)
> src_ready = false;
> if (!src_ready || ctx->dst_queue_cnt == 0)
> - clear_work_bit(ctx);
> + clear_work_bit_irqsave(ctx);
>
> return 0;
> }

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland