diff mbox series

[v3] ALSA: control: Add memory consumption limit to user controls

Message ID 20210408103149.40357-1-o-takashi@sakamocchi.jp (mailing list archive)
State New
Headers show
Series [v3] ALSA: control: Add memory consumption limit to user controls | expand

Commit Message

Takashi Sakamoto April 8, 2021, 10:31 a.m. UTC
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 (ILP32) or
1024 bytes (LP64) 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. ALSA 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_alloc_size', so that
user can increase/decrease the limit if really needed, too.

Co-developed-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
v1->v2: Drop alloc_size field from user_element, calculate at private_free
v2->v3: Rebase. Fix boundary error. Obsolete macro usage relying on modern
        compiler optimization. Change comment style by modern coding
        convention. Rename module parameter so that users get it easily.
        Patch comment improvements.
---
 include/sound/core.h |  2 +-
 sound/core/control.c | 75 ++++++++++++++++++++++++++++++--------------
 2 files changed, 52 insertions(+), 25 deletions(-)

Comments

Takashi Sakamoto April 8, 2021, 10:50 a.m. UTC | #1
Hi,

Some supplements.

On Thu, Apr 08, 2021 at 07:31:49PM +0900, Takashi Sakamoto wrote:
> 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 (ILP32) or
> 1024 bytes (LP64) 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. ALSA 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_alloc_size', so that
> user can increase/decrease the limit if really needed, too.
> 
> Co-developed-by: Takashi Iwai <tiwai@suse.de>
> Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
> v1->v2: Drop alloc_size field from user_element, calculate at private_free
> v2->v3: Rebase. Fix boundary error. Obsolete macro usage relying on modern
>         compiler optimization. Change comment style by modern coding
>         convention. Rename module parameter so that users get it easily.
>         Patch comment improvements.
> ---
>  include/sound/core.h |  2 +-
>  sound/core/control.c | 75 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 52 insertions(+), 25 deletions(-)

The original content of patch comes from Iwai-san[1]. I have no clear
idea to handle the case so add 'Co-developed-by' tag to the patch. If
this is not good, I apologize the lack of my understanding to the
development process in Linux kernel.

In this v3 patch, I add below changes to v2 patch:

 * Rebase to current HEAD of for-next branch (884c7094a272).
 * Fix boundary error.
   * Original patch uses 'bigger-or-equal' to max allocation size
 * Obsolete macro usage relying on modern compiler optimization
   * this seems to be friendry to any static code analyzer
 * Change comment style by modern coding convention
   * '//' is acceptable and friendry to any static code analyzer
 * Rename module parameter so that users get it easily.
   * The name with enough length makes users to get it easily
 * Patch comment improvements.
   * Some explanations are not necessarily correct

I did test this patch by with below script, written with alsa-gobject[2].

```
#!/usr/bin/env python3

from sys import argv, exit
from re import match
import gi
gi.require_version('ALSACtl', '0.0')
from gi.repository import ALSACtl

if len(argv) < 2:
    print('One argument is required for card numeric ID.')
    exit(1)
card_id = int(argv[1])

card = ALSACtl.Card.new()
card.open(card_id, 0)

# Retrieve current value.
curr_cap = 0
with open('/sys/module/snd/parameters/max_user_ctl_alloc_size', 'r') as f:
    buf = f.read()
    curr_cap = int(buf)
print('current value of max_user_ctl_alloc_size:', curr_cap)

# Constants.
BYTES_PER_USET_ELEMENT_STRUCT = 320
BYTES_PER_ELEM_VALUE_ENUMERATED = 4
VALUE_COUNT = 128


def user_elem_size(elem_count, label_consumption, tlv_consumption):
    return ((BYTES_PER_USET_ELEMENT_STRUCT + elem_count *
             BYTES_PER_ELEM_VALUE_ENUMERATED * VALUE_COUNT) +
            label_consumption + tlv_consumption)


def calculate_expected_iteration(elem_count, label_consumption,
                                 tlv_consumption, curr_cap):
    expected_iteration = 0

    consumption = 0
    while True:
        allocation = user_elem_size(elem_count, label_consumption,
                                    tlv_consumption)
        if consumption + allocation > curr_cap:
            break
        consumption += allocation
        expected_iteration += 1

    return expected_iteration


def test_allocation(card, elem_count, curr_cap):
    labels = (
        'Opinion is the medium ',
        'between knowledge and ',
        'ignorance.',
        'Rhythm and harmony ',
        'find their way into the ',
        'inward places of the soul.',
    )
    label_consumption = 0
    for label in labels:
        label_consumption += len(label) + 1

    tlv_cntr = [0] * 24
    tlv_consumption = len(tlv_cntr) * 4

    expected_iteration = calculate_expected_iteration(
            elem_count,
            label_consumption,
            tlv_consumption,
            curr_cap)

    elem_info = ALSACtl.ElemInfo.new(ALSACtl.ElemType.ENUMERATED)
    elem_info.set_enum_data(labels)
    access = (ALSACtl.ElemAccessFlag.READ |
              ALSACtl.ElemAccessFlag.TLV_READ |
              ALSACtl.ElemAccessFlag.TLV_WRITE)
    elem_info.set_property('access', access)
    elem_info.set_property('value-count', VALUE_COUNT)

    consumption = 0
    iteration = 0
    added_elems = []
    while True:
        name = 'test-{}'.format(iteration)

        elem_id = ALSACtl.ElemId.new_by_name(ALSACtl.ElemIfaceType.MIXER,
                                             0, 0, name, 0)
        try:
            elem_id_list = card.add_elems(elem_id, elem_count, elem_info)
            added_elems.extend(elem_id_list)
            card.write_elem_tlv(elem_id_list[0], tlv_cntr)
            consumption += user_elem_size(
                    elem_count,
                    label_consumption,
                    tlv_consumption)
            iteration += 1
        except Exception as e:
            groups = match('ioctl\\(.+\\) ([0-9]+)\\(.+\\)', e.message)
            if groups is None or int(groups[1]) != 12:
                print('unexpected error',
                      iteration, len(added_elems), consumption, curr_cap)
            elif iteration != expected_iteration:
                print('unexpected iteration {} but expected {}, {}'.format(
                      iteration, expected_iteration, consumption))
            break

    print('expected_iteration: {}, iteration: {}, consumption {}'.format(
        expected_iteration, iteration, consumption))

    for elem_id in added_elems:
        try:
            card.remove_elems(elem_id)
        except Exception:
            pass


for i in range(1, 20):
    test_allocation(card, i, curr_cap)
```

