Re: [bug report] ALSA: oxfw: configure packet format in pcm.hw_params callback

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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