diff mbox series

ALSA: control: expand limitation on the number of user-defined control element set per card

Message ID 20210122082032.103066-1-o-takashi@sakamocchi.jp (mailing list archive)
State New, archived
Headers show
Series ALSA: control: expand limitation on the number of user-defined control element set per card | expand

Commit Message

Takashi Sakamoto Jan. 22, 2021, 8:20 a.m. UTC
ALSA control core allows usespace application to register control element
set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added
control elements are called as 'user-defined'. Currently sound card has
limitation on the number of the user-defined control element set up
to 32.

The limitation is inconvenient to drivers in ALSA firewire stack since
the drivers expect userspace applications to implement function to
control device functionalities such as mixing and routing. As the
userspace application, snd-firewire-ctl-services project starts:
https://github.com/alsa-project/snd-firewire-ctl-services/

The project supports many devices supported by ALSA firewire stack. The
limitation is mostly good since routing and mixing controls can be
represented by control element set, which includes multiple control element
with the same parameters. Nevertheless, it's actually inconvenient to
device which has many varied functionalities. For example, plugin effect
such as channel strip and reverb has many parameters. For the case, many
control elements are required to configure the parameters and control
element set cannot aggregates them for the parameters. At present, the
implementations for below models requires more control element sets
than 32:

 * snd-bebob-ctl-service
   * Apogee Ensemble (31 sets for 34 elements)
 * snd-dice-ctl-service
   * TC Electronic Konnekt 24d (78 sets for 94 elements)
   * TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
   * TC Electronic Konnekt Live (88 sets for 104 elements)
   * TC Electronic Impact Twin (70 sets for 86 elements)
   * Focusrite Liquid Saffire 56 (37 sets for 52 elements)

This commit expands the limitation according to requirement from the above
applications. As a result, userspace applications can add control element
sets up to 150 per sound card. It results in 154,200 user-defined control
elements as maximum since one control element set can include 1028 control
elements.

The new limitation is decided without comprehensive criteria to sound card.
It could be changed according to requirement from the other type of
userspace applications.

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

Comments

Takashi Iwai Jan. 22, 2021, 2:04 p.m. UTC | #1
On Fri, 22 Jan 2021 09:20:32 +0100,
Takashi Sakamoto wrote:
> 
> ALSA control core allows usespace application to register control element
> set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added
> control elements are called as 'user-defined'. Currently sound card has
> limitation on the number of the user-defined control element set up
> to 32.
> 
> The limitation is inconvenient to drivers in ALSA firewire stack since
> the drivers expect userspace applications to implement function to
> control device functionalities such as mixing and routing. As the
> userspace application, snd-firewire-ctl-services project starts:
> https://github.com/alsa-project/snd-firewire-ctl-services/
> 
> The project supports many devices supported by ALSA firewire stack. The
> limitation is mostly good since routing and mixing controls can be
> represented by control element set, which includes multiple control element
> with the same parameters. Nevertheless, it's actually inconvenient to
> device which has many varied functionalities. For example, plugin effect
> such as channel strip and reverb has many parameters. For the case, many
> control elements are required to configure the parameters and control
> element set cannot aggregates them for the parameters. At present, the
> implementations for below models requires more control element sets
> than 32:
> 
>  * snd-bebob-ctl-service
>    * Apogee Ensemble (31 sets for 34 elements)
>  * snd-dice-ctl-service
>    * TC Electronic Konnekt 24d (78 sets for 94 elements)
>    * TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
>    * TC Electronic Konnekt Live (88 sets for 104 elements)
>    * TC Electronic Impact Twin (70 sets for 86 elements)
>    * Focusrite Liquid Saffire 56 (37 sets for 52 elements)
> 
> This commit expands the limitation according to requirement from the above
> applications. As a result, userspace applications can add control element
> sets up to 150 per sound card. It results in 154,200 user-defined control
> elements as maximum since one control element set can include 1028 control
> elements.

Thinking of this change again after reading your description, I find
that a more flexible and safer approach would be to limit the number
of total elements.  That is, count the number of items in each
element, and set the max to (32 * MAX_CONTROL_COUNT).  This will keep
the same max as the current implementation can achieve, while it
allows more elements as long as they contain lower number of items.

So, something like below (totally untested).


thanks,

Takashi

--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -18,10 +18,11 @@
 #include <sound/info.h>
 #include <sound/control.h>
 
-/* max number of user-defined controls */
-#define MAX_USER_CONTROLS	32
 #define MAX_CONTROL_COUNT	1028
 
+/* max number of user-defined controls */
+#define MAX_USER_CONTROLS	(32 * MAX_CONTROL_COUNT)
+
 struct snd_kctl_ioctl {
 	struct list_head list;		/* list of all ioctls */
 	snd_kctl_ioctl_func_t fioctl;
@@ -520,6 +521,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
 	struct snd_card *card = file->card;
 	struct snd_kcontrol *kctl;
 	int idx, ret;
+	int count;
 
 	down_write(&card->controls_rwsem);
 	kctl = snd_ctl_find_id(card, id);
@@ -536,10 +538,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;
@@ -1435,18 +1438,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 			return err;
 	}
 
+	/* Check the number of elements for this userspace control. */
+	count = info->owner;
+	if (count == 0)
+		count = 1;
+
 	/*
 	 * The number of userspace controls are counted control by control,
 	 * not element by element.
 	 */
-	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
+	if (card->user_ctl_count + count > MAX_USER_CONTROLS)
 		return -ENOMEM;
 
-	/* Check the number of elements for this userspace control. */
-	count = info->owner;
-	if (count == 0)
-		count = 1;
-
 	/* Arrange access permissions if needed. */
 	access = info->access;
 	if (access == 0)
@@ -1535,7 +1538,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	 * which locks the element.
 	 */
 
-	card->user_ctl_count++;
+	card->user_ctl_count += count;
 
  unlock:
 	up_write(&card->controls_rwsem);
Takashi Sakamoto Jan. 23, 2021, 3:10 a.m. UTC | #2
Hi,

On Fri, Jan 22, 2021 at 03:04:56PM +0100, Takashi Iwai wrote:
> On Fri, 22 Jan 2021 09:20:32 +0100,
> Takashi Sakamoto wrote:
> > 
> > ALSA control core allows usespace application to register control element
> > set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added
> > control elements are called as 'user-defined'. Currently sound card has
> > limitation on the number of the user-defined control element set up
> > to 32.
> > 
> > The limitation is inconvenient to drivers in ALSA firewire stack since
> > the drivers expect userspace applications to implement function to
> > control device functionalities such as mixing and routing. As the
> > userspace application, snd-firewire-ctl-services project starts:
> > https://github.com/alsa-project/snd-firewire-ctl-services/
> > 
> > The project supports many devices supported by ALSA firewire stack. The
> > limitation is mostly good since routing and mixing controls can be
> > represented by control element set, which includes multiple control element
> > with the same parameters. Nevertheless, it's actually inconvenient to
> > device which has many varied functionalities. For example, plugin effect
> > such as channel strip and reverb has many parameters. For the case, many
> > control elements are required to configure the parameters and control
> > element set cannot aggregates them for the parameters. At present, the
> > implementations for below models requires more control element sets
> > than 32:
> > 
> >  * snd-bebob-ctl-service
> >    * Apogee Ensemble (31 sets for 34 elements)
> >  * snd-dice-ctl-service
> >    * TC Electronic Konnekt 24d (78 sets for 94 elements)
> >    * TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
> >    * TC Electronic Konnekt Live (88 sets for 104 elements)
> >    * TC Electronic Impact Twin (70 sets for 86 elements)
> >    * Focusrite Liquid Saffire 56 (37 sets for 52 elements)
> > 
> > This commit expands the limitation according to requirement from the above
> > applications. As a result, userspace applications can add control element
> > sets up to 150 per sound card. It results in 154,200 user-defined control
> > elements as maximum since one control element set can include 1028 control
> > elements.
> 
> Thinking of this change again after reading your description, I find
> that a more flexible and safer approach would be to limit the number
> of total elements.  That is, count the number of items in each
> element, and set the max to (32 * MAX_CONTROL_COUNT).  This will keep
> the same max as the current implementation can achieve, while it
> allows more elements as long as they contain lower number of items.
> 
> So, something like below (totally untested).
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -18,10 +18,11 @@
>  #include <sound/info.h>
>  #include <sound/control.h>
>  
> -/* max number of user-defined controls */
> -#define MAX_USER_CONTROLS	32
>  #define MAX_CONTROL_COUNT	1028
>  
> +/* max number of user-defined controls */
> +#define MAX_USER_CONTROLS	(32 * MAX_CONTROL_COUNT)
> +
>  struct snd_kctl_ioctl {
>  	struct list_head list;		/* list of all ioctls */
>  	snd_kctl_ioctl_func_t fioctl;
> @@ -520,6 +521,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
>  	struct snd_card *card = file->card;
>  	struct snd_kcontrol *kctl;
>  	int idx, ret;
> +	int count;
>  
>  	down_write(&card->controls_rwsem);
>  	kctl = snd_ctl_find_id(card, id);
> @@ -536,10 +538,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;
> @@ -1435,18 +1438,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  			return err;
>  	}
>  
> +	/* Check the number of elements for this userspace control. */
> +	count = info->owner;
> +	if (count == 0)
> +		count = 1;
> +
>  	/*
>  	 * The number of userspace controls are counted control by control,
>  	 * not element by element.
>  	 */
> -	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
> +	if (card->user_ctl_count + count > MAX_USER_CONTROLS)
>  		return -ENOMEM;
>  
> -	/* Check the number of elements for this userspace control. */
> -	count = info->owner;
> -	if (count == 0)
> -		count = 1;
> -
>  	/* Arrange access permissions if needed. */
>  	access = info->access;
>  	if (access == 0)
> @@ -1535,7 +1538,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
>  	 * which locks the element.
>  	 */
>  
> -	card->user_ctl_count++;
> +	card->user_ctl_count += count;
>  
>   unlock:
>  	up_write(&card->controls_rwsem);