The parameter is configured to 12551 and 12552 for boundary check. As a
result:

```
current value of max_user_ctl_alloc_size: 12552
expected_iteration: 11, iteration: 11, consumption 11627
expected_iteration: 8, iteration: 8, consumption 12552
...

current value of max_user_ctl_alloc_size: 12551
expected_iteration: 11, iteration: 11, consumption 11627
expected_iteration: 7, iteration: 7, consumption 10983
...
```

It looks well.


Regards

[1] https://mailman.alsa-project.org/pipermail/alsa-devel/2021-January/179683.html
[2] https://github.com/alsa-project/alsa-gobject/

Takashi Sakamoto
Takashi Iwai April 8, 2021, 11:33 a.m. UTC | #2
On Thu, 08 Apr 2021 12:50:25 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> Some supplements.
> 
> On Thu, Apr 08, 2021 at 07:31:49PM +0900, Takashi Sakamoto wrote:
> > 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 (ILP32) or
> > 1024 bytes (LP64) 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. ALSA 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_alloc_size', so that
> > user can increase/decrease the limit if really needed, too.
> > 
> > Co-developed-by: Takashi Iwai <tiwai@suse.de>
> > Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > ---
> > v1->v2: Drop alloc_size field from user_element, calculate at private_free
> > v2->v3: Rebase. Fix boundary error. Obsolete macro usage relying on modern
> >         compiler optimization. Change comment style by modern coding
> >         convention. Rename module parameter so that users get it easily.
> >         Patch comment improvements.
> > ---
> >  include/sound/core.h |  2 +-
> >  sound/core/control.c | 75 ++++++++++++++++++++++++++++++--------------
> >  2 files changed, 52 insertions(+), 25 deletions(-)
> 
> The original content of patch comes from Iwai-san[1]. I have no clear
> idea to handle the case so add 'Co-developed-by' tag to the patch. If
> this is not good, I apologize the lack of my understanding to the
> development process in Linux kernel.

It depends.  In some cases, you just carry the patch with the original
authorship (From address) and put your sign-off.  In some cases,
Co-developed-by can be used.  I don't mind much either way, so I took
your v3 patch now (with the addition of the Link URL to v2 patch).


Thanks!

Takashi
Takashi Sakamoto April 9, 2021, 2:27 a.m. UTC | #3
Hi,

On Thu, Apr 08, 2021 at 01:33:41PM +0200, Takashi Iwai wrote:
> On Thu, 08 Apr 2021 12:50:25 +0200, Takashi Sakamoto wrote:
> > On Thu, Apr 08, 2021 at 07:31:49PM +0900, Takashi Sakamoto wrote:
> > > 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 (ILP32) or
> > > 1024 bytes (LP64) 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. ALSA 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_alloc_size', so that
> > > user can increase/decrease the limit if really needed, too.
> > > 
> > > Co-developed-by: Takashi Iwai <tiwai@suse.de>
> > > Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > ---
> > > v1->v2: Drop alloc_size field from user_element, calculate at private_free
> > > v2->v3: Rebase. Fix boundary error. Obsolete macro usage relying on modern
> > >         compiler optimization. Change comment style by modern coding
> > >         convention. Rename module parameter so that users get it easily.
> > >         Patch comment improvements.
> > > ---
> > >  include/sound/core.h |  2 +-
> > >  sound/core/control.c | 75 ++++++++++++++++++++++++++++++--------------
> > >  2 files changed, 52 insertions(+), 25 deletions(-)
> > 
> > The original content of patch comes from Iwai-san[1]. I have no clear
> > idea to handle the case so add 'Co-developed-by' tag to the patch. If
> > this is not good, I apologize the lack of my understanding to the
> > development process in Linux kernel.
> 
> It depends.  In some cases, you just carry the patch with the original
> authorship (From address) and put your sign-off.  In some cases,
> Co-developed-by can be used.  I don't mind much either way, so I took
> your v3 patch now (with the addition of the Link URL to v2 patch).

Thanks for applying the patch as is. I would post it just with my sign-off
without no changes to your patch, However in the case I added some changes,
so I have no conviction to it...

Well, relevant to the function, I have some ideas to refactor ALSA control
core. If you have room to discuss about them, I'd like to ask your opinion.

At present, I have five ideas:

