Re: [PATCH] ALSA: control_led: fix stack frame usage over 1024 bytes in snd_ctl_led_get()

[Takashi Iwai <tiwai@suse.de>]

On Tue, 13 Apr 2021 22:53:34 +0200,
Nathan Chancellor wrote:
> 
> On Sun, Apr 04, 2021 at 03:59:29PM +0900, Takashi Sakamoto wrote:
> > GCC warns that snd_ctl_led_get() uses stack frame over 1024 bytes.
> > 
> > control_led.c: In function ‘snd_ctl_led_get’:
> > control_led.c:128:1: warning: the frame size of 1504 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > 
> > When taking care of convension in Linux kernel development. it's preferable
> > to suppress too large usage of kernel stack, when the function call is not
> > in critical part.
> > 
> > This commit fixes the bug, including some minor code refactoring relevant
> > to variable names.
> > 
> > Fixes: 22d8de62f11b ("ALSA: control - add generic LED trigger module as the new control layer")
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > ---
> >  sound/core/control_led.c | 68 ++++++++++++++++++++++++++--------------
> >  1 file changed, 44 insertions(+), 24 deletions(-)
> > 
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index b97f118cd54e..1be854a861f0 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -94,34 +94,35 @@ static struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access)
> >  	return &snd_ctl_leds[group];
> >  }
> >  
> > -static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl)
> > +static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl, struct snd_ctl_elem_info *elem_info,
> > +			   struct snd_ctl_elem_value *elem_value)
> >  {
> >  	struct snd_kcontrol *kctl = lctl->kctl;
> > -	struct snd_ctl_elem_info info;
> > -	struct snd_ctl_elem_value value;
> >  	unsigned int i;
> > -	int result;
> > -
> > -	memset(&info, 0, sizeof(info));
> > -	info.id = kctl->id;
> > -	info.id.index += lctl->index_offset;
> > -	info.id.numid += lctl->index_offset;
> > -	result = kctl->info(kctl, &info);
> > -	if (result < 0)
> > +	int err;
> > +
> > +	memset(elem_info, 0, sizeof(*elem_info));
> > +	elem_info->id = kctl->id;
> > +	elem_info->id.index += lctl->index_offset;
> > +	elem_info->id.numid += lctl->index_offset;
> > +	err = kctl->info(kctl, elem_info);
> > +	if (err < 0)
> >  		return -1;
> > -	memset(&value, 0, sizeof(value));
> > -	value.id = info.id;
> > -	result = kctl->get(kctl, &value);
> > -	if (result < 0)
> > +
> > +	memset(elem_value, 0, sizeof(*elem_value));
> > +	elem_value->id = elem_info->id;
> > +	err = kctl->get(kctl, elem_value);
> > +	if (err < 0)
> >  		return -1;
> > -	if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
> > -	    info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
> > -		for (i = 0; i < info.count; i++)
> > -			if (value.value.integer.value[i] != info.value.integer.min)
> > +
> > +	if (elem_info->type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
> > +	    elem_info->type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
> > +		for (i = 0; i < elem_info->count; i++)
> > +			if (elem_value->value.integer.value[i] != elem_info->value.integer.min)
> >  				return 1;
> > -	} else if (info.type == SNDRV_CTL_ELEM_TYPE_INTEGER64) {
> > -		for (i = 0; i < info.count; i++)
> > -			if (value.value.integer64.value[i] != info.value.integer64.min)
> > +	} else if (elem_info->type == SNDRV_CTL_ELEM_TYPE_INTEGER64) {
> > +		for (i = 0; i < elem_info->count; i++)
> > +			if (elem_value->value.integer64.value[i] != elem_info->value.integer64.min)
> >  				return 1;
> >  	}
> >  	return 0;
> > @@ -132,6 +133,8 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
> >  {
> >  	struct snd_ctl_led *led;
> >  	struct snd_ctl_led_ctl *lctl;
> > +	struct snd_ctl_elem_info *elem_info;
> > +	struct snd_ctl_elem_value *elem_value;
> >  	int route;
> >  	bool found;
> >  
> > @@ -146,10 +149,24 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
> >  		mutex_unlock(&snd_ctl_led_mutex);
> >  		return;
> >  	}
> > +
> > +	elem_info = kmalloc(sizeof(*elem_info), GFP_KERNEL);
> > +	if (!elem_info) {
> > +		mutex_unlock(&snd_ctl_led_mutex);
> > +		return;
> > +	}
> > +
> > +	elem_value = kmalloc(sizeof(*elem_value), GFP_KERNEL);
> > +	if (!elem_value) {
> > +		kfree(elem_info);
> > +		mutex_unlock(&snd_ctl_led_mutex);
> > +		return;
> > +	}
> > +
> >  	list_for_each_entry(lctl, &led->controls, list) {
> >  		if (lctl->kctl == kctl && lctl->index_offset == ioff)
> >  			found = true;
> > -		UPDATE_ROUTE(route, snd_ctl_led_get(lctl));
> > +		UPDATE_ROUTE(route, snd_ctl_led_get(lctl, elem_info, elem_value));
> >  	}
> >  	if (!found && kctl && card) {
> >  		lctl = kzalloc(sizeof(*lctl), GFP_KERNEL);
> > @@ -159,10 +176,13 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
> >  			lctl->kctl = kctl;
> >  			lctl->index_offset = ioff;
> >  			list_add(&lctl->list, &led->controls);
> > -			UPDATE_ROUTE(route, snd_ctl_led_get(lctl));
> > +			UPDATE_ROUTE(route, snd_ctl_led_get(lctl, elem_info, elem_value));
> >  		}
> >  		kfree(lctl);
> 
> This patch is still relevant on latest -next and this hunk prevents the
> patch from applying cleanly because that patch was NAK'd:
> 
> https://lore.kernel.org/alsa-devel/20210404064031.48711-1-o-takashi@sakamocchi.jp/

Can anyone re-submit a version that just does fix this issue without
doing anything else?


Takashi