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

8 Apr 2021


      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.ht...
[2] https://github.com/alsa-project/alsa-gobject/
Takashi Sakamoto