LinuxLists.cc - mmotm 2010-01-13-12-17 uploaded

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Sat, 16 Jan 2010 00:38:32 +0100
Jiri Slaby <[email protected]> wrote:

> (fixed subject)
>
> On 01/15/2010 08:33 PM, Jiri Slaby wrote:
> > On 01/13/2010 09:17 PM, [email protected] wrote:
> >> The mm-of-the-moment snapshot 2010-01-13-12-17 has been uploaded to
> >
> > Hi, it crashes on my machine while booting up. It is a regression
> > against 2010-01-06-14-34. Doesn't it ring a bell by a chance?
>
> Well, memcpying to something like this:
> char *uevent_helper = CONFIG_UEVENT_HELPER_PATH;
> doesn't sound like a good idea :).
>
> And it's racy with sysctl path anyway.
>
> Looks like added by:
> sysctl-convert-hotplug-helper-string-to-proc_rcu_string.patch
>
> Andi, did you forget to change uevent_helper_store?

Jiri did you forget to Cc Andi? ;)

> > BUG: unable to handle kernel paging request at ffffffff816efe8e
> > IP: [<ffffffff811acf60>] memcpy_c+0x10/0x20
> > PGD 1807067 PUD 180b063 PMD 80000000016001e1
> > Oops: 0003 [#1] SMP
> > last sysfs file: /sys/kernel/uevent_helper
> > CPU 0
> > Pid: 957, comm: boot.udev Not tainted 2.6.33-rc4-mm1_64 #928 To be
> > filled by O.E.M./To Be Filled By O.E.M.
> > RIP: 0010:[<ffffffff811acf60>] [<ffffffff811acf60>] memcpy_c+0x10/0x20
> > RSP: 0018:ffff8801c3127e80 EFLAGS: 00010202
> > RAX: ffffffff816efe8e RBX: 0000000000000001 RCX: 0000000000000001
> > RDX: 0000000000000001 RSI: ffff8801c30b9000 RDI: ffffffff816efe8e
> > RBP: ffff8801c3127e98 R08: ffffffff81058620 R09: 00000000000c2c7e
> > R10: 0000000000000001 R11: 0000000000000246 R12: ffff8801c3127f48
> > R13: ffff8801cbc26688 R14: ffffffff81829d10 R15: ffff8801cbc903b8
> > FS: 00007fe572b106f0(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: ffffffff816efe8e CR3: 00000001c30f2000 CR4: 00000000000006f0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > Process boot.udev (pid: 957, threadinfo ffff8801c3126000, task
> > ffff8801cbcdbf90)
> > Stack:
> > ffffffff8105865f ffff8801c3127ea8 ffff8801c4a3faa0 ffff8801c3127ea8
> > <0> ffffffff811a51b7 ffff8801c3127ef8 ffffffff811224ec 0000000000000001
> > <0> ffff8801c4a3fa80 ffff8801c3127ee8 0000000000000001 00007fe572b4c000
> > Call Trace:
> > [<ffffffff8105865f>] ? uevent_helper_store+0x3f/0x80
> > [<ffffffff811a51b7>] kobj_attr_store+0x17/0x20
> > [<ffffffff811224ec>] sysfs_write_file+0x9c/0xf0
> > [<ffffffff810c9ba8>] vfs_write+0xc8/0x190
> > [<ffffffff810ca4ec>] sys_write+0x4c/0x80
> > [<ffffffff81002e6b>] system_call_fastpath+0x16/0x1b
> > Code: 00 48 3b 42 20 73 07 48 8b 50 f9 31 c0 c3 31 d2 48 c7 c0 f2 ff ff
> > ff c3 90 90 90 48 89 f8 89 d1 c1 e9 03 83 e2 07 f3 48 a5 89 d1 <f3> a4
> > c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 eb de f8 89 d1
> > RIP [<ffffffff811acf60>] memcpy_c+0x10/0x20
> > RSP <ffff8801c3127e80>
> > CR2: ffffffff816efe8e
> > ---[ end trace 309d1f0b04265911 ]---
>

2010-01-15 19:33:21

[permalink] [raw]

Subject: Re: mmotm 2010-01-13-12-17 uploaded

On 01/13/2010 09:17 PM, [email protected] wrote:
> The mm-of-the-moment snapshot 2010-01-13-12-17 has been uploaded to

Hi, it crashes on my machine while booting up. It is a regression
against 2010-01-06-14-34. Doesn't it ring a bell by a chance?

BUG: unable to handle kernel paging request at ffffffff816efe8e
IP: [<ffffffff811acf60>] memcpy_c+0x10/0x20
PGD 1807067 PUD 180b063 PMD 80000000016001e1
Oops: 0003 [#1] SMP
last sysfs file: /sys/kernel/uevent_helper
CPU 0
Pid: 957, comm: boot.udev Not tainted 2.6.33-rc4-mm1_64 #928 To be
filled by O.E.M./To Be Filled By O.E.M.
RIP: 0010:[<ffffffff811acf60>] [<ffffffff811acf60>] memcpy_c+0x10/0x20
RSP: 0018:ffff8801c3127e80 EFLAGS: 00010202
RAX: ffffffff816efe8e RBX: 0000000000000001 RCX: 0000000000000001
RDX: 0000000000000001 RSI: ffff8801c30b9000 RDI: ffffffff816efe8e
RBP: ffff8801c3127e98 R08: ffffffff81058620 R09: 00000000000c2c7e
R10: 0000000000000001 R11: 0000000000000246 R12: ffff8801c3127f48
R13: ffff8801cbc26688 R14: ffffffff81829d10 R15: ffff8801cbc903b8
FS: 00007fe572b106f0(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffff816efe8e CR3: 00000001c30f2000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process boot.udev (pid: 957, threadinfo ffff8801c3126000, task
ffff8801cbcdbf90)
Stack:
ffffffff8105865f ffff8801c3127ea8 ffff8801c4a3faa0 ffff8801c3127ea8
<0> ffffffff811a51b7 ffff8801c3127ef8 ffffffff811224ec 0000000000000001
<0> ffff8801c4a3fa80 ffff8801c3127ee8 0000000000000001 00007fe572b4c000
Call Trace:
[<ffffffff8105865f>] ? uevent_helper_store+0x3f/0x80
[<ffffffff811a51b7>] kobj_attr_store+0x17/0x20
[<ffffffff811224ec>] sysfs_write_file+0x9c/0xf0
[<ffffffff810c9ba8>] vfs_write+0xc8/0x190
[<ffffffff810ca4ec>] sys_write+0x4c/0x80
[<ffffffff81002e6b>] system_call_fastpath+0x16/0x1b
Code: 00 48 3b 42 20 73 07 48 8b 50 f9 31 c0 c3 31 d2 48 c7 c0 f2 ff ff
ff c3 90 90 90 48 89 f8 89 d1 c1 e9 03 83 e2 07 f3 48 a5 89 d1 <f3> a4
c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 eb de f8 89 d1
RIP [<ffffffff811acf60>] memcpy_c+0x10/0x20
RSP <ffff8801c3127e80>
CR2: ffffffff816efe8e
---[ end trace 309d1f0b04265911 ]---

thanks,
--
js

2010-01-15 23:38:43

[permalink] [raw]

Subject: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

(fixed subject)

On 01/15/2010 08:33 PM, Jiri Slaby wrote:
> On 01/13/2010 09:17 PM, [email protected] wrote:
>> The mm-of-the-moment snapshot 2010-01-13-12-17 has been uploaded to
>
> Hi, it crashes on my machine while booting up. It is a regression
> against 2010-01-06-14-34. Doesn't it ring a bell by a chance?

Well, memcpying to something like this:
char *uevent_helper = CONFIG_UEVENT_HELPER_PATH;
doesn't sound like a good idea :).

And it's racy with sysctl path anyway.

Looks like added by:
sysctl-convert-hotplug-helper-string-to-proc_rcu_string.patch

Andi, did you forget to change uevent_helper_store?

> BUG: unable to handle kernel paging request at ffffffff816efe8e
> IP: [<ffffffff811acf60>] memcpy_c+0x10/0x20
> PGD 1807067 PUD 180b063 PMD 80000000016001e1
> Oops: 0003 [#1] SMP
> last sysfs file: /sys/kernel/uevent_helper
> CPU 0
> Pid: 957, comm: boot.udev Not tainted 2.6.33-rc4-mm1_64 #928 To be
> filled by O.E.M./To Be Filled By O.E.M.
> RIP: 0010:[<ffffffff811acf60>] [<ffffffff811acf60>] memcpy_c+0x10/0x20
> RSP: 0018:ffff8801c3127e80 EFLAGS: 00010202
> RAX: ffffffff816efe8e RBX: 0000000000000001 RCX: 0000000000000001
> RDX: 0000000000000001 RSI: ffff8801c30b9000 RDI: ffffffff816efe8e
> RBP: ffff8801c3127e98 R08: ffffffff81058620 R09: 00000000000c2c7e
> R10: 0000000000000001 R11: 0000000000000246 R12: ffff8801c3127f48
> R13: ffff8801cbc26688 R14: ffffffff81829d10 R15: ffff8801cbc903b8
> FS: 00007fe572b106f0(0000) GS:ffff880028200000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffffff816efe8e CR3: 00000001c30f2000 CR4: 00000000000006f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process boot.udev (pid: 957, threadinfo ffff8801c3126000, task
> ffff8801cbcdbf90)
> Stack:
> ffffffff8105865f ffff8801c3127ea8 ffff8801c4a3faa0 ffff8801c3127ea8
> <0> ffffffff811a51b7 ffff8801c3127ef8 ffffffff811224ec 0000000000000001
> <0> ffff8801c4a3fa80 ffff8801c3127ee8 0000000000000001 00007fe572b4c000
> Call Trace:
> [<ffffffff8105865f>] ? uevent_helper_store+0x3f/0x80
> [<ffffffff811a51b7>] kobj_attr_store+0x17/0x20
> [<ffffffff811224ec>] sysfs_write_file+0x9c/0xf0
> [<ffffffff810c9ba8>] vfs_write+0xc8/0x190
> [<ffffffff810ca4ec>] sys_write+0x4c/0x80
> [<ffffffff81002e6b>] system_call_fastpath+0x16/0x1b
> Code: 00 48 3b 42 20 73 07 48 8b 50 f9 31 c0 c3 31 d2 48 c7 c0 f2 ff ff
> ff c3 90 90 90 48 89 f8 89 d1 c1 e9 03 83 e2 07 f3 48 a5 89 d1 <f3> a4
> c3 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 eb de f8 89 d1
> RIP [<ffffffff811acf60>] memcpy_c+0x10/0x20
> RSP <ffff8801c3127e80>
> CR2: ffffffff816efe8e
> ---[ end trace 309d1f0b04265911 ]---

--
js

2010-02-10 19:54:22

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On 01/23/2010 12:52 AM, Andrew Morton wrote:
> On Sat, 16 Jan 2010 00:38:32 +0100
> Jiri Slaby <[email protected]> wrote:
>
>> (fixed subject)
>>
>> On 01/15/2010 08:33 PM, Jiri Slaby wrote:
>>> On 01/13/2010 09:17 PM, [email protected] wrote:
>>>> The mm-of-the-moment snapshot 2010-01-13-12-17 has been uploaded to
>>>
>>> Hi, it crashes on my machine while booting up. It is a regression
>>> against 2010-01-06-14-34. Doesn't it ring a bell by a chance?
>>
>> Well, memcpying to something like this:
>> char *uevent_helper = CONFIG_UEVENT_HELPER_PATH;
>> doesn't sound like a good idea :).
>>
>> And it's racy with sysctl path anyway.
>>
>> Looks like added by:
>> sysctl-convert-hotplug-helper-string-to-proc_rcu_string.patch

Ping Andi.

--
js

2010-02-11 20:57:48

by Andi Kleen

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Wed, Feb 10, 2010 at 08:54:11PM +0100, Jiri Slaby wrote:
> On 01/23/2010 12:52 AM, Andrew Morton wrote:
> > On Sat, 16 Jan 2010 00:38:32 +0100
> > Jiri Slaby <[email protected]> wrote:
> >
> >> (fixed subject)
> >>
> >> On 01/15/2010 08:33 PM, Jiri Slaby wrote:
> >>> On 01/13/2010 09:17 PM, [email protected] wrote:
> >>>> The mm-of-the-moment snapshot 2010-01-13-12-17 has been uploaded to
> >>>
> >>> Hi, it crashes on my machine while booting up. It is a regression
> >>> against 2010-01-06-14-34. Doesn't it ring a bell by a chance?
> >>
> >> Well, memcpying to something like this:
> >> char *uevent_helper = CONFIG_UEVENT_HELPER_PATH;
> >> doesn't sound like a good idea :).
> >>
> >> And it's racy with sysctl path anyway.
> >>
> >> Looks like added by:
> >> sysctl-convert-hotplug-helper-string-to-proc_rcu_string.patch
>
> Ping Andi.

Sorry, busy with other stuff right now. There was at least one
other unsolved problem in this patch series. I'll try to look
at it on the weekend.

Andrew, it's ok for me if you just drop the series for now;
I'll resubmit.

-Andi

--
[email protected] -- Speaking for myself only.

2010-02-11 22:26:07

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Thu, 11 Feb 2010 21:57:45 +0100
Andi Kleen <[email protected]> wrote:

> On Wed, Feb 10, 2010 at 08:54:11PM +0100, Jiri Slaby wrote:
> > On 01/23/2010 12:52 AM, Andrew Morton wrote:
> > > On Sat, 16 Jan 2010 00:38:32 +0100
> > > Jiri Slaby <[email protected]> wrote:
> > >
> > >> (fixed subject)
> > >>
> > >> On 01/15/2010 08:33 PM, Jiri Slaby wrote:
> > >>> On 01/13/2010 09:17 PM, [email protected] wrote:
> > >>>> The mm-of-the-moment snapshot 2010-01-13-12-17 has been uploaded to
> > >>>
> > >>> Hi, it crashes on my machine while booting up. It is a regression
> > >>> against 2010-01-06-14-34. Doesn't it ring a bell by a chance?
> > >>
> > >> Well, memcpying to something like this:
> > >> char *uevent_helper = CONFIG_UEVENT_HELPER_PATH;
> > >> doesn't sound like a good idea :).
> > >>
> > >> And it's racy with sysctl path anyway.
> > >>
> > >> Looks like added by:
> > >> sysctl-convert-hotplug-helper-string-to-proc_rcu_string.patch
> >
> > Ping Andi.
>
> Sorry, busy with other stuff right now. There was at least one
> other unsolved problem in this patch series. I'll try to look
> at it on the weekend.
>
> Andrew, it's ok for me if you just drop the series for now;
> I'll resubmit.

urgh, must I? That trashes Neil's
kmod-add-init-function-to-usermodehelper.patch and
kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
and probably requires repairing other stuff and sets the testing status
back to "square one".

If you have patches queued, please make the time to support them!

2010-02-12 02:39:52

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Thu, Feb 11, 2010 at 02:25:13PM -0800, Andrew Morton wrote:
> On Thu, 11 Feb 2010 21:57:45 +0100
> Andi Kleen <[email protected]> wrote:
>
> > On Wed, Feb 10, 2010 at 08:54:11PM +0100, Jiri Slaby wrote:
> > > On 01/23/2010 12:52 AM, Andrew Morton wrote:
> > > > On Sat, 16 Jan 2010 00:38:32 +0100
> > > > Jiri Slaby <[email protected]> wrote:
> > > >
> > > >> (fixed subject)
> > > >>
> > > >> On 01/15/2010 08:33 PM, Jiri Slaby wrote:
> > > >>> On 01/13/2010 09:17 PM, [email protected] wrote:
> > > >>>> The mm-of-the-moment snapshot 2010-01-13-12-17 has been uploaded to
> > > >>>
> > > >>> Hi, it crashes on my machine while booting up. It is a regression
> > > >>> against 2010-01-06-14-34. Doesn't it ring a bell by a chance?
> > > >>
> > > >> Well, memcpying to something like this:
> > > >> char *uevent_helper = CONFIG_UEVENT_HELPER_PATH;
> > > >> doesn't sound like a good idea :).
> > > >>
> > > >> And it's racy with sysctl path anyway.
> > > >>
> > > >> Looks like added by:
> > > >> sysctl-convert-hotplug-helper-string-to-proc_rcu_string.patch
> > >
> > > Ping Andi.
> >
> > Sorry, busy with other stuff right now. There was at least one
> > other unsolved problem in this patch series. I'll try to look
> > at it on the weekend.
> >
> > Andrew, it's ok for me if you just drop the series for now;
> > I'll resubmit.
>
> urgh, must I? That trashes Neil's
> kmod-add-init-function-to-usermodehelper.patch and
> kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> and probably requires repairing other stuff and sets the testing status
> back to "square one".
>
> If you have patches queued, please make the time to support them!
>
>
>
Andrew, I'll take a closer look at it first thing in the morning. If we can at
all help it, I'd really like to avoid dropping the whole series. I'll let you
know just as soon as I have something.
Neil

2010-02-12 03:12:05

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Thu, 11 Feb 2010 21:39:34 -0500 Neil Horman <[email protected]> wrote:

> Andrew, I'll take a closer look at it first thing in the morning. If we can at
> all help it, I'd really like to avoid dropping the whole series. I'll let you
> know just as soon as I have something.

Thanks.

If it isn't obvious then I suggest you give up - we're at -rc7 and it's
getting pretty late for these patches. We'll soon need to shelve them
and respin yours against mainline.

2010-02-12 05:21:32

by Andi Kleen

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

> urgh, must I? That trashes Neil's
> kmod-add-init-function-to-usermodehelper.patch and
> kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> and probably requires repairing other stuff and sets the testing status
> back to "square one".
>
> If you have patches queued, please make the time to support them!

Ok, understood. I'll try to look into it today. You want incrementals?

-Andi
--
[email protected] -- Speaking for myself only.

2010-02-12 05:32:32

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Fri, 12 Feb 2010 06:21:26 +0100 Andi Kleen <[email protected]> wrote:

> > urgh, must I? That trashes Neil's
> > kmod-add-init-function-to-usermodehelper.patch and
> > kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> > and probably requires repairing other stuff and sets the testing status
> > back to "square one".
> >
> > If you have patches queued, please make the time to support them!
>
> Ok, understood. I'll try to look into it today.

Ta. As I mentioned to Neil, if it looks serious then let's shelve it
all and revisit for 2.6.35.

> You want incrementals?

If convenient, please. Otherwise we can drop--and-remerge.

2010-02-12 17:06:40

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Thu, Feb 11, 2010 at 09:27:08PM -0800, Andrew Morton wrote:
> On Fri, 12 Feb 2010 06:21:26 +0100 Andi Kleen <[email protected]> wrote:
>
> > > urgh, must I? That trashes Neil's
> > > kmod-add-init-function-to-usermodehelper.patch and
> > > kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> > > and probably requires repairing other stuff and sets the testing status
> > > back to "square one".
> > >
> > > If you have patches queued, please make the time to support them!
> >
> > Ok, understood. I'll try to look into it today.
>
> Ta. As I mentioned to Neil, if it looks serious then let's shelve it
> all and revisit for 2.6.35.
>
> > You want incrementals?
>
> If convenient, please. Otherwise we can drop--and-remerge.
>
>

Ok, this fixes the oops Jiri reported for me. Its been tested by me, but only
minimally, and my rcu-foo is not the greatest, so through reviews appreciated.
The patch is incremental against the latest mmotm as of this AM.

Thanks!
Neil

Fix up remaining references to uevent_helper to play nice with Andi's
uevent_helper/rcu changes.

Some changes were made recently which modified uevent_helper to be an rcu
protected pointer, rather than a static char array. This has led to a few
missed points in which the sysfs path still assumed that:
1) the uevent_helper symbol could still be accessed safely without
rcu_dereference
2) that the sysfs path could copy data to that pointer safely.

I've fixed this by chaging the sysfs path so that it duplicates the string on
uevent_helper_store, and freeing it (only if it doesn't point to the
CONFIG_DEFAULT_UEVENT_HELPER string), in a call_rcu post-quiescent point. I've
also fixed up the remaining references to the uevent_helper pointers to use
rcu_dereference.

Signed-off-by: Neil Horman <[email protected]>

kernel/ksysfs.c | 38 ++++++++++++++++++++++++++++++++++++--
lib/kobject_uevent.c | 4 +++-
2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
index 21fe3c4..66d1e5b 100644
--- a/kernel/ksysfs.c
+++ b/kernel/ksysfs.c
@@ -37,19 +37,53 @@ KERNEL_ATTR_RO(uevent_seqnum);
static ssize_t uevent_helper_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{
- return sprintf(buf, "%s\n", uevent_helper);
+ return sprintf(buf, "%s\n", rcu_dereference(uevent_helper));
}
+
+struct uevent_helper_rcu {
+ char *oldptr;
+ struct rcu_head rcu;
+};
+
+static void free_old_uevent_ptr(struct rcu_head *list)
+{
+ struct uevent_helper_rcu *ptr;
+ char *dfl = CONFIG_UEVENT_HELPER_PATH;
+ ptr = container_of(list, struct uevent_helper_rcu, rcu);
+ if (ptr->oldptr && (ptr->oldptr != dfl))
+ kfree(ptr->oldptr);
+
+ kfree(ptr);
+}
+
static ssize_t uevent_helper_store(struct kobject *kobj,
struct kobj_attribute *attr,
const char *buf, size_t count)
{
+ char *kbuf;
+ struct uevent_helper_rcu *old;
+
if (count+1 > UEVENT_HELPER_PATH_LEN)
return -ENOENT;
- memcpy(uevent_helper, buf, count);
+ kbuf = kstrndup(buf, UEVENT_HELPER_PATH_LEN, GFP_KERNEL);
+ if (!kbuf)
+ return -ENOMEM;
uevent_helper[count] = '\0';
if (count && uevent_helper[count-1] == '\n')
uevent_helper[count-1] = '\0';
+ old = kmalloc(sizeof(struct uevent_helper_rcu), GFP_KERNEL);
+ if (!old)
+ goto out_free;
+
+ old->oldptr = rcu_dereference(uevent_helper);
+ rcu_assign_pointer(uevent_helper, kbuf);
+ call_rcu(&old->rcu, free_old_uevent_ptr);
+
return count;
+
+out_free:
+ kfree(kbuf);
+ return -ENOMEM;
}
KERNEL_ATTR_RW(uevent_helper);
#endif
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index c2383f3..211f846 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -126,6 +126,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
struct kset *kset;
const struct kset_uevent_ops *uevent_ops;
u64 seq;
+ const char *helper;
int i = 0;
int retval = 0;

@@ -272,7 +273,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
#endif

/* call uevent_helper, usually only enabled during early boot */
- if (uevent_helper[0])
+ helper = rcu_dereference(uevent_helper);
+ if (helper[0])
retval = uevent_call_helper(subsystem, env);

exit:

2010-02-14 17:32:30

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On 02/12/2010 06:06 PM, Neil Horman wrote:
> Fix up remaining references to uevent_helper to play nice with Andi's
> uevent_helper/rcu changes.
>
> Some changes were made recently which modified uevent_helper to be an rcu
> protected pointer, rather than a static char array. This has led to a few
> missed points in which the sysfs path still assumed that:
> 1) the uevent_helper symbol could still be accessed safely without
> rcu_dereference
> 2) that the sysfs path could copy data to that pointer safely.
>
> I've fixed this by chaging the sysfs path so that it duplicates the string on
> uevent_helper_store, and freeing it (only if it doesn't point to the
> CONFIG_DEFAULT_UEVENT_HELPER string), in a call_rcu post-quiescent point. I've
> also fixed up the remaining references to the uevent_helper pointers to use
> rcu_dereference.
>
> Signed-off-by: Neil Horman <[email protected]>

Tested-by: Jiri Slaby <[email protected]>

--
js

2010-02-14 17:55:00

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Sun, Feb 14, 2010 at 06:32:20PM +0100, Jiri Slaby wrote:
> On 02/12/2010 06:06 PM, Neil Horman wrote:
> > Fix up remaining references to uevent_helper to play nice with Andi's
> > uevent_helper/rcu changes.
> >
> > Some changes were made recently which modified uevent_helper to be an rcu
> > protected pointer, rather than a static char array. This has led to a few
> > missed points in which the sysfs path still assumed that:
> > 1) the uevent_helper symbol could still be accessed safely without
> > rcu_dereference
> > 2) that the sysfs path could copy data to that pointer safely.
> >
> > I've fixed this by chaging the sysfs path so that it duplicates the string on
> > uevent_helper_store, and freeing it (only if it doesn't point to the
> > CONFIG_DEFAULT_UEVENT_HELPER string), in a call_rcu post-quiescent point. I've
> > also fixed up the remaining references to the uevent_helper pointers to use
> > rcu_dereference.
> >
> > Signed-off-by: Neil Horman <[email protected]>
>
> Tested-by: Jiri Slaby <[email protected]>
>
Thanks Jiri!
Neil

