Re: [PATCH v2 3/6] ALSA: control: Apply sanity check of input values for user elements

[Takashi Iwai <tiwai@xxxxxxx>]

On Sat, 15 Jun 2024 10:02:35 +0200,
Takashi Sakamoto wrote:
> 
> On Sat, Jun 15, 2024 at 09:28:50AM +0200, Takashi Iwai wrote:
> > > In the commit coment, I can see "that's the only way to filter out the
> > > invalid values", however it not so good idea, since the ALSA control core
> > > function loses transparency against control elements somehow.
> > 
> > Transparency?  The sanity check of input values is done in each driver
> > side, hence some overhead is more or less always present, depending on
> > the implementation.
> >
> > > Furthermore, I can see "there is no corresponding driver", however it is
> > > suspicious somehow. It would be smart to charge the validation
> > > implementation for user-defined control element set if forcing it.
> > 
> > The context there implies that, in the case of user elements, all
> > handled in sound/core/control.c, and there is no other dedicated
> > driver code handling the control put for those controls, hence
> > sound/core/control.c is the only place where we can address the
> > issue.
> 
> If you can force the validation to _all_ of the existing drivers by any
> kind of mechanism, it would be. Actually, not. We can have such driver
> which handles the write request without such validation, and control core
> allows it. The kernel configuration is to ease the detection of such
> drivers (and applications) in application runtime. Therefore the
> transparency would be lost by the patch.

In principle, the validation should be done for *every* kcontrol.  The
lack of the validation was ignored so far with a naive assumption that
the driver treats properly nevertheless.  But since we're checking it
more strictly in kselftest, the problem became more obvious, and this
is a corresponding fix for user control element part.  HD-audio driver
had another issues and they are fixed in other patches of this
series.

> Assuming that two control element exist in a sound card, which has the
> same information and TLV response, except for the flag of
> SNDRV_CTL_ELEM_ACCESS_USER. For the same value data, one operation with
> SNDRV_CTL_IOCTL_ELEM_WRITE is successful, and another operation with
> SNDRV_CTL_ELEM_ACCESS_USER is failed. When encountering this issue,
> the programmer of the application suspect the bug pertaining to the latter
> control, then the programmer find the latter has
> SNDRV_CTL_ELEM_ACCESS_USER. Then the programmer would judge that 'I got
> it, it is a bug of user-defined control element set' even if the program
> includes the bug for min/max/step computation and the underlying sound
> driver includes the bug not to validate value data.

No, it's a wrong understanding, other way round: the driver must
validate the values by itself.

> The patch loses transparency in the above step. Without the patch, both
> operations finish with the equivalent result.
> 
> Nevertheless, I think the validation is itself preferable.

The validation is not "preferable" but rather "mandatory".

> In my opinion,
> the validation before/after the call of 'snd_kcontrol_put_t' would result
> in the different argument. The 'validate-before-call' is the argument of
> control core function, while 'validate-after-call is the argument of
> implementation of user-defined element set. The patch should belong to the
> latter to extend current implementation of user-defined element set.
> Thus I suggest to put the validation into the put callback function,
> regardless of the optimization to which you address.

I don't get the argument, sorry.
If you have a better point, please submit an incremental patch.


thanks,

Takashi