1. Split code relevant to user-defined element set into new module

Although the function is itself useful to me, it's useless in the case
to use driver in which every functions are in kernel land, especially in
embedded systems. The layering function introduced recently (and ctl ioctl
registration function) enables to capsulate it into module. This results
in building the function according to kernel configuration and reduction
of the size of snd.ko for embedded systems. (But I wish usual desktop
environment enables it...)

In my plan, the name of new module is snd_ctl_user_elem_set.ko and the
configuration is CONFIG_SND_CTL_USER_ELEM_SETS. I've already written
patchset in my hand and find some negative points:

 * Comparing environments in which the function is enable or disabled,
   we have difference about the system behaviour against some ioctl
   requests (ELEM_ADD, ELEM_REPLACE, ELEM_REMOVE). I have no idea to
   judge whether this is evil or not.
 * Some internal functions and tables in snd.ko should be expoted to the
   new module; e.g. 'value_sizes' or 'snd_ctl_new()'. The symbol table
   is increased.
 * Some code should be moved from compatibility layer of ALSA control
   core. This seems to increate the cost of maintenance for the layer.

2. Introduce control component structure and move codes from card structure

This is just an idea and preparation for following items. Historically,
ALSA card structure includes some control-related stuffs. The card has
two Linux device structures for pseudo card (card_dev) and control
cdev (ctl_dev).  The card also aggregates the list of the other
components such as pcm, hwdep. In this item, I add a new control
structure and split control related stuffs from card structure. As a
result, the control component becomes to be equivalent to the other
components, in a point of both relationship to pseudo card device and
relationship to cdev.

The change results in the reduction of size of card structure somehow. I
expect it to be friendly to memory object allocator, and to be clear
view of code structure.

At present, I don't prepare any patch. But I guess some negative
points:
 * I don't get the range of code influenced by the change yet. If it's
   huge, I would give up the idea itself...
 * Theoretically, the new control structure is released as the same way
   as the other components such as PCM. However I'm afraid of
   fatal regressions comes from structural problems in complicated release
   process of ALSA core...
 * Any change of behaviour relevant to kobject in a view of userspace.

3. Add kobject attributes into the control device

At present, card structure has kobject attributes. Some kernel APIs are
exposed to in-kernel drivers and some drivers already use it; e.g. the
series of line6 drivers.

In this item, referring to the idea of case for card structure, I add
kobject attributes into the control device, and add mechanism for
in-kernel drivers to register own attributes as well as common
attributes.

As you know, kobject attributes exposed via sysfs node is often abused,
like recent patch for any name of card structure. It should be done with
enough care of future change, since it's a part of interface to
userspace once exposed to userspace,

4. Add `max_user_ctl_alloc_size` kobject attribute to the control device

In the patch, a new module parameter 'max_user_ctl_alloc_size' is added.
In the item, I use the value of this parameter as initial value per
control device. The value per control device can be changed via sysfs
node.

The `max_user_ctl_alloc_size` is really the attribute of control device,
so I think it acceptable. Additionally, 'curr_user_ctl_alloc_size' is
also added so that userspace applications get current status.

5. add any mechanism to bind lifetime of user-defined element set to user
   process

At present, the lifetime of user-defined element set is bound to card
itself. However, it's convenient to user processes to bind the lifetime
to process itself. I add any mechanism for it.

For recent years I've made some patches in house but never arrive at the
best one. In the patches, I utilize access flags but in general the
maintenance of lifetime is not easy issue. I tackle again in this time.

Anyway, I'm a developer in private time, so it's my convenient to hear
maintainer's opinion, especially about 'go' or 'no-go', to use my life
time efficiently. I'm happy to receive any of your opinions.


Thanks

Takashi Sakamoto
Takashi Iwai April 9, 2021, 10:59 a.m. UTC | #4
On Fri, 09 Apr 2021 04:27:35 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Thu, Apr 08, 2021 at 01:33:41PM +0200, Takashi Iwai wrote:
> > On Thu, 08 Apr 2021 12:50:25 +0200, Takashi Sakamoto wrote:
> > > On Thu, Apr 08, 2021 at 07:31:49PM +0900, Takashi Sakamoto wrote:
> > > > 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 (ILP32) or
> > > > 1024 bytes (LP64) 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. ALSA 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_alloc_size', so that
> > > > user can increase/decrease the limit if really needed, too.
> > > > 
> > > > Co-developed-by: Takashi Iwai <tiwai@suse.de>
> > > > Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > > Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > > ---
> > > > v1->v2: Drop alloc_size field from user_element, calculate at private_free
> > > > v2->v3: Rebase. Fix boundary error. Obsolete macro usage relying on modern
> > > >         compiler optimization. Change comment style by modern coding
> > > >         convention. Rename module parameter so that users get it easily.
> > > >         Patch comment improvements.
> > > > ---
> > > >  include/sound/core.h |  2 +-
> > > >  sound/core/control.c | 75 ++++++++++++++++++++++++++++++--------------
> > > >  2 files changed, 52 insertions(+), 25 deletions(-)
> > > 
> > > The original content of patch comes from Iwai-san[1]. I have no clear
> > > idea to handle the case so add 'Co-developed-by' tag to the patch. If
> > > this is not good, I apologize the lack of my understanding to the
> > > development process in Linux kernel.
> > 
> > It depends.  In some cases, you just carry the patch with the original
> > authorship (From address) and put your sign-off.  In some cases,
> > Co-developed-by can be used.  I don't mind much either way, so I took
> > your v3 patch now (with the addition of the Link URL to v2 patch).
> 
> Thanks for applying the patch as is. I would post it just with my sign-off
> without no changes to your patch, However in the case I added some changes,
> so I have no conviction to it...
> 
> Well, relevant to the function, I have some ideas to refactor ALSA control
> core. If you have room to discuss about them, I'd like to ask your opinion.
> 
> At present, I have five ideas:
> 
> 1. Split code relevant to user-defined element set into new module
> 
> Although the function is itself useful to me, it's useless in the case
> to use driver in which every functions are in kernel land, especially in
> embedded systems. The layering function introduced recently (and ctl ioctl
> registration function) enables to capsulate it into module. This results
> in building the function according to kernel configuration and reduction
> of the size of snd.ko for embedded systems. (But I wish usual desktop
> environment enables it...)
> 
> In my plan, the name of new module is snd_ctl_user_elem_set.ko and the
> configuration is CONFIG_SND_CTL_USER_ELEM_SETS. I've already written
> patchset in my hand and find some negative points:
> 
>  * Comparing environments in which the function is enable or disabled,
>    we have difference about the system behaviour against some ioctl
>    requests (ELEM_ADD, ELEM_REPLACE, ELEM_REMOVE). I have no idea to
>    judge whether this is evil or not.
>  * Some internal functions and tables in snd.ko should be expoted to the
>    new module; e.g. 'value_sizes' or 'snd_ctl_new()'. The symbol table
>    is increased.
>  * Some code should be moved from compatibility layer of ALSA control
>    core. This seems to increate the cost of maintenance for the layer.

