[alsa-devel] [PATCH 3/3] ALSA: seq: Protect racy pool manipulation from OSS sequencer

[Takashi Iwai]

On Mon, 15 Apr 2019 10:52:01 +0200,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Mon, 15 Apr 2019, Takashi Iwai wrote:
> 
> > > > +void snd_seq_client_ioctl_unlock(int clientid)
> > > > +{
> > > > +	struct snd_seq_client *client;
> > > > +
> > > > +	client = snd_seq_client_use_ptr(clientid);
> > > > +	if (WARN_ON(!client))
> > > > +		return;
> > > > +	mutex_unlock(&client->ioctl_mutex);
> > > > +	snd_use_lock_free(&client->use_lock);
> > > > +	snd_seq_client_unlock(client);
> > > 
> > > is that double-unlock intentional? snd_seq_client_unlock() seems to call
> > > the same snd_use_lock_free for the same handle as on the previous line.
> > 
> > Yes, it's intentional.  The trick is snd_seq_client_ioctl_lock()
> > doesn't call snd_seq_client_unlock(), i.e. keeps refcount up.  This
> > is, in turn, released in snd_seq_client_ioctl_unlock().
> > snd_seq_client_ioctl_unlock() gets its refcount at use_ptr(), hence
> > this releases twice.  Maybe it's worth for more comments.
> 
> ah yes, ok thanks, just checking. :) I was misled by the asymmetric 
> names. As you are taking the refcount with two client_use_ptr() calls in 
> both lock and unlock, would it be more logical to have two 
> snd_seq_client_unlock() calls in unlock?

Well I thought it might be misleading to call twice, but yeah, that's
rather confusing.  I'll change to two sequential unlock calls with
more comments.


thanks,

Takashi