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

From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
Subject: Re: [PATCH v3] ALSA: control: Add memory consumption limit to user controls
Date: Sun, 18 Apr 2021 23:27:32 +0900	[thread overview]
Message-ID: <20210418142732.GB36507@workstation> (raw)
In-Reply-To: <s5h5z0v67wh.wl-tiwai@suse.de>

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