I have no objection to any change as long as it allows the service programs
to add control elements enough for target device. However, it's unclear
what is flexible and safe in the above patch.

When adding user-defined control element set, some objects are allocated
for below structures with some variable-length members:
 * struct snd_kcontrol (in include/sound/control.h)
 * struct user_element (in sound/core/control.h)

Current implementation is to avoid too much allocation for the above
against userspace applications with bugs or mis-programming. It's
reasonable to limit the allocation according to count of added control
element set for the purpose.

On the other hand, when counting the number of added control element for
the limitation, the above becomes loose. In the worst case, the patch
allows 32,896 sets to be allocated and against comments in my previous
patch.


Regards

Takashi Sakamoto
Takashi Iwai Jan. 23, 2021, 9:10 a.m. UTC | #3
On Sat, 23 Jan 2021 04:10:25 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Fri, Jan 22, 2021 at 03:04:56PM +0100, Takashi Iwai wrote:
> > On Fri, 22 Jan 2021 09:20:32 +0100,
> > Takashi Sakamoto wrote:
> > > 
> > > ALSA control core allows usespace application to register control element
> > > set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added
> > > control elements are called as 'user-defined'. Currently sound card has
> > > limitation on the number of the user-defined control element set up
> > > to 32.
> > > 
> > > The limitation is inconvenient to drivers in ALSA firewire stack since
> > > the drivers expect userspace applications to implement function to
> > > control device functionalities such as mixing and routing. As the
> > > userspace application, snd-firewire-ctl-services project starts:
> > > https://github.com/alsa-project/snd-firewire-ctl-services/
> > > 
> > > The project supports many devices supported by ALSA firewire stack. The
> > > limitation is mostly good since routing and mixing controls can be
> > > represented by control element set, which includes multiple control element
> > > with the same parameters. Nevertheless, it's actually inconvenient to
> > > device which has many varied functionalities. For example, plugin effect
> > > such as channel strip and reverb has many parameters. For the case, many
> > > control elements are required to configure the parameters and control
> > > element set cannot aggregates them for the parameters. At present, the
> > > implementations for below models requires more control element sets
> > > than 32:
> > > 
> > >  * snd-bebob-ctl-service
> > >    * Apogee Ensemble (31 sets for 34 elements)
> > >  * snd-dice-ctl-service
> > >    * TC Electronic Konnekt 24d (78 sets for 94 elements)
> > >    * TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
> > >    * TC Electronic Konnekt Live (88 sets for 104 elements)
> > >    * TC Electronic Impact Twin (70 sets for 86 elements)
> > >    * Focusrite Liquid Saffire 56 (37 sets for 52 elements)
> > > 
> > > This commit expands the limitation according to requirement from the above
> > > applications. As a result, userspace applications can add control element
> > > sets up to 150 per sound card. It results in 154,200 user-defined control
> > > elements as maximum since one control element set can include 1028 control
> > > elements.
> > 
> > Thinking of this change again after reading your description, I find
> > that a more flexible and safer approach would be to limit the number
> > of total elements.  That is, count the number of items in each
> > element, and set the max to (32 * MAX_CONTROL_COUNT).  This will keep
> > the same max as the current implementation can achieve, while it
> > allows more elements as long as they contain lower number of items.
> > 
> > So, something like below (totally untested).
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > --- a/sound/core/control.c
> > +++ b/sound/core/control.c
> > @@ -18,10 +18,11 @@
> >  #include <sound/info.h>
> >  #include <sound/control.h>
> >  
> > -/* max number of user-defined controls */
> > -#define MAX_USER_CONTROLS	32
> >  #define MAX_CONTROL_COUNT	1028
> >  
> > +/* max number of user-defined controls */
> > +#define MAX_USER_CONTROLS	(32 * MAX_CONTROL_COUNT)
> > +
> >  struct snd_kctl_ioctl {
> >  	struct list_head list;		/* list of all ioctls */
> >  	snd_kctl_ioctl_func_t fioctl;
> > @@ -520,6 +521,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
> >  	struct snd_card *card = file->card;
> >  	struct snd_kcontrol *kctl;
> >  	int idx, ret;
> > +	int count;
> >  
> >  	down_write(&card->controls_rwsem);
> >  	kctl = snd_ctl_find_id(card, id);
> > @@ -536,10 +538,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;
> > @@ -1435,18 +1438,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> >  			return err;
> >  	}
> >  
> > +	/* Check the number of elements for this userspace control. */
> > +	count = info->owner;
> > +	if (count == 0)
> > +		count = 1;
> > +
> >  	/*
> >  	 * The number of userspace controls are counted control by control,
> >  	 * not element by element.
> >  	 */
> > -	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
> > +	if (card->user_ctl_count + count > MAX_USER_CONTROLS)
> >  		return -ENOMEM;
> >  
> > -	/* Check the number of elements for this userspace control. */
> > -	count = info->owner;
> > -	if (count == 0)
> > -		count = 1;
> > -
> >  	/* Arrange access permissions if needed. */
> >  	access = info->access;
> >  	if (access == 0)
> > @@ -1535,7 +1538,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> >  	 * which locks the element.
> >  	 */
> >  
> > -	card->user_ctl_count++;
> > +	card->user_ctl_count += count;
> >  
> >   unlock:
> >  	up_write(&card->controls_rwsem);
> 
> I have no objection to any change as long as it allows the service programs
> to add control elements enough for target device. However, it's unclear
> what is flexible and safe in the above patch.
> 
> When adding user-defined control element set, some objects are allocated
> for below structures with some variable-length members:
>  * struct snd_kcontrol (in include/sound/control.h)
>  * struct user_element (in sound/core/control.h)
> 
> Current implementation is to avoid too much allocation for the above
> against userspace applications with bugs or mis-programming. It's
> reasonable to limit the allocation according to count of added control
> element set for the purpose.
> 
> On the other hand, when counting the number of added control element for
> the limitation, the above becomes loose. In the worst case, the patch
> allows 32,896 sets to be allocated and against comments in my previous
> patch.

OK, my previous patch was too simplified (I forgot to take the
private_data into account), but the point is that what we want is to
cap the worst case memory consumption.

If I calculate correctly, user_element is 320 bytes, and the value is
up to 512 bytes for each item, and snd_kcontrol is 144 bytes, and
snd_kcontrol_volatile is 16 bytes per item.  And each element may
contain 1028 items.  So, the worst case scenario of the memory
consumption is:
  (320 + 512*1028 + 144 + 16*1024) * 32 = 17383936
That is, currently we allow 16MB at most.

By increasing MAX_USER_CONTROLS to 150, it'll become 77MB.

And, think what if you'd need to increase more in future, e.g. 512
elements.  The max consumption also increases linearly.

OTOH, imagine that we cap the memory consumption to 16MB instead of
limiting only the MAX_USER_CONTROLS.  This lets user still allow to
allocate more elements with smaller number of items (that is the
common use case).  In this way, we don't take a risk of higher memory
consumption while user can deploy the user elements more flexibly.


Takashi
Jaroslav Kysela Jan. 23, 2021, 10:25 a.m. UTC | #4
Dne 23. 01. 21 v 10:10 Takashi Iwai napsal(a):

> OTOH, imagine that we cap the memory consumption to 16MB instead of
> limiting only the MAX_USER_CONTROLS.  This lets user still allow to
> allocate more elements with smaller number of items (that is the
> common use case).  In this way, we don't take a risk of higher memory
> consumption while user can deploy the user elements more flexibly.

This sounds really reasonable!

				Thanks,
					Jaroslav
Takashi Sakamoto Jan. 24, 2021, 5:52 a.m. UTC | #5
Hi,

