[PATCH] ASoC: soc-pcm: don't ignore -EINVAL on soc_pcm_ret()

[PATCH] ASoC: soc-pcm: don't ignore -EINVAL on soc_pcm_ret()

[Kuninori Morimoto]

commit 1f5664351410 ("ASoC: lower "no backend DAIs enabled for ... Port"
log severity") ignores -EINVAL error message on common soc_pcm_ret().
It is used from many functions, ignoring -EINVAL is not good idea.

The reason why -EINVAL was ignored is only for ignoring
dpcm_fe_dai_prepare() error flood.
It should be handled at dpcm_fe_dai_prepare() side, not soc_pcm_ret()
side.

Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-pcm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 1150455619aa4..7ea580f0a6f7f 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -38,7 +38,6 @@ static inline int _soc_pcm_ret(struct snd_soc_pcm_runtime *rtd,
 	switch (ret) {
 	case -EPROBE_DEFER:
 	case -ENOTSUPP:
-	case -EINVAL:
 		break;
 	default:
 		dev_err(rtd->dev,
@@ -2560,8 +2559,8 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
 			     fe->dai_link->name);
 		dev_dbg(fe->dev, "ASoC: no backend DAIs enabled for %s\n",
 			fe->dai_link->name);
-		ret = -EINVAL;
-		goto out;
+		/* don't use soc_pcm_ret() to lower error log severity */
+		return -EINVAL;
 	}
 
 	ret = dpcm_be_dai_prepare(fe, stream);
-- 
2.43.0

Re: [PATCH] ASoC: soc-pcm: don't ignore -EINVAL on soc_pcm_ret()

[Hans de Goede]

Hi,

On 12-Dec-24 3:23 AM, Kuninori Morimoto wrote:
> commit 1f5664351410 ("ASoC: lower "no backend DAIs enabled for ... Port"
> log severity") ignores -EINVAL error message on common soc_pcm_ret().
> It is used from many functions, ignoring -EINVAL is not good idea.
> 
> The reason why -EINVAL was ignored is only for ignoring
> dpcm_fe_dai_prepare() error flood.
> It should be handled at dpcm_fe_dai_prepare() side, not soc_pcm_ret()
> side.

-EINVAL really should only be used upon invalid parameters coming from
userspace and in that case we don't want to log an error since we do
not want to give userspace a way to do a denial-of-service attack
on the syslog / diskspace.

I'm not convinced that this change is a good idea, but I also
have no strong objections.

There also is a bug in this patch which needs to be fixed before this
can be accepted, see my inline comment below.

> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/soc-pcm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 1150455619aa4..7ea580f0a6f7f 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -38,7 +38,6 @@ static inline int _soc_pcm_ret(struct snd_soc_pcm_runtime *rtd,
>  	switch (ret) {
>  	case -EPROBE_DEFER:
>  	case -ENOTSUPP:
> -	case -EINVAL:
>  		break;
>  	default:
>  		dev_err(rtd->dev,
> @@ -2560,8 +2559,8 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
>  			     fe->dai_link->name);
>  		dev_dbg(fe->dev, "ASoC: no backend DAIs enabled for %s\n",
>  			fe->dai_link->name);
> -		ret = -EINVAL;
> -		goto out;
> +		/* don't use soc_pcm_ret() to lower error log severity */
> +		return -EINVAL;

You cannot just do a return here, you are now missing these 2 lines from
the "goto out" path:

        dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
        snd_soc_dpcm_mutex_unlock(fe);


>  	}
>  
>  	ret = dpcm_be_dai_prepare(fe, stream);

Regards,

Hans

Re: [PATCH] ASoC: soc-pcm: don't ignore -EINVAL on soc_pcm_ret()

[Kuninori Morimoto]

Hi Hans

Thank you for the feedback

> -EINVAL really should only be used upon invalid parameters coming from
> userspace and in that case we don't want to log an error since we do
> not want to give userspace a way to do a denial-of-service attack
> on the syslog / diskspace.
>
> I'm not convinced that this change is a good idea, but I also
> have no strong objections.

Thanks but hmm...
In reality, -EINVAL is used not only from userspace, and many inside
kernel functions are using it. So don't indicate -EINVAL for all case
is over-kill in real world. But thank you for clarify the situation.
I think indicating it on comment is good idea.


> > @@ -2560,8 +2559,8 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
> >  			     fe->dai_link->name);
> >  		dev_dbg(fe->dev, "ASoC: no backend DAIs enabled for %s\n",
> >  			fe->dai_link->name);
> > -		ret = -EINVAL;
> > -		goto out;
> > +		/* don't use soc_pcm_ret() to lower error log severity */
> > +		return -EINVAL;
> 
> You cannot just do a return here, you are now missing these 2 lines from
> the "goto out" path:

Oh, yes, indeed.
Thank you for pointing it. Will fix in v2

Thank you for your help !!

Best regards
---
Kuninori Morimoto