On Wed, Dec 12, 2018 at 01:15:26PM +0100, Christian Gromm wrote:
> This patch avoids that a sound card is created and registered with ALSA
> every time a channel is being linked. Instead the channels are hooked on
> the same card, which is registered not until the final link has been added
> to the component.
>
> Signed-off-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx>
> ---
> drivers/staging/most/sound/sound.c | 127 +++++++++++++++++++++++++------------
> 1 file changed, 87 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
> index 89b02fc..41bcd2c 100644
> --- a/drivers/staging/most/sound/sound.c
> +++ b/drivers/staging/most/sound/sound.c
> @@ -10,6 +10,7 @@
> #include <linux/module.h>
> #include <linux/printk.h>
> #include <linux/kernel.h>
> +#include <linux/slab.h>
> #include <linux/init.h>
> #include <sound/core.h>
> #include <sound/pcm.h>
> @@ -20,7 +21,6 @@
>
> #define DRIVER_NAME "sound"
>
> -static struct list_head dev_list;
> static struct core_component comp;
>
> /**
> @@ -56,6 +56,17 @@ struct channel {
> void (*copy_fn)(void *alsa, void *most, unsigned int bytes);
> };
>
> +struct sound_adapter {
> + struct list_head dev_list;
> + struct most_interface *iface;
> + struct snd_card *card;
> + struct list_head list;
> + bool registered;
> + int pcm_dev_idx;
> +};
> +
> +static struct list_head adpt_list;
> +
> #define MOST_PCM_INFO (SNDRV_PCM_INFO_MMAP | \
> SNDRV_PCM_INFO_MMAP_VALID | \
> SNDRV_PCM_INFO_BATCH | \
> @@ -157,9 +168,10 @@ static void most_to_alsa_copy32(void *alsa, void *most, unsigned int bytes)
> static struct channel *get_channel(struct most_interface *iface,
> int channel_id)
> {
> + struct sound_adapter *adpt = iface->priv;
> struct channel *channel, *tmp;
>
> - list_for_each_entry_safe(channel, tmp, &dev_list, list) {
> + list_for_each_entry_safe(channel, tmp, &adpt->dev_list, list) {
> if ((channel->iface == iface) && (channel->id == channel_id))
> return channel;
> }
> @@ -460,7 +472,7 @@ static const struct snd_pcm_ops pcm_ops = {
> };
>
> static int split_arg_list(char *buf, char **card_name, u16 *ch_num,
> - char **sample_res)
> + char **sample_res, u8 *create)
> {
> char *num;
> int ret;
> @@ -479,6 +491,9 @@ static int split_arg_list(char *buf, char **card_name, u16 *ch_num,
> *sample_res = strsep(&buf, ".\n");
> if (!*sample_res)
> goto err;
> +
> + if (buf && !strcmp(buf, "create"))
> + *create = 1;
This comes from userspace, right? So we're adding a new API?
> return 0;
>
> err:
> @@ -536,6 +551,19 @@ static int audio_set_hw_params(struct snd_pcm_hardware *pcm_hw,
> return 0;
> }
>
> +static void release_adapter(struct sound_adapter *adpt)
> +{
> + struct channel *channel, *tmp;
> +
> + list_for_each_entry_safe(channel, tmp, &adpt->dev_list, list) {
> + list_del(&channel->list);
> + kfree(channel);
> + }
> + snd_card_free(adpt->card);
> + list_del(&adpt->list);
> + kfree(adpt);
> +}
> +
> /**
> * audio_probe_channel - probe function of the driver module
> * @iface: pointer to interface instance
> @@ -553,7 +581,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
> char *arg_list)
> {
> struct channel *channel;
> - struct snd_card *card;
> + struct sound_adapter *adpt;
> struct snd_pcm *pcm;
> int playback_count = 0;
> int capture_count = 0;
> @@ -561,6 +589,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
> int direction;
> char *card_name;
> u16 ch_num;
> + u8 create = 0;
> char *sample_res;
>
> if (!iface)
> @@ -571,6 +600,40 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
> return -EINVAL;
> }
>
> + ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res,
> + &create);
> + if (ret < 0)
> + return ret;
> +
> + list_for_each_entry(adpt, &adpt_list, list) {
> + if (adpt->iface == iface && adpt->registered)
> + return -ENOSPC;
> + if (!adpt->registered) {
This is very confusing and I'm sorry but I don't think it even works...
:(
We add new allocations to the &adpt_list, but then if audio_probe_channel()
fails, then we free "adpt" in the error handling. But we don't remove
the adpt from the list so now if we iterate through the list again it's
a use after free.
It would be easy enought to remove the item from the list, but really my
issue is that I don't understand what we're trying to do here. It looks
like this patch changes the user space interface and adds a "create"
argument? I just think that's not a good API design.
> + adpt->pcm_dev_idx++;
> + goto skip_adpt_alloc;
> + }
> + }
> + adpt = kzalloc(sizeof(*adpt), GFP_KERNEL);
> + if (!adpt)
> + return -ENOMEM;
> +
> + adpt->iface = iface;
> + adpt->registered = false;
No need for this. adpt was allocated with kzalloc().
> + adpt->pcm_dev_idx = 0;
Same.
> + INIT_LIST_HEAD(&adpt->dev_list);
> + iface->priv = adpt;
> + ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
> + sizeof(*channel), &adpt->card);
> + if (ret < 0)
> + return ret;
> + snprintf(adpt->card->driver, sizeof(adpt->card->driver),
> + "%s", DRIVER_NAME);
> + snprintf(adpt->card->shortname, sizeof(adpt->card->shortname),
> + "Microchip MOST:%d", adpt->card->number);
> + snprintf(adpt->card->longname, sizeof(adpt->card->longname),
> + "%s at %s, ch %d", adpt->card->shortname, iface->description,
> + channel_id);
> +skip_adpt_alloc:
> if (get_channel(iface, channel_id)) {
> pr_err("channel (%s:%d) is already linked\n",
> iface->description, channel_id);
> @@ -584,36 +647,25 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
> capture_count = 1;
> direction = SNDRV_PCM_STREAM_CAPTURE;
> }
> -
> - ret = split_arg_list(arg_list, &card_name, &ch_num, &sample_res);
> - if (ret < 0)
> - return ret;
> -
> - ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
> - sizeof(*channel), &card);
> - if (ret < 0)
> - return ret;
> -
> - channel = card->private_data;
> - channel->card = card;
> + channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> + if (!channel)
> + goto err_free_card;
We need to set "ret = -ENOMEM;" before the goto.
> + channel->card = adpt->card;
> channel->cfg = cfg;
> channel->iface = iface;
> channel->id = channel_id;
> init_waitqueue_head(&channel->playback_waitq);
> + list_add_tail(&channel->list, &adpt->dev_list);
> + list_add_tail(&adpt->list, &adpt_list);
>
> ret = audio_set_hw_params(&channel->pcm_hardware, ch_num, sample_res,
> cfg);
> if (ret)
> goto err_free_card;
>
> - snprintf(card->driver, sizeof(card->driver), "%s", DRIVER_NAME);
> - snprintf(card->shortname, sizeof(card->shortname), "Microchip MOST:%d",
> - card->number);
> - snprintf(card->longname, sizeof(card->longname), "%s at %s, ch %d",
> - card->shortname, iface->description, channel_id);
> + ret = snd_pcm_new(adpt->card, card_name, adpt->pcm_dev_idx,
> + playback_count, capture_count, &pcm);
>
> - ret = snd_pcm_new(card, card_name, 0, playback_count,
> - capture_count, &pcm);
> if (ret < 0)
> goto err_free_card;
>
> @@ -621,16 +673,16 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
>
> snd_pcm_set_ops(pcm, direction, &pcm_ops);
>
> - ret = snd_card_register(card);
> - if (ret < 0)
> - goto err_free_card;
> -
> - list_add_tail(&channel->list, &dev_list);
> -
> + if (create) {
> + ret = snd_card_register(adpt->card);
> + if (ret < 0)
> + goto err_free_card;
> + adpt->registered = true;
> + }
> return 0;
>
> err_free_card:
> - snd_card_free(card);
> + release_adapter(adpt);
> return ret;
> }
>
> @@ -647,6 +699,7 @@ static int audio_disconnect_channel(struct most_interface *iface,
> int channel_id)
> {
> struct channel *channel;
> + struct sound_adapter *adpt = iface->priv;
>
> channel = get_channel(iface, channel_id);
> if (!channel) {
> @@ -656,8 +709,10 @@ static int audio_disconnect_channel(struct most_interface *iface,
> }
>
> list_del(&channel->list);
> - snd_card_free(channel->card);
>
> + kfree(channel);
> + if (list_empty(&adpt->dev_list))
> + release_adapter(adpt);
This is racy. If we connect a new device at the same time then it leads
to a use after free. And the thing is say a device gets bumped then it
can trigger a disconnect followed by an immediate reconnect so I could
easily imagine hitting this bug.
I don't really know this driver well (or at all) so can you maybe
explain the problem a bit and we can figure out a better solution? Do
we not know which channels are available at probe time? Do they change?
It feels like we should just take the list of channels and register them
all in a for loop and then register the sound card. But obviously if it
were that simple, you would have done that so there is a problem here
which I have not seen...
regards,
dan carpenter
> return 0;
> }
>
> @@ -733,22 +788,14 @@ static int __init audio_init(void)
> {
> pr_info("init()\n");
>
> - INIT_LIST_HEAD(&dev_list);
> + INIT_LIST_HEAD(&adpt_list);
>
> return most_register_component(&comp);
> }
>
> static void __exit audio_exit(void)
> {
> - struct channel *channel, *tmp;
> -
> pr_info("exit()\n");
> -
> - list_for_each_entry_safe(channel, tmp, &dev_list, list) {
> - list_del(&channel->list);
> - snd_card_free(channel->card);
> - }
> -
> most_deregister_component(&comp);
> }
>
> --
> 2.7.4
>
> _______________________________________________
> devel mailing list
> devel@xxxxxxxxxxxxxxxxxxxxxx
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