On Sat, Jan 23, 2021 at 10:10:20AM +0100, Takashi Iwai wrote:
> On Sat, 23 Jan 2021 04:10:25 +0100,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > On Fri, Jan 22, 2021 at 03:04:56PM +0100, Takashi Iwai wrote:
> > > On Fri, 22 Jan 2021 09:20:32 +0100,
> > > Takashi Sakamoto wrote:
> > > > 
> > > > ALSA control core allows usespace application to register control element
> > > > set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added
> > > > control elements are called as 'user-defined'. Currently sound card has
> > > > limitation on the number of the user-defined control element set up
> > > > to 32.
> > > > 
> > > > The limitation is inconvenient to drivers in ALSA firewire stack since
> > > > the drivers expect userspace applications to implement function to
> > > > control device functionalities such as mixing and routing. As the
> > > > userspace application, snd-firewire-ctl-services project starts:
> > > > https://github.com/alsa-project/snd-firewire-ctl-services/
> > > > 
> > > > The project supports many devices supported by ALSA firewire stack. The
> > > > limitation is mostly good since routing and mixing controls can be
> > > > represented by control element set, which includes multiple control element
> > > > with the same parameters. Nevertheless, it's actually inconvenient to
> > > > device which has many varied functionalities. For example, plugin effect
> > > > such as channel strip and reverb has many parameters. For the case, many
> > > > control elements are required to configure the parameters and control
> > > > element set cannot aggregates them for the parameters. At present, the
> > > > implementations for below models requires more control element sets
> > > > than 32:
> > > > 
> > > >  * snd-bebob-ctl-service
> > > >    * Apogee Ensemble (31 sets for 34 elements)
> > > >  * snd-dice-ctl-service
> > > >    * TC Electronic Konnekt 24d (78 sets for 94 elements)
> > > >    * TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
> > > >    * TC Electronic Konnekt Live (88 sets for 104 elements)
> > > >    * TC Electronic Impact Twin (70 sets for 86 elements)
> > > >    * Focusrite Liquid Saffire 56 (37 sets for 52 elements)
> > > > 
> > > > This commit expands the limitation according to requirement from the above
> > > > applications. As a result, userspace applications can add control element
> > > > sets up to 150 per sound card. It results in 154,200 user-defined control
> > > > elements as maximum since one control element set can include 1028 control
> > > > elements.
> > > 
> > > Thinking of this change again after reading your description, I find
> > > that a more flexible and safer approach would be to limit the number
> > > of total elements.  That is, count the number of items in each
> > > element, and set the max to (32 * MAX_CONTROL_COUNT).  This will keep
> > > the same max as the current implementation can achieve, while it
> > > allows more elements as long as they contain lower number of items.
> > > 
> > > So, something like below (totally untested).
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > --- a/sound/core/control.c
> > > +++ b/sound/core/control.c
> > > @@ -18,10 +18,11 @@
> > >  #include <sound/info.h>
> > >  #include <sound/control.h>
> > >  
> > > -/* max number of user-defined controls */
> > > -#define MAX_USER_CONTROLS	32
> > >  #define MAX_CONTROL_COUNT	1028
> > >  
> > > +/* max number of user-defined controls */
> > > +#define MAX_USER_CONTROLS	(32 * MAX_CONTROL_COUNT)
> > > +
> > >  struct snd_kctl_ioctl {
> > >  	struct list_head list;		/* list of all ioctls */
> > >  	snd_kctl_ioctl_func_t fioctl;
> > > @@ -520,6 +521,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
> > >  	struct snd_card *card = file->card;
> > >  	struct snd_kcontrol *kctl;
> > >  	int idx, ret;
> > > +	int count;
> > >  
> > >  	down_write(&card->controls_rwsem);
> > >  	kctl = snd_ctl_find_id(card, id);
> > > @@ -536,10 +538,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;
> > > @@ -1435,18 +1438,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> > >  			return err;
> > >  	}
> > >  
> > > +	/* Check the number of elements for this userspace control. */
> > > +	count = info->owner;
> > > +	if (count == 0)
> > > +		count = 1;
> > > +
> > >  	/*
> > >  	 * The number of userspace controls are counted control by control,
> > >  	 * not element by element.
> > >  	 */
> > > -	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
> > > +	if (card->user_ctl_count + count > MAX_USER_CONTROLS)
> > >  		return -ENOMEM;
> > >  
> > > -	/* Check the number of elements for this userspace control. */
> > > -	count = info->owner;
> > > -	if (count == 0)
> > > -		count = 1;
> > > -
> > >  	/* Arrange access permissions if needed. */
> > >  	access = info->access;
> > >  	if (access == 0)
> > > @@ -1535,7 +1538,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> > >  	 * which locks the element.
> > >  	 */
> > >  
> > > -	card->user_ctl_count++;
> > > +	card->user_ctl_count += count;
> > >  
> > >   unlock:
> > >  	up_write(&card->controls_rwsem);
> > 
> > I have no objection to any change as long as it allows the service programs
> > to add control elements enough for target device. However, it's unclear
> > what is flexible and safe in the above patch.
> > 
> > When adding user-defined control element set, some objects are allocated
> > for below structures with some variable-length members:
> >  * struct snd_kcontrol (in include/sound/control.h)
> >  * struct user_element (in sound/core/control.h)
> > 
> > Current implementation is to avoid too much allocation for the above
> > against userspace applications with bugs or mis-programming. It's
> > reasonable to limit the allocation according to count of added control
> > element set for the purpose.
> > 
> > On the other hand, when counting the number of added control element for
> > the limitation, the above becomes loose. In the worst case, the patch
> > allows 32,896 sets to be allocated and against comments in my previous
> > patch.
> 
> OK, my previous patch was too simplified (I forgot to take the
> private_data into account), but the point is that what we want is to
> cap the worst case memory consumption.
> 
> If I calculate correctly, user_element is 320 bytes, and the value is
> up to 512 bytes for each item, and snd_kcontrol is 144 bytes, and
> snd_kcontrol_volatile is 16 bytes per item.  And each element may
> contain 1028 items.  So, the worst case scenario of the memory
> consumption is:
>   (320 + 512*1028 + 144 + 16*1024) * 32 = 17383936
> That is, currently we allow 16MB at most.
> 
> By increasing MAX_USER_CONTROLS to 150, it'll become 77MB.
> 
> And, think what if you'd need to increase more in future, e.g. 512
> elements.  The max consumption also increases linearly.
> 
> OTOH, imagine that we cap the memory consumption to 16MB instead of
> limiting only the MAX_USER_CONTROLS.  This lets user still allow to
> allocate more elements with smaller number of items (that is the
> common use case).  In this way, we don't take a risk of higher memory
> consumption while user can deploy the user elements more flexibly.

The memory object for data of Type-Length-Value style is underestimate in
your calculation for the worst case. For user-defined control element set,
the size is (1024 * 128) per control element set[1].

Of cource, it's possible to judge that usual userspace application don't
use data of Type-Length-Value style so much. However, we are assuming
the worst case now.

```
Objects linearly increased according to the number of user-defined control
element sets:
 * struct snd_kcontrol (144 bytes)
 * struct user_element (320 bytes)
 * data of TLV ((1024 * 128) bytes as maximum)

Objects linearly increased according to the number of control elements:
 * data of values (max. 1024 bytes in System V ABI with LP64 data type)
 * data of snd_kcontrol_volatile (16 bytes)

Memory consumption under current limitation:
 (144 + 320 + 1024 * 128) * 32 + (1024 + 16) * 1028 = 5,278,272

Scenario for the worst case when appying the patch:
* adding 32,896 control element sets
 * for elements: (1024 + 16) * 1028 * 32 = 34,211,840
 * for element sets: (144 + 320 + 1024 * 128) * 32896 = 4,327,008,256

Scenario for the worst case when applying my patch:
* adding 150 control element sets
 * for elements: (1024 + 16) * 1028 * 150 = 160,368,000
 * for element sets: (144 + 320 + 1024 * 128) * 150 = 19,730,400
```

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n1299


Regards

Takashi Sakamoto
Takashi Iwai Jan. 24, 2021, 7:49 a.m. UTC | #6
On Sun, 24 Jan 2021 06:52:25 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Sat, Jan 23, 2021 at 10:10:20AM +0100, Takashi Iwai wrote:
> > On Sat, 23 Jan 2021 04:10:25 +0100,
> > Takashi Sakamoto wrote:
> > > 
> > > Hi,
> > > 
> > > On Fri, Jan 22, 2021 at 03:04:56PM +0100, Takashi Iwai wrote:
> > > > On Fri, 22 Jan 2021 09:20:32 +0100,
> > > > Takashi Sakamoto wrote:
> > > > > 
> > > > > ALSA control core allows usespace application to register control element
> > > > > set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added
> > > > > control elements are called as 'user-defined'. Currently sound card has
> > > > > limitation on the number of the user-defined control element set up
> > > > > to 32.
> > > > > 
> > > > > The limitation is inconvenient to drivers in ALSA firewire stack since
> > > > > the drivers expect userspace applications to implement function to
> > > > > control device functionalities such as mixing and routing. As the
> > > > > userspace application, snd-firewire-ctl-services project starts:
> > > > > https://github.com/alsa-project/snd-firewire-ctl-services/
> > > > > 
> > > > > The project supports many devices supported by ALSA firewire stack. The
> > > > > limitation is mostly good since routing and mixing controls can be
> > > > > represented by control element set, which includes multiple control element
> > > > > with the same parameters. Nevertheless, it's actually inconvenient to
> > > > > device which has many varied functionalities. For example, plugin effect
> > > > > such as channel strip and reverb has many parameters. For the case, many
> > > > > control elements are required to configure the parameters and control
> > > > > element set cannot aggregates them for the parameters. At present, the
> > > > > implementations for below models requires more control element sets
> > > > > than 32:
> > > > > 
> > > > >  * snd-bebob-ctl-service
> > > > >    * Apogee Ensemble (31 sets for 34 elements)
> > > > >  * snd-dice-ctl-service
> > > > >    * TC Electronic Konnekt 24d (78 sets for 94 elements)
> > > > >    * TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
> > > > >    * TC Electronic Konnekt Live (88 sets for 104 elements)
> > > > >    * TC Electronic Impact Twin (70 sets for 86 elements)
> > > > >    * Focusrite Liquid Saffire 56 (37 sets for 52 elements)
> > > > > 
> > > > > This commit expands the limitation according to requirement from the above
> > > > > applications. As a result, userspace applications can add control element
> > > > > sets up to 150 per sound card. It results in 154,200 user-defined control
> > > > > elements as maximum since one control element set can include 1028 control
> > > > > elements.
> > > > 
> > > > Thinking of this change again after reading your description, I find
> > > > that a more flexible and safer approach would be to limit the number
> > > > of total elements.  That is, count the number of items in each
> > > > element, and set the max to (32 * MAX_CONTROL_COUNT).  This will keep
> > > > the same max as the current implementation can achieve, while it
> > > > allows more elements as long as they contain lower number of items.
> > > > 
> > > > So, something like below (totally untested).
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > > 
> > > > --- a/sound/core/control.c
> > > > +++ b/sound/core/control.c
> > > > @@ -18,10 +18,11 @@
> > > >  #include <sound/info.h>
> > > >  #include <sound/control.h>
> > > >  
> > > > -/* max number of user-defined controls */
> > > > -#define MAX_USER_CONTROLS	32
> > > >  #define MAX_CONTROL_COUNT	1028
> > > >  
> > > > +/* max number of user-defined controls */
> > > > +#define MAX_USER_CONTROLS	(32 * MAX_CONTROL_COUNT)
> > > > +
> > > >  struct snd_kctl_ioctl {
> > > >  	struct list_head list;		/* list of all ioctls */
> > > >  	snd_kctl_ioctl_func_t fioctl;
> > > > @@ -520,6 +521,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
> > > >  	struct snd_card *card = file->card;
> > > >  	struct snd_kcontrol *kctl;
> > > >  	int idx, ret;
> > > > +	int count;
> > > >  
> > > >  	down_write(&card->controls_rwsem);
> > > >  	kctl = snd_ctl_find_id(card, id);
> > > > @@ -536,10 +538,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;
> > > > @@ -1435,18 +1438,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> > > >  			return err;
> > > >  	}
> > > >  
> > > > +	/* Check the number of elements for this userspace control. */
> > > > +	count = info->owner;
> > > > +	if (count == 0)
> > > > +		count = 1;
> > > > +
> > > >  	/*
> > > >  	 * The number of userspace controls are counted control by control,
> > > >  	 * not element by element.
> > > >  	 */
> > > > -	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
> > > > +	if (card->user_ctl_count + count > MAX_USER_CONTROLS)
> > > >  		return -ENOMEM;
> > > >  
> > > > -	/* Check the number of elements for this userspace control. */
> > > > -	count = info->owner;
> > > > -	if (count == 0)
> > > > -		count = 1;
> > > > -
> > > >  	/* Arrange access permissions if needed. */
> > > >  	access = info->access;
> > > >  	if (access == 0)
> > > > @@ -1535,7 +1538,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> > > >  	 * which locks the element.
> > > >  	 */
> > > >  
> > > > -	card->user_ctl_count++;
> > > > +	card->user_ctl_count += count;
> > > >  
> > > >   unlock:
> > > >  	up_write(&card->controls_rwsem);
> > > 
> > > I have no objection to any change as long as it allows the service programs
> > > to add control elements enough for target device. However, it's unclear
> > > what is flexible and safe in the above patch.
> > > 
> > > When adding user-defined control element set, some objects are allocated
> > > for below structures with some variable-length members:
> > >  * struct snd_kcontrol (in include/sound/control.h)
> > >  * struct user_element (in sound/core/control.h)
> > > 
> > > Current implementation is to avoid too much allocation for the above
> > > against userspace applications with bugs or mis-programming. It's
> > > reasonable to limit the allocation according to count of added control
> > > element set for the purpose.
> > > 
> > > On the other hand, when counting the number of added control element for
> > > the limitation, the above becomes loose. In the worst case, the patch
> > > allows 32,896 sets to be allocated and against comments in my previous
> > > patch.
> > 
> > OK, my previous patch was too simplified (I forgot to take the
> > private_data into account), but the point is that what we want is to
> > cap the worst case memory consumption.
> > 
> > If I calculate correctly, user_element is 320 bytes, and the value is
> > up to 512 bytes for each item, and snd_kcontrol is 144 bytes, and
> > snd_kcontrol_volatile is 16 bytes per item.  And each element may
> > contain 1028 items.  So, the worst case scenario of the memory
> > consumption is:
> >   (320 + 512*1028 + 144 + 16*1024) * 32 = 17383936
> > That is, currently we allow 16MB at most.
> > 
> > By increasing MAX_USER_CONTROLS to 150, it'll become 77MB.
> > 
> > And, think what if you'd need to increase more in future, e.g. 512
> > elements.  The max consumption also increases linearly.
> > 
> > OTOH, imagine that we cap the memory consumption to 16MB instead of
> > limiting only the MAX_USER_CONTROLS.  This lets user still allow to
> > allocate more elements with smaller number of items (that is the
> > common use case).  In this way, we don't take a risk of higher memory
> > consumption while user can deploy the user elements more flexibly.
> 
> The memory object for data of Type-Length-Value style is underestimate in
> your calculation for the worst case. For user-defined control element set,
> the size is (1024 * 128) per control element set[1].
> 
> Of cource, it's possible to judge that usual userspace application don't
> use data of Type-Length-Value style so much. However, we are assuming
> the worst case now.

