| 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: <3ff43351-9236-43a6-aea8-ab492cc86699@amd.com>
Date: Fri, 17 Oct 2025 17:53:27 +0800
From: "Du, Bin" <bin.du@....com>
To: Sultan Alsawaf <sultan@...neltoast.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 10/17/2025 4:34 PM, Sultan Alsawaf wrote:
> 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?
>
Yes, Sultan, we moved v4l2_device_unregister_subdev to isp4sd_deinit as
you suggested, so current isp4sd_deinit() looks like this
void isp4sd_deinit(struct isp4_subdev *isp_subdev)
{
struct isp4_interface *ispif = &isp_subdev->ispif;
isp4vid_dev_deinit(&isp_subdev->isp_vdev);
v4l2_device_unregister_subdev(&isp_subdev->sdev);
media_entity_cleanup(&isp_subdev->sdev.entity);
isp4if_deinit(ispif);
isp4sd_module_enable(isp_subdev, false);
ispif->status = ISP4IF_STATUS_PWR_OFF;
}
You are correct and I believe both isp4vid_dev_deinit and
v4l2_device_unregister_subdev can cause media_create_pad_link() being
cleaned up. Because isp4vid_dev_deinit is called first, so the link will
be cleaned by it, here is the call stack FYI, does it make sense?
[ 5.198328] Call Trace:
[ 5.198329] <TASK>
[ 5.198331] dump_stack_lvl+0x76/0xa0
[ 5.198336] dump_stack+0x10/0x20
[ 5.198338] __media_entity_remove_link+0xdf/0x1f0 [mc]
[ 5.198342] __media_entity_remove_links+0x31/0x70 [mc]
[ 5.198344] __media_device_unregister_entity+0x93/0xf0 [mc]
[ 5.198346] media_device_unregister_entity+0x2f/0x50 [mc]
[ 5.198348] v4l2_device_release+0x112/0x190 [videodev]
[ 5.198355] device_release+0x38/0xa0
[ 5.198358] kobject_put+0x9e/0x200
[ 5.198359] put_device+0x13/0x30
[ 5.198359] vb2_video_unregister_device+0x8e/0xd0 [videobuf2_v4l2]
[ 5.198362] isp4vid_dev_deinit+0xe/0x20 [amd_capture]
[ 5.198364] isp4sd_deinit+0x25/0x80 [amd_capture]
[ 5.198366] isp4_capture_probe+0x1ec/0x2f0 [amd_capture]
[ 5.198368] platform_probe+0x3f/0xb0
[ 5.198370] really_probe+0xf4/0x3b0
> Sultan
--
Regards,
Bin
Powered by blists - more mailing lists