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

[Takashi Iwai]

At Fri, 13 Feb 2015 08:06:45 +0900,
Takashi Sakamoto wrote:
> 
> On Feb 12 2015 22:29, Takashi Iwai wrote:
> > 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.
> 
> No.
> 
> In userspace control APIs, several controls with the same feature can be
> added in one ioctl (SNDRV_CTL_IOCTL_ELEM_ADD). This is achieved by
> setting the number of controls to struct snd_ctl_elem_info.owner. As a
> result, the number is set to struct snd_kcontro.count.
> 
> However struct snd_card.user_ctl_count is increment/decrement by 1,
> ignoring the value of struct snd_kcontrol.count.

So?  This is a count of the element groups.  That's all.

> Of cource, there're no APIs for userspace library (alsa-lib) to set the
> owner field, thus it's always zero. Then kernel control code set 1 to
> struct snd_kcontrol.count. In normal usage, current kernel code looks fine.
> 
> But in a point of kernel code itself, this is a bug.

No, this is *no* bug, especially from user-space POV.

> This patch is for
> this bug. I believe there're no regression as you said.

No, no.  You misunderstand the definition of a regression.
If any user-space program that worked before gets broken by a kernel
change, this is a kernel regression, no matter what.  And, in general,
the kernel *must not* give any regression.  Even if it's seen as a
kernel-side bug fix, it cannot be justified always.

And, in this case, what merit would we have with your patch?  The
current code can already limit the usage to at most a couple of MB
slab, which is fine from OS operation POV.

Don't get me wrong: I'm not against your change.  But you must
understand that you're going to break user-space stuff if it's
absurdly programmed.  And if this really happens, we have to fix
*kernel*, not user-space.  That is, either revert this change or
increase the limit.

You have to take this into account and revise the patch and
description accordingly.


Takashi