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

Groups > linux.kernel > #1310104 > unrolled thread

Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables

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] dell-wmi: Stop storing pointers to DMI tables Jean Delvare <jdelvare@suse.de> - 2016-01-15 14:30 +0100
    Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@amacapital.net> - 2016-01-15 17:30 +0100
      Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Jean Delvare <jdelvare@suse.de> - 2016-01-15 19:00 +0100
        Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@amacapital.net> - 2016-01-15 21:10 +0100
          Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Jean Delvare <jdelvare@suse.de> - 2016-01-19 10:20 +0100

#1310104 — Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables

Hi Andy,

Sorry for the late reply.

On Wed, 13 Jan 2016 14:28:18 -0800, Andy Lutomirski wrote:
> [cc: Jean Delvare]
> 
> On Tue, Jan 12, 2016 at 6:25 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Monday 11 January 2016 13:58:20 Andy Lutomirski wrote:
> >> On Sun, Jan 3, 2016 at 6:52 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >> > The dmi_walk function maps the DMI table, walks it, and unmaps it.
> >> > This means that the dell_bios_hotkey_table that find_hk_type stores
> >> > points to unmapped memory by the time it gets read.
> >> >
> >> > I've been able to trigger crashes caused by the stale pointer a
> >> > couple of times, but never on a stock kernel.
> >> >
> >> > Fix it by generating the keymap in the dmi_walk callback instead of
> >> > storing a pointer.
> >>
> >> Quick ping: has anyone had a chance to look at this?
> >
> > Hi Andy, I looked at this patch, but I think some people from -mm or DMI
> > code should look at it as it is memory problem... We also has one in
> > dell-laptop.ko (wrong API usage) and so -mm people could know it better.
> 
> Let's ask:
> 
> Jean, am I right that drivers must not store pointers to DMI tables
> that they find through dmi_walk?

Yes, you are right.

> Is there any alternative interface
> that could be used to get a longer-lived pointer to DMI tables, or
> should drivers just parse them and copy out any info needed from the
> dmi_walk callback?

There is no alternative for OEM type records. Drivers are indeed
expected to copy the information they need to their own buffers.

> There are at least two platform drivers (dell-wmi and dell-laptop)
> that don't play well with the current interface.  This patch is
> intended to fix one of them.

Couldn't see any problem with dell-laptop.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [next] | [standalone]

#1310251

On Fri, Jan 15, 2016 at 5:27 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Andy,
>
> Sorry for the late reply.
>
> On Wed, 13 Jan 2016 14:28:18 -0800, Andy Lutomirski wrote:
>> [cc: Jean Delvare]
>>
>> On Tue, Jan 12, 2016 at 6:25 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
>> > On Monday 11 January 2016 13:58:20 Andy Lutomirski wrote:
>> >> On Sun, Jan 3, 2016 at 6:52 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> >> > The dmi_walk function maps the DMI table, walks it, and unmaps it.
>> >> > This means that the dell_bios_hotkey_table that find_hk_type stores
>> >> > points to unmapped memory by the time it gets read.
>> >> >
>> >> > I've been able to trigger crashes caused by the stale pointer a
>> >> > couple of times, but never on a stock kernel.
>> >> >
>> >> > Fix it by generating the keymap in the dmi_walk callback instead of
>> >> > storing a pointer.
>> >>
>> >> Quick ping: has anyone had a chance to look at this?
>> >
>> > Hi Andy, I looked at this patch, but I think some people from -mm or DMI
>> > code should look at it as it is memory problem... We also has one in
>> > dell-laptop.ko (wrong API usage) and so -mm people could know it better.
>>
>> Let's ask:
>>
>> Jean, am I right that drivers must not store pointers to DMI tables
>> that they find through dmi_walk?
>
> Yes, you are right.
>
>> Is there any alternative interface
>> that could be used to get a longer-lived pointer to DMI tables, or
>> should drivers just parse them and copy out any info needed from the
>> dmi_walk callback?
>
> There is no alternative for OEM type records. Drivers are indeed
> expected to copy the information they need to their own buffers.

FWIW, especially if we consider mapping it persistently, maybe we
should use ioremap_prot and map it both cached and ro.

Actually, switching to a cached mapping regardless of persistence
could noticeably help boot times.  UC accesses are very, very slow.

--Andy

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

#1310331

Hi Andy,

Le Friday 15 January 2016 à 08:27 -0800, Andy Lutomirski a écrit :
> FWIW, especially if we consider mapping it persistently, maybe we
> should use ioremap_prot and map it both cached and ro.
> 
> Actually, switching to a cached mapping regardless of persistence
> could noticeably help boot times.  UC accesses are very, very slow.

Sorry if it is obvious for everybody else, but what does "UC accesses"
mean?

-- 
Jean Delvare
SUSE L3 Support

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

#1310437

On Fri, Jan 15, 2016 at 9:53 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Andy,
>
> Le Friday 15 January 2016 à 08:27 -0800, Andy Lutomirski a écrit :
>> FWIW, especially if we consider mapping it persistently, maybe we
>> should use ioremap_prot and map it both cached and ro.
>>
>> Actually, switching to a cached mapping regardless of persistence
>> could noticeably help boot times.  UC accesses are very, very slow.
>
> Sorry if it is obvious for everybody else, but what does "UC accesses"
> mean?
>

Sorry, I sometimes have my head buried too far in the CPU :)

UC means uncached.  ioremap, on x86, asks for an uncached mapping, so
every memory access (load or store) hits main memory individually.
Assuming that the spec says that whatever physical memory the DMI
tables live in is permitted to be used with cached accesses, asking
for the CPU cache to be permitted on those accesses will make them a
whole lot faster.

If that isn't safe, you could also just copy each table out of the
ioremap space into normal RAM as needed using MOVNTDQA.  I forget what
the helper for that is called, but it basically does a fast streaming
IO read and then writes to normal RAM, memcpy style.  Most modern CPUs
support it.

--Andy

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

#1312034

Hi Andy,

On Fri, 15 Jan 2016 12:00:02 -0800, Andy Lutomirski wrote:
> UC means uncached.  ioremap, on x86, asks for an uncached mapping, so
> every memory access (load or store) hits main memory individually.
> Assuming that the spec says that whatever physical memory the DMI
> tables live in is permitted to be used with cached accesses, asking
> for the CPU cache to be permitted on those accesses will make them a
> whole lot faster.
> 
> If that isn't safe, you could also just copy each table out of the
> ioremap space into normal RAM as needed using MOVNTDQA.  I forget what
> the helper for that is called, but it basically does a fast streaming
> IO read and then writes to normal RAM, memcpy style.  Most modern CPUs
> support it.

I have no idea what is allowed and what isn't, sorry. You would have to
check the SMBIOS specification but also the UEFI specification.

I have to admit I never understood why dmi_alloc is arch-specific nor
why dmi_remap is needed in the first place (and even less why
dmi_early_remap is different.) So I'm not going to mess up with that
code.

I have no idea how costly dmi_remap() is, but certainly it is being
called more and more as we can see dmi_walk() gaining in popularity. So
if anyone is worried about the performance, I'll be happy to review and
test patches.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web