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

[Takashi Iwai <tiwai@suse.de>]

On Sat, 17 Oct 2026 12:31:54 +0200,
Takashi Iwai wrote:
> 
> 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.

I meant something like below (totally untested)


Takashi

-- 8< --
diff --git a/sound/core/oss/mixer_oss.c b/sound/core/oss/mixer_oss.c
index f1fa42e634fc..2e686bd6c5f9 100644
--- a/sound/core/oss/mixer_oss.c
+++ b/sound/core/oss/mixer_oss.c
@@ -1309,13 +1309,18 @@ static int snd_mixer_oss_free1(void *private)
 static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
 {
 	struct snd_mixer_oss *mixer;
+	int err;
 
 	if (cmd == SND_MIXER_OSS_NOTIFY_REGISTER) {
-		int idx, err;
+		int idx;
 
 		mixer = kzalloc_objs(*mixer, 2);
 		if (mixer == NULL)
 			return -ENOMEM;
+		if (!try_module_get(THIS_MODULE)) {
+			kfree(mixer);
+			return -ENODEV;
+		}
 		mutex_init(&mixer->reg_mutex);
 		err = snd_register_oss_device(SNDRV_OSS_DEVICE_TYPE_MIXER,
 					      card, 0,
@@ -1325,6 +1330,7 @@ static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
 				"unable to register OSS mixer device %i:%i\n",
 				card->number, 0);
 			kfree(mixer);
+			module_put(THIS_MODULE);
 			return err;
 		}
 		mixer->oss_dev_alloc = 1;
@@ -1358,7 +1364,9 @@ static int snd_mixer_oss_notify_handler(struct snd_card *card, int cmd)
 		if (cmd == SND_MIXER_OSS_NOTIFY_DISCONNECT)
 			return 0;
 		snd_mixer_oss_proc_done(mixer);
-		return snd_mixer_oss_free1(mixer);
+		err = snd_mixer_oss_free1(mixer);
+		module_put(THIS_MODULE);
+		return err;
 	}
 	return 0;
 }
diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
index bd19328dca9e..3d35f9d2d675 100644
--- a/sound/core/oss/pcm_oss.c
+++ b/sound/core/oss/pcm_oss.c
@@ -3151,6 +3151,9 @@ static void register_oss_dsp(struct snd_pcm *pcm, int index)
 
 static int snd_pcm_oss_register_minor(struct snd_pcm *pcm)
 {
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
 	pcm->oss.reg = 0;
 	if (dsp_map[pcm->card->number] == (int)pcm->device) {
 		char name[128];
@@ -3207,6 +3210,7 @@ static int snd_pcm_oss_unregister_minor(struct snd_pcm *pcm)
 {
 	snd_pcm_oss_disconnect_minor(pcm);
 	snd_pcm_oss_proc_done(pcm);
+	module_put(THIS_MODULE);
 	return 0;
 }