The module would be useful if this can work additionally on top of the
others.  And, in the case of user-element, it has nothing to do with
the driver, so if the module is split, user would have to load the
module manually -- which is inconvenient.

If your concern is about the driver size, the needed change isn't
about splitting to another module but the conditional builds either
with ifdef or factor out to another file (and conditionally build via
Makefile).

> 2. Introduce control component structure and move codes from card structure
> 
> This is just an idea and preparation for following items. Historically,
> ALSA card structure includes some control-related stuffs. The card has
> two Linux device structures for pseudo card (card_dev) and control
> cdev (ctl_dev).  The card also aggregates the list of the other
> components such as pcm, hwdep. In this item, I add a new control
> structure and split control related stuffs from card structure. As a
> result, the control component becomes to be equivalent to the other
> components, in a point of both relationship to pseudo card device and
> relationship to cdev.
> 
> The change results in the reduction of size of card structure somehow. I
> expect it to be friendly to memory object allocator, and to be clear
> view of code structure.

Well, moving the control-related fields into another allocated object
wouldn't reduce the size in total, so I don't see any big merit by
that.  Note that the control API is mandatory for each card, hence it
can be never optional; that's the difference from other components.

Though, moving control-related fields into another struct and embed it
in snd_card would be fine if it improves the readability.  It'll be
essentially just grouping and renaming.


> At present, I don't prepare any patch. But I guess some negative
> points:
>  * I don't get the range of code influenced by the change yet. If it's
>    huge, I would give up the idea itself...
>  * Theoretically, the new control structure is released as the same way
>    as the other components such as PCM. However I'm afraid of
>    fatal regressions comes from structural problems in complicated release
>    process of ALSA core...
>  * Any change of behaviour relevant to kobject in a view of userspace.
> 
> 3. Add kobject attributes into the control device
> 
> At present, card structure has kobject attributes. Some kernel APIs are
> exposed to in-kernel drivers and some drivers already use it; e.g. the
> series of line6 drivers.
> 
> In this item, referring to the idea of case for card structure, I add
> kobject attributes into the control device, and add mechanism for
> in-kernel drivers to register own attributes as well as common
> attributes.
> 
> As you know, kobject attributes exposed via sysfs node is often abused,
> like recent patch for any name of card structure. It should be done with
> enough care of future change, since it's a part of interface to
> userspace once exposed to userspace,

Do you mean to add some sysfs files under /sys/class/control*/?
That should be easy, just a few lines of the code.  For creating files
at the device instantiation, just setting the groups like the card
object should work.


> 4. Add `max_user_ctl_alloc_size` kobject attribute to the control device
> 
> In the patch, a new module parameter 'max_user_ctl_alloc_size' is added.
> In the item, I use the value of this parameter as initial value per
> control device. The value per control device can be changed via sysfs
> node.
> 
> The `max_user_ctl_alloc_size` is really the attribute of control device,
> so I think it acceptable. Additionally, 'curr_user_ctl_alloc_size' is
> also added so that userspace applications get current status.

So that's the primary purpose?  Then it makes sense, yeah.


> 5. add any mechanism to bind lifetime of user-defined element set to user
>    process
> 
> At present, the lifetime of user-defined element set is bound to card
> itself. However, it's convenient to user processes to bind the lifetime
> to process itself. I add any mechanism for it.
> 
> For recent years I've made some patches in house but never arrive at the
> best one. In the patches, I utilize access flags but in general the
> maintenance of lifetime is not easy issue. I tackle again in this time.

It sounds interesting, but I don't know how easily you can manage it.
The driver doesn't care much about the user process lifetime, but
mostly concentrate on the file handle...


thanks,

