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?

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