Right, that should be taken into account.

> ```
> Objects linearly increased according to the number of user-defined control
> element sets:
>  * struct snd_kcontrol (144 bytes)
>  * struct user_element (320 bytes)
>  * data of TLV ((1024 * 128) bytes as maximum)
> 
> Objects linearly increased according to the number of control elements:
>  * data of values (max. 1024 bytes in System V ABI with LP64 data type)
>  * data of snd_kcontrol_volatile (16 bytes)
> 
> Memory consumption under current limitation:
>  (144 + 320 + 1024 * 128) * 32 + (1024 + 16) * 1028 = 5,278,272
> 
> Scenario for the worst case when appying the patch:
> * adding 32,896 control element sets
>  * for elements: (1024 + 16) * 1028 * 32 = 34,211,840
>  * for element sets: (144 + 320 + 1024 * 128) * 32896 = 4,327,008,256

Forget my previous patch.  The code change suggested there is
obviously not sufficient, but you need only consider about its idea.

> Scenario for the worst case when applying my patch:
> * adding 150 control element sets
>  * for elements: (1024 + 16) * 1028 * 150 = 160,368,000
>  * for element sets: (144 + 320 + 1024 * 128) * 150 = 19,730,400
> ```

See that your patch increases the memory consumption so much?
That's my point.

We may give more user elements without increasing the worst-case
memory footprint in normal scenarios.  So scratch MAX_USER_CONTROLS
but count each user element's memory consumption and define its total
limit instead.


Takashi
Takashi Sakamoto Jan. 25, 2021, 12:56 a.m. UTC | #7
Hi,

On Sun, Jan 24, 2021 at 08:49:11AM +0100, Takashi Iwai wrote:
> On Sun, 24 Jan 2021 06:52:25 +0100,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > On Sat, Jan 23, 2021 at 10:10:20AM +0100, Takashi Iwai wrote:
> > > On Sat, 23 Jan 2021 04:10:25 +0100,
> > > Takashi Sakamoto wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Fri, Jan 22, 2021 at 03:04:56PM +0100, Takashi Iwai wrote:
> > > > > On Fri, 22 Jan 2021 09:20:32 +0100,
> > > > > Takashi Sakamoto wrote:
> > > > > > 
> > > > > > ALSA control core allows usespace application to register control element
> > > > > > set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added
> > > > > > control elements are called as 'user-defined'. Currently sound card has
> > > > > > limitation on the number of the user-defined control element set up
> > > > > > to 32.
> > > > > > 
> > > > > > The limitation is inconvenient to drivers in ALSA firewire stack since
> > > > > > the drivers expect userspace applications to implement function to
> > > > > > control device functionalities such as mixing and routing. As the
> > > > > > userspace application, snd-firewire-ctl-services project starts:
> > > > > > https://github.com/alsa-project/snd-firewire-ctl-services/
> > > > > > 
> > > > > > The project supports many devices supported by ALSA firewire stack. The
> > > > > > limitation is mostly good since routing and mixing controls can be
> > > > > > represented by control element set, which includes multiple control element
> > > > > > with the same parameters. Nevertheless, it's actually inconvenient to
> > > > > > device which has many varied functionalities. For example, plugin effect
> > > > > > such as channel strip and reverb has many parameters. For the case, many
> > > > > > control elements are required to configure the parameters and control
> > > > > > element set cannot aggregates them for the parameters. At present, the
> > > > > > implementations for below models requires more control element sets
> > > > > > than 32:
> > > > > > 
> > > > > >  * snd-bebob-ctl-service
> > > > > >    * Apogee Ensemble (31 sets for 34 elements)
> > > > > >  * snd-dice-ctl-service
> > > > > >    * TC Electronic Konnekt 24d (78 sets for 94 elements)
> > > > > >    * TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
> > > > > >    * TC Electronic Konnekt Live (88 sets for 104 elements)
> > > > > >    * TC Electronic Impact Twin (70 sets for 86 elements)
> > > > > >    * Focusrite Liquid Saffire 56 (37 sets for 52 elements)
> > > > > > 
> > > > > > This commit expands the limitation according to requirement from the above
> > > > > > applications. As a result, userspace applications can add control element
> > > > > > sets up to 150 per sound card. It results in 154,200 user-defined control
> > > > > > elements as maximum since one control element set can include 1028 control
> > > > > > elements.
> > > > > 
> > > > > Thinking of this change again after reading your description, I find
> > > > > that a more flexible and safer approach would be to limit the number
> > > > > of total elements.  That is, count the number of items in each
> > > > > element, and set the max to (32 * MAX_CONTROL_COUNT).  This will keep
> > > > > the same max as the current implementation can achieve, while it
> > > > > allows more elements as long as they contain lower number of items.
> > > > > 
> > > > > So, something like below (totally untested).
> > > > > 
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > Takashi
> > > > > 
> > > > > --- a/sound/core/control.c
> > > > > +++ b/sound/core/control.c
> > > > > @@ -18,10 +18,11 @@
> > > > >  #include <sound/info.h>
> > > > >  #include <sound/control.h>
> > > > >  
> > > > > -/* max number of user-defined controls */
> > > > > -#define MAX_USER_CONTROLS	32
> > > > >  #define MAX_CONTROL_COUNT	1028
> > > > >  
> > > > > +/* max number of user-defined controls */
> > > > > +#define MAX_USER_CONTROLS	(32 * MAX_CONTROL_COUNT)
> > > > > +
> > > > >  struct snd_kctl_ioctl {
> > > > >  	struct list_head list;		/* list of all ioctls */
> > > > >  	snd_kctl_ioctl_func_t fioctl;
> > > > > @@ -520,6 +521,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
> > > > >  	struct snd_card *card = file->card;
> > > > >  	struct snd_kcontrol *kctl;
> > > > >  	int idx, ret;
> > > > > +	int count;
> > > > >  
> > > > >  	down_write(&card->controls_rwsem);
> > > > >  	kctl = snd_ctl_find_id(card, id);
> > > > > @@ -536,10 +538,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;
> > > > > @@ -1435,18 +1438,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> > > > >  			return err;
> > > > >  	}
> > > > >  
> > > > > +	/* Check the number of elements for this userspace control. */
> > > > > +	count = info->owner;
> > > > > +	if (count == 0)
> > > > > +		count = 1;
> > > > > +
> > > > >  	/*
> > > > >  	 * The number of userspace controls are counted control by control,
> > > > >  	 * not element by element.
> > > > >  	 */
> > > > > -	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
> > > > > +	if (card->user_ctl_count + count > MAX_USER_CONTROLS)
> > > > >  		return -ENOMEM;
> > > > >  
> > > > > -	/* Check the number of elements for this userspace control. */
> > > > > -	count = info->owner;
> > > > > -	if (count == 0)
> > > > > -		count = 1;
> > > > > -
> > > > >  	/* Arrange access permissions if needed. */
> > > > >  	access = info->access;
> > > > >  	if (access == 0)
> > > > > @@ -1535,7 +1538,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> > > > >  	 * which locks the element.
> > > > >  	 */
> > > > >  
> > > > > -	card->user_ctl_count++;
> > > > > +	card->user_ctl_count += count;
> > > > >  
> > > > >   unlock:
> > > > >  	up_write(&card->controls_rwsem);
> > > > 
> > > > I have no objection to any change as long as it allows the service programs
> > > > to add control elements enough for target device. However, it's unclear
> > > > what is flexible and safe in the above patch.
> > > > 
> > > > When adding user-defined control element set, some objects are allocated
> > > > for below structures with some variable-length members:
> > > >  * struct snd_kcontrol (in include/sound/control.h)
> > > >  * struct user_element (in sound/core/control.h)
> > > > 
> > > > Current implementation is to avoid too much allocation for the above
> > > > against userspace applications with bugs or mis-programming. It's
> > > > reasonable to limit the allocation according to count of added control
> > > > element set for the purpose.
> > > > 
> > > > On the other hand, when counting the number of added control element for
> > > > the limitation, the above becomes loose. In the worst case, the patch
> > > > allows 32,896 sets to be allocated and against comments in my previous
> > > > patch.
> > > 
> > > OK, my previous patch was too simplified (I forgot to take the
> > > private_data into account), but the point is that what we want is to
> > > cap the worst case memory consumption.
> > > 
> > > If I calculate correctly, user_element is 320 bytes, and the value is
> > > up to 512 bytes for each item, and snd_kcontrol is 144 bytes, and
> > > snd_kcontrol_volatile is 16 bytes per item.  And each element may
> > > contain 1028 items.  So, the worst case scenario of the memory
> > > consumption is:
> > >   (320 + 512*1028 + 144 + 16*1024) * 32 = 17383936
> > > That is, currently we allow 16MB at most.
> > > 
> > > By increasing MAX_USER_CONTROLS to 150, it'll become 77MB.
> > > 
> > > And, think what if you'd need to increase more in future, e.g. 512
> > > elements.  The max consumption also increases linearly.
> > > 
> > > OTOH, imagine that we cap the memory consumption to 16MB instead of
> > > limiting only the MAX_USER_CONTROLS.  This lets user still allow to
> > > allocate more elements with smaller number of items (that is the
> > > common use case).  In this way, we don't take a risk of higher memory
> > > consumption while user can deploy the user elements more flexibly.
> > 
> > The memory object for data of Type-Length-Value style is underestimate in
> > your calculation for the worst case. For user-defined control element set,
> > the size is (1024 * 128) per control element set[1].
> > 
> > Of cource, it's possible to judge that usual userspace application don't
> > use data of Type-Length-Value style so much. However, we are assuming
> > the worst case now.
> 
> Right, that should be taken into account.
> 
> > ```
> > Objects linearly increased according to the number of user-defined control
> > element sets:
> >  * struct snd_kcontrol (144 bytes)
> >  * struct user_element (320 bytes)
> >  * data of TLV ((1024 * 128) bytes as maximum)
> > 
> > Objects linearly increased according to the number of control elements:
> >  * data of values (max. 1024 bytes in System V ABI with LP64 data type)
> >  * data of snd_kcontrol_volatile (16 bytes)
> > 
> > Memory consumption under current limitation:
> >  (144 + 320 + 1024 * 128) * 32 + (1024 + 16) * 1028 = 5,278,272
> > 
> > Scenario for the worst case when appying the patch:
> > * adding 32,896 control element sets
> >  * for elements: (1024 + 16) * 1028 * 32 = 34,211,840
> >  * for element sets: (144 + 320 + 1024 * 128) * 32896 = 4,327,008,256
> 
> Forget my previous patch.  The code change suggested there is
> obviously not sufficient, but you need only consider about its idea.
> 
> > Scenario for the worst case when applying my patch:
> > * adding 150 control element sets
> >  * for elements: (1024 + 16) * 1028 * 150 = 160,368,000
> >  * for element sets: (144 + 320 + 1024 * 128) * 150 = 19,730,400
> > ```
> 
> See that your patch increases the memory consumption so much?
> That's my point.
> 
> We may give more user elements without increasing the worst-case
> memory footprint in normal scenarios.  So scratch MAX_USER_CONTROLS
> but count each user element's memory consumption and define its total
> limit instead.