Takashi
Jaroslav Kysela April 9, 2021, 1:34 p.m. UTC | #5
Dne 09. 04. 21 v 12:59 Takashi Iwai napsal(a):

>> 5. add any mechanism to bind lifetime of user-defined element set to user
>>    process
>>
>> At present, the lifetime of user-defined element set is bound to card
>> itself. However, it's convenient to user processes to bind the lifetime
>> to process itself. I add any mechanism for it.
>>
>> For recent years I've made some patches in house but never arrive at the
>> best one. In the patches, I utilize access flags but in general the
>> maintenance of lifetime is not easy issue. I tackle again in this time.
> 
> It sounds interesting, but I don't know how easily you can manage it.
> The driver doesn't care much about the user process lifetime, but
> mostly concentrate on the file handle...

It should be easy to trace which process created the user element and
automatically remove this element when the process close the file descriptor.
Something like 'bind lifetime of the control to the active control file
descriptor'.

					Jaroslav
Takashi Iwai April 9, 2021, 2:18 p.m. UTC | #6
On Fri, 09 Apr 2021 15:34:14 +0200,
Jaroslav Kysela wrote:
> 
> Dne 09. 04. 21 v 12:59 Takashi Iwai napsal(a):
> 
> >> 5. add any mechanism to bind lifetime of user-defined element set to user
> >>    process
> >>
> >> At present, the lifetime of user-defined element set is bound to card
> >> itself. However, it's convenient to user processes to bind the lifetime
> >> to process itself. I add any mechanism for it.
> >>
> >> For recent years I've made some patches in house but never arrive at the
> >> best one. In the patches, I utilize access flags but in general the
> >> maintenance of lifetime is not easy issue. I tackle again in this time.
> > 
> > It sounds interesting, but I don't know how easily you can manage it.
> > The driver doesn't care much about the user process lifetime, but
> > mostly concentrate on the file handle...
> 
> It should be easy to trace which process created the user element and
> automatically remove this element when the process close the file descriptor.
> Something like 'bind lifetime of the control to the active control file
> descriptor'.

If it's tied only with the file handle, it's easy.  But I thought this
is about the process?


Takashi
Takashi Iwai April 9, 2021, 4:09 p.m. UTC | #7
On Fri, 09 Apr 2021 12:59:10 +0200,
Takashi Iwai wrote:
> 
> On Fri, 09 Apr 2021 04:27:35 +0200,
> Takashi Sakamoto wrote:
> > 
> > 4. Add `max_user_ctl_alloc_size` kobject attribute to the control device
> > 
> > In the patch, a new module parameter 'max_user_ctl_alloc_size' is added.
> > In the item, I use the value of this parameter as initial value per
> > control device. The value per control device can be changed via sysfs
> > node.
> > 
> > The `max_user_ctl_alloc_size` is really the attribute of control device,
> > so I think it acceptable. Additionally, 'curr_user_ctl_alloc_size' is
> > also added so that userspace applications get current status.
> 
> So that's the primary purpose?  Then it makes sense, yeah.

You meant something like below, right?


Takashi
Takashi Sakamoto April 10, 2021, 8:20 a.m. UTC | #8
On Fri, Apr 09, 2021 at 06:09:02PM +0200, Takashi Iwai wrote:
> On Fri, 09 Apr 2021 12:59:10 +0200,
> Takashi Iwai wrote:
> > 
> > On Fri, 09 Apr 2021 04:27:35 +0200,
> > Takashi Sakamoto wrote:
> > > 
> > > 4. Add `max_user_ctl_alloc_size` kobject attribute to the control device
> > > 
> > > In the patch, a new module parameter 'max_user_ctl_alloc_size' is added.
> > > In the item, I use the value of this parameter as initial value per
> > > control device. The value per control device can be changed via sysfs
> > > node.
> > > 
> > > The `max_user_ctl_alloc_size` is really the attribute of control device,
> > > so I think it acceptable. Additionally, 'curr_user_ctl_alloc_size' is
> > > also added so that userspace applications get current status.
> > 
> > So that's the primary purpose?  Then it makes sense, yeah.
> 
> You meant something like below, right?

If you were carefully reading my items in the order, you would have
realized that the patch includes problem to share attribute group
table between several modules...

Device attribute is one of userspace interface expected to be stable. I'd
like to avoid careless changes which our known developer tends to do.


Thanks

Takashi Sakamoto
Takashi Sakamoto April 10, 2021, 8:22 a.m. UTC | #9
On Fri, Apr 09, 2021 at 04:18:00PM +0200, Takashi Iwai wrote:
> On Fri, 09 Apr 2021 15:34:14 +0200,
> Jaroslav Kysela wrote:
> > 
> > Dne 09. 04. 21 v 12:59 Takashi Iwai napsal(a):
> > 
> > >> 5. add any mechanism to bind lifetime of user-defined element set to user
> > >>    process
> > >>
> > >> At present, the lifetime of user-defined element set is bound to card
> > >> itself. However, it's convenient to user processes to bind the lifetime
> > >> to process itself. I add any mechanism for it.
> > >>
> > >> For recent years I've made some patches in house but never arrive at the
> > >> best one. In the patches, I utilize access flags but in general the
> > >> maintenance of lifetime is not easy issue. I tackle again in this time.
> > > 
> > > It sounds interesting, but I don't know how easily you can manage it.
> > > The driver doesn't care much about the user process lifetime, but
> > > mostly concentrate on the file handle...
> > 
> > It should be easy to trace which process created the user element and
> > automatically remove this element when the process close the file descriptor.
> > Something like 'bind lifetime of the control to the active control file
> > descriptor'.
> 
> If it's tied only with the file handle, it's easy.  But I thought this
> is about the process?

