Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1677903 > unrolled thread

Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage  pvr2_eeprom_analyze() Ben Hutchings <ben.hutchings@codethink.co.uk> - 2017-06-29 18:20 +0200
    Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage  pvr2_eeprom_analyze() Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-03 09:40 +0200
      Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze() Arnd Bergmann <arnd@arndb.de> - 2017-07-03 09:50 +0200
        Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze() Arnd Bergmann <arnd@arndb.de> - 2017-07-03 16:40 +0200
        Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage  pvr2_eeprom_analyze() Ben Hutchings <ben.hutchings@codethink.co.uk> - 2017-07-03 17:10 +0200

#1677903 — Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze()

On Mon, 2017-06-19 at 23:20 +0800, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> commit 6830733d53a4517588e56227b9c8538633f0c496 upstream.
> 
> The driver uses a relatively large data structure on the stack, which
> showed up on my radar as we get a warning with the "latent entropy"
> GCC plugin:
> 
> drivers/media/usb/pvrusb2/pvrusb2-eeprom.c:153:1: error: the frame size of 1376 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
> 
> The warning is usually hidden as we raise the warning limit to 2048
> when the plugin is enabled, but I'd like to lower that again in the
> future, and making this function smaller helps to do that without
> build regressions.
> 
> Further analysis shows that putting an 'i2c_client' structure on
> the stack is not really supported, as the embedded 'struct device'
> is not initialized here, and we are only saved by the fact that
> the function that is called here does not use the pointer at all.
[...]

That is not true in 4.4-stable.  This commit depends on:

commit 6037b3ca28f4258d913dbe77248fd77827702ae3
Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Date:   Wed Nov 16 14:21:48 2016 -0200

    [media] tveeprom: print log messages using pr_foo()

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

[toc] | [next] | [standalone]

#1679703

On Thu, Jun 29, 2017 at 05:15:17PM +0100, Ben Hutchings wrote:
> On Mon, 2017-06-19 at 23:20 +0800, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > commit 6830733d53a4517588e56227b9c8538633f0c496 upstream.
> > 
> > The driver uses a relatively large data structure on the stack, which
> > showed up on my radar as we get a warning with the "latent entropy"
> > GCC plugin:
> > 
> > drivers/media/usb/pvrusb2/pvrusb2-eeprom.c:153:1: error: the frame size of 1376 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
> > 
> > The warning is usually hidden as we raise the warning limit to 2048
> > when the plugin is enabled, but I'd like to lower that again in the
> > future, and making this function smaller helps to do that without
> > build regressions.
> > 
> > Further analysis shows that putting an 'i2c_client' structure on
> > the stack is not really supported, as the embedded 'struct device'
> > is not initialized here, and we are only saved by the fact that
> > the function that is called here does not use the pointer at all.
> [...]
> 
> That is not true in 4.4-stable.  This commit depends on:
> 
> commit 6037b3ca28f4258d913dbe77248fd77827702ae3
> Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Date:   Wed Nov 16 14:21:48 2016 -0200
> 
>     [media] tveeprom: print log messages using pr_foo()

It does?  I don't understand how the two are connected.  Removing
i2c_client off of the stack is a good thing.  Ah, I see how the pointer
is used in tveeprom_hauppauge_analog(), but this shouldn't matter here,
right?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1679714 — Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze()

On Mon, Jul 3, 2017 at 9:31 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 29, 2017 at 05:15:17PM +0100, Ben Hutchings wrote:
>> On Mon, 2017-06-19 at 23:20 +0800, Greg Kroah-Hartman wrote:
>> > 4.4-stable review patch.  If anyone has any objections, please let me know.
>> >
>> > From: Arnd Bergmann <arnd@arndb.de>
>> >
>> > commit 6830733d53a4517588e56227b9c8538633f0c496 upstream.
>> >
>> > The driver uses a relatively large data structure on the stack, which
>> > showed up on my radar as we get a warning with the "latent entropy"
>> > GCC plugin:
>> >
>> > drivers/media/usb/pvrusb2/pvrusb2-eeprom.c:153:1: error: the frame size of 1376 bytes is larger than 1152 bytes [-Werror=frame-larger-than=]
>> >
>> > The warning is usually hidden as we raise the warning limit to 2048
>> > when the plugin is enabled, but I'd like to lower that again in the
>> > future, and making this function smaller helps to do that without
>> > build regressions.
>> >
>> > Further analysis shows that putting an 'i2c_client' structure on
>> > the stack is not really supported, as the embedded 'struct device'
>> > is not initialized here, and we are only saved by the fact that
>> > the function that is called here does not use the pointer at all.
>> [...]
>>
>> That is not true in 4.4-stable.  This commit depends on:
>>
>> commit 6037b3ca28f4258d913dbe77248fd77827702ae3
>> Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>> Date:   Wed Nov 16 14:21:48 2016 -0200
>>
>>     [media] tveeprom: print log messages using pr_foo()
>
> It does?  I don't understand how the two are connected.  Removing
> i2c_client off of the stack is a good thing.  Ah, I see how the pointer
> is used in tveeprom_hauppauge_analog(), but this shouldn't matter here,
> right?

My reading of the two patches is that we actually need at least one
of them to avoid interpreting uninitialized dev->class/bus:  With just my
6830733d53a patch, we replace the uninitialized data with a NULL
pointer, which is handled gracefully in __dev_printk(), while the
6037b3ca28 patch by itself will avoid using the 'dev' pointer completely,
and give a saner output (no "(NULL device)" string or worse).

I think we probably want both of them backported to 4.4, but I don't see a
dependency between them.

        Arnd

[toc] | [prev] | [next] | [standalone]

#1680284 — Re: [PATCH 4.4 16/30] [media] pvrusb2: reduce stack usage pvr2_eeprom_analyze()

On Mon, Jul 3, 2017 at 3:37 PM, Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> On Mon, 2017-07-03 at 09:47 +0200, Arnd Bergmann wrote:
>> On Mon, Jul 3, 2017 at 9:31 AM, Greg Kroah-Hartman
>>
>> I think we probably want both of them backported to 4.4, but I don't see a
>> dependency between them.
>
> Sorry, I mixed up two commits.  The one actually needed to avoid a null
> dereference is:
>
> commit 7cda4c5bae46ffca3abeadc4c1882d9325ee3102
> Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Date:   Thu Oct 13 10:51:20 2016 -0300
>
>     [media] tveeprom: use dev_foo() for printk messages
>
> Then "[media] tveeprom: print log messages using pr_foo()" is cleanup on
> top of that.

Ah, I see now. We do indeed need both of Mauro's patches before we
can apply mine then.

        Arnd

[toc] | [prev] | [next] | [standalone]

#1680408

On Mon, 2017-07-03 at 09:47 +0200, Arnd Bergmann wrote:
> On Mon, Jul 3, 2017 at 9:31 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Jun 29, 2017 at 05:15:17PM +0100, Ben Hutchings wrote:
> >> On Mon, 2017-06-19 at 23:20 +0800, Greg Kroah-Hartman wrote:
> >> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> >> >
> >> > From: Arnd Bergmann <arnd@arndb.de>
> >> >
> >> > commit 6830733d53a4517588e56227b9c8538633f0c496 upstream.
[...]
> >> > Further analysis shows that putting an 'i2c_client' structure on
> >> > the stack is not really supported, as the embedded 'struct device'
> >> > is not initialized here, and we are only saved by the fact that
> >> > the function that is called here does not use the pointer at all.
> >> [...]
> >>
> >> That is not true in 4.4-stable.  This commit depends on:
> >>
> >> commit 6037b3ca28f4258d913dbe77248fd77827702ae3
> >> Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> >> Date:   Wed Nov 16 14:21:48 2016 -0200
> >>
> >>     [media] tveeprom: print log messages using pr_foo()
> >
> > It does?  I don't understand how the two are connected.  Removing
> > i2c_client off of the stack is a good thing.  Ah, I see how the pointer
> > is used in tveeprom_hauppauge_analog(), but this shouldn't matter here,
> > right?
> 
> My reading of the two patches is that we actually need at least one
> of them to avoid interpreting uninitialized dev->class/bus:  With just my
> 6830733d53a patch, we replace the uninitialized data with a NULL
> pointer, which is handled gracefully in __dev_printk(), while the
> 6037b3ca28 patch by itself will avoid using the 'dev' pointer completely,
> and give a saner output (no "(NULL device)" string or worse).
> 
> I think we probably want both of them backported to 4.4, but I don't see a
> dependency between them.

Sorry, I mixed up two commits.  The one actually needed to avoid a null
dereference is:

commit 7cda4c5bae46ffca3abeadc4c1882d9325ee3102
Author: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Date:   Thu Oct 13 10:51:20 2016 -0300

    [media] tveeprom: use dev_foo() for printk messages

Then "[media] tveeprom: print log messages using pr_foo()" is cleanup on
top of that.

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web