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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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.

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. This patch is for
this bug. I believe there're no regression as you said.

Please confirm that info->count/info->owner are related to the count in
snd_ctl_elem_add(), and the latter is assigned to struct snd_kcontrol.count.


Thanks

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