> --
> js
>

2010-02-15 20:12:37

by Paul E. McKenney

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Fri, Feb 12, 2010 at 12:06:24PM -0500, Neil Horman wrote:
> On Thu, Feb 11, 2010 at 09:27:08PM -0800, Andrew Morton wrote:
> > On Fri, 12 Feb 2010 06:21:26 +0100 Andi Kleen <[email protected]> wrote:
> >
> > > > urgh, must I? That trashes Neil's
> > > > kmod-add-init-function-to-usermodehelper.patch and
> > > > kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> > > > and probably requires repairing other stuff and sets the testing status
> > > > back to "square one".
> > > >
> > > > If you have patches queued, please make the time to support them!
> > >
> > > Ok, understood. I'll try to look into it today.
> >
> > Ta. As I mentioned to Neil, if it looks serious then let's shelve it
> > all and revisit for 2.6.35.
> >
> > > You want incrementals?
> >
> > If convenient, please. Otherwise we can drop--and-remerge.
> >
> >
>
>
> Ok, this fixes the oops Jiri reported for me. Its been tested by me, but only
> minimally, and my rcu-foo is not the greatest, so through reviews appreciated.
> The patch is incremental against the latest mmotm as of this AM.

A few questions interspersed below...

Thanx, Paul

> Thanks!
> Neil
>
>
> Fix up remaining references to uevent_helper to play nice with Andi's
> uevent_helper/rcu changes.
>
> Some changes were made recently which modified uevent_helper to be an rcu
> protected pointer, rather than a static char array. This has led to a few
> missed points in which the sysfs path still assumed that:
> 1) the uevent_helper symbol could still be accessed safely without
> rcu_dereference
> 2) that the sysfs path could copy data to that pointer safely.
>
> I've fixed this by chaging the sysfs path so that it duplicates the string on
> uevent_helper_store, and freeing it (only if it doesn't point to the
> CONFIG_DEFAULT_UEVENT_HELPER string), in a call_rcu post-quiescent point. I've
> also fixed up the remaining references to the uevent_helper pointers to use
> rcu_dereference.
>
> Signed-off-by: Neil Horman <[email protected]>
>
>
> kernel/ksysfs.c | 38 ++++++++++++++++++++++++++++++++++++--
> lib/kobject_uevent.c | 4 +++-
> 2 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> index 21fe3c4..66d1e5b 100644
> --- a/kernel/ksysfs.c
> +++ b/kernel/ksysfs.c
> @@ -37,19 +37,53 @@ KERNEL_ATTR_RO(uevent_seqnum);
> static ssize_t uevent_helper_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> {
> - return sprintf(buf, "%s\n", uevent_helper);
> + return sprintf(buf, "%s\n", rcu_dereference(uevent_helper));
> }
> +
> +struct uevent_helper_rcu {
> + char *oldptr;
> + struct rcu_head rcu;
> +};
> +
> +static void free_old_uevent_ptr(struct rcu_head *list)
> +{
> + struct uevent_helper_rcu *ptr;
> + char *dfl = CONFIG_UEVENT_HELPER_PATH;

Given that you kfree() something that might be equal to dfl, I am
hoping that the CONFIG_UEVENT_HELPER_PATH macro expands to something
that kfree() can do something with...

Or did you mean to put a "return;" in the then-clause of the "if"
statement below?

> + ptr = container_of(list, struct uevent_helper_rcu, rcu);
> + if (ptr->oldptr && (ptr->oldptr != dfl))
> + kfree(ptr->oldptr);
> +
> + kfree(ptr);
> +}
> +
> static ssize_t uevent_helper_store(struct kobject *kobj,
> struct kobj_attribute *attr,
> const char *buf, size_t count)
> {
> + char *kbuf;
> + struct uevent_helper_rcu *old;
> +
> if (count+1 > UEVENT_HELPER_PATH_LEN)
> return -ENOENT;
> - memcpy(uevent_helper, buf, count);
> + kbuf = kstrndup(buf, UEVENT_HELPER_PATH_LEN, GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> uevent_helper[count] = '\0';
> if (count && uevent_helper[count-1] == '\n')
> uevent_helper[count-1] = '\0';
> + old = kmalloc(sizeof(struct uevent_helper_rcu), GFP_KERNEL);
> + if (!old)
> + goto out_free;
> +
> + old->oldptr = rcu_dereference(uevent_helper);
> + rcu_assign_pointer(uevent_helper, kbuf);

Some lock protects this? Or does something else prevent multiple
instances of uevent_helper_store() from executing concurrently?

> + call_rcu(&old->rcu, free_old_uevent_ptr);
> +
> return count;
> +
> +out_free:
> + kfree(kbuf);
> + return -ENOMEM;
> }
> KERNEL_ATTR_RW(uevent_helper);
> #endif
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index c2383f3..211f846 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -126,6 +126,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> struct kset *kset;
> const struct kset_uevent_ops *uevent_ops;
> u64 seq;
> + const char *helper;
> int i = 0;
> int retval = 0;
>
> @@ -272,7 +273,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> #endif
>
> /* call uevent_helper, usually only enabled during early boot */
> - if (uevent_helper[0])
> + helper = rcu_dereference(uevent_helper);

This is protected by a pre-existing rcu_read_lock() somewhere?

If not, an rcu_read_lock() / rcu_read_unlock() pair is needed that
covers both the rcu_dereference() and any subsequent dereferences of the
pointer returned by rcu_dereference().

> + if (helper[0])
> retval = uevent_call_helper(subsystem, env);
>
> exit:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2010-02-15 21:43:20

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Mon, Feb 15, 2010 at 12:12:33PM -0800, Paul E. McKenney wrote:
> On Fri, Feb 12, 2010 at 12:06:24PM -0500, Neil Horman wrote:
> > On Thu, Feb 11, 2010 at 09:27:08PM -0800, Andrew Morton wrote:
> > > On Fri, 12 Feb 2010 06:21:26 +0100 Andi Kleen <[email protected]> wrote:
> > >
> > > > > urgh, must I? That trashes Neil's
> > > > > kmod-add-init-function-to-usermodehelper.patch and
> > > > > kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> > > > > and probably requires repairing other stuff and sets the testing status
> > > > > back to "square one".
> > > > >
> > > > > If you have patches queued, please make the time to support them!
> > > >
> > > > Ok, understood. I'll try to look into it today.
> > >
> > > Ta. As I mentioned to Neil, if it looks serious then let's shelve it
> > > all and revisit for 2.6.35.
> > >
> > > > You want incrementals?
> > >
> > > If convenient, please. Otherwise we can drop--and-remerge.
> > >
> > >
> >
> >
> > Ok, this fixes the oops Jiri reported for me. Its been tested by me, but only
> > minimally, and my rcu-foo is not the greatest, so through reviews appreciated.
> > The patch is incremental against the latest mmotm as of this AM.
>
> A few questions interspersed below...
>
> Thanx, Paul
>
> > Thanks!
> > Neil
> >
> >
> > Fix up remaining references to uevent_helper to play nice with Andi's
> > uevent_helper/rcu changes.
> >
> > Some changes were made recently which modified uevent_helper to be an rcu
> > protected pointer, rather than a static char array. This has led to a few
> > missed points in which the sysfs path still assumed that:
> > 1) the uevent_helper symbol could still be accessed safely without
> > rcu_dereference
> > 2) that the sysfs path could copy data to that pointer safely.
> >
> > I've fixed this by chaging the sysfs path so that it duplicates the string on
> > uevent_helper_store, and freeing it (only if it doesn't point to the
> > CONFIG_DEFAULT_UEVENT_HELPER string), in a call_rcu post-quiescent point. I've
> > also fixed up the remaining references to the uevent_helper pointers to use
> > rcu_dereference.
> >
> > Signed-off-by: Neil Horman <[email protected]>
> >
> >
> > kernel/ksysfs.c | 38 ++++++++++++++++++++++++++++++++++++--
> > lib/kobject_uevent.c | 4 +++-
> > 2 files changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> > index 21fe3c4..66d1e5b 100644
> > --- a/kernel/ksysfs.c
> > +++ b/kernel/ksysfs.c
> > @@ -37,19 +37,53 @@ KERNEL_ATTR_RO(uevent_seqnum);
> > static ssize_t uevent_helper_show(struct kobject *kobj,
> > struct kobj_attribute *attr, char *buf)
> > {
> > - return sprintf(buf, "%s\n", uevent_helper);
> > + return sprintf(buf, "%s\n", rcu_dereference(uevent_helper));
> > }
> > +
> > +struct uevent_helper_rcu {
> > + char *oldptr;
> > + struct rcu_head rcu;
> > +};
> > +
> > +static void free_old_uevent_ptr(struct rcu_head *list)
> > +{
> > + struct uevent_helper_rcu *ptr;
> > + char *dfl = CONFIG_UEVENT_HELPER_PATH;
>
> Given that you kfree() something that might be equal to dfl, I am
> hoping that the CONFIG_UEVENT_HELPER_PATH macro expands to something
> that kfree() can do something with...
>
> Or did you mean to put a "return;" in the then-clause of the "if"
> statement below?
>
No, I meant what I have. CONFIG_UEVENT_HELPER_PATH expands to a string. if the
old pointer is not NULL, and doesn't point to the default string (we don't want
to free something in the string table), then we kfree it, since it must have
been allocated in uevent_helper_store

