[PATCH 0/5] ALSA: control: improve core codes with intrusion

[PATCH 0/5] ALSA: control: improve core codes with intrusion

[Takashi Sakamoto]

This patchset is for improving core control codes. Most of them have any
effects for processing, messaging or status codes.

This patchset is based on my previous one:
[alsa-devel] [PATCH 0/8] ALSA: control: refactoring core codes
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/087783.html

Takashi Sakamoto (5):
  ALSA: control: drop a sub-effect for caller's data
  ALSA: control: use dev_dbg() for warning to add duplicated controls
  ALSA: control: fix logic error about control count in a device
  ALSA: control: improve returned value because of the rest
  ALSA: control: save stack usage at creating new instance

 sound/core/control.c | 209 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 116 insertions(+), 93 deletions(-)

-- 
2.1.0

[PATCH 1/5] ALSA: control: drop a sub-effect for caller's data

[Takashi Sakamoto]

In snd_ctl_elem_add(), the given information data is changed when
calling snd_ctl_elem_add(). The numid field is set as zero.

This commit drops this sub-effect for a better API design.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index af95783..d98b990 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1192,11 +1192,12 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
 	access |= SNDRV_CTL_ELEM_ACCESS_USER;
 
-	info->id.numid = 0;
 	memset(&kctl, 0, sizeof(kctl));
+	kctl.id = info->id;
+	kctl.id.numid = 0;
 
 	if (replace) {
-		err = snd_ctl_remove_user_ctl(file, &info->id);
+		err = snd_ctl_remove_user_ctl(file, &kctl.id);
 		if (err)
 			return err;
 	}
@@ -1204,7 +1205,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (card->user_ctl_count >= MAX_USER_CONTROLS)
 		return -ENOMEM;
 
-	memcpy(&kctl.id, &info->id, sizeof(info->id));
 	kctl.count = info->owner ? info->owner : 1;
 	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
 		kctl.info = snd_ctl_elem_user_enum_info;
-- 
2.1.0

[PATCH 2/5] ALSA: control: use dev_dbg() for warning to add duplicated controls

[Takashi Sakamoto]

When duplicated controls are added, snd module generates message and
the callers receive -EBUSY. The messaging is redundant, especially for
userspace applications because it's caller's fault and callers get
the status code.

This commit change its messaging level to suppress messages in normal
usage.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index d98b990..1edd6c5 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -349,7 +349,7 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
 	down_write(&card->controls_rwsem);
 	if (snd_ctl_find_id(card, &id)) {
 		up_write(&card->controls_rwsem);
-		dev_err(card->dev,
+		dev_dbg(card->dev,
 			"control %i:%i:%i:%s:%i is already present\n",
 			id.iface, id.device, id.subdevice, id.name, id.index);
 		err = -EBUSY;
-- 
2.1.0

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

[Takashi Sakamoto]

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.

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

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

[Takashi Iwai]

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

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.

However, changing the error code is no-go.


thanks,

Takashi

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

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

[Takashi Sakamoto]

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

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

[Takashi Iwai]

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

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

[Takashi Sakamoto]

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

Re: [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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 4/5] ALSA: control: improve returned value because of the rest

[Takashi Sakamoto]

When a character device doesn't have enough spaces for requested number
of controls or values, currently return -ENOMEM. This should be -ENOSPC
(No space left on device).

This commit improves this behaviour.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index bce4730..9944d75 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -313,7 +313,7 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
 
 		/* this situation is very unlikely */
 		dev_err(card->dev, "unable to allocate new control numid\n");
-		return -ENOMEM;
+		return -ENOSPC;
 	}
 	return 0;
 }
@@ -355,9 +355,9 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
 		err = -EBUSY;
 		goto error;
 	}
-	if (snd_ctl_find_hole(card, kcontrol->count) < 0) {
+	err = snd_ctl_find_hole(card, kcontrol->count);
+	if (err < 0) {
 		up_write(&card->controls_rwsem);
-		err = -ENOMEM;
 		goto error;
 	}
 	list_add_tail(&kcontrol->list, &card->controls);
@@ -422,9 +422,9 @@ int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol,
 		goto error;
 	}
 add:
-	if (snd_ctl_find_hole(card, kcontrol->count) < 0) {
+	err = snd_ctl_find_hole(card, kcontrol->count);
+	if (err < 0) {
 		up_write(&card->controls_rwsem);
-		ret = -ENOMEM;
 		goto error;
 	}
 	list_add_tail(&kcontrol->list, &card->controls);
-- 
2.1.0

Re: [PATCH 4/5] ALSA: control: improve returned value because of the rest

[Takashi Iwai]

At Wed, 11 Feb 2015 19:40:12 +0900,
Takashi Sakamoto wrote:
 
> When a character device doesn't have enough spaces for requested number
> of controls or values, currently return -ENOMEM. This should be -ENOSPC
> (No space left on device).
> 
> This commit improves this behaviour.

Well, if the error code is passed to user-space, we must not change
the value now unless this change is mandatory.  Otherwise this would
break the user-space behavior.


Takashi

> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/control.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index bce4730..9944d75 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -313,7 +313,7 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count)
>  
>  		/* this situation is very unlikely */
>  		dev_err(card->dev, "unable to allocate new control numid\n");
> -		return -ENOMEM;
> +		return -ENOSPC;
>  	}
>  	return 0;
>  }
> @@ -355,9 +355,9 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
>  		err = -EBUSY;
>  		goto error;
>  	}
> -	if (snd_ctl_find_hole(card, kcontrol->count) < 0) {
> +	err = snd_ctl_find_hole(card, kcontrol->count);
> +	if (err < 0) {
>  		up_write(&card->controls_rwsem);
> -		err = -ENOMEM;
>  		goto error;
>  	}
>  	list_add_tail(&kcontrol->list, &card->controls);
> @@ -422,9 +422,9 @@ int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol,
>  		goto error;
>  	}
>  add:
> -	if (snd_ctl_find_hole(card, kcontrol->count) < 0) {
> +	err = snd_ctl_find_hole(card, kcontrol->count);
> +	if (err < 0) {
>  		up_write(&card->controls_rwsem);
> -		ret = -ENOMEM;
>  		goto error;
>  	}
>  	list_add_tail(&kcontrol->list, &card->controls);
> -- 
> 2.1.0
> 

[PATCH 5/5] ALSA: control: save stack usage at creating new instance

[Takashi Sakamoto]

When creating a new instance of control, both of userspace or kernel driver
uses stack for a instance of kernel control structure with parameters, then
keep memory object and copy the instance. This way is a waste of stack.

This commit change to keep memory object at first, then copy the parameters
to the object. These items are applied:
 * change prototype of ctl_new() with new parameters
 * call ctl_new() at first and get memory objects
 * then set identical info and callback by callers's side

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 198 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 107 insertions(+), 91 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 9944d75..e7fabbb 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -193,30 +193,35 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask,
 }
 EXPORT_SYMBOL(snd_ctl_notify);
 
-static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
-				    unsigned int access)
+static int ctl_new(struct snd_kcontrol **kctl, unsigned int count,
+		   unsigned int access, unsigned int private_size)
 {
-	struct snd_kcontrol *kctl;
 	unsigned int size;
 	unsigned int i;
 	
-	if (snd_BUG_ON(!control || !control->count))
-		return NULL;
+	if (count == 0 || count > MAX_VALUES_COUNT)
+		return -EINVAL;
 
-	if (control->count > MAX_VALUES_COUNT)
-		return NULL;
+	size  = sizeof(struct snd_kcontrol);
+	size += sizeof(struct snd_kcontrol_volatile) * count;
 
-	size  = sizeof(*kctl);
-	size += sizeof(struct snd_kcontrol_volatile) * control->count;
-	kctl = kzalloc(size, GFP_KERNEL);
-	if (kctl == NULL) {
-		pr_err("ALSA: Cannot allocate control instance\n");
-		return NULL;
+	*kctl = kzalloc(size, GFP_KERNEL);
+	if (*kctl == NULL)
+		return -ENOMEM;
+
+	if (private_size > 0) {
+		(*kctl)->private_data = kzalloc(private_size, GFP_KERNEL);
+		if ((*kctl)->private_data == NULL) {
+			kfree(*kctl);
+			return -ENOMEM;
+		}
 	}
-	*kctl = *control;
-	for (i = 0; i < kctl->count; i++)
-		kctl->vd[i].access = access;
-	return kctl;
+
+	for (i = 0; i < count; i++)
+		(*kctl)->vd[i].access = access;
+	(*kctl)->count = count;
+
+	return 0;
 }
 
 /**
@@ -233,37 +238,52 @@ static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control,
 struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol,
 				  void *private_data)
 {
-	struct snd_kcontrol kctl;
+	struct snd_kcontrol *kctl;
+	unsigned int count;
 	unsigned int access;
 	
 	if (snd_BUG_ON(!ncontrol || !ncontrol->info))
 		return NULL;
-	memset(&kctl, 0, sizeof(kctl));
-	kctl.id.iface = ncontrol->iface;
-	kctl.id.device = ncontrol->device;
-	kctl.id.subdevice = ncontrol->subdevice;
+
+	count = ncontrol->count;
+	if (count == 0)
+		count = 1;
+
+	access = ncontrol->access;
+	if (access == 0)
+		access = SNDRV_CTL_ELEM_ACCESS_READWRITE;
+	access &= SNDRV_CTL_ELEM_ACCESS_READWRITE |
+		  SNDRV_CTL_ELEM_ACCESS_VOLATILE |
+		  SNDRV_CTL_ELEM_ACCESS_INACTIVE |
+		  SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE |
+		  SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND |
+		  SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
+
+	if (ctl_new(&kctl, count, access, 0) < 0) {
+		pr_err("ALSA: Cannot allocate control instance\n");
+		return NULL;
+	}
+
+	kctl->id.iface = ncontrol->iface;
+	kctl->id.device = ncontrol->device;
+	kctl->id.subdevice = ncontrol->subdevice;
+	kctl->id.index = ncontrol->index;
 	if (ncontrol->name) {
-		strlcpy(kctl.id.name, ncontrol->name, sizeof(kctl.id.name));
-		if (strcmp(ncontrol->name, kctl.id.name) != 0)
+		strlcpy(kctl->id.name, ncontrol->name, sizeof(kctl->id.name));
+		if (strcmp(ncontrol->name, kctl->id.name) != 0)
 			pr_warn("ALSA: Control name '%s' truncated to '%s'\n",
-				ncontrol->name, kctl.id.name);
+				ncontrol->name, kctl->id.name);
 	}
-	kctl.id.index = ncontrol->index;
-	kctl.count = ncontrol->count ? ncontrol->count : 1;
-	access = ncontrol->access == 0 ? SNDRV_CTL_ELEM_ACCESS_READWRITE :
-		 (ncontrol->access & (SNDRV_CTL_ELEM_ACCESS_READWRITE|
-				      SNDRV_CTL_ELEM_ACCESS_VOLATILE|
-				      SNDRV_CTL_ELEM_ACCESS_INACTIVE|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND|
-				      SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK));
-	kctl.info = ncontrol->info;
-	kctl.get = ncontrol->get;
-	kctl.put = ncontrol->put;
-	kctl.tlv.p = ncontrol->tlv.p;
-	kctl.private_value = ncontrol->private_value;
-	kctl.private_data = private_data;
-	return ctl_new(&kctl, access);
+
+	kctl->info = ncontrol->info;
+	kctl->get = ncontrol->get;
+	kctl->put = ncontrol->put;
+	kctl->tlv.p = ncontrol->tlv.p;
+
+	kctl->private_value = ncontrol->private_value;
+	kctl->private_data = private_data;
+
+	return kctl;
 }
 EXPORT_SYMBOL(snd_ctl_new1);
 
@@ -1175,14 +1195,29 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		[SNDRV_CTL_ELEM_TYPE_INTEGER64] = 64,
 	};
 	struct snd_card *card = file->card;
-	struct snd_kcontrol kctl, *_kctl;
+	struct snd_kcontrol *kctl;
+	unsigned int count;
 	unsigned int access;
 	unsigned int elem_data_size;
 	struct user_element *ue;
 	int i, err;
 
-	if (info->count < 1)
+	if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
+	    info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
+		return -EINVAL;
+	if (info->count < 1 || info->count > max_value_counts[info->type])
 		return -EINVAL;
+	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED &&
+	    info->value.enumerated.items == 0)
+		return -EINVAL;
+	elem_data_size = value_sizes[info->type] * info->count;
+
+	/*
+	 * The number of controls with the same feature, distinguished by index.
+	 */
+	count = info->owner;
+	if (count == 0)
+		count = 1;
 
 	access = info->access;
 	if (access == 0)
@@ -1194,48 +1229,36 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 		access |= SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
 	access |= SNDRV_CTL_ELEM_ACCESS_USER;
 
-	memset(&kctl, 0, sizeof(kctl));
-	kctl.id = info->id;
-	kctl.id.numid = 0;
+	err = ctl_new(&kctl, count, access,
+		      sizeof(struct user_element) + elem_data_size);
+	if (err < 0)
+		return err;
+	kctl->private_free = snd_ctl_elem_user_free;
 
+	kctl->id = info->id;
 	if (replace) {
-		err = snd_ctl_remove_user_ctl(file, &kctl.id);
-		if (err)
-			return err;
+		err = snd_ctl_remove_user_ctl(file, &kctl->id);
+		if (err < 0)
+			goto error_allocated;
 	}
 
-	/*
-	 * 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;
+	if (card->user_ctl_count + kctl->count > MAX_USER_CONTROLS) {
+		err = -ENOSPC;
+		goto error_allocated;
+	}
 
 	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
-		kctl.info = snd_ctl_elem_user_enum_info;
+		kctl->info = snd_ctl_elem_user_enum_info;
 	else
-		kctl.info = snd_ctl_elem_user_info;
+		kctl->info = snd_ctl_elem_user_info;
 	if (access & SNDRV_CTL_ELEM_ACCESS_READ)
-		kctl.get = snd_ctl_elem_user_get;
+		kctl->get = snd_ctl_elem_user_get;
 	if (access & SNDRV_CTL_ELEM_ACCESS_WRITE)
-		kctl.put = snd_ctl_elem_user_put;
+		kctl->put = snd_ctl_elem_user_put;
 	if (access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE)
-		kctl.tlv.c = snd_ctl_elem_user_tlv;
+		kctl->tlv.c = snd_ctl_elem_user_tlv;
 
-	if (info->type < SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
-	    info->type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
-		return -EINVAL;
-	if (info->count > max_value_counts[info->type])
-		return -EINVAL;
-	if (info->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED &&
-	    info->value.enumerated.items == 0)
-		return -EINVAL;
-	elem_data_size = value_sizes[info->type] * info->count;
-	ue = kzalloc(sizeof(struct user_element) + elem_data_size, GFP_KERNEL);
-	if (ue == NULL)
-		return -ENOMEM;
+	ue = (struct user_element *)kctl->private_data;
 	ue->card = card;
 	ue->info = *info;
 	ue->info.access = 0;
@@ -1243,33 +1266,26 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	ue->elem_data_size = elem_data_size;
 	if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) {
 		err = snd_ctl_elem_init_enum_names(ue);
-		if (err < 0) {
-			kfree(ue);
-			return err;
-		}
-	}
-	kctl.private_free = snd_ctl_elem_user_free;
-	_kctl = ctl_new(&kctl, access);
-	if (_kctl == NULL) {
-		kfree(ue->priv_data);
-		kfree(ue);
-		return -ENOMEM;
+		if (err < 0)
+			goto error_allocated;
 	}
-	_kctl->private_data = ue;
 
 	/* Lock values in this user controls. */
-	for (i = 0; i < _kctl->count; i++)
-		_kctl->vd[i].owner = file;
+	for (i = 0; i < kctl->count; i++)
+		kctl->vd[i].owner = file;
 
-	err = snd_ctl_add(card, _kctl);
+	err = snd_ctl_add(card, kctl);
 	if (err < 0)
-		return err;
+		return err;	/* already freed. */
 
 	down_write(&card->controls_rwsem);
-	card->user_ctl_count += _kctl->count;
+	card->user_ctl_count += kctl->count;
 	up_write(&card->controls_rwsem);
 
 	return 0;
+error_allocated:
+	snd_ctl_free_one(kctl);
+	return err;
 }
 
 static int snd_ctl_elem_add_user(struct snd_ctl_file *file,
-- 
2.1.0