Re: [PATCH 3/5] ALSA: control: fix logic error about control count in a device

[Takashi Iwai <tiwai@suse.de>]

At Thu, 12 Feb 2015 22:20:48 +0900,
Takashi Sakamoto wrote:
> 
> On 2015年02月11日 22:15, Takashi Iwai wrote:
> > At Wed, 11 Feb 2015 19:40:11 +0900,
> > Takashi Sakamoto wrote:
> >>
> >> It's assumed that the number of userspace controls is just 1 in several
> >> parts, while this assumptions is not always true because the value of
> >> 'owner' member can be assigned to.
> >>
> >> This commit fixes this issue.
> > 
> > Well, the current code isn't incorrect, it deals with the number of
> > grouped elements, not the total number of elements.
> 
> I didn't read such design from these comments.
> 
> include/sound/core.h:
> struct snd_card {
> ...
>     int controls_count;             /* count of all controls */
>     int user_ctl_count;             /* count of all user controls */
> }}}
> 
> But '32' is a bit little as maximum number of userspace controls, so
> your explaination may be true. If so, the comment should be 'count of
> user control groups', at least, different expression should be used.

Actually the text wasn't updated when we changed the code to allow
multiple counts.

> > So, this is rather a change of the semantics of card->user_ctl_count
> > field than a fix, and it's the question: whether we should limit for
> > the whole number of elements.
> 
> We should assume that userspace applications include any bugs. There may
> be an application which adds too many controls. In this reason, we
> should limit the maximum number of elements.

It's already limited (as each type has the limited number of max
elements).  Your patch would just limit it more strictly.

> > There is a very slight chance of user-space breakage by counting the
> > whole numbers, but pragmatically seen, I think it's acceptable from
> > the safety POV.
> 
> Kernel drivers don't add so many controls, thus such breakage is caused
> by userspace applications. But I cannot imagine such breakage. How it
> occurs?

The patch essentially reduces the max user elements.  If a user-space
program knows of the limitation and works around it secretly by use of
multiple counts, this application would be broken after your patch.
This can be seen as a kernel regression.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel