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

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

[Takashi Sakamoto]

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);
 	}
+
+	kfree(elem_value);
+	kfree(elem_info);
 	mutex_unlock(&snd_ctl_led_mutex);
 	switch (led->mode) {
 	case MODE_OFF:		route = 1; break;
-- 
2.27.0

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

[Jaroslav Kysela]

Dne 04. 04. 21 v 8:59 Takashi Sakamoto napsal(a):
> 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)

I think info, value should be enough as names for the arguments here. It
increases readability.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Nathan Chancellor]

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/

Cheers,
Nathan

>  	}
> +
> +	kfree(elem_value);
> +	kfree(elem_info);
>  	mutex_unlock(&snd_ctl_led_mutex);
>  	switch (led->mode) {
>  	case MODE_OFF:		route = 1; break;
> -- 
> 2.27.0
> 

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

[Takashi Iwai]

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

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

[Jaroslav Kysela]

Dne 14. 04. 21 v 9:32 Takashi Iwai napsal(a):

>> 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?

I'll create a proper fix with the cached allocations in struct snd_ctl_led.

				Thanks,
					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.