[PATCH] ALSA: firewire-tascam: add missing unwind goto in snd_tscm_stream_start_duplex()

[PATCH] ALSA: firewire-tascam: add missing unwind goto in snd_tscm_stream_start_duplex()

[Xu Biang]

Smatch Warns:
sound/firewire/tascam/tascam-stream.c:493 snd_tscm_stream_start_duplex()
warn: missing unwind goto?

The direct return will cause the stream list of "&tscm->domain" unemptied
and the session in "tscm" unfinished if amdtp_domain_start() returns with
an error.

Fix this by changing the direct return to a goto which will empty the
stream list of "&tscm->domain" and finish the session in "tscm".

The snd_tscm_stream_start_duplex() function is called in the prepare
callback of PCM. According to "ALSA Kernel API Documentation", the prepare
callback of PCM will be called many times at each setup. So, if the
"&d->streams" list is not emptied, when the prepare callback is called
next time, snd_tscm_stream_start_duplex() will receive -EBUSY from
amdtp_domain_add_stream() that tries to add an existing stream to the
domain. The error handling code after the "error" label will be executed
in this case, and the "&d->streams" list will be emptied. So not emptying
the "&d->streams" list will not cause an issue. But it is more efficient
and readable to empty it on the first error by changing the direct return
to a goto statement.

The session in "tscm" has been begun before amdtp_domain_start(), so it
needs to be finished when amdtp_domain_start() fails.

Fixes: c281d46a51e3 ("ALSA: firewire-tascam: support AMDTP domain")
Signed-off-by: Xu Biang <xubiang@hust.edu.cn>
Reviewed-by: Dan Carpenter <error27@gmail.com>
---
Note that this finding is from static analysis and not tested.

 sound/firewire/tascam/tascam-stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/tascam/tascam-stream.c b/sound/firewire/tascam/tascam-stream.c
index 53e094cc411f..dfe783d01d7d 100644
--- a/sound/firewire/tascam/tascam-stream.c
+++ b/sound/firewire/tascam/tascam-stream.c
@@ -490,7 +490,7 @@ int snd_tscm_stream_start_duplex(struct snd_tscm *tscm, unsigned int rate)
 		// packet is important for media clock recovery.
 		err = amdtp_domain_start(&tscm->domain, tx_init_skip_cycles, true, true);
 		if (err < 0)
-			return err;
+			goto error;
 
 		if (!amdtp_domain_wait_ready(&tscm->domain, READY_TIMEOUT_MS)) {
 			err = -ETIMEDOUT;
-- 
2.17.1

Re: [PATCH] ALSA: firewire-tascam: add missing unwind goto in snd_tscm_stream_start_duplex()

[Takashi Sakamoto]

Hi,

On Thu, Apr 06, 2023 at 06:28:01AM -0700, Xu Biang wrote:
> Smatch Warns:
> sound/firewire/tascam/tascam-stream.c:493 snd_tscm_stream_start_duplex()
> warn: missing unwind goto?
> 
> The direct return will cause the stream list of "&tscm->domain" unemptied
> and the session in "tscm" unfinished if amdtp_domain_start() returns with
> an error.
> 
> Fix this by changing the direct return to a goto which will empty the
> stream list of "&tscm->domain" and finish the session in "tscm".
> 
> The snd_tscm_stream_start_duplex() function is called in the prepare
> callback of PCM. According to "ALSA Kernel API Documentation", the prepare
> callback of PCM will be called many times at each setup. So, if the
> "&d->streams" list is not emptied, when the prepare callback is called
> next time, snd_tscm_stream_start_duplex() will receive -EBUSY from
> amdtp_domain_add_stream() that tries to add an existing stream to the
> domain. The error handling code after the "error" label will be executed
> in this case, and the "&d->streams" list will be emptied. So not emptying
> the "&d->streams" list will not cause an issue. But it is more efficient
> and readable to empty it on the first error by changing the direct return
> to a goto statement.
> 
> The session in "tscm" has been begun before amdtp_domain_start(), so it
> needs to be finished when amdtp_domain_start() fails.
> 
> Fixes: c281d46a51e3 ("ALSA: firewire-tascam: support AMDTP domain")
> Signed-off-by: Xu Biang <xubiang@hust.edu.cn>
> Reviewed-by: Dan Carpenter <error27@gmail.com>
> ---
> Note that this finding is from static analysis and not tested.
> 
>  sound/firewire/tascam/tascam-stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Indeed. I overlooked it when posting the patch. The bug exists Linux
kernel v5.4 or later and the fix should be forward to stable kernels.

Acked-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

> diff --git a/sound/firewire/tascam/tascam-stream.c b/sound/firewire/tascam/tascam-stream.c
> index 53e094cc411f..dfe783d01d7d 100644
> --- a/sound/firewire/tascam/tascam-stream.c
> +++ b/sound/firewire/tascam/tascam-stream.c
> @@ -490,7 +490,7 @@ int snd_tscm_stream_start_duplex(struct snd_tscm *tscm, unsigned int rate)
>  		// packet is important for media clock recovery.
>  		err = amdtp_domain_start(&tscm->domain, tx_init_skip_cycles, true, true);
>  		if (err < 0)
> -			return err;
> +			goto error;
>  
>  		if (!amdtp_domain_wait_ready(&tscm->domain, READY_TIMEOUT_MS)) {
>  			err = -ETIMEDOUT;
> -- 
> 2.17.1


Thanks

Takashi Sakamoto

Re: [PATCH] ALSA: firewire-tascam: add missing unwind goto in snd_tscm_stream_start_duplex()

[Takashi Iwai]

On Thu, 06 Apr 2023 15:28:01 +0200,
Xu Biang wrote:
> 
> Smatch Warns:
> sound/firewire/tascam/tascam-stream.c:493 snd_tscm_stream_start_duplex()
> warn: missing unwind goto?
> 
> The direct return will cause the stream list of "&tscm->domain" unemptied
> and the session in "tscm" unfinished if amdtp_domain_start() returns with
> an error.
> 
> Fix this by changing the direct return to a goto which will empty the
> stream list of "&tscm->domain" and finish the session in "tscm".
> 
> The snd_tscm_stream_start_duplex() function is called in the prepare
> callback of PCM. According to "ALSA Kernel API Documentation", the prepare
> callback of PCM will be called many times at each setup. So, if the
> "&d->streams" list is not emptied, when the prepare callback is called
> next time, snd_tscm_stream_start_duplex() will receive -EBUSY from
> amdtp_domain_add_stream() that tries to add an existing stream to the
> domain. The error handling code after the "error" label will be executed
> in this case, and the "&d->streams" list will be emptied. So not emptying
> the "&d->streams" list will not cause an issue. But it is more efficient
> and readable to empty it on the first error by changing the direct return
> to a goto statement.
> 
> The session in "tscm" has been begun before amdtp_domain_start(), so it
> needs to be finished when amdtp_domain_start() fails.
> 
> Fixes: c281d46a51e3 ("ALSA: firewire-tascam: support AMDTP domain")
> Signed-off-by: Xu Biang <xubiang@hust.edu.cn>
> Reviewed-by: Dan Carpenter <error27@gmail.com>

Thanks, applied now.


Takashi