The 'lifetime' relates any operations from userspace relevant to
element. In the point, it's not so easy. It's better to see the list of
ioctl request to ALSA control character device, guys.


Regards

Takashi Sakamoto
Takashi Iwai April 10, 2021, 8:47 a.m. UTC | #10
On Sat, 10 Apr 2021 10:20:16 +0200,
Takashi Sakamoto wrote:
> 
> On Fri, Apr 09, 2021 at 06:09:02PM +0200, Takashi Iwai wrote:
> > On Fri, 09 Apr 2021 12:59:10 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Fri, 09 Apr 2021 04:27:35 +0200,
> > > Takashi Sakamoto wrote:
> > > > 
> > > > 4. Add `max_user_ctl_alloc_size` kobject attribute to the control device
> > > > 
> > > > In the patch, a new module parameter 'max_user_ctl_alloc_size' is added.
> > > > In the item, I use the value of this parameter as initial value per
> > > > control device. The value per control device can be changed via sysfs
> > > > node.
> > > > 
> > > > The `max_user_ctl_alloc_size` is really the attribute of control device,
> > > > so I think it acceptable. Additionally, 'curr_user_ctl_alloc_size' is
> > > > also added so that userspace applications get current status.
> > > 
> > > So that's the primary purpose?  Then it makes sense, yeah.
> > 
> > You meant something like below, right?
> 
> If you were carefully reading my items in the order, you would have
> realized that the patch includes problem to share attribute group
> table between several modules...

Which several modules...?  The control API is mandatory, hence it
can't be separated from each card core stuff.  So splitting to another
module makes no sense, as I already replied earlier.

> Device attribute is one of userspace interface expected to be stable. I'd
> like to avoid careless changes which our known developer tends to do.

Sure, it has to be set on stone once after put in the tree.
However, currently it's just a brain storming phase, and no need to
grumble for a dreadful future vision.


Takashi
Takashi Iwai April 10, 2021, 8:56 a.m. UTC | #11
On Sat, 10 Apr 2021 10:22:18 +0200,
Takashi Sakamoto wrote:
> 
> On Fri, Apr 09, 2021 at 04:18:00PM +0200, Takashi Iwai wrote:
> > On Fri, 09 Apr 2021 15:34:14 +0200,
> > Jaroslav Kysela wrote:
> > > 
> > > Dne 09. 04. 21 v 12:59 Takashi Iwai napsal(a):
> > > 
> > > >> 5. add any mechanism to bind lifetime of user-defined element set to user
> > > >>    process
> > > >>
> > > >> At present, the lifetime of user-defined element set is bound to card
> > > >> itself. However, it's convenient to user processes to bind the lifetime
> > > >> to process itself. I add any mechanism for it.
> > > >>
> > > >> For recent years I've made some patches in house but never arrive at the
> > > >> best one. In the patches, I utilize access flags but in general the
> > > >> maintenance of lifetime is not easy issue. I tackle again in this time.
> > > > 
> > > > It sounds interesting, but I don't know how easily you can manage it.
> > > > The driver doesn't care much about the user process lifetime, but
> > > > mostly concentrate on the file handle...
> > > 
> > > It should be easy to trace which process created the user element and
> > > automatically remove this element when the process close the file descriptor.
> > > Something like 'bind lifetime of the control to the active control file
> > > descriptor'.
> > 
> > If it's tied only with the file handle, it's easy.  But I thought this
> > is about the process?
> 
> The 'lifetime' relates any operations from userspace relevant to
> element. In the point, it's not so easy. It's better to see the list of
> ioctl request to ALSA control character device, guys.

I'm lost.  What makes so difficult if the element is tied with the
file descriptor?  You'd nuke the corresponding element at
snd_ctl_release(), and it should be enough.

If it's not tied with the file descriptor, it's indeed difficult, but
it has nothing to do with the number of ioctl implementations.


Takashi
Takashi Sakamoto April 18, 2021, 2:15 p.m. UTC | #12
Hi,

Sorry to be late.

On Sat, Apr 10, 2021 at 10:47:22AM +0200, Takashi Iwai wrote:
> On Sat, 10 Apr 2021 10:20:16 +0200,
> Takashi Sakamoto wrote:
> > 
> > On Fri, Apr 09, 2021 at 06:09:02PM +0200, Takashi Iwai wrote:
> > > On Fri, 09 Apr 2021 12:59:10 +0200,
> > > Takashi Iwai wrote:
> > > > 
> > > > On Fri, 09 Apr 2021 04:27:35 +0200,
> > > > Takashi Sakamoto wrote:
> > > > > 
> > > > > 4. Add `max_user_ctl_alloc_size` kobject attribute to the control device
> > > > > 
> > > > > In the patch, a new module parameter 'max_user_ctl_alloc_size' is added.
> > > > > In the item, I use the value of this parameter as initial value per
> > > > > control device. The value per control device can be changed via sysfs
> > > > > node.
> > > > > 
> > > > > The `max_user_ctl_alloc_size` is really the attribute of control device,
> > > > > so I think it acceptable. Additionally, 'curr_user_ctl_alloc_size' is
> > > > > also added so that userspace applications get current status.
> > > > 
> > > > So that's the primary purpose?  Then it makes sense, yeah.
> > > 
> > > You meant something like below, right?
> > 
> > If you were carefully reading my items in the order, you would have
> > realized that the patch includes problem to share attribute group
> > table between several modules...
> 
> Which several modules...?  The control API is mandatory, hence it
> can't be separated from each card core stuff.  So splitting to another
> module makes no sense, as I already replied earlier.
> 
> > Device attribute is one of userspace interface expected to be stable. I'd
> > like to avoid careless changes which our known developer tends to do.
> 
> Sure, it has to be set on stone once after put in the tree.
> However, currently it's just a brain storming phase, and no need to
> grumble for a dreadful future vision.