> > + ptr = container_of(list, struct uevent_helper_rcu, rcu);
> > + if (ptr->oldptr && (ptr->oldptr != dfl))
> > + kfree(ptr->oldptr);
> > +
> > + kfree(ptr);
> > +}
> > +
> > static ssize_t uevent_helper_store(struct kobject *kobj,
> > struct kobj_attribute *attr,
> > const char *buf, size_t count)
> > {
> > + char *kbuf;
> > + struct uevent_helper_rcu *old;
> > +
> > if (count+1 > UEVENT_HELPER_PATH_LEN)
> > return -ENOENT;
> > - memcpy(uevent_helper, buf, count);
> > + kbuf = kstrndup(buf, UEVENT_HELPER_PATH_LEN, GFP_KERNEL);
> > + if (!kbuf)
> > + return -ENOMEM;
> > uevent_helper[count] = '\0';
> > if (count && uevent_helper[count-1] == '\n')
> > uevent_helper[count-1] = '\0';
> > + old = kmalloc(sizeof(struct uevent_helper_rcu), GFP_KERNEL);
> > + if (!old)
> > + goto out_free;
> > +
> > + old->oldptr = rcu_dereference(uevent_helper);
> > + rcu_assign_pointer(uevent_helper, kbuf);
>
> Some lock protects this? Or does something else prevent multiple
> instances of uevent_helper_store() from executing concurrently?
>
I had thought that there was a per-file sysfs lock that protected this, but I
wasn't sure. Regardless, I would have thought this was safe, since
rcu_assign_pointer was serialized by rcu quiescence. Am I mistaken?

> > + call_rcu(&old->rcu, free_old_uevent_ptr);
> > +
> > return count;
> > +
> > +out_free:
> > + kfree(kbuf);
> > + return -ENOMEM;
> > }
> > KERNEL_ATTR_RW(uevent_helper);
> > #endif
> > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> > index c2383f3..211f846 100644
> > --- a/lib/kobject_uevent.c
> > +++ b/lib/kobject_uevent.c
> > @@ -126,6 +126,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> > struct kset *kset;
> > const struct kset_uevent_ops *uevent_ops;
> > u64 seq;
> > + const char *helper;
> > int i = 0;
> > int retval = 0;
> >
> > @@ -272,7 +273,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> > #endif
> >
> > /* call uevent_helper, usually only enabled during early boot */
> > - if (uevent_helper[0])
> > + helper = rcu_dereference(uevent_helper);
>
> This is protected by a pre-existing rcu_read_lock() somewhere?
>
Does rcu_dereference not start a quiescence point the same way rcu_read_lock
does? AS I said, rcu isn't my strong suit :)

