| 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: <CAC=S1nheT46K+jkmtq4EJxOVO=nwasan0LCJwv-HTEK6P6DgxA@mail.gmail.com>
Date: Thu, 14 Aug 2025 17:47:08 +0800
From: Fei Shao <fshao@...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] media: mediatek: vcodec: Use spinlock for context list
protection lock
On Thu, Aug 14, 2025 at 5:06 PM Chen-Yu Tsai <wenst@...omium.org> wrote:
>
> On Thu, Aug 14, 2025 at 4:59 PM Fei Shao <fshao@...omium.org> wrote:
> >
> > On Thu, Aug 14, 2025 at 4:38 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>
> > > ---
> > > .../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 | 4 ++--
> > > .../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 | 4 ++--
> > > 7 files changed, 26 insertions(+), 20 deletions(-)
> > >
> >
> > [...]
> >
> > > 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..e9b5cac9c63b 100644
> > > --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > > +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > > @@ -77,14 +77,14 @@ static bool vpu_dec_check_ap_inst(struct mtk_vcodec_dec_dev *dec_dev, struct vde
> > > struct mtk_vcodec_dec_ctx *ctx;
> > > int ret = false;
> > >
> > > - mutex_lock(&dec_dev->dev_ctx_lock);
> > > + spin_lock(&dec_dev->dev_ctx_lock);
> >
> > Do you mean spin_lock_irqsave()?
>
> This function is only called from the handler below (outside the diff
> context), which itself is called from hard IRQ context. This is mentioned
> in the comment above the handler.
I see. I only searched here but didn't check the full source.
Leaving a comment would be clearer if a revision is made, but it's not
worth resending just for that alone.
Reviewed-by: Fei Shao <fshao@...omium.org>
On Thu, Aug 14, 2025 at 5:06 PM Chen-Yu Tsai <wenst@...omium.org> wrote:
>
> On Thu, Aug 14, 2025 at 4:59 PM Fei Shao <fshao@...omium.org> wrote:
> >
> > On Thu, Aug 14, 2025 at 4:38 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>
> > > ---
> > > .../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 | 4 ++--
> > > .../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 | 4 ++--
> > > 7 files changed, 26 insertions(+), 20 deletions(-)
> > >
> >
> > [...]
> >
> > > 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..e9b5cac9c63b 100644
> > > --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > > +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c
> > > @@ -77,14 +77,14 @@ static bool vpu_dec_check_ap_inst(struct mtk_vcodec_dec_dev *dec_dev, struct vde
> > > struct mtk_vcodec_dec_ctx *ctx;
> > > int ret = false;
> > >
> > > - mutex_lock(&dec_dev->dev_ctx_lock);
> > > + spin_lock(&dec_dev->dev_ctx_lock);
> >
> > Do you mean spin_lock_irqsave()?
>
> This function is only called from the handler below (outside the diff
> context), which itself is called from hard IRQ context. This is mentioned
> in the comment above the handler.
>
> > > 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(&dec_dev->dev_ctx_lock);
> > >
> > > return ret;
> > > }
> >
> > [...]
> >
> > > 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..79a91283da78 100644
> > > --- a/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > > +++ b/drivers/media/platform/mediatek/vcodec/encoder/venc_vpu_if.c
> > > @@ -47,14 +47,14 @@ static bool vpu_enc_check_ap_inst(struct mtk_vcodec_enc_dev *enc_dev, struct ven
> > > struct mtk_vcodec_enc_ctx *ctx;
> > > int ret = false;
> > >
> > > - mutex_lock(&enc_dev->dev_ctx_lock);
> > > + spin_lock(&enc_dev->dev_ctx_lock);
> >
> > Also here.
>
> Same reasoning applies here as well.
>
> ChenYu
>
> > Regards,
> > Fei
> >
> > > 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(&enc_dev->dev_ctx_lock);
> > >
> > > return ret;
> > > }
> > > --
> > > 2.51.0.rc1.163.g2494970778-goog
> > >
> > >
Powered by blists - more mailing lists