Oops, I realised to have overlooked your previous post:
https://lore.kernel.org/alsa-devel/s5h5z0v67wh.wl-tiwai@suse.de/

Let me back to it so that we share the premises for the discussion.


Thanks

Takashi Sakamoto
Takashi Sakamoto April 18, 2021, 2:27 p.m. UTC | #13
Hi,

Sorry to be late for catching up...

On Fri, Apr 09, 2021 at 12:59:10PM +0200, Takashi Iwai wrote:
> On Fri, 09 Apr 2021 04:27:35 +0200,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > On Thu, Apr 08, 2021 at 01:33:41PM +0200, Takashi Iwai wrote:
> > > On Thu, 08 Apr 2021 12:50:25 +0200, Takashi Sakamoto wrote:
> > > > On Thu, Apr 08, 2021 at 07:31:49PM +0900, Takashi Sakamoto wrote:
> > > > > 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 (ILP32) or
> > > > > 1024 bytes (LP64) 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. ALSA 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_alloc_size', so that
> > > > > user can increase/decrease the limit if really needed, too.
> > > > > 
> > > > > Co-developed-by: Takashi Iwai <tiwai@suse.de>
> > > > > Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > > > Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > > > > ---
> > > > > v1->v2: Drop alloc_size field from user_element, calculate at private_free
> > > > > v2->v3: Rebase. Fix boundary error. Obsolete macro usage relying on modern
> > > > >         compiler optimization. Change comment style by modern coding
> > > > >         convention. Rename module parameter so that users get it easily.
> > > > >         Patch comment improvements.
> > > > > ---
> > > > >  include/sound/core.h |  2 +-
> > > > >  sound/core/control.c | 75 ++++++++++++++++++++++++++++++--------------
> > > > >  2 files changed, 52 insertions(+), 25 deletions(-)
> > > > 
> > > > The original content of patch comes from Iwai-san[1]. I have no clear
> > > > idea to handle the case so add 'Co-developed-by' tag to the patch. If
> > > > this is not good, I apologize the lack of my understanding to the
> > > > development process in Linux kernel.
> > > 
> > > It depends.  In some cases, you just carry the patch with the original
> > > authorship (From address) and put your sign-off.  In some cases,
> > > Co-developed-by can be used.  I don't mind much either way, so I took
> > > your v3 patch now (with the addition of the Link URL to v2 patch).
> > 
> > Thanks for applying the patch as is. I would post it just with my sign-off
> > without no changes to your patch, However in the case I added some changes,
> > so I have no conviction to it...
> > 
> > Well, relevant to the function, I have some ideas to refactor ALSA control
> > core. If you have room to discuss about them, I'd like to ask your opinion.
> > 
> > At present, I have five ideas:
> > 
> > 1. Split code relevant to user-defined element set into new module
> > 
> > Although the function is itself useful to me, it's useless in the case
> > to use driver in which every functions are in kernel land, especially in
> > embedded systems. The layering function introduced recently (and ctl ioctl
> > registration function) enables to capsulate it into module. This results
> > in building the function according to kernel configuration and reduction
> > of the size of snd.ko for embedded systems. (But I wish usual desktop
> > environment enables it...)
> > 
> > In my plan, the name of new module is snd_ctl_user_elem_set.ko and the
> > configuration is CONFIG_SND_CTL_USER_ELEM_SETS. I've already written
> > patchset in my hand and find some negative points:
> > 
> >  * Comparing environments in which the function is enable or disabled,
> >    we have difference about the system behaviour against some ioctl
> >    requests (ELEM_ADD, ELEM_REPLACE, ELEM_REMOVE). I have no idea to
> >    judge whether this is evil or not.
> >  * Some internal functions and tables in snd.ko should be expoted to the
> >    new module; e.g. 'value_sizes' or 'snd_ctl_new()'. The symbol table
> >    is increased.
> >  * Some code should be moved from compatibility layer of ALSA control
> >    core. This seems to increate the cost of maintenance for the layer.
> 
> The module would be useful if this can work additionally on top of the
> others.  And, in the case of user-element, it has nothing to do with
> the driver, so if the module is split, user would have to load the
> module manually -- which is inconvenient.
> 
> If your concern is about the driver size, the needed change isn't
> about splitting to another module but the conditional builds either
> with ifdef or factor out to another file (and conditionally build via
> Makefile).
 
In a point of driver side, we have some solution. Usage of
'request_module()', as control-led layer does. Or exported symbol from
the module takes userspace kernel module loader to work expectedly
according to module dependency graph, as long as device driver refers to
it.

Nevertheless, in a point of userspace application side, we seems to have
no good way in non-privilege process. In this point, I agree with the
inconvenience about which you mentioned.

