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

From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org, clemens@ladisch.de
Subject: Re: [PATCH 3/5] ALSA: control: fix logic error about control count in a device
Date: Thu, 12 Feb 2015 22:20:48 +0900	[thread overview]
Message-ID: <54DCA8B0.1050306@sakamocchi.jp> (raw)
In-Reply-To: <s5hwq3ooah3.wl-tiwai@suse.de>

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.

> 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.

> 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?

> However, changing the error code is no-go.

This is my fault to create this patchset...

Thanks

Takashi Sakamoto

>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>  sound/core/control.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index 1edd6c5..bce4730 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -514,6 +514,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>>  {
>>  	struct snd_card *card = file->card;
>>  	struct snd_kcontrol *kctl;
>> +	unsigned int count;
>>  	int i, ret;
>>  
>>  	down_write(&card->controls_rwsem);
>> @@ -531,10 +532,11 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>>  			ret = -EBUSY;
>>  			goto error;
>>  		}
>> +	count = kctl->count;
>>  	ret = snd_ctl_remove(card, kctl);
>>  	if (ret < 0)
>>  		goto error;
>> -	card->user_ctl_count--;
>> +	card->user_ctl_count -= count;
>>  error:
>>  	up_write(&card->controls_rwsem);
>>  	return ret;
>> @@ -1202,10 +1204,15 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>>  			return err;
>>  	}
>>  
>> -	if (card->user_ctl_count >= MAX_USER_CONTROLS)
>> -		return -ENOMEM;
>> +	/*
>> +	 * The number of controls with the same feature, distinguished by index.
>> +	 */
>> +	kctl.count = info->owner;
>> +	if (kctl.count == 0)
>> +		kctl.count = 1;
>> +	if (card->user_ctl_count + kctl.count > MAX_USER_CONTROLS)
>> +		return -ENOSPC;
>>  
>> -	kctl.count = info->owner ? info->owner : 1;
>>  	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
>>  		kctl.info = snd_ctl_elem_user_enum_info;
>>  	else
>> @@ -1259,7 +1266,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>>  		return err;
>>  
>>  	down_write(&card->controls_rwsem);
>> -	card->user_ctl_count++;
>> +	card->user_ctl_count += _kctl->count;
>>  	up_write(&card->controls_rwsem);
>>  
>>  	return 0;
>> -- 
>> 2.1.0
>>
> 

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