Re: [bug report] ALSA: rawmidi: Avoid racy info ioctl via ctl device

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Mon, Feb 01, 2021 at 02:22:10PM +0100, Takashi Iwai wrote:
> On Mon, 01 Feb 2021 13:55:16 +0100,
> Dan Carpenter wrote:
> > 
> > Hello Takashi Iwai,
> > 
> > The patch c1cfd9025cc3: "ALSA: rawmidi: Avoid racy info ioctl via ctl
> > device" from Dec 14, 2017, leads to the following static checker
> > warning:
> > 
> > 	sound/core/rawmidi.c:651 snd_rawmidi_info_select()
> > 	warn: called with lock held.  '&register_mutex'
> > 
> > sound/core/seq/seq_midi.c
> >    297          mutex_lock(&register_mutex);
> >                            ^^^^^^^^^^^^^^^
> > Holding this lock.
> > 
> >    298          client = synths[card->number];
> >    299          if (client == NULL) {
> >    300                  newclient = 1;
> >    301                  client = kzalloc(sizeof(*client), GFP_KERNEL);
> >    302                  if (client == NULL) {
> >    303                          mutex_unlock(&register_mutex);
> >    304                          kfree(info);
> >    305                          return -ENOMEM;
> >    306                  }
> >    307                  client->seq_client =
> >    308                          snd_seq_create_kernel_client(
> >    309                                  card, 0, "%s", card->shortname[0] ?
> >    310                                  (const char *)card->shortname : "External MIDI");
> >    311                  if (client->seq_client < 0) {
> >    312                          kfree(client);
> >    313                          mutex_unlock(&register_mutex);
> >    314                          kfree(info);
> >    315                          return -ENOMEM;
> >    316                  }
> >    317          }
> >    318  
> >    319          msynth = kcalloc(ports, sizeof(struct seq_midisynth), GFP_KERNEL);
> >    320          port = kmalloc(sizeof(*port), GFP_KERNEL);
> >    321          if (msynth == NULL || port == NULL)
> >    322                  goto __nomem;
> >    323  
> >    324          for (p = 0; p < ports; p++) {
> >    325                  ms = &msynth[p];
> >    326  
> >    327                  if (snd_seq_midisynth_new(ms, card, device, p) < 0)
> >    328                          goto __nomem;
> >    329  
> >    330                  /* declare port */
> >    331                  memset(port, 0, sizeof(*port));
> >    332                  port->addr.client = client->seq_client;
> >    333                  port->addr.port = device * (256 / SNDRV_RAWMIDI_DEVICES) + p;
> >    334                  port->flags = SNDRV_SEQ_PORT_FLG_GIVEN_PORT;
> >    335                  memset(info, 0, sizeof(*info));
> >    336                  info->device = device;
> >    337                  if (p < output_count)
> >    338                          info->stream = SNDRV_RAWMIDI_STREAM_OUTPUT;
> >    339                  else
> >    340                          info->stream = SNDRV_RAWMIDI_STREAM_INPUT;
> >    341                  info->subdevice = p;
> >    342                  if (snd_rawmidi_info_select(card, info) >= 0)
> >                             ^^^^^^^^^^^^^^^^^^^^^^^
> > We can't call this function when we're holding the lock or it leads to
> > a deadlock.
> 
> Those two register_mutex are different ones; i.e. both are local static
> variables, hence its scope is only for rawmidi.c and seq_client.c,
> hence they can't conflict with each other.
> 
> Or am I missing something else?
> 

Ah...  Sorry.  And I think I just sent you another bogus bug report a
few minutes ago.  :/

regards,
dan carpenter