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

[Cen Zhang]

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.

Your point about the global mutex deserves careful consideration. Do you
have any suggestions for a better way to synchronize these paths?

Best regards,
Cen Zhang