Re: [PATCH] ALSA: info: Serialize freeing with recursive registration

[Takashi Iwai <tiwai@suse.de>]

On Sat, 17 Oct 2026 11:27:54 +0200,
Cen Zhang wrote:
> 
> Hi Takashi,
> 
> >
> > Does the above crash really happen with unmodified code?
> > The PCM oss proc unregister is per card, hence there can't be race
> > between register and unregister for the same card.
> >
> > The hardening itself in the proc code is good, per se (although it's
> > doubtful whether we really need a global mutex from the performance
> > POV), though.
> >
> >
> > thanks,
> >
> > Takashi
> 
> 
> Thank you for the review. The quoted KASAN report used scheduling
> instrumentation
> and a small test module that called snd_card_register() on an already registered
> dummy card.
> 
> But reviewing the source, I found a concrete scenario involving
> existing in-tree callers that appears to allow this race.
> 
> Consider a registered card with PCM A and its published OSS proc
> entry. A later bind of another codec B on the same card can reach
> hda_codec_driver_probe(), which calls snd_card_register() when
> !bus_probing && card->registered.
> 
> At the same time, snd-pcm-oss can be unloaded while the card remains
> registered, provided no active OSS references prevent the unload.
> alsa_pcm_oss_exit() calls snd_pcm_notify(..., 1), which invokes
> n_unregister() for the registered PCM devices and frees their OSS
> proc entries.
> 
> The interleaving I believe the source permits is:
> 
>   HDA codec B probe                  snd-pcm-oss module exit
>   -----------------                  -----------------------
>   snd_card_register(card)
>     snd_device_register_all(card)
>     /* PCM register_mutex released */
> 
>                                      lock(register_mutex)
>                                      n_unregister(PCM A)
>                                        snd_info_free_entry(entry)
>                                          proc_remove(entry->p)
>                                          lock(info_mutex)
>                                          entry->p = NULL
>                                          unlock(info_mutex)
>                                          /* entry still linked */
> 
>     lock(snd_card_mutex)
>     snd_info_card_register(card)
>       snd_info_register(card->proc_root)
>         find PCM A's linked entry
>         lock(info_mutex)
>         recreate proc node with
>           entry as private data
>         unlock(info_mutex)
>     unlock(snd_card_mutex)
> 
>                                      unlink entry
>                                      kfree(entry)
>                                      unlock(register_mutex)
> 
> The card-info traversal does not hold the PCM register_mutex, and
> OSS notifier removal does not take snd_card_mutex. The parent proc
> directories also remain present because the card and PCM A have
> not been removed.
> 
> This appears to allow the new proc node to retain a pointer to the
> freed entry. A subsequent open could then reach alloc_info_private()
> with that stale pointer. Please let me know if I have overlooked
> another serialization point in this path.

OK, the arbitrary OSS removal is the problem child.

IMO, instead of hardening piece-by-piece in ad hoc ways, we can pin
the OSS PCM and mixer module when any card is added and unpin at the
card removal, so that we can't unload OSS modules while there are any
actually used cards.

The cases with the partial HDA codec probe/remove is a different
problem, though.  This has to be addressed somehow better.


thanks,

Takashi