Neil

> If not, an rcu_read_lock() / rcu_read_unlock() pair is needed that
> covers both the rcu_dereference() and any subsequent dereferences of the
> pointer returned by rcu_dereference().
>
> > + if (helper[0])
> > retval = uevent_call_helper(subsystem, env);
> >
> > exit:
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>

2010-02-15 22:16:15

by Paul E. McKenney

[permalink] [raw]

Subject: Re: oops in uevent_helper [was: mmotm 2010-01-13-12-17 uploaded]

On Mon, Feb 15, 2010 at 04:42:52PM -0500, Neil Horman wrote:
> On Mon, Feb 15, 2010 at 12:12:33PM -0800, Paul E. McKenney wrote:
> > On Fri, Feb 12, 2010 at 12:06:24PM -0500, Neil Horman wrote:
> > > On Thu, Feb 11, 2010 at 09:27:08PM -0800, Andrew Morton wrote:
> > > > On Fri, 12 Feb 2010 06:21:26 +0100 Andi Kleen <[email protected]> wrote:
> > > >
> > > > > > urgh, must I? That trashes Neil's
> > > > > > kmod-add-init-function-to-usermodehelper.patch and
> > > > > > kmod-replace-call_usermodehelper_pipe-with-use-of-umh-init-function-and-resolve-limit.patch
> > > > > > and probably requires repairing other stuff and sets the testing status
> > > > > > back to "square one".
> > > > > >
> > > > > > If you have patches queued, please make the time to support them!
> > > > >
> > > > > Ok, understood. I'll try to look into it today.
> > > >
> > > > Ta. As I mentioned to Neil, if it looks serious then let's shelve it
> > > > all and revisit for 2.6.35.
> > > >
> > > > > You want incrementals?
> > > >
> > > > If convenient, please. Otherwise we can drop--and-remerge.
> > > >
> > > >
> > >
> > >
> > > Ok, this fixes the oops Jiri reported for me. Its been tested by me, but only
> > > minimally, and my rcu-foo is not the greatest, so through reviews appreciated.
> > > The patch is incremental against the latest mmotm as of this AM.
> >
> > A few questions interspersed below...
> >
> > Thanx, Paul
> >
> > > Thanks!
> > > Neil
> > >
> > >
> > > Fix up remaining references to uevent_helper to play nice with Andi's
> > > uevent_helper/rcu changes.
> > >
> > > Some changes were made recently which modified uevent_helper to be an rcu
> > > protected pointer, rather than a static char array. This has led to a few
> > > missed points in which the sysfs path still assumed that:
> > > 1) the uevent_helper symbol could still be accessed safely without
> > > rcu_dereference
> > > 2) that the sysfs path could copy data to that pointer safely.
> > >
> > > I've fixed this by chaging the sysfs path so that it duplicates the string on
> > > uevent_helper_store, and freeing it (only if it doesn't point to the
> > > CONFIG_DEFAULT_UEVENT_HELPER string), in a call_rcu post-quiescent point. I've
> > > also fixed up the remaining references to the uevent_helper pointers to use
> > > rcu_dereference.
> > >
> > > Signed-off-by: Neil Horman <[email protected]>
> > >
> > >
> > > kernel/ksysfs.c | 38 ++++++++++++++++++++++++++++++++++++--
> > > lib/kobject_uevent.c | 4 +++-
> > > 2 files changed, 39 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
> > > index 21fe3c4..66d1e5b 100644
> > > --- a/kernel/ksysfs.c
> > > +++ b/kernel/ksysfs.c
> > > @@ -37,19 +37,53 @@ KERNEL_ATTR_RO(uevent_seqnum);
> > > static ssize_t uevent_helper_show(struct kobject *kobj,
> > > struct kobj_attribute *attr, char *buf)
> > > {
> > > - return sprintf(buf, "%s\n", uevent_helper);
> > > + return sprintf(buf, "%s\n", rcu_dereference(uevent_helper));
> > > }
> > > +
> > > +struct uevent_helper_rcu {
> > > + char *oldptr;
> > > + struct rcu_head rcu;
> > > +};
> > > +
> > > +static void free_old_uevent_ptr(struct rcu_head *list)
> > > +{
> > > + struct uevent_helper_rcu *ptr;
> > > + char *dfl = CONFIG_UEVENT_HELPER_PATH;
> >
> > Given that you kfree() something that might be equal to dfl, I am
> > hoping that the CONFIG_UEVENT_HELPER_PATH macro expands to something
> > that kfree() can do something with...
> >
> > Or did you mean to put a "return;" in the then-clause of the "if"
> > statement below?
> >
> No, I meant what I have. CONFIG_UEVENT_HELPER_PATH expands to a string. if the
> old pointer is not NULL, and doesn't point to the default string (we don't want
> to free something in the string table), then we kfree it, since it must have
> been allocated in uevent_helper_store

