| 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: <3c51d1d6503354e75ea620e30758649547db5374.camel@collabora.com>
Date: Fri, 23 May 2025 13:20:22 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: "Jackson.lee" <jackson.lee@...psnmedia.com>, mchehab@...nel.org,
hverkuil-cisco@...all.nl, sebastian.fricke@...labora.com,
bob.beckett@...labora.com, dafna.hirschfeld@...labora.com
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
lafley.kim@...psnmedia.com, b-brnich@...com, hverkuil@...all.nl,
nas.chung@...psnmedia.com
Subject: Re: [PATCH v2 1/7] media: chips-media: wave5: Fix Null reference
while testing fluster
Hi,
Le jeudi 22 mai 2025 à 16:26 +0900, Jackson.lee a écrit :
> From: Jackson Lee <jackson.lee@...psnmedia.com>
>
> When multi instances are created/destroyed, many interrupts happens
> or structures for decoder are removed.
> "struct vpu_instance" this structure is shared for all flow in decoder,
> so if the structure is not protected by lock, Null reference exception
> could happens sometimes.
> IRQ Handler was spilt to two phases and Lock was added as well.
>
> Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
> Signed-off-by: Jackson Lee <jackson.lee@...psnmedia.com>
> Signed-off-by: Nas Chung <nas.chung@...psnmedia.com>
> ---
> .../platform/chips-media/wave5/wave5-helper.c | 10 ++-
> .../chips-media/wave5/wave5-vpu-dec.c | 5 ++
> .../chips-media/wave5/wave5-vpu-enc.c | 5 ++
> .../platform/chips-media/wave5/wave5-vpu.c | 69 ++++++++++++++++---
> .../platform/chips-media/wave5/wave5-vpuapi.h | 6 ++
> 5 files changed, 86 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c b/drivers/media/platform/chips-media/wave5/wave5-
> helper.c
> index 2c9d8cbca6e4..5d9969bb7ada 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> @@ -49,7 +49,7 @@ void wave5_cleanup_instance(struct vpu_instance *inst)
> v4l2_fh_del(&inst->v4l2_fh);
> v4l2_fh_exit(&inst->v4l2_fh);
> }
> - list_del_init(&inst->list);
> + kfifo_free(&inst->irq_status);
> ida_free(&inst->dev->inst_ida, inst->id);
> kfree(inst->codec_info);
> kfree(inst);
> @@ -61,8 +61,16 @@ int wave5_vpu_release_device(struct file *filp,
> {
> struct vpu_instance *inst = wave5_to_vpu_inst(filp->private_data);
> int ret = 0;
> + unsigned long flags;
>
> v4l2_m2m_ctx_release(inst->v4l2_fh.m2m_ctx);
> + ret = mutex_lock_interruptible(&inst->dev->irq_lock);
> + if (ret)
> + return ret;
This is quite some heavy locking, why do you need both the mutex and
the spinlock ?
> + spin_lock_irqsave(&inst->dev->irq_spinlock, flags);
> + list_del_init(&inst->list);
> + spin_unlock_irqrestore(&inst->dev->irq_spinlock, flags);
> + mutex_unlock(&inst->dev->irq_lock);
> if (inst->state != VPU_INST_STATE_NONE) {
> u32 fail_res;
>
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-
> media/wave5/wave5-vpu-dec.c
> index fd71f0c43ac3..32de43de1870 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> @@ -1811,6 +1811,11 @@ static int wave5_vpu_open_dec(struct file *filp)
> inst->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>
> init_completion(&inst->irq_done);
> + ret = kfifo_alloc(&inst->irq_status, 16 * sizeof(int), GFP_KERNEL);
> + if (ret) {
> + dev_err(inst->dev->dev, "failed to allocate fifo\n");
> + goto cleanup_inst;
> + }
>
> inst->id = ida_alloc(&inst->dev->inst_ida, GFP_KERNEL);
> if (inst->id < 0) {
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-
> media/wave5/wave5-vpu-enc.c
> index 1e5fc5f8b856..52a1a00fd9bb 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> @@ -1760,6 +1760,11 @@ static int wave5_vpu_open_enc(struct file *filp)
> inst->frame_rate = 30;
>
> init_completion(&inst->irq_done);
> + ret = kfifo_alloc(&inst->irq_status, 16 * sizeof(int), GFP_KERNEL);
> + if (ret) {
> + dev_err(inst->dev->dev, "failed to allocate fifo\n");
> + goto cleanup_inst;
> + }
>
> inst->id = ida_alloc(&inst->dev->inst_ida, GFP_KERNEL);
> if (inst->id < 0) {
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-
> vpu.c
> index e1715d3f43b0..a2c09523d76b 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> @@ -51,8 +51,11 @@ static void wave5_vpu_handle_irq(void *dev_id)
> u32 seq_done;
> u32 cmd_done;
> u32 irq_reason;
> - struct vpu_instance *inst;
> + u32 irq_subreason;
> + struct vpu_instance *inst, *tmp;
> struct vpu_device *dev = dev_id;
> + int val;
> + unsigned long flags;
>
> irq_reason = wave5_vdi_read_register(dev, W5_VPU_VINT_REASON);
> seq_done = wave5_vdi_read_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO);
> @@ -60,7 +63,8 @@ static void wave5_vpu_handle_irq(void *dev_id)
> wave5_vdi_write_register(dev, W5_VPU_VINT_REASON_CLR, irq_reason);
> wave5_vdi_write_register(dev, W5_VPU_VINT_CLEAR, 0x1);
>
> - list_for_each_entry(inst, &dev->instances, list) {
> + spin_lock_irqsave(&dev->irq_spinlock, flags);
> + list_for_each_entry_safe(inst, tmp, &dev->instances, list) {
>
> if (irq_reason & BIT(INT_WAVE5_INIT_SEQ) ||
> irq_reason & BIT(INT_WAVE5_ENC_SET_PARAM)) {
> @@ -82,14 +86,22 @@ static void wave5_vpu_handle_irq(void *dev_id)
> irq_reason & BIT(INT_WAVE5_ENC_PIC)) {
> if (cmd_done & BIT(inst->id)) {
> cmd_done &= ~BIT(inst->id);
> - wave5_vdi_write_register(dev, W5_RET_QUEUE_CMD_DONE_INST,
> - cmd_done);
> - inst->ops->finish_process(inst);
> + if (dev->irq >= 0) {
> + irq_subreason =
> + wave5_vdi_read_register(dev, W5_VPU_VINT_REASON);
> + if (!(irq_subreason & BIT(INT_WAVE5_DEC_PIC)))
> + wave5_vdi_write_register(dev,
> + W5_RET_QUEUE_CMD_DONE_INST,
> + cmd_done);
> + }
> + val = BIT(INT_WAVE5_DEC_PIC);
> + kfifo_in(&inst->irq_status, &val, sizeof(int));
> }
> }
> -
> - wave5_vpu_clear_interrupt(inst, irq_reason);
> }
> + spin_unlock_irqrestore(&dev->irq_spinlock, flags);
> +
> + up(&dev->irq_sem);
> }
>
> static irqreturn_t wave5_vpu_irq_thread(int irq, void *dev_id)
> @@ -121,6 +133,35 @@ static enum hrtimer_restart wave5_vpu_timer_callback(struct hrtimer *timer)
> return HRTIMER_RESTART;
> }
>
> +static int irq_thread(void *data)
> +{
> + struct vpu_device *dev = (struct vpu_device *)data;
> + struct vpu_instance *inst, *tmp;
> + int irq_status, ret;
> +
> + while (!kthread_should_stop()) {
> + if (down_interruptible(&dev->irq_sem))
> + continue;
> +
> + if (kthread_should_stop())
> + break;
> +
> + mutex_lock(&dev->irq_lock);
> + list_for_each_entry_safe(inst, tmp, &dev->instances, list) {
> + while (kfifo_len(&inst->irq_status)) {
> + ret = kfifo_out(&inst->irq_status, &irq_status, sizeof(int));
> + if (!ret)
> + break;
> +
> + inst->ops->finish_process(inst);
> + }
> + }
> + mutex_unlock(&dev->irq_lock);
> + }
> +
> + return 0;
> +}
> +
> static int wave5_vpu_load_firmware(struct device *dev, const char *fw_name,
> u32 *revision)
> {
> @@ -224,6 +265,8 @@ static int wave5_vpu_probe(struct platform_device *pdev)
>
> mutex_init(&dev->dev_lock);
> mutex_init(&dev->hw_lock);
> + mutex_init(&dev->irq_lock);
> + spin_lock_init(&dev->irq_spinlock);
> dev_set_drvdata(&pdev->dev, dev);
> dev->dev = &pdev->dev;
>
> @@ -266,6 +309,10 @@ static int wave5_vpu_probe(struct platform_device *pdev)
> }
> dev->product = wave5_vpu_get_product_id(dev);
>
> + sema_init(&dev->irq_sem, 1);
> + INIT_LIST_HEAD(&dev->instances);
> + dev->irq_thread = kthread_run(irq_thread, dev, "irq thread");
> +
> dev->irq = platform_get_irq(pdev, 0);
> if (dev->irq < 0) {
> dev_err(&pdev->dev, "failed to get irq resource, falling back to polling\n");
> @@ -288,7 +335,6 @@ static int wave5_vpu_probe(struct platform_device *pdev)
> }
> }
>
> - INIT_LIST_HEAD(&dev->instances);
> ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> if (ret) {
> dev_err(&pdev->dev, "v4l2_device_register, fail: %d\n", ret);
> @@ -351,6 +397,12 @@ static void wave5_vpu_remove(struct platform_device *pdev)
> {
> struct vpu_device *dev = dev_get_drvdata(&pdev->dev);
>
> + if (dev->irq_thread) {
> + kthread_stop(dev->irq_thread);
> + up(&dev->irq_sem);
> + dev->irq_thread = NULL;
> + }
> +
> if (dev->irq < 0) {
> kthread_destroy_worker(dev->worker);
> hrtimer_cancel(&dev->hrtimer);
> @@ -361,6 +413,7 @@ static void wave5_vpu_remove(struct platform_device *pdev)
>
> mutex_destroy(&dev->dev_lock);
> mutex_destroy(&dev->hw_lock);
> + mutex_destroy(&dev->irq_lock);
> reset_control_assert(dev->resets);
> clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> wave5_vpu_enc_unregister_device(dev);
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-
> vpuapi.h
> index 45615c15beca..f3c1ad6fb3be 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> @@ -8,6 +8,7 @@
> #ifndef VPUAPI_H_INCLUDED
> #define VPUAPI_H_INCLUDED
>
> +#include <linux/kfifo.h>
> #include <linux/idr.h>
> #include <linux/genalloc.h>
> #include <media/v4l2-device.h>
> @@ -747,6 +748,7 @@ struct vpu_device {
> struct video_device *video_dev_enc;
> struct mutex dev_lock; /* lock for the src, dst v4l2 queues */
> struct mutex hw_lock; /* lock hw configurations */
> + struct mutex irq_lock;
> int irq;
> enum product_id product;
> struct vpu_attr attr;
> @@ -764,7 +766,10 @@ struct vpu_device {
> struct kthread_worker *worker;
> int vpu_poll_interval;
> int num_clks;
> + struct task_struct *irq_thread;
> + struct semaphore irq_sem;
Can you comment on what they actually protect ?
> struct reset_control *resets;
> + spinlock_t irq_spinlock; /* protect instances list */
> };
>
> struct vpu_instance;
> @@ -788,6 +793,7 @@ struct vpu_instance {
> enum v4l2_ycbcr_encoding ycbcr_enc;
> enum v4l2_quantization quantization;
>
> + struct kfifo irq_status;
> enum vpu_instance_state state;
> enum vpu_instance_type type;
> const struct vpu_instance_ops *ops;
Powered by blists - more mailing lists