linux-kernel - Re: [PATCH v4 5/7] media: platform: amd: isp4 video node and buffers handling added

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aPH_iHmPFWTrrOQE@sultan-box>
Date: Fri, 17 Oct 2025 01:34:16 -0700
From: Sultan Alsawaf <sultan@...neltoast.com>
To: "Du, Bin" <bin.du@....com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
	laurent.pinchart+renesas@...asonboard.com,
	bryan.odonoghue@...aro.org, sakari.ailus@...ux.intel.com,
	prabhakar.mahadev-lad.rj@...renesas.com,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
	gjorgji.rosikopulos@....com, Phil.Jawich@....com,
	Dominic.Antony@....com, mario.limonciello@....com,
	richard.gong@....com, anson.tsao@....com,
	Alexey Zagorodnikov <xglooom@...il.com>
Subject: Re: [PATCH v4 5/7] media: platform: amd: isp4 video node and buffers
 handling added

On Thu, Oct 16, 2025 at 04:13:47PM +0800, Du, Bin wrote:
> On 10/11/2025 5:30 PM, Du, Bin wrote:
> > On 10/1/2025 2:53 PM, Sultan Alsawaf wrote:
> > > On Thu, Sep 11, 2025 at 06:08:45PM +0800, Bin Du wrote:
> > > > +++ b/drivers/media/platform/amd/isp4/isp4.c
> > > > @@ -178,6 +178,16 @@ static int isp4_capture_probe(struct
> > > > platform_device *pdev)
> > > >           goto err_isp4_deinit;
> > > >       }
> > > > +    ret = media_create_pad_link(&isp_dev->isp_sdev.sdev.entity,
> > > > +                    0, &isp_dev->isp_sdev.isp_vdev.vdev.entity,
> > > > +                    0,
> > > > +                    MEDIA_LNK_FL_ENABLED |
> > > > +                    MEDIA_LNK_FL_IMMUTABLE);
> > > > +    if (ret) {
> > > > +        dev_err(dev, "fail to create pad link %d\n", ret);
> > > > +        goto err_isp4_deinit;
> > > > +    }
> > > > +
> > > 
> > > Two problems with this hunk:
> > > 
> > > 1. According to the comment in include/media/media-device.h [1],
> > >     media_create_pad_link() should be called before
> > > media_device_register():
> > > 
> > >      * So drivers need to first initialize the media device,
> > > register any entity
> > >      * within the media device, create pad to pad links and then
> > > finally register
> > >      * the media device by calling media_device_register() as a
> > > final step.
> > > 
> > > 2. Missing call to media_device_unregister() on error when
> > >     media_create_pad_link() fails.
> > > 
> > > Since the media_create_pad_link() will be moved before
> > > media_device_register(),
> > > we will need to clean up media_create_pad_link() when
> > > media_device_register()
> > > fails.
> > > 
> > > The clean-up function for media_create_pad_link() is
> > > media_device_unregister().
> > > According to the comment for media_device_unregister() [2], it is
> > > safe to call
> > > media_device_unregister() on an unregistered media device that is
> > > initialized
> > > (through media_device_init()).
> > > 
> > > In addition, this made me realize that there's no call to
> > > media_device_cleanup()
> > > in the entire driver too. This is the cleanup function for
> > > media_device_init(),
> > > so it should be called on error and on module unload.
> > > 
> > > To summarize, make the following changes:
> > > 
> > > 1. Move the media_create_pad_link() up, right before
> > > media_device_register().
> > > 
> > > 2. When media_device_register() fails, clean up
> > > media_create_pad_link() by
> > >     calling media_device_unregister().
> > > 
> > > 3. Add a missing call to media_device_cleanup() on error and module
> > > unload to
> > >     clean up media_device_init().
> > > 
> > 
> > Very clear guide, will follow your advice.
> > 
> > > >       platform_set_drvdata(pdev, isp_dev);
> > > >       return 0;
> 
> For 2, we found when media_device_register() fails, calling
> media_device_unregister() won't clean up media_create_pad_link() because it
> simply returns without doing anything(see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/mc/mc-device.c?h=v6.17-rc7#n797).
> Therefore like other kernel drivers, we'd rather not call
> media_device_unregister() in this scenario, it doesn't cause issues, but
> it's not logically correct. Cleanup for media_create_pad_link() occurs
> during error handling via isp4sd_deinit()->isp4vid_dev_deinit()->vb2_video_unregister_device()->...->media_entity_remove_link().
> What do you think?

Oh, good catch! You are right about media_device_unregister() not cleaning up
media_create_pad_link().

But I don't see how vb2_video_unregister_device() ends up calling
media_entity_remove_links().

It looks like media_create_pad_link() is actually cleaned up via
v4l2_device_unregister_subdev()->media_device_unregister_entity()->__media_device_unregister_entity()->__media_entity_remove_links()

And I mentioned before to add a missing call to v4l2_device_unregister_subdev()
on error, so it looks like that will cover the media_create_pad_link() cleanup
and therefore you don't need to call media_device_unregister() in this scenario.

Does that look correct?

Sultan