Got it, thank you! I confused ptr->oldptr with ptr. :-(

> > > + ptr = container_of(list, struct uevent_helper_rcu, rcu);
> > > + if (ptr->oldptr && (ptr->oldptr != dfl))
> > > + kfree(ptr->oldptr);
> > > +
> > > + kfree(ptr);
> > > +}
> > > +
> > > static ssize_t uevent_helper_store(struct kobject *kobj,
> > > struct kobj_attribute *attr,
> > > const char *buf, size_t count)
> > > {
> > > + char *kbuf;
> > > + struct uevent_helper_rcu *old;
> > > +
> > > if (count+1 > UEVENT_HELPER_PATH_LEN)
> > > return -ENOENT;
> > > - memcpy(uevent_helper, buf, count);
> > > + kbuf = kstrndup(buf, UEVENT_HELPER_PATH_LEN, GFP_KERNEL);
> > > + if (!kbuf)
> > > + return -ENOMEM;
> > > uevent_helper[count] = '\0';
> > > if (count && uevent_helper[count-1] == '\n')
> > > uevent_helper[count-1] = '\0';
> > > + old = kmalloc(sizeof(struct uevent_helper_rcu), GFP_KERNEL);
> > > + if (!old)
> > > + goto out_free;
> > > +
> > > + old->oldptr = rcu_dereference(uevent_helper);
> > > + rcu_assign_pointer(uevent_helper, kbuf);
> >
> > Some lock protects this? Or does something else prevent multiple
> > instances of uevent_helper_store() from executing concurrently?
> >
> I had thought that there was a per-file sysfs lock that protected this, but I
> wasn't sure. Regardless, I would have thought this was safe, since
> rcu_assign_pointer was serialized by rcu quiescence. Am I mistaken?

Indeed you are -- rcu_assign_pointer() coordinates with any concurrent
rcu_dereference() invocations, but does nothing to protect against other
concurrent rcu_assign_pointer() invocations. So you should have something
coordinating the update, be it a lock or whatever.

> > > + call_rcu(&old->rcu, free_old_uevent_ptr);
> > > +
> > > return count;
> > > +
> > > +out_free:
> > > + kfree(kbuf);
> > > + return -ENOMEM;
> > > }
> > > KERNEL_ATTR_RW(uevent_helper);
> > > #endif
> > > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> > > index c2383f3..211f846 100644
> > > --- a/lib/kobject_uevent.c
> > > +++ b/lib/kobject_uevent.c
> > > @@ -126,6 +126,7 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> > > struct kset *kset;
> > > const struct kset_uevent_ops *uevent_ops;
> > > u64 seq;
> > > + const char *helper;
> > > int i = 0;
> > > int retval = 0;
> > >
> > > @@ -272,7 +273,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> > > #endif
> > >
> > > /* call uevent_helper, usually only enabled during early boot */
> > > - if (uevent_helper[0])
> > > + helper = rcu_dereference(uevent_helper);
> >
> > This is protected by a pre-existing rcu_read_lock() somewhere?
> >
> Does rcu_dereference not start a quiescence point the same way rcu_read_lock
> does? AS I said, rcu isn't my strong suit :)

