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

Groups > linux.kernel > #1320106 > unrolled thread

Re: [PATCH] cpufreq: Fix NULL reference crash while accessing policy->governor_data

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] cpufreq: Fix NULL reference crash while accessing policy->governor_data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-28 00:00 +0100
    Re: [PATCH] cpufreq: Fix NULL reference crash while accessing  policy->governor_data Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-28 03:20 +0100
      Re: [PATCH] cpufreq: Fix NULL reference crash while accessing  policy->governor_data Juri Lelli <juri.lelli@arm.com> - 2016-01-28 11:50 +0100
      Re: [PATCH] cpufreq: Fix NULL reference crash while accessing policy->governor_data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-29 04:40 +0100

#1320106 — Re: [PATCH] cpufreq: Fix NULL reference crash while accessing policy->governor_data

On Monday, January 25, 2016 10:33:46 PM Viresh Kumar wrote:
> There is a little race discovered by Juri, where we are able to:
> - create and read a sysfs file before policy->governor_data is being set
>   to a non NULL value.
>   OR
> - set policy->governor_data to NULL, and reading a file before being
>   destroyed.
> 
> And so such a crash is reported:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> pgd = edfc8000
> [0000000c] *pgd=bfc8c835
> Internal error: Oops: 17 [#1] SMP ARM
> Modules linked in:
> CPU: 4 PID: 1730 Comm: cat Not tainted 4.5.0-rc1+ #463
> Hardware name: ARM-Versatile Express
> task: ee8e8480 ti: ee930000 task.ti: ee930000
> PC is at show_ignore_nice_load_gov_pol+0x24/0x34
> LR is at show+0x4c/0x60
> pc : [<c058f1bc>]    lr : [<c058ae88>]    psr: a0070013
> sp : ee931dd0  ip : ee931de0  fp : ee931ddc
> r10: ee4bc290  r9 : 00001000  r8 : ef2cb000
> r7 : ee4bc200  r6 : ef2cb000  r5 : c0af57b0  r4 : ee4bc2e0
> r3 : 00000000  r2 : 00000000  r1 : c0928df4  r0 : ef2cb000
> Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: adfc806a  DAC: 00000051
> Process cat (pid: 1730, stack limit = 0xee930210)
> Stack: (0xee931dd0 to 0xee932000)
> 1dc0:                                     ee931dfc ee931de0 c058ae88 c058f1a4
> 1de0: edce3bc0 c07bfca4 edce3ac0 00001000 ee931e24 ee931e00 c01fcb90 c058ae48
> 1e00: 00000001 edce3bc0 00000000 00000001 ee931e50 ee8ff480 ee931e34 ee931e28
> 1e20: c01fb33c c01fcb0c ee931e8c ee931e38 c01a5210 c01fb314 ee931e9c ee931e48
> 1e40: 00000000 edce3bf0 befe4a00 ee931f78 00000000 00000000 000001e4 00000000
> 1e60: c00545a8 edce3ac0 00001000 00001000 befe4a00 ee931f78 00000000 00001000
> 1e80: ee931ed4 ee931e90 c01fbed8 c01a5038 ed085a58 00020000 00000000 00000000
> 1ea0: c0ad72e4 ee931f78 ee8ff488 ee8ff480 c077f3fc 00001000 befe4a00 ee931f78
> 1ec0: 00000000 00001000 ee931f44 ee931ed8 c017c328 c01fbdc4 00001000 00000000
> 1ee0: ee8ff480 00001000 ee931f44 ee931ef8 c017c65c c03deb10 ee931fac ee931f08
> 1f00: c0009270 c001f290 c0a8d968 ef2cb000 ef2cb000 ee8ff480 00000020 ee8ff480
> 1f20: ee8ff480 befe4a00 00001000 ee931f78 00000000 00000000 ee931f74 ee931f48
> 1f40: c017d1ec c017c2f8 c019c724 c019c684 ee8ff480 ee8ff480 00001000 befe4a00
> 1f60: 00000000 00000000 ee931fa4 ee931f78 c017d2a8 c017d160 00000000 00000000
> 1f80: 000a9f20 00001000 befe4a00 00000003 c000ffe4 ee930000 00000000 ee931fa8
> 1fa0: c000fe40 c017d264 000a9f20 00001000 00000003 befe4a00 00001000 00000000
> Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> 1fc0: 000a9f20 00001000 befe4a00 00000003 00000000 00000000 00000003 00000001
> pgd = edfc4000
> [0000000c] *pgd=bfcac835
> 1fe0: 00000000 befe49dc 000197f8 b6e35dfc 60070010 00000003 3065b49d 134ac2c9
> 
> [<c058f1bc>] (show_ignore_nice_load_gov_pol) from [<c058ae88>] (show+0x4c/0x60)
> [<c058ae88>] (show) from [<c01fcb90>] (sysfs_kf_seq_show+0x90/0xfc)
> [<c01fcb90>] (sysfs_kf_seq_show) from [<c01fb33c>] (kernfs_seq_show+0x34/0x38)
> [<c01fb33c>] (kernfs_seq_show) from [<c01a5210>] (seq_read+0x1e4/0x4e4)
> [<c01a5210>] (seq_read) from [<c01fbed8>] (kernfs_fop_read+0x120/0x1a0)
> [<c01fbed8>] (kernfs_fop_read) from [<c017c328>] (__vfs_read+0x3c/0xe0)
> [<c017c328>] (__vfs_read) from [<c017d1ec>] (vfs_read+0x98/0x104)
> [<c017d1ec>] (vfs_read) from [<c017d2a8>] (SyS_read+0x50/0x90)
> [<c017d2a8>] (SyS_read) from [<c000fe40>] (ret_fast_syscall+0x0/0x1c)
> Code: e5903044 e1a00001 e3081df4 e34c1092 (e593300c)
> ---[ end trace 5994b9a5111f35ee ]---
> 
> Fix that by making sure, policy->governor_data is updated at the right
> places only.
> 
> Cc: <stable@vger.kernel.org> # v4.2+
> Reported-by: Juri Lelli <juri.lelli@arm.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

So I've applied this, but I'm not sure it is sufficient yet.

Have you double checked whether or not stuff cannot be reordered by
the CPU and/or the compiler and no additional memory barriers are needed?

Rafael

[toc] | [next] | [standalone]

#1320225 — Re: [PATCH] cpufreq: Fix NULL reference crash while accessing policy->governor_data

On 27-01-16, 23:54, Rafael J. Wysocki wrote:
> So I've applied this, but I'm not sure it is sufficient yet.

At least, this solves the crash Juri was hitting on a multi cluster
box.

> Have you double checked whether or not stuff cannot be reordered by
> the CPU and/or the compiler and no additional memory barriers are needed?

I don't think CPU will reorder things before a function call. It can
reorder lines, which CPU thinks aren't related but it can't assume the
same in this case. We have tons of code like this.

@Juri: What do you say? 

-- 
viresh

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

#1320544 — Re: [PATCH] cpufreq: Fix NULL reference crash while accessing policy->governor_data

On 28/01/16 07:45, Viresh Kumar wrote:
> On 27-01-16, 23:54, Rafael J. Wysocki wrote:
> > So I've applied this, but I'm not sure it is sufficient yet.
> 
> At least, this solves the crash Juri was hitting on a multi cluster
> box.
> 
> > Have you double checked whether or not stuff cannot be reordered by
> > the CPU and/or the compiler and no additional memory barriers are needed?
> 
> I don't think CPU will reorder things before a function call. It can
> reorder lines, which CPU thinks aren't related but it can't assume the
> same in this case. We have tons of code like this.
> 
> @Juri: What do you say? 
> 

Yeah, it looks good on my boxes (even though I'll run some more tests
later today). I'm not entirely sure either about the reordering, but
reordering across a function call (of a different compilation unit)
seems quite unlikely to me as well.

Best,

- Juri

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

#1321465

On Thursday, January 28, 2016 07:45:53 AM Viresh Kumar wrote:
> On 27-01-16, 23:54, Rafael J. Wysocki wrote:
> > So I've applied this, but I'm not sure it is sufficient yet.
> 
> At least, this solves the crash Juri was hitting on a multi cluster
> box.

Yes, it makes the crash go away in his setup.

> > Have you double checked whether or not stuff cannot be reordered by
> > the CPU and/or the compiler and no additional memory barriers are needed?
> 
> I don't think CPU will reorder things before a function call.

It can do that in theory.

First of all, functions may be inlined by the compiler.

Second, even if they aren't, the call instruction only means "take the next
instruction from that other location in memory" to the CPU and the instructions
following the call go into the pipeline along with the ones preceding it and
they may be reordered in the process.

> It can reorder lines,

Not lines, but instructions.

> which CPU thinks aren't related but it can't assume the
> same in this case. We have tons of code like this.

Code that relies on specific ordering of instructions executed by different
CPUs for correctness usually requires memory barriers.

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web