* [bug report] ALSA: rawmidi: Avoid racy info ioctl via ctl device
@ 2021-02-01 12:55 Dan Carpenter
2021-02-01 13:22 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-02-01 12:55 UTC (permalink / raw)
To: tiwai; +Cc: alsa-devel
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. 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] ALSA: rawmidi: Avoid racy info ioctl via ctl device
2021-02-01 12:55 [bug report] ALSA: rawmidi: Avoid racy info ioctl via ctl device Dan Carpenter
@ 2021-02-01 13:22 ` Takashi Iwai
2021-02-02 6:11 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2021-02-01 13:22 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] ALSA: rawmidi: Avoid racy info ioctl via ctl device
2021-02-01 13:22 ` Takashi Iwai
@ 2021-02-02 6:11 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2021-02-02 6:11 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
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. '®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?
>
Ah... Sorry. And I think I just sent you another bogus bug report a
few minutes ago. :/
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-02 6:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-01 12:55 [bug report] ALSA: rawmidi: Avoid racy info ioctl via ctl device Dan Carpenter
2021-02-01 13:22 ` Takashi Iwai
2021-02-02 6:11 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.