What rcu_dereference() does is coordinate with any concurrent
rcu_assign_pointer() calls to the same pointer. If you (incorrectly)
replace the rcu_dereference() and rcu_assign_pointer() calls with
simple assignments, then the helper[0] below might see the garbage
values that were in place before the initialization that preceded the
rcu_assign_pointer().

The rcu_read_lock() and rcu_read_unlock() prevent an RCU grace period both
starting and ending during the enclosed RCU read-side critical section.
A pre-existing RCU grace period might end during this critical section,
or a new RCU grace period might start during this critical section, but
any RCU grace period that starts during a given RCU read-side critical
section is not permitted to end until after that RCU read-side critical
section ends.

So the interaction between rcu_read_lock()/rcu_read_unlock on the one
hand and synchronize_rcu() and call_rcu() on the other hand is what
guarantees that any RCU-protect data structure referenced within an
RCU read-side critical section will remain in existence throughout the
remainder of that RCU read-side critical section.

This arrangement might seem a bit strange at first, but it is what
gives RCU its read-side performance, scalability, and deadlock immunity.

For more detail, please take a look at:

http://lwn.net/Articles/262464/ (What is RCU, Fundamentally?)
http://lwn.net/Articles/263130/ (What is RCU's Usage?)
http://lwn.net/Articles/264090/ (What is RCU's API?)

Thanx, Paul

> Neil
>
> > If not, an rcu_read_lock() / rcu_read_unlock() pair is needed that
> > covers both the rcu_dereference() and any subsequent dereferences of the
> > pointer returned by rcu_dereference().
> >
> > > + if (helper[0])
> > > retval = uevent_call_helper(subsystem, env);
> > >
> > > exit:
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
> >

2010-02-16 01:37:21