[alsa-devel] [PATCH 2/3] ALSA: control: add dimension validator for userspace element

[Takashi Iwai]

On Wed, 06 Jul 2016 16:18:48 +0200,
Takashi Sakamoto wrote:
> 
> On Jul 6 2016 22:34, Takashi Iwai wrote:
> > Yes, a validation of info->count is a good idea.
> > And it can be even a simple WARN_ON().  It's a clear driver bug and
> > it's better to be exposed as loudly as possible.
> 
> OK.
> 
> Then another question. The same function, snd_ctl_elem_info() uses a
> combination of  at CONFIG_SND_DEBUG and snd_BUG_ON() to detect
> info->access bug of each driver.
> 
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n823
> 
> I guess that this is an assertion that each driver must not touch
> info->access in its implementations of 'snd_kcontrol_info_t'. On the
> other hand, the detection is just enabled at CONFIG_SND_DEBUG.
> 
> To me, it's strange, because ALSA control core changes its behaviour
> depending on CONFIG_SND_DEBUG. If the option is off, buggy driver works.
> Else, it brings kernel panic. This is quite confusing to both developers
> of each driver and userspace applications.
> 
> In ALSA kernel/userspace interfaces, info->access is described as read-only.
> http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h#n886
> 
> Therefore, I think it better to set zero in advance of calling
> 'snd_kcontrol_info_t' then check the return value with WARN_ON macro,
> regardless of CONFIG_SND_DEBUG.
> 
> Or should I apply snd_WARN_ON() or snd_BUG_ON() to validators of
> info->type and info->count to keep code consistency?

It's because we preferred the size optimization over the sanity check
in the past.  A sanity check is a thing to be done only during the
debug / development phase, and a production kernel we already have
must be clean from such a stupid bug -- that's the idea behind it.
That's why snd_BUG_ON() is enabled only with the debug option.
(BTW, there is no snd_WARN_ON().  snd_BUG_ON() corresponds to
 WARN_ON().)

This kind of concept was commonly seen in the earlier Linux kernels,
as we had to care more about the memory footprint ("each byte
matteres!"). Meanwhile, we have plenty of memory nowadays, and only
few people really care about the kernel size.  Hence people prefer a
standard macro (WARN_ON()) to an ALSA-specific one (snd_BUG_ON()) in
general recently.  But, it doesn't necessarily mean that we *should*
use / convert to WARN_ON().  It would still increase the kernel memory
footprint.

If I were to implement something, I'd take WARN_ON() for now.  But
keeping the same snd_BUG_ON() would be also good.  That said, I have
no preference at all.  This is merely a sanity check, and each driver
developer should test both with and without the debug option once,
after all.


Takashi