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

From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: tiwai@suse.de
Cc: alsa-devel@alsa-project.org
Subject: Re: [PATCH v3] ALSA: control: Add memory consumption limit to user controls
Date: Thu, 8 Apr 2021 19:50:25 +0900	[thread overview]
Message-ID: <20210408105025.GB40407@workstation> (raw)
In-Reply-To: <20210408103149.40357-1-o-takashi@sakamocchi.jp>

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