| 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: <CAAFQd5DTOJ2=ibyykpCxDnUhUM8EXm4-MShKxBbH02FA8fzQGw@mail.gmail.com>
Date: Wed, 20 Aug 2025 17:13:52 +0900
From: Tomasz Figa <tfiga@...omium.org>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Yunfei Dong <yunfei.dong@...iatek.com>, Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans Verkuil <hverkuil@...all.nl>, Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v2] media: mediatek: vcodec: Use spinlock for context list
protection lock
On Wed, Aug 20, 2025 at 4:54 PM Chen-Yu Tsai <wenst@...omium.org> wrote:
>
> Previously a mutex was added to protect the encoder and decoder context
> lists from unexpected changes originating from the SCP IP block, causing
> the context pointer to go invalid, resulting in a NULL pointer
> dereference in the IPI handler.
>
> Turns out on the MT8173, the VPU IPI handler is called from hard IRQ
> context. This causes a big warning from the scheduler. This was first
> reported downstream on the ChromeOS kernels, but is also reproducible
> on mainline using Fluster with the FFmpeg v4l2m2m decoders. Even though
> the actual capture format is not supported, the affected code paths
> are triggered.
>
> Since this lock just protects the context list and operations on it are
> very fast, it should be OK to switch to a spinlock.
>
> Fixes: 6467cda18c9f ("media: mediatek: vcodec: adding lock to protect decoder context list")
> Fixes: afaaf3a0f647 ("media: mediatek: vcodec: adding lock to protect encoder context list")
> Cc: Yunfei Dong <yunfei.dong@...iatek.com>
> Signed-off-by: Chen-Yu Tsai <wenst@...omium.org>
> ---
> Changes since v1:
> - Switched to _irqsave / _irqrestore variants even in helper only called
> by IRQ handler (Tomasz)
>
> .../mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 10 ++++++----
> .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c | 12 +++++++-----
> .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h | 2 +-
> .../platform/mediatek/vcodec/decoder/vdec_vpu_if.c | 5 +++--
> .../mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c | 12 +++++++-----
> .../mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h | 2 +-
> .../platform/mediatek/vcodec/encoder/venc_vpu_if.c | 5 +++--
> 7 files changed, 28 insertions(+), 20 deletions(-)
Thanks!
Reviewed-by: Tomasz Figa <tfiga@...omium.org>
Best,
Tomasz
>
> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
> index d7027d600208..223fb2294894 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c
> @@ -47,30 +47,32 @@ static void mtk_vcodec_vpu_reset_dec_handler(void *priv)
> {
> struct mtk_vcodec_dec_dev *dev = priv;
> struct mtk_vcodec_dec_ctx *ctx;
> + unsigned long flags;
>
> dev_err(&dev->plat_dev->dev, "Watchdog timeout!!");
>
> - mutex_lock(&dev->dev_ctx_lock);
> + spin_lock_irqsave(&dev->dev_ctx_lock, flags);
> list_for_each_entry(ctx, &dev->ctx_list, list) {
> ctx->state = MTK_STATE_ABORT;
> mtk_v4l2_vdec_dbg(0, ctx, "[%d] Change to state MTK_STATE_ABORT", ctx->id);
> }
> - mutex_unlock(&dev->dev_ctx_lock);
> + spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
> }
>
> static void mtk_vcodec_vpu_reset_enc_handler(void *priv)
> {
> struct mtk_vcodec_enc_dev *dev = priv;
> struct mtk_vcodec_enc_ctx *ctx;
> + unsigned long flags;
>
> dev_err(&dev->plat_dev->dev, "Watchdog timeout!!");
>
> - mutex_lock(&dev->dev_ctx_lock);
> + spin_lock_irqsave(&dev->dev_ctx_lock, flags);
> list_for_each_entry(ctx, &dev->ctx_list, list) {
> ctx->state = MTK_STATE_ABORT;
> mtk_v4l2_vdec_dbg(0, ctx, "[%d] Change to state MTK_STATE_ABORT", ctx->id);
> }
> - mutex_unlock(&dev->dev_ctx_lock);
> + spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
> }
>
> static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> index 46d176e6de63..3b81fae9f913 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> @@ -198,6 +198,7 @@ static int fops_vcodec_open(struct file *file)
> struct mtk_vcodec_dec_ctx *ctx = NULL;
> int ret = 0, i, hw_count;
> struct vb2_queue *src_vq;
> + unsigned long flags;
>
> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> @@ -267,9 +268,9 @@ static int fops_vcodec_open(struct file *file)
>
> ctx->dev->vdec_pdata->init_vdec_params(ctx);
>
> - mutex_lock(&dev->dev_ctx_lock);
> + spin_lock_irqsave(&dev->dev_ctx_lock, flags);
> list_add(&ctx->list, &dev->ctx_list);
> - mutex_unlock(&dev->dev_ctx_lock);
> + spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
> mtk_vcodec_dbgfs_create(ctx);
>
> mutex_unlock(&dev->dev_mutex);
> @@ -294,6 +295,7 @@ static int fops_vcodec_release(struct file *file)
> {
> struct mtk_vcodec_dec_dev *dev = video_drvdata(file);
> struct mtk_vcodec_dec_ctx *ctx = file_to_dec_ctx(file);
> + unsigned long flags;
>
> mtk_v4l2_vdec_dbg(0, ctx, "[%d] decoder", ctx->id);
> mutex_lock(&dev->dev_mutex);
> @@ -312,9 +314,9 @@ static int fops_vcodec_release(struct file *file)
> v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
>
> mtk_vcodec_dbgfs_remove(dev, ctx->id);
> - mutex_lock(&dev->dev_ctx_lock);
> + spin_lock_irqsave(&dev->dev_ctx_lock, flags);
> list_del_init(&ctx->list);
> - mutex_unlock(&dev->dev_ctx_lock);
> + spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
> kfree(ctx);
> mutex_unlock(&dev->dev_mutex);
> return 0;
> @@ -407,7 +409,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> for (i = 0; i < MTK_VDEC_HW_MAX; i++)
> mutex_init(&dev->dec_mutex[i]);
> mutex_init(&dev->dev_mutex);
> - mutex_init(&dev->dev_ctx_lock);
> + spin_lock_init(&dev->dev_ctx_lock);
> spin_lock_init(&dev->irqlock);
>
> snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), "%s",
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> index d047d7c580fb..9d68808e8f9c 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> @@ -285,7 +285,7 @@ struct mtk_vcodec_dec_dev {
> /* decoder hardware mutex lock */
> struct mutex dec_mutex[MTK_VDEC_HW_MAX];
> struct mutex dev_mutex;
> - struct mutex dev_ctx_lock;
> + spinlock_t dev_ctx_lock;
> struct workqueue_struct *decode_workqueue;
>
> spinlock_t irqlock;
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> index 145958206e38..40b97f114cf6 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> @@ -75,16 +75,17 @@ static void handle_get_param_msg_ack(const struct vdec_vpu_ipi_get_param_ack *ms
> static bool vpu_dec_check_ap_inst(struct mtk_vcodec_dec_dev *dec_dev, struct vdec_vpu_inst *vpu)
> {
> struct mtk_vcodec_dec_ctx *ctx;
> + unsigned long flags;
> int ret = false;
>
> - mutex_lock(&dec_dev->dev_ctx_lock);
> + spin_lock_irqsave(&dec_dev->dev_ctx_lock, flags);
> list_for_each_entry(ctx, &dec_dev->ctx_list, list) {
> if (!IS_ERR_OR_NULL(ctx) && ctx->vpu_inst == vpu) {
> ret = true;
> break;
> }
> }
> - mutex_unlock(&dec_dev->dev_ctx_lock);
> + spin_unlock_irqrestore(&dec_dev->dev_ctx_lock, flags);
>
> return ret;
> }
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c
> index fb1c3bdc2dae..82b8ff38e8f1 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.c
> @@ -117,6 +117,7 @@ static int fops_vcodec_open(struct file *file)
> struct mtk_vcodec_enc_ctx *ctx = NULL;
> int ret = 0;
> struct vb2_queue *src_vq;
> + unsigned long flags;
>
> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> @@ -176,9 +177,9 @@ static int fops_vcodec_open(struct file *file)
> mtk_v4l2_venc_dbg(2, ctx, "Create instance [%d]@%p m2m_ctx=%p ",
> ctx->id, ctx, ctx->m2m_ctx);
>
> - mutex_lock(&dev->dev_ctx_lock);
> + spin_lock_irqsave(&dev->dev_ctx_lock, flags);
> list_add(&ctx->list, &dev->ctx_list);
> - mutex_unlock(&dev->dev_ctx_lock);
> + spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
>
> mutex_unlock(&dev->dev_mutex);
> mtk_v4l2_venc_dbg(0, ctx, "%s encoder [%d]", dev_name(&dev->plat_dev->dev),
> @@ -203,6 +204,7 @@ static int fops_vcodec_release(struct file *file)
> {
> struct mtk_vcodec_enc_dev *dev = video_drvdata(file);
> struct mtk_vcodec_enc_ctx *ctx = file_to_enc_ctx(file);
> + unsigned long flags;
>
> mtk_v4l2_venc_dbg(1, ctx, "[%d] encoder", ctx->id);
> mutex_lock(&dev->dev_mutex);
> @@ -213,9 +215,9 @@ static int fops_vcodec_release(struct file *file)
> v4l2_fh_exit(&ctx->fh);
> v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
>
> - mutex_lock(&dev->dev_ctx_lock);
> + spin_lock_irqsave(&dev->dev_ctx_lock, flags);
> list_del_init(&ctx->list);
> - mutex_unlock(&dev->dev_ctx_lock);
> + spin_unlock_irqrestore(&dev->dev_ctx_lock, flags);
> kfree(ctx);
> mutex_unlock(&dev->dev_mutex);
> return 0;
> @@ -297,7 +299,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
>
> mutex_init(&dev->enc_mutex);
> mutex_init(&dev->dev_mutex);
> - mutex_init(&dev->dev_ctx_lock);
> + spin_lock_init(&dev->dev_ctx_lock);
> spin_lock_init(&dev->irqlock);
>
> snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), "%s",
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h
> index 5b304a551236..0cddfa13594f 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/mtk_vcodec_enc_drv.h
> @@ -206,7 +206,7 @@ struct mtk_vcodec_enc_dev {
> /* encoder hardware mutex lock */
> struct mutex enc_mutex;
> struct mutex dev_mutex;
> - struct mutex dev_ctx_lock;
> + spinlock_t dev_ctx_lock;
> struct workqueue_struct *encode_workqueue;
>
> int enc_irq;
> diff --git a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> index 51bb7ee141b9..3c229b1f6b21 100644
> --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> @@ -45,16 +45,17 @@ static void handle_enc_encode_msg(struct venc_vpu_inst *vpu, const void *data)
> static bool vpu_enc_check_ap_inst(struct mtk_vcodec_enc_dev *enc_dev, struct venc_vpu_inst *vpu)
> {
> struct mtk_vcodec_enc_ctx *ctx;
> + unsigned long flags;
> int ret = false;
>
> - mutex_lock(&enc_dev->dev_ctx_lock);
> + spin_lock_irqsave(&enc_dev->dev_ctx_lock, flags);
> list_for_each_entry(ctx, &enc_dev->ctx_list, list) {
> if (!IS_ERR_OR_NULL(ctx) && ctx->vpu_inst == vpu) {
> ret = true;
> break;
> }
> }
> - mutex_unlock(&enc_dev->dev_ctx_lock);
> + spin_unlock_irqrestore(&enc_dev->dev_ctx_lock, flags);
>
> return ret;
> }
> --
> 2.51.0.rc1.167.g924127e9c0-goog
>
Powered by blists - more mailing lists