Re: [PATCH] ALSA: pcmtest: fix reference leak on failed device registration

Re: [PATCH] ALSA: pcmtest: fix reference leak on failed device registration

[Takashi Iwai]

On Fri, 17 Apr 2026 09:57:20 +0200,
Takashi Iwai wrote:
> 
> On Wed, 15 Apr 2026 21:31:38 +0200,
> Guangshuo Li wrote:
> > 
> > When platform_device_register() fails in mod_init(), the embedded struct
> > device in pcmtst_pdev has already been initialized by
> > device_initialize(), but the failure path returns the error without
> > dropping the device reference for the current platform device:
> > 
> >   mod_init()
> >     -> platform_device_register(&pcmtst_pdev)
> >        -> device_initialize(&pcmtst_pdev.dev)
> >        -> setup_pdev_dma_masks(&pcmtst_pdev)
> >        -> platform_device_add(&pcmtst_pdev)
> > 
> > This leads to a reference leak when platform_device_register() fails.
> > Fix this by calling platform_device_put() before returning the error.
> > 
> > The issue was identified by a static analysis tool I developed and
> > confirmed by manual review.
> > 
> > Fixes: 315a3d57c64c5 ("ALSA: Implement the new Virtual PCM Test Driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> 
> Thanks, applied now.

... and now I looked through the whole tree, and noticed that the
majority of callers of platform_device_register() don't care the error
cases without calling platform_device_put().  There are over a hundred
callers of platform_device_register() while only 5 or so are doing the
proper cleanup at the error.

Judging from the numbers above, it might be better to change the
behavior of platform_device_register() itself to call *_put() at the
error case internally.  Or, if we keep the current behavior, at least
we should properly document it.

Greg, Rafael, Danilo, what do you think?


thanks,

Takashi

Re: [PATCH] ALSA: pcmtest: fix reference leak on failed device registration

[Guangshuo Li]

Hi Takashi,

Thanks for reviewing.

On Fri, 17 Apr 2026 at 18:52, Takashi Iwai <tiwai@suse.de> wrote:
>
>
> ... and now I looked through the whole tree, and noticed that the
> majority of callers of platform_device_register() don't care the error
> cases without calling platform_device_put().  There are over a hundred
> callers of platform_device_register() while only 5 or so are doing the
> proper cleanup at the error.
>
> Judging from the numbers above, it might be better to change the
> behavior of platform_device_register() itself to call *_put() at the
> error case internally.  Or, if we keep the current behavior, at least
> we should properly document it.
>
> Greg, Rafael, Danilo, what do you think?
>
>
> thanks,
>
> Takashi

I agree with your observation. Given that many callers of
platform_device_register() do not handle the error path with a
matching platform_device_put(), this does seem more like an issue
related to the core/API behavior, or at least something that should be
documented more clearly.

Also, the underlying issue here appears to be related to the
platform_device_register() core/API behavior. We are currently
discussing in another similar case whether the better fix, if any,
should be made in the core/API code rather than in individual callers:

https://patchew.org/linux/20260415174159.3625777-1-lgs201920130244@gmail.com/

Once that discussion reaches a conclusion, we will revisit this and
make the appropriate fix if needed.

Thanks,
Guangshuo

Re: [PATCH] ALSA: pcmtest: fix reference leak on failed device registration

[Jiri Slaby]

On 17. 04. 26, 12:52, Takashi Iwai wrote:
> On Fri, 17 Apr 2026 09:57:20 +0200,
> Takashi Iwai wrote:
>>
>> On Wed, 15 Apr 2026 21:31:38 +0200,
>> Guangshuo Li wrote:
>>>
>>> When platform_device_register() fails in mod_init(), the embedded struct
>>> device in pcmtst_pdev has already been initialized by
>>> device_initialize(), but the failure path returns the error without
>>> dropping the device reference for the current platform device:
>>>
>>>    mod_init()
>>>      -> platform_device_register(&pcmtst_pdev)
>>>         -> device_initialize(&pcmtst_pdev.dev)
>>>         -> setup_pdev_dma_masks(&pcmtst_pdev)
>>>         -> platform_device_add(&pcmtst_pdev)
>>>
>>> This leads to a reference leak when platform_device_register() fails.
>>> Fix this by calling platform_device_put() before returning the error.
>>>
>>> The issue was identified by a static analysis tool I developed and
>>> confirmed by manual review.
>>>
>>> Fixes: 315a3d57c64c5 ("ALSA: Implement the new Virtual PCM Test Driver")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
>>
>> Thanks, applied now.
> 
> ... and now I looked through the whole tree, and noticed that the
> majority of callers of platform_device_register() don't care the error
> cases without calling platform_device_put().  There are over a hundred
> callers of platform_device_register() while only 5 or so are doing the
> proper cleanup at the error.

Moreover, unless the static pcmtst_pdev has ->type->release or ->release 
set, the patch triggers a warning upon put().

> Judging from the numbers above, it might be better to change the
> behavior of platform_device_register() itself to call *_put() at the
> error case internally.

Yes, that's what should be done instead.

thanks,
-- 
js
suse labs

Re: [PATCH] ALSA: pcmtest: fix reference leak on failed device registration

[Takashi Iwai]

On Thu, 23 Apr 2026 07:19:20 +0200,
Jiri Slaby wrote:
> 
> On 17. 04. 26, 12:52, Takashi Iwai wrote:
> > On Fri, 17 Apr 2026 09:57:20 +0200,
> > Takashi Iwai wrote:
> >> 
> >> On Wed, 15 Apr 2026 21:31:38 +0200,
> >> Guangshuo Li wrote:
> >>> 
> >>> When platform_device_register() fails in mod_init(), the embedded struct
> >>> device in pcmtst_pdev has already been initialized by
> >>> device_initialize(), but the failure path returns the error without
> >>> dropping the device reference for the current platform device:
> >>> 
> >>>    mod_init()
> >>>      -> platform_device_register(&pcmtst_pdev)
> >>>         -> device_initialize(&pcmtst_pdev.dev)
> >>>         -> setup_pdev_dma_masks(&pcmtst_pdev)
> >>>         -> platform_device_add(&pcmtst_pdev)
> >>> 
> >>> This leads to a reference leak when platform_device_register() fails.
> >>> Fix this by calling platform_device_put() before returning the error.
> >>> 
> >>> The issue was identified by a static analysis tool I developed and
> >>> confirmed by manual review.
> >>> 
> >>> Fixes: 315a3d57c64c5 ("ALSA: Implement the new Virtual PCM Test Driver")
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> >> 
> >> Thanks, applied now.
> > 
> > ... and now I looked through the whole tree, and noticed that the
> > majority of callers of platform_device_register() don't care the error
> > cases without calling platform_device_put().  There are over a hundred
> > callers of platform_device_register() while only 5 or so are doing the
> > proper cleanup at the error.
> 
> Moreover, unless the static pcmtst_pdev has ->type->release or
> ->release set, the patch triggers a warning upon put().

Ah a good point, the call should be conditional.

> > Judging from the numbers above, it might be better to change the
> > behavior of platform_device_register() itself to call *_put() at the
> > error case internally.
> 
> Yes, that's what should be done instead.

OK, I'm going to revert this patch for pcmtest.c for now (so back to
the upstream state again), so that we can work on the
platform_device_register() side.


thanks,

Takashi