The point is which stuff is dominant to determine usage of the function,
in my opinion. In the case of ALSA firewire stack (or HDA driver for
some platforms), it's driver side. In the case of softvol plugin in
alsa-lib, it's usrspace side. When standing on the former, modular
function is enough convenient. On the other hand, for the latter,
it's not necessarily convenient.

> > 2. Introduce control component structure and move codes from card structure
> > 
> > This is just an idea and preparation for following items. Historically,
> > ALSA card structure includes some control-related stuffs. The card has
> > two Linux device structures for pseudo card (card_dev) and control
> > cdev (ctl_dev).  The card also aggregates the list of the other
> > components such as pcm, hwdep. In this item, I add a new control
> > structure and split control related stuffs from card structure. As a
> > result, the control component becomes to be equivalent to the other
> > components, in a point of both relationship to pseudo card device and
> > relationship to cdev.
> > 
> > The change results in the reduction of size of card structure somehow. I
> > expect it to be friendly to memory object allocator, and to be clear
> > view of code structure.
> 
> Well, moving the control-related fields into another allocated object
> wouldn't reduce the size in total, so I don't see any big merit by
> that.  Note that the control API is mandatory for each card, hence it
> can be never optional; that's the difference from other components.
> 
> Though, moving control-related fields into another struct and embed it
> in snd_card would be fine if it improves the readability.  It'll be
> essentially just grouping and renaming.

The readability is certainly improved by grouping and renaming. But in a
point of actual memory consumption in slab allocation, I can find another
merit in scenario to split structures, since larger structure brings
larger unused space in object according to cache size of slab
allocation, in theory.

In system V ABI for LP64, the size of 'struct snd_card' is 2248 bytes as
maximum, then 4k object is used for it. When splitting into structures,
we can reduce the unused space. As long as I calculated, the issued
control structure reduces the size of card structure up to 1400 bytes,
and its size is 864 bytes (including pointer to card and member for
devices list), then 2k and 1k objects are used. Rough calculation brings
1k free memory between these cases (for simplicity I omit administration
space).

As you note, control component is not optional for card. However,
control component is actually maintained as device component. As current
card implementation maintains each component successfully, it's worth to
investigate putting control-related members from card to unique
structure behind private data of component. Additionally, when integrating
control functionality, it's convenient to me that relevant stuffs are
capsulated apart from card structure. In short, I'd like 'divide and
conquer' method in code refactoring.


Thanks

Takashi Sakamoto
Jaroslav Kysela April 18, 2021, 6:42 p.m. UTC | #14
Dne 18. 04. 21 v 16:27 Takashi Sakamoto napsal(a):

> As you note, control component is not optional for card. However,
> control component is actually maintained as device component. As current
> card implementation maintains each component successfully, it's worth to
> investigate putting control-related members from card to unique
> structure behind private data of component. Additionally, when integrating
> control functionality, it's convenient to me that relevant stuffs are
> capsulated apart from card structure. In short, I'd like 'divide and
> conquer' method in code refactoring.

I guess, you may show us some code for comments (just a prototype - no full
implementation). As you noted, the separate allocations may save some space
depending on the slab implementation, but it should not be the main reason for
such changes.

The device objects were added to the ALSA control stack later (they simply
didn't exist when we designed it). Also, the low-level drivers communicate
with the control layer using the card structure pointer. The additional
translation is another challenge.

						Jaroslav
diff mbox series

Patch

diff --git a/include/sound/core.h b/include/sound/core.h
index 2e24f194ef70..1f9aef0adbc9 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 20d707d4ef40..a076c08c21b6 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,8 +19,11 @@ 
 #include <sound/info.h>
 #include <sound/control.h>
 
-/* max number of user-defined controls */
-#define MAX_USER_CONTROLS	32
+// Max allocation size for user controls.
+static int max_user_ctl_alloc_size = 8 * 1024 * 1024;
+module_param_named(max_user_ctl_alloc_size, max_user_ctl_alloc_size, int, 0444);
+MODULE_PARM_DESC(max_user_ctl_alloc_size, "Max allocation size for user controls");
+
 #define MAX_CONTROL_COUNT	1028
 
 struct snd_kctl_ioctl {
@@ -561,9 +565,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;
@@ -1265,6 +1266,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 check_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)
 {
@@ -1342,6 +1349,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 (check_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);
@@ -1359,11 +1370,16 @@  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;
+	// decremented at private_free.
+	ue->card->user_ctl_alloc_size += size;
 
 	mask |= SNDRV_CTL_EVENT_MASK_TLV;
 	for (i = 0; i < kctl->count; ++i)
@@ -1405,16 +1421,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 (check_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);
@@ -1428,14 +1445,27 @@  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;
 }
 
+static size_t compute_user_elem_size(size_t size, unsigned int count)
+{
+	return 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 -= compute_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);
@@ -1449,6 +1479,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;
@@ -1466,13 +1497,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)
@@ -1503,6 +1527,10 @@  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 = compute_user_elem_size(private_size, count);
+
+	if (check_user_elem_overflow(card, alloc_size))
+		return -ENOMEM;
 
 	/*
 	 * Keep memory object for this userspace control. After passing this
@@ -1514,16 +1542,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;
@@ -1565,9 +1595,6 @@  static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	 * applications because the field originally means PID of a process
 	 * which locks the element.
 	 */
-
-	card->user_ctl_count++;
-
  unlock:
 	up_write(&card->controls_rwsem);
 	return err;