Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies

[Takashi Iwai]

On Wed, 20 Nov 2019 16:21:36 +0100,
Andrew Gabbasov wrote:
> 
> Hello Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@xxxxxxx]
> > Sent: Wednesday, November 20, 2019 5:34 PM
> > To: Gabbasov, Andrew
> > Cc: alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jaroslav
> > Kysela; Takashi Iwai; Timo Wischer
> > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> > instead of jiffies
> > 
> > On Wed, 20 Nov 2019 12:58:55 +0100,
> > Andrew Gabbasov wrote:
> > > +/* call in loopback->cable_lock */
> > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > > +{
> > > +	int err = 0;
> > > +	struct snd_timer_id tid = {
> > > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > > +	};
> > > +	struct snd_timer_instance *timeri;
> > > +	struct loopback_cable *cable = dpcm->cable;
> > > +
> > > +	spin_lock_irq(&cable->lock);
> > > +
> > > +	/* check if timer was already opened. It is only opened once
> > > +	 * per playback and capture subdevice (aka cable).
> > > +	 */
> > > +	if (cable->snd_timer.instance)
> > > +		goto unlock;
> > > +
> > > +	err = loopback_parse_timer_id(dpcm->loopback->timer_source, &tid);
> > > +	if (err < 0) {
> > > +		pcm_err(dpcm->substream->pcm,
> > > +			"Parsing timer source \'%s\' failed with %d",
> > > +			dpcm->loopback->timer_source, err);
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	cable->snd_timer.stream = dpcm->substream->stream;
> > > +	cable->snd_timer.id = tid;
> > > +
> > > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > > +	if (!timeri) {
> > > +		err = -ENOMEM;
> > > +		goto unlock;
> > > +	}
> > > +	/* The callback has to be called from another tasklet. If
> > > +	 * SNDRV_TIMER_IFLG_FAST is specified it will be called from the
> > > +	 * snd_pcm_period_elapsed() call of the selected sound card.
> > > +	 * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave().
> > > +	 * Due to our callback loopback_snd_timer_function() also calls
> > > +	 * snd_pcm_period_elapsed() which calls
> > snd_pcm_stream_lock_irqsave().
> > > +	 * This would end up in a dead lock.
> > > +	 */
> > > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > > +	timeri->callback = loopback_snd_timer_function;
> > > +	timeri->callback_data = (void *)cable;
> > > +	timeri->ccallback = loopback_snd_timer_event;
> > > +
> > > +	/* snd_timer_close() and snd_timer_open() should not be called with
> > > +	 * locked spinlock because both functions can block on a mutex. The
> > > +	 * mutex loopback->cable_lock is kept locked. Therefore
> > snd_timer_open()
> > > +	 * cannot be called a second time by the other device of the same
> > cable.
> > > +	 * Therefore the following issue cannot happen:
> > > +	 * [proc1] Call loopback_timer_open() ->
> > > +	 *	   Unlock cable->lock for snd_timer_close/open() call
> > > +	 * [proc2] Call loopback_timer_open() -> snd_timer_open(),
> > > +	 *	   snd_timer_start()
> > > +	 * [proc1] Call snd_timer_open() and overwrite running timer
> > > +	 *	   instance
> > > +	 */
> > > +	spin_unlock_irq(&cable->lock);
> > > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
> > > +	if (err < 0) {
> > > +		pcm_err(dpcm->substream->pcm,
> > > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > > +			cable->snd_timer.id.card,
> > > +			cable->snd_timer.id.device,
> > > +			cable->snd_timer.id.subdevice,
> > > +			err);
> > > +		snd_timer_instance_free(timeri);
> > > +		return err;
> > > +	}
> > > +	spin_lock_irq(&cable->lock);
> > > +
> > > +	cable->snd_timer.instance = timeri;
> > > +
> > > +	/* initialise a tasklet used for draining */
> > > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > > +		     loopback_snd_timer_tasklet, (unsigned long)timeri);
> > 
> > This has to be set before snd_timer_open().  The callback might be
> > called immediately after snd_timer_open().
> 
> This tasklet is used/scheduled only in ccallback (not regular tick
> callback),
> and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really happen
> immediately after snd_timer_open()?

Why not?  The master timer can be stopped at any time, even between
these two lines.

Beware that there are fuzzer programs that can trigger such racy
things, and you're adding the code to the target that is actively
slapped by them :)


thanks,

Takashi