When preparing for safety, we should not assume anything as long as the
worst case is clear, in my opinion.

The initial maximum memory consumption is 5.2 MB. With limitation of
control element by control element, it's so large due to the data of TLV
per control element set. With limitation of control element set by control
element set, it's 19.7 MB and deterministic (in my patch).

It's can be investigated to arrange relevant parameters, but it
certainly brings regression to the old-versioned userspace applications.
It would not be better to change the size of data of TLV and the number
of control elements in control element set.

At first place, my request is to relax limitation according to userspace
application. Expansion of memory consumption is unavoidable. The linear
increase of memory consumption about which you mentioned is not so worse
as a result of compromise to the above.


Regards

Takashi Sakamoto
Takashi Iwai Jan. 25, 2021, 7:09 a.m. UTC | #8
On Mon, 25 Jan 2021 01:56:19 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Sun, Jan 24, 2021 at 08:49:11AM +0100, Takashi Iwai wrote:
> > On Sun, 24 Jan 2021 06:52:25 +0100,
> > Takashi Sakamoto wrote:
> > > 
> > > Hi,
> > > 
> > > On Sat, Jan 23, 2021 at 10:10:20AM +0100, Takashi Iwai wrote:
> > > > On Sat, 23 Jan 2021 04:10:25 +0100,
> > > > Takashi Sakamoto wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > On Fri, Jan 22, 2021 at 03:04:56PM +0100, Takashi Iwai wrote:
> > > > > > On Fri, 22 Jan 2021 09:20:32 +0100,
> > > > > > Takashi Sakamoto wrote:
> > > > > > > 
> > > > > > > ALSA control core allows usespace application to register control element
> > > > > > > set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added
> > > > > > > control elements are called as 'user-defined'. Currently sound card has
> > > > > > > limitation on the number of the user-defined control element set up
> > > > > > > to 32.
> > > > > > > 
> > > > > > > The limitation is inconvenient to drivers in ALSA firewire stack since
> > > > > > > the drivers expect userspace applications to implement function to
> > > > > > > control device functionalities such as mixing and routing. As the
> > > > > > > userspace application, snd-firewire-ctl-services project starts:
> > > > > > > https://github.com/alsa-project/snd-firewire-ctl-services/
> > > > > > > 
> > > > > > > The project supports many devices supported by ALSA firewire stack. The
> > > > > > > limitation is mostly good since routing and mixing controls can be
> > > > > > > represented by control element set, which includes multiple control element
> > > > > > > with the same parameters. Nevertheless, it's actually inconvenient to
> > > > > > > device which has many varied functionalities. For example, plugin effect
> > > > > > > such as channel strip and reverb has many parameters. For the case, many
> > > > > > > control elements are required to configure the parameters and control
> > > > > > > element set cannot aggregates them for the parameters. At present, the
> > > > > > > implementations for below models requires more control element sets
> > > > > > > than 32:
> > > > > > > 
> > > > > > >  * snd-bebob-ctl-service
> > > > > > >    * Apogee Ensemble (31 sets for 34 elements)
> > > > > > >  * snd-dice-ctl-service
> > > > > > >    * TC Electronic Konnekt 24d (78 sets for 94 elements)
> > > > > > >    * TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
> > > > > > >    * TC Electronic Konnekt Live (88 sets for 104 elements)
> > > > > > >    * TC Electronic Impact Twin (70 sets for 86 elements)
> > > > > > >    * Focusrite Liquid Saffire 56 (37 sets for 52 elements)
> > > > > > > 
> > > > > > > This commit expands the limitation according to requirement from the above
> > > > > > > applications. As a result, userspace applications can add control element
> > > > > > > sets up to 150 per sound card. It results in 154,200 user-defined control
> > > > > > > elements as maximum since one control element set can include 1028 control
> > > > > > > elements.
> > > > > > 
> > > > > > Thinking of this change again after reading your description, I find
> > > > > > that a more flexible and safer approach would be to limit the number
> > > > > > of total elements.  That is, count the number of items in each
> > > > > > element, and set the max to (32 * MAX_CONTROL_COUNT).  This will keep
> > > > > > the same max as the current implementation can achieve, while it
> > > > > > allows more elements as long as they contain lower number of items.
> > > > > > 
> > > > > > So, something like below (totally untested).
> > > > > > 
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > Takashi
> > > > > > 
> > > > > > --- a/sound/core/control.c
> > > > > > +++ b/sound/core/control.c
> > > > > > @@ -18,10 +18,11 @@
> > > > > >  #include <sound/info.h>
> > > > > >  #include <sound/control.h>
> > > > > >  
> > > > > > -/* max number of user-defined controls */
> > > > > > -#define MAX_USER_CONTROLS	32
> > > > > >  #define MAX_CONTROL_COUNT	1028
> > > > > >  
> > > > > > +/* max number of user-defined controls */
> > > > > > +#define MAX_USER_CONTROLS	(32 * MAX_CONTROL_COUNT)
> > > > > > +
> > > > > >  struct snd_kctl_ioctl {
> > > > > >  	struct list_head list;		/* list of all ioctls */
> > > > > >  	snd_kctl_ioctl_func_t fioctl;
> > > > > > @@ -520,6 +521,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
> > > > > >  	struct snd_card *card = file->card;
> > > > > >  	struct snd_kcontrol *kctl;
> > > > > >  	int idx, ret;
> > > > > > +	int count;
> > > > > >  
> > > > > >  	down_write(&card->controls_rwsem);
> > > > > >  	kctl = snd_ctl_find_id(card, id);
> > > > > > @@ -536,10 +538,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;
> > > > > > @@ -1435,18 +1438,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> > > > > >  			return err;
> > > > > >  	}
> > > > > >  
> > > > > > +	/* Check the number of elements for this userspace control. */
> > > > > > +	count = info->owner;
> > > > > > +	if (count == 0)
> > > > > > +		count = 1;
> > > > > > +
> > > > > >  	/*
> > > > > >  	 * The number of userspace controls are counted control by control,
> > > > > >  	 * not element by element.
> > > > > >  	 */
> > > > > > -	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
> > > > > > +	if (card->user_ctl_count + count > MAX_USER_CONTROLS)
> > > > > >  		return -ENOMEM;
> > > > > >  
> > > > > > -	/* Check the number of elements for this userspace control. */
> > > > > > -	count = info->owner;
> > > > > > -	if (count == 0)
> > > > > > -		count = 1;
> > > > > > -
> > > > > >  	/* Arrange access permissions if needed. */
> > > > > >  	access = info->access;
> > > > > >  	if (access == 0)
> > > > > > @@ -1535,7 +1538,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
> > > > > >  	 * which locks the element.
> > > > > >  	 */
> > > > > >  
> > > > > > -	card->user_ctl_count++;
> > > > > > +	card->user_ctl_count += count;
> > > > > >  
> > > > > >   unlock:
> > > > > >  	up_write(&card->controls_rwsem);
> > > > > 
> > > > > I have no objection to any change as long as it allows the service programs
> > > > > to add control elements enough for target device. However, it's unclear
> > > > > what is flexible and safe in the above patch.
> > > > > 
> > > > > When adding user-defined control element set, some objects are allocated
> > > > > for below structures with some variable-length members:
> > > > >  * struct snd_kcontrol (in include/sound/control.h)
> > > > >  * struct user_element (in sound/core/control.h)
> > > > > 
> > > > > Current implementation is to avoid too much allocation for the above
> > > > > against userspace applications with bugs or mis-programming. It's
> > > > > reasonable to limit the allocation according to count of added control
> > > > > element set for the purpose.
> > > > > 
> > > > > On the other hand, when counting the number of added control element for
> > > > > the limitation, the above becomes loose. In the worst case, the patch
> > > > > allows 32,896 sets to be allocated and against comments in my previous
> > > > > patch.
> > > > 
> > > > OK, my previous patch was too simplified (I forgot to take the
> > > > private_data into account), but the point is that what we want is to
> > > > cap the worst case memory consumption.
> > > > 
> > > > If I calculate correctly, user_element is 320 bytes, and the value is
> > > > up to 512 bytes for each item, and snd_kcontrol is 144 bytes, and
> > > > snd_kcontrol_volatile is 16 bytes per item.  And each element may
> > > > contain 1028 items.  So, the worst case scenario of the memory
> > > > consumption is:
> > > >   (320 + 512*1028 + 144 + 16*1024) * 32 = 17383936
> > > > That is, currently we allow 16MB at most.
> > > > 
> > > > By increasing MAX_USER_CONTROLS to 150, it'll become 77MB.
> > > > 
> > > > And, think what if you'd need to increase more in future, e.g. 512
> > > > elements.  The max consumption also increases linearly.
> > > > 
> > > > OTOH, imagine that we cap the memory consumption to 16MB instead of
> > > > limiting only the MAX_USER_CONTROLS.  This lets user still allow to
> > > > allocate more elements with smaller number of items (that is the
> > > > common use case).  In this way, we don't take a risk of higher memory
> > > > consumption while user can deploy the user elements more flexibly.
> > > 
> > > The memory object for data of Type-Length-Value style is underestimate in
> > > your calculation for the worst case. For user-defined control element set,
> > > the size is (1024 * 128) per control element set[1].
> > > 
> > > Of cource, it's possible to judge that usual userspace application don't
> > > use data of Type-Length-Value style so much. However, we are assuming
> > > the worst case now.
> > 
> > Right, that should be taken into account.
> > 
> > > ```
> > > Objects linearly increased according to the number of user-defined control
> > > element sets:
> > >  * struct snd_kcontrol (144 bytes)
> > >  * struct user_element (320 bytes)
> > >  * data of TLV ((1024 * 128) bytes as maximum)
> > > 
> > > Objects linearly increased according to the number of control elements:
> > >  * data of values (max. 1024 bytes in System V ABI with LP64 data type)
> > >  * data of snd_kcontrol_volatile (16 bytes)
> > > 
> > > Memory consumption under current limitation:
> > >  (144 + 320 + 1024 * 128) * 32 + (1024 + 16) * 1028 = 5,278,272
> > > 
> > > Scenario for the worst case when appying the patch:
> > > * adding 32,896 control element sets
> > >  * for elements: (1024 + 16) * 1028 * 32 = 34,211,840
> > >  * for element sets: (144 + 320 + 1024 * 128) * 32896 = 4,327,008,256
> > 
> > Forget my previous patch.  The code change suggested there is
> > obviously not sufficient, but you need only consider about its idea.
> > 
> > > Scenario for the worst case when applying my patch:
> > > * adding 150 control element sets
> > >  * for elements: (1024 + 16) * 1028 * 150 = 160,368,000
> > >  * for element sets: (144 + 320 + 1024 * 128) * 150 = 19,730,400
> > > ```
> > 
> > See that your patch increases the memory consumption so much?
> > That's my point.
> > 
> > We may give more user elements without increasing the worst-case
> > memory footprint in normal scenarios.  So scratch MAX_USER_CONTROLS
> > but count each user element's memory consumption and define its total
> > limit instead.
> 
> When preparing for safety, we should not assume anything as long as the
> worst case is clear, in my opinion.
> 
> The initial maximum memory consumption is 5.2 MB. With limitation of
> control element by control element, it's so large due to the data of TLV
> per control element set. With limitation of control element set by control
> element set, it's 19.7 MB and deterministic (in my patch).
> 
> It's can be investigated to arrange relevant parameters, but it
> certainly brings regression to the old-versioned userspace applications.
> It would not be better to change the size of data of TLV and the number
> of control elements in control element set.
> 
> At first place, my request is to relax limitation according to userspace
> application. Expansion of memory consumption is unavoidable. The linear
> increase of memory consumption about which you mentioned is not so worse
> as a result of compromise to the above.

Hm?  We really care only the *max* memory consumption case (i.e. the
worst case scenario), not about the memory consumption in your
particular use case.  And this can be certainly accountable.

Whether to calculate the amount of TLV data separately from the main
user elements is another thing to be discussed, but in either way, we
can know the data size at allocation time, and we can also calculate
the theoretical max of the current implementation beforehand.


Takashi
Takashi Iwai Jan. 26, 2021, 3:57 p.m. UTC | #9
On Mon, 25 Jan 2021 08:09:10 +0100,
Takashi Iwai wrote:
> 
> On Mon, 25 Jan 2021 01:56:19 +0100,
> Takashi Sakamoto wrote:
> > 
> > When preparing for safety, we should not assume anything as long as the
> > worst case is clear, in my opinion.
> > 
> > The initial maximum memory consumption is 5.2 MB. With limitation of
> > control element by control element, it's so large due to the data of TLV
> > per control element set. With limitation of control element set by control
> > element set, it's 19.7 MB and deterministic (in my patch).
> > 
> > It's can be investigated to arrange relevant parameters, but it
> > certainly brings regression to the old-versioned userspace applications.
> > It would not be better to change the size of data of TLV and the number
> > of control elements in control element set.
> > 
> > At first place, my request is to relax limitation according to userspace
> > application. Expansion of memory consumption is unavoidable. The linear
> > increase of memory consumption about which you mentioned is not so worse
> > as a result of compromise to the above.
> 
> Hm?  We really care only the *max* memory consumption case (i.e. the
> worst case scenario), not about the memory consumption in your
> particular use case.  And this can be certainly accountable.
> 
> Whether to calculate the amount of TLV data separately from the main
> user elements is another thing to be discussed, but in either way, we
> can know the data size at allocation time, and we can also calculate
> the theoretical max of the current implementation beforehand.

Now I counted the actual memory consumption in the old code, and this
could lead to ca 32MB without TLV and enum strings.  TLV and enum
strings may attribute 6MB more total, so it can go up to 38MB.
It's really high, and we'd rather want to reduce it, instead of
increasing it.

Below is a revised patch, that calculates the memory consumption in
more details.  In this patch, the max memory is reduced to 8MB --
which should be large enough for any sensible use cases.  I guess this
will fulfill your requirement?

The patch also makes the limit as a module option.  I'm not 100% sure
whether it's needed, but since the patch reduces the size, it might
need some cure for an extreme case.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: control: Add memory consumption limit to user controls

ALSA control interface allows users to add arbitrary control elements
(called "user controls" or "user elements"), and its resource usage is
limited just by the max number of control sets (currently 32).  This
limit, however, is quite loose: each allocation of control set may
have 1028 elements, and each element may have up to 512 bytes of value
data.  Moreover, each control set may contain the enum strings and TLV
data, which can be up to 64kB and 128kB, respectively.  Totally, the
whole memory consumption may go over 38MB -- it's quite large, and
we'd rather like to reduce the size.

OTOH, there have been other requests even to increase the max number
of user elements; e.g. FireWire stack require the more user controls,
hence we want to raise the bar, too.

For satisfying both requirements, this patch changes the management of
user controls: instead of setting the upper limit of the number of
user controls, we check the actual memory allocation size and set the
upper limit of the total allocation in bytes.  As long as the memory
consumption stays below the limit, more user controls are allowed than
the current limit 32.  At the same time, we set the lower limit (8MB)
as default than the current theoretical limit, in order to lower the
risk of DoS.

As a compromise for lowering the default limit, now the actual memory
limit is defined as a module option, max_user_ctl, so that user can
increase/decrease the limit if really needed, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/core.h |  2 +-
 sound/core/control.c | 66 +++++++++++++++++++++++++++++---------------
 2 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index 0462c577d7a3..a21b14451810 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -100,7 +100,7 @@ struct snd_card {
 	struct rw_semaphore controls_rwsem;	/* controls list lock */
 	rwlock_t ctl_files_rwlock;	/* ctl_files list lock */
 	int controls_count;		/* count of all controls */
-	int user_ctl_count;		/* count of all user controls */
+	size_t user_ctl_alloc_size;	/* current memory allocation by user controls */
 	struct list_head controls;	/* all controls for this card */
 	struct list_head ctl_files;	/* active control files */
 
diff --git a/sound/core/control.c b/sound/core/control.c
index 5165741a8400..bbc4ee61f965 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -7,6 +7,7 @@
 #include <linux/threads.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/time.h>
@@ -18,10 +19,13 @@
 #include <sound/info.h>
 #include <sound/control.h>
 
-/* max number of user-defined controls */
-#define MAX_USER_CONTROLS	32
 #define MAX_CONTROL_COUNT	1028
 
+/* Max allocation size for user controls */
+static int max_user_ctl_alloc_size = 8 * 1024 * 1024;
+module_param_named(max_user_ctl, max_user_ctl_alloc_size, int, 0444);
+MODULE_PARM_DESC(max_user_ctl, "Max allocation size for user controls");
+
 struct snd_kctl_ioctl {
 	struct list_head list;		/* list of all ioctls */
 	snd_kctl_ioctl_func_t fioctl;
@@ -537,9 +541,6 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
 			goto error;
 		}
 	ret = snd_ctl_remove(card, kctl);
-	if (ret < 0)
-		goto error;
-	card->user_ctl_count--;
 error:
 	up_write(&card->controls_rwsem);
 	return ret;
@@ -1226,11 +1227,18 @@ struct user_element {
 	struct snd_card *card;
 	char *elem_data;		/* element data */
 	unsigned long elem_data_size;	/* size of element data in bytes */
+	size_t alloc_size;		/* allocated size */
 	void *tlv_data;			/* TLV data */
 	unsigned long tlv_data_size;	/* TLV data size */
 	void *priv_data;		/* private data (like strings for enumerated type) */
 };
 
+/* check whether the addition (in bytes) of user ctl element may overflow the limit */
+static bool user_elem_overflow(struct snd_card *card, ssize_t add)
+{
+	return (ssize_t)card->user_ctl_alloc_size + add >= max_user_ctl_alloc_size;
+}
+
 static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_info *uinfo)
 {
@@ -1309,6 +1317,10 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
 	if (size > 1024 * 128)	/* sane value */
 		return -EINVAL;
 
+	/* does the TLV size change cause overflow? */
+	if (user_elem_overflow(ue->card, (ssize_t)(size - ue->tlv_data_size)))
+		return -ENOMEM;
+
 	container = vmemdup_user(buf, size);
 	if (IS_ERR(container))
 		return PTR_ERR(container);
@@ -1326,11 +1338,15 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
 		for (i = 0; i < kctl->count; ++i)
 			kctl->vd[i].access |= SNDRV_CTL_ELEM_ACCESS_TLV_READ;
 		mask = SNDRV_CTL_EVENT_MASK_INFO;
+	} else {
+		ue->card->user_ctl_alloc_size -= ue->tlv_data_size;
+		ue->tlv_data_size = 0;
+		kvfree(ue->tlv_data);
 	}
 
-	kvfree(ue->tlv_data);
 	ue->tlv_data = container;
 	ue->tlv_data_size = size;
+	ue->card->user_ctl_alloc_size += size;
 
 	mask |= SNDRV_CTL_EVENT_MASK_TLV;
 	for (i = 0; i < kctl->count; ++i) {
@@ -1374,16 +1390,17 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 	unsigned int i;
 	const uintptr_t user_ptrval = ue->info.value.enumerated.names_ptr;
 
-	if (ue->info.value.enumerated.names_length > 64 * 1024)
+	buf_len = ue->info.value.enumerated.names_length;
+	if (buf_len > 64 * 1024)
 		return -EINVAL;
 
-	names = vmemdup_user((const void __user *)user_ptrval,
-		ue->info.value.enumerated.names_length);
+	if (user_elem_overflow(ue->card, buf_len))
+		return -ENOMEM;
+	names = vmemdup_user((const void __user *)user_ptrval, buf_len);
 	if (IS_ERR(names))
 		return PTR_ERR(names);
 
 	/* check that there are enough valid names */
-	buf_len = ue->info.value.enumerated.names_length;
 	p = names;
 	for (i = 0; i < ue->info.value.enumerated.items; ++i) {
 		name_len = strnlen(p, buf_len);
@@ -1397,6 +1414,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 
 	ue->priv_data = names;
 	ue->info.value.enumerated.names_ptr = 0;
+	ue->alloc_size += buf_len;
 
 	return 0;
 }
@@ -1405,6 +1423,8 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol)
 {
 	struct user_element *ue = kcontrol->private_data;
 
+	ue->card->user_ctl_alloc_size -= ue->alloc_size;
+	ue->card->user_ctl_alloc_size -= ue->tlv_data_size;
 	kvfree(ue->tlv_data);
 	kvfree(ue->priv_data);
 	kfree(ue);
@@ -1418,6 +1438,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	unsigned int count;
 	unsigned int access;
 	long private_size;
+	size_t alloc_size;
 	struct user_element *ue;
 	unsigned int offset;
 	int err;
@@ -1435,13 +1456,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 			return err;
 	}
 
-	/*
-	 * The number of userspace controls are counted control by control,
-	 * not element by element.
-	 */
-	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
-		return -ENOMEM;
-
 	/* Check the number of elements for this userspace control. */
 	count = info->owner;
 	if (count == 0)
@@ -1472,6 +1486,11 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (info->count < 1)
 		return -EINVAL;
 	private_size = value_sizes[info->type] * info->count;
+	alloc_size = sizeof(struct user_element) + private_size * count;
+
+	/* Check the memory allocation limit */
+	if (user_elem_overflow(card, alloc_size))
+		return -ENOMEM;
 
 	/*
 	 * Keep memory object for this userspace control. After passing this
@@ -1483,19 +1502,22 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (err < 0)
 		return err;
 	memcpy(&kctl->id, &info->id, sizeof(kctl->id));
-	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count,
-				     GFP_KERNEL);
-	if (kctl->private_data == NULL) {
+	ue = kzalloc(alloc_size, GFP_KERNEL);
+	if (!ue) {
 		kfree(kctl);
 		return -ENOMEM;
 	}
+	kctl->private_data = ue;
 	kctl->private_free = snd_ctl_elem_user_free;
 
+	/* increment the allocated size; decremented again at private_free */
+	card->user_ctl_alloc_size += alloc_size;
+
 	/* Set private data for this userspace control. */
-	ue = (struct user_element *)kctl->private_data;
 	ue->card = card;
 	ue->info = *info;
 	ue->info.access = 0;
+	ue->alloc_size = alloc_size;
 	ue->elem_data = (char *)ue + sizeof(*ue);
 	ue->elem_data_size = private_size;
 	if (ue->info.type == SNDRV_CTL_ELEM_TYPE_ENUMERATED) {
@@ -1535,8 +1557,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	 * which locks the element.
 	 */
 
-	card->user_ctl_count++;
-
  unlock:
 	up_write(&card->controls_rwsem);
 	return err;
Jaroslav Kysela Jan. 26, 2021, 4:25 p.m. UTC | #10
Dne 26. 01. 21 v 16:57 Takashi Iwai napsal(a):

> @@ -1226,11 +1227,18 @@ struct user_element {
>  	struct snd_card *card;
>  	char *elem_data;		/* element data */
>  	unsigned long elem_data_size;	/* size of element data in bytes */
> +	size_t alloc_size;		/* allocated size */

I think that introduction of this new member is not required. The allocated
size can be easily computed at runtime (function).

> @@ -1397,6 +1414,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
>  
>  	ue->priv_data = names;
>  	ue->info.value.enumerated.names_ptr = 0;
> +	ue->alloc_size += buf_len;

The buf_len variable is the remaining count. The
ue->info.value.enumerated.names_length already contains allocated bytes. This
code can be elimited if alloc_size is not introduced.

Thank you for this patch.

					Jaroslav
Takashi Iwai Jan. 26, 2021, 4:41 p.m. UTC | #11
On Tue, 26 Jan 2021 17:25:24 +0100,
Jaroslav Kysela wrote:
> 
> Dne 26. 01. 21 v 16:57 Takashi Iwai napsal(a):
> 
> > @@ -1226,11 +1227,18 @@ struct user_element {
> >  	struct snd_card *card;
> >  	char *elem_data;		/* element data */
> >  	unsigned long elem_data_size;	/* size of element data in bytes */
> > +	size_t alloc_size;		/* allocated size */
> 
> I think that introduction of this new member is not required. The allocated
> size can be easily computed at runtime (function).

True.  OTOH, I thought this addition would be error-prone.

> > @@ -1397,6 +1414,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
> >  
> >  	ue->priv_data = names;
> >  	ue->info.value.enumerated.names_ptr = 0;
> > +	ue->alloc_size += buf_len;
> 
> The buf_len variable is the remaining count. The
> ue->info.value.enumerated.names_length already contains allocated bytes. This
> code can be elimited if alloc_size is not introduced.

Oh yeah, that's a wrong count.


thanks,

Takashi
Takashi Iwai Jan. 26, 2021, 5:02 p.m. UTC | #12
On Tue, 26 Jan 2021 17:41:36 +0100,
Takashi Iwai wrote:
> 
> On Tue, 26 Jan 2021 17:25:24 +0100,
> Jaroslav Kysela wrote:
> > 
> > Dne 26. 01. 21 v 16:57 Takashi Iwai napsal(a):
> > 
> > > @@ -1226,11 +1227,18 @@ struct user_element {
> > >  	struct snd_card *card;
> > >  	char *elem_data;		/* element data */
> > >  	unsigned long elem_data_size;	/* size of element data in bytes */
> > > +	size_t alloc_size;		/* allocated size */
> > 
> > I think that introduction of this new member is not required. The allocated
> > size can be easily computed at runtime (function).
> 
> True.  OTOH, I thought this addition would be error-prone.
> 
> > > @@ -1397,6 +1414,7 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
> > >  
> > >  	ue->priv_data = names;
> > >  	ue->info.value.enumerated.names_ptr = 0;
> > > +	ue->alloc_size += buf_len;
> > 
> > The buf_len variable is the remaining count. The
> > ue->info.value.enumerated.names_length already contains allocated bytes. This
> > code can be elimited if alloc_size is not introduced.
> 
> Oh yeah, that's a wrong count.

The v2 patch is below.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: control: Add memory consumption limit to user
 controls

ALSA control interface allows users to add arbitrary control elements
(called "user controls" or "user elements"), and its resource usage is
limited just by the max number of control sets (currently 32).  This
limit, however, is quite loose: each allocation of control set may
have 1028 elements, and each element may have up to 512 bytes of value
data.  Moreover, each control set may contain the enum strings and TLV
data, which can be up to 64kB and 128kB, respectively.  Totally, the
whole memory consumption may go over 38MB -- it's quite large, and
we'd rather like to reduce the size.

OTOH, there have been other requests even to increase the max number
of user elements; e.g. FireWire stack require the more user controls,
hence we want to raise the bar, too.

For satisfying both requirements, this patch changes the management of
user controls: instead of setting the upper limit of the number of
user controls, we check the actual memory allocation size and set the
upper limit of the total allocation in bytes.  As long as the memory
consumption stays below the limit, more user controls are allowed than
the current limit 32.  At the same time, we set the lower limit (8MB)
as default than the current theoretical limit, in order to lower the
risk of DoS.

As a compromise for lowering the default limit, now the actual memory
limit is defined as a module option, max_user_ctl, so that user can
increase/decrease the limit if really needed, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Drop alloc_size field from user_element, calculate at private_free

 include/sound/core.h |  2 +-
 sound/core/control.c | 73 ++++++++++++++++++++++++++++++--------------
 2 files changed, 51 insertions(+), 24 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index 0462c577d7a3..a21b14451810 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -100,7 +100,7 @@ struct snd_card {
 	struct rw_semaphore controls_rwsem;	/* controls list lock */
 	rwlock_t ctl_files_rwlock;	/* ctl_files list lock */
 	int controls_count;		/* count of all controls */
-	int user_ctl_count;		/* count of all user controls */
+	size_t user_ctl_alloc_size;	/* current memory allocation by user controls */
 	struct list_head controls;	/* all controls for this card */
 	struct list_head ctl_files;	/* active control files */
 
diff --git a/sound/core/control.c b/sound/core/control.c
index 5165741a8400..1a9a3115e4a3 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -7,6 +7,7 @@
 #include <linux/threads.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/time.h>
@@ -18,10 +19,13 @@
 #include <sound/info.h>
 #include <sound/control.h>
 
-/* max number of user-defined controls */
-#define MAX_USER_CONTROLS	32
 #define MAX_CONTROL_COUNT	1028
 
+/* Max allocation size for user controls */
+static int max_user_ctl_alloc_size = 8 * 1024 * 1024;
+module_param_named(max_user_ctl, max_user_ctl_alloc_size, int, 0444);
+MODULE_PARM_DESC(max_user_ctl, "Max allocation size for user controls");
+
 struct snd_kctl_ioctl {
 	struct list_head list;		/* list of all ioctls */
 	snd_kctl_ioctl_func_t fioctl;
@@ -537,9 +541,6 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
 			goto error;
 		}
 	ret = snd_ctl_remove(card, kctl);
-	if (ret < 0)
-		goto error;
-	card->user_ctl_count--;
 error:
 	up_write(&card->controls_rwsem);
 	return ret;
@@ -1231,6 +1232,12 @@ struct user_element {
 	void *priv_data;		/* private data (like strings for enumerated type) */
 };
 
+/* check whether the addition (in bytes) of user ctl element may overflow the limit */
+static bool user_elem_overflow(struct snd_card *card, ssize_t add)
+{
+	return (ssize_t)card->user_ctl_alloc_size + add >= max_user_ctl_alloc_size;
+}
+
 static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_info *uinfo)
 {
@@ -1309,6 +1316,10 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
 	if (size > 1024 * 128)	/* sane value */
 		return -EINVAL;
 
+	/* does the TLV size change cause overflow? */
+	if (user_elem_overflow(ue->card, (ssize_t)(size - ue->tlv_data_size)))
+		return -ENOMEM;
+
 	container = vmemdup_user(buf, size);
 	if (IS_ERR(container))
 		return PTR_ERR(container);
@@ -1326,11 +1337,15 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
 		for (i = 0; i < kctl->count; ++i)
 			kctl->vd[i].access |= SNDRV_CTL_ELEM_ACCESS_TLV_READ;
 		mask = SNDRV_CTL_EVENT_MASK_INFO;
+	} else {
+		ue->card->user_ctl_alloc_size -= ue->tlv_data_size;
+		ue->tlv_data_size = 0;
+		kvfree(ue->tlv_data);
 	}
 
-	kvfree(ue->tlv_data);
 	ue->tlv_data = container;
 	ue->tlv_data_size = size;
+	ue->card->user_ctl_alloc_size += size; /* decremented at private_free */
 
 	mask |= SNDRV_CTL_EVENT_MASK_TLV;
 	for (i = 0; i < kctl->count; ++i) {
@@ -1374,16 +1389,17 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 	unsigned int i;
 	const uintptr_t user_ptrval = ue->info.value.enumerated.names_ptr;
 
-	if (ue->info.value.enumerated.names_length > 64 * 1024)
+	buf_len = ue->info.value.enumerated.names_length;
+	if (buf_len > 64 * 1024)
 		return -EINVAL;
 
-	names = vmemdup_user((const void __user *)user_ptrval,
-		ue->info.value.enumerated.names_length);
+	if (user_elem_overflow(ue->card, buf_len))
+		return -ENOMEM;
+	names = vmemdup_user((const void __user *)user_ptrval, buf_len);
 	if (IS_ERR(names))
 		return PTR_ERR(names);
 
 	/* check that there are enough valid names */
-	buf_len = ue->info.value.enumerated.names_length;
 	p = names;
 	for (i = 0; i < ue->info.value.enumerated.items; ++i) {
 		name_len = strnlen(p, buf_len);
@@ -1397,14 +1413,26 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 
 	ue->priv_data = names;
 	ue->info.value.enumerated.names_ptr = 0;
+	/* increment the allocation size; decremented again at private_free */
+	ue->card->user_ctl_alloc_size += ue->info.value.enumerated.names_length;
 
 	return 0;
 }
 
+#define user_elem_size(size, count) \
+	(sizeof(struct user_element) + (size) * count)
+
 static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol)
 {
 	struct user_element *ue = kcontrol->private_data;
 
+	/* decrement the allocation size */
+	ue->card->user_ctl_alloc_size -= user_elem_size(ue->elem_data_size,
+							kcontrol->count);
+	ue->card->user_ctl_alloc_size -= ue->tlv_data_size;
+	if (ue->priv_data)
+		ue->card->user_ctl_alloc_size -= ue->info.value.enumerated.names_length;
+
 	kvfree(ue->tlv_data);
 	kvfree(ue->priv_data);
 	kfree(ue);
@@ -1418,6 +1446,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	unsigned int count;
 	unsigned int access;
 	long private_size;
+	size_t alloc_size;
 	struct user_element *ue;
 	unsigned int offset;
 	int err;
@@ -1435,13 +1464,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 			return err;
 	}
 
-	/*
-	 * The number of userspace controls are counted control by control,
-	 * not element by element.
-	 */
-	if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
-		return -ENOMEM;
-
 	/* Check the number of elements for this userspace control. */
 	count = info->owner;
 	if (count == 0)
@@ -1472,6 +1494,11 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (info->count < 1)
 		return -EINVAL;
 	private_size = value_sizes[info->type] * info->count;
+	alloc_size = user_elem_size(private_size, count);
+
+	/* Check the memory allocation limit */
+	if (user_elem_overflow(card, alloc_size))
+		return -ENOMEM;
 
 	/*
 	 * Keep memory object for this userspace control. After passing this
@@ -1483,16 +1510,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	if (err < 0)
 		return err;
 	memcpy(&kctl->id, &info->id, sizeof(kctl->id));
-	kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count,
-				     GFP_KERNEL);
-	if (kctl->private_data == NULL) {
+	ue = kzalloc(alloc_size, GFP_KERNEL);
+	if (!ue) {
 		kfree(kctl);
 		return -ENOMEM;
 	}
+	kctl->private_data = ue;
 	kctl->private_free = snd_ctl_elem_user_free;
 
+	/* increment the allocated size; decremented again at private_free */
+	card->user_ctl_alloc_size += alloc_size;
+
 	/* Set private data for this userspace control. */
-	ue = (struct user_element *)kctl->private_data;
 	ue->card = card;
 	ue->info = *info;
 	ue->info.access = 0;
@@ -1535,8 +1564,6 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	 * which locks the element.
 	 */
 
-	card->user_ctl_count++;
-
  unlock:
 	up_write(&card->controls_rwsem);
 	return err;
diff mbox series

Patch

diff --git a/sound/core/control.c b/sound/core/control.c
index 5165741a8400..5a19bde27830 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -18,8 +18,13 @@ 
 #include <sound/info.h>
 #include <sound/control.h>
 
-/* max number of user-defined controls */
-#define MAX_USER_CONTROLS	32
+// The maximum number of control element sets per sound card added by
+// userspace applications. The value is decided just to satisfy requirement
+// from control service programs in userspace for devices supported by
+// drivers in ALSA firewire stack. It's possible to relax the limitation
+// according to requirements from the other kind of applications.
+#define MAX_USER_CONTROLS	150
+
 #define MAX_CONTROL_COUNT	1028
 
 struct snd_kctl_ioctl {