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. '®ister_mutex'
>
> sound/core/seq/seq_midi.c
> 297 mutex_lock(®ister_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(®ister_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(®ister_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?
thanks,
Takashi
> Before the patch, we used to rely on the callers to take
> the lock before calling snd_rawmidi_info_select() but the patch moved
> the lock inside the function.
>
> 343 strcpy(port->name, info->subname);
> 344 if (! port->name[0]) {
> 345 if (info->name[0]) {
> 346 if (ports > 1)
> 347 snprintf(port->name, sizeof(port->name), "%s-%u", info->name, p);
> 348 else
> 349 snprintf(port->name, sizeof(port->name), "%s", info->name);
>
> regards,
> dan carpenter
>
