* [bug report] ALSA: oxfw: configure packet format in pcm.hw_params callback
@ 2019-06-19 10:16 Dan Carpenter
2019-06-22 1:35 ` Takashi Sakamoto
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2019-06-19 10:16 UTC (permalink / raw)
To: o-takashi; +Cc: alsa-devel
Hello Takashi Sakamoto,
The patch 4f380d007052: "ALSA: oxfw: configure packet format in
pcm.hw_params callback" from Jun 12, 2019, leads to the following
static checker warning:
sound/firewire/oxfw/oxfw-stream.c:357 snd_oxfw_stream_start_duplex()
warn: 'oxfw->rx_stream.buffer.packets' double freed
sound/firewire/oxfw/oxfw-stream.c
317 int snd_oxfw_stream_start_duplex(struct snd_oxfw *oxfw)
318 {
319 int err;
320
321 if (oxfw->substreams_count == 0)
322 return -EIO;
323
324 if (amdtp_streaming_error(&oxfw->rx_stream) ||
325 amdtp_streaming_error(&oxfw->tx_stream)) {
326 amdtp_stream_stop(&oxfw->rx_stream);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
327 cmp_connection_break(&oxfw->in_conn);
328
329 if (oxfw->has_output) {
330 amdtp_stream_stop(&oxfw->tx_stream);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
331 cmp_connection_break(&oxfw->out_conn);
332 }
333 }
334
335 if (!amdtp_stream_running(&oxfw->rx_stream)) {
336 err = start_stream(oxfw, &oxfw->rx_stream);
337 if (err < 0) {
338 dev_err(&oxfw->unit->device,
339 "fail to start rx stream: %d\n", err);
340 goto error;
341 }
342 }
343
344 if (oxfw->has_output) {
345 if (!amdtp_stream_running(&oxfw->tx_stream)) {
346 err = start_stream(oxfw, &oxfw->tx_stream);
347 if (err < 0) {
348 dev_err(&oxfw->unit->device,
349 "fail to start tx stream: %d\n", err);
350 goto error;
351 }
352 }
353 }
354
355 return 0;
356 error:
357 amdtp_stream_stop(&oxfw->rx_stream);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Smatch is basically complaining that we call amdtp_stream_stop() and
it's not clear that we necessarily reset things. I don't know this code
very well so I have maybe missed something.
358 cmp_connection_break(&oxfw->in_conn);
359 if (oxfw->has_output) {
360 amdtp_stream_stop(&oxfw->tx_stream);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
361 cmp_connection_break(&oxfw->out_conn);
362 }
363 return err;
364 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] ALSA: oxfw: configure packet format in pcm.hw_params callback
2019-06-19 10:16 [bug report] ALSA: oxfw: configure packet format in pcm.hw_params callback Dan Carpenter
@ 2019-06-22 1:35 ` Takashi Sakamoto
2019-06-23 20:35 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Sakamoto @ 2019-06-22 1:35 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel
Hi,
On Wed, Jun 19, 2019 at 01:16:39PM +0300, Dan Carpenter wrote:
> Hello Takashi Sakamoto,
>
> The patch 4f380d007052: "ALSA: oxfw: configure packet format in
> pcm.hw_params callback" from Jun 12, 2019, leads to the following
> static checker warning:
>
> sound/firewire/oxfw/oxfw-stream.c:357 snd_oxfw_stream_start_duplex()
> warn: 'oxfw->rx_stream.buffer.packets' double freed
>
> sound/firewire/oxfw/oxfw-stream.c
> 317 int snd_oxfw_stream_start_duplex(struct snd_oxfw *oxfw)
> 318 {
> 319 int err;
> 320
> 321 if (oxfw->substreams_count == 0)
> 322 return -EIO;
> 323
> 324 if (amdtp_streaming_error(&oxfw->rx_stream) ||
> 325 amdtp_streaming_error(&oxfw->tx_stream)) {
> 326 amdtp_stream_stop(&oxfw->rx_stream);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 327 cmp_connection_break(&oxfw->in_conn);
> 328
> 329 if (oxfw->has_output) {
> 330 amdtp_stream_stop(&oxfw->tx_stream);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 331 cmp_connection_break(&oxfw->out_conn);
> 332 }
> 333 }
> 334
> 335 if (!amdtp_stream_running(&oxfw->rx_stream)) {
> 336 err = start_stream(oxfw, &oxfw->rx_stream);
> 337 if (err < 0) {
> 338 dev_err(&oxfw->unit->device,
> 339 "fail to start rx stream: %d\n", err);
> 340 goto error;
> 341 }
> 342 }
> 343
> 344 if (oxfw->has_output) {
> 345 if (!amdtp_stream_running(&oxfw->tx_stream)) {
> 346 err = start_stream(oxfw, &oxfw->tx_stream);
> 347 if (err < 0) {
> 348 dev_err(&oxfw->unit->device,
> 349 "fail to start tx stream: %d\n", err);
> 350 goto error;
> 351 }
> 352 }
> 353 }
> 354
> 355 return 0;
> 356 error:
> 357 amdtp_stream_stop(&oxfw->rx_stream);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Smatch is basically complaining that we call amdtp_stream_stop() and
> it's not clear that we necessarily reset things. I don't know this code
> very well so I have maybe missed something.
>
> 358 cmp_connection_break(&oxfw->in_conn);
> 359 if (oxfw->has_output) {
> 360 amdtp_stream_stop(&oxfw->tx_stream);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 361 cmp_connection_break(&oxfw->out_conn);
> 362 }
> 363 return err;
> 364 }
>
> regards,
> dan carpenter
Thanks for you report, however the double-free doesn't occur because the
data of 'struct amdtp_stream' is protected by mutex and 'context' member
is a flag to call kernel APIs for 'struct iso_packets_buffer'. The kernel
API, amdtp_stream_stop()' can be called several times with no corruption.
```
(sound/firewire/amdtp-stream.c)
void amdtp_stream_stop(struct amdtp_stream *s)
{
mutex_lock(&s->mutex);
if (!amdtp_stream_running(s)) {
mutex_unlock(&s->mutex);
return;
}
...
fw_iso_context_destroy(s->context);
s->context = ERR_PTR(-1);
iso_packets_buffer_destroy(&s->buffer, s->unit);
...
mutex_unlock(&s->mutex);
}
EXPORT_SYMBOL(amdtp_stream_stop);
```
``
(sound/firewire/amdtp-stream.h)
static inline bool amdtp_stream_running(struct amdtp_stream *s)
{
return !IS_ERR(s->context);
}
``
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] ALSA: oxfw: configure packet format in pcm.hw_params callback
2019-06-22 1:35 ` Takashi Sakamoto
@ 2019-06-23 20:35 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2019-06-23 20:35 UTC (permalink / raw)
To: alsa-devel
On Sat, Jun 22, 2019 at 10:35:23AM +0900, Takashi Sakamoto wrote:
> (sound/firewire/amdtp-stream.c)
> void amdtp_stream_stop(struct amdtp_stream *s)
> {
> mutex_lock(&s->mutex);
>
> if (!amdtp_stream_running(s)) {
> mutex_unlock(&s->mutex);
> return;
> }
> ...
> fw_iso_context_destroy(s->context);
> s->context = ERR_PTR(-1);
It might be nice to wrap this assignment in a function call:
void mark_stream_not_running(struct amdtp_stream *s)
{
s->context = ERR_PTR(-1);
}
Or we could leave it as is... It doesn't really matter too much.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-06-23 20:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-19 10:16 [bug report] ALSA: oxfw: configure packet format in pcm.hw_params callback Dan Carpenter
2019-06-22 1:35 ` Takashi Sakamoto
2019-06-23 20:35 ` 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.