Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-18 Thread Oleg Nesterov
On 03/17, Oleg Nesterov wrote: > > Ah, I didn't mean the patch makes sense because of optimization. My > point was, we can fix the race without making this code worse (in fact > it tries to make the code better but this is subjective). OK, I am stupid. argv_free() passes the wrong poiinter to

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-18 Thread Oleg Nesterov
On 03/17, Oleg Nesterov wrote: Ah, I didn't mean the patch makes sense because of optimization. My point was, we can fix the race without making this code worse (in fact it tries to make the code better but this is subjective). OK, I am stupid. argv_free() passes the wrong poiinter to

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-17 Thread Oleg Nesterov
On 03/16, Andi Kleen wrote: > > On Sat, Mar 16, 2013 at 10:23:51PM +0100, Oleg Nesterov wrote: > > > > > > rcu strings has a helper function to copy the string for sleepy cases. > > > > Then you need to pre-allocate, take rcu_read_lock(), copy, and check > > that it actually fits the pre-allocated

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-17 Thread Oleg Nesterov
On 03/16, Andi Kleen wrote: On Sat, Mar 16, 2013 at 10:23:51PM +0100, Oleg Nesterov wrote: rcu strings has a helper function to copy the string for sleepy cases. Then you need to pre-allocate, take rcu_read_lock(), copy, and check that it actually fits the pre-allocated buffer. Not

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Andi Kleen
On Sat, Mar 16, 2013 at 10:23:51PM +0100, Oleg Nesterov wrote: > On 03/16, Andi Kleen wrote: > > > > > Perhaps rcu can be better, although a global rwsem looks simpler, > > > I dunno. > > > > It's a general problem with lots of sysctls. > > > > > > But argv_split() or its usage should be changed

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Oleg Nesterov
On 03/16, Andi Kleen wrote: > > > Perhaps rcu can be better, although a global rwsem looks simpler, > > I dunno. > > It's a general problem with lots of sysctls. > > > > But argv_split() or its usage should be changed anyway, and GFP_KERNEL > > won't work under rcu_read_lock(). > > rcu strings has

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Andi Kleen
> Perhaps rcu can be better, although a global rwsem looks simpler, > I dunno. It's a general problem with lots of sysctls. > > But argv_split() or its usage should be changed anyway, and GFP_KERNEL > won't work under rcu_read_lock(). rcu strings has a helper function to copy the string for

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Oleg Nesterov
On 03/16, Andi Kleen wrote: > > On Sat, Mar 16, 2013 at 09:23:27PM +0100, Oleg Nesterov wrote: > > On 03/15, Oleg Nesterov wrote: > > > > > > To remind, say, argv_split(poweroff_cmd) can race with sysctl changing > > > this > > > string, in this case it can write to the memory after argv[] array.

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Andi Kleen
On Sat, Mar 16, 2013 at 09:23:27PM +0100, Oleg Nesterov wrote: > On 03/15, Oleg Nesterov wrote: > > > > To remind, say, argv_split(poweroff_cmd) can race with sysctl changing this > > string, in this case it can write to the memory after argv[] array. We can > > fix this, or we can rewrite

[PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Oleg Nesterov
On 03/15, Oleg Nesterov wrote: > > To remind, say, argv_split(poweroff_cmd) can race with sysctl changing this > string, in this case it can write to the memory after argv[] array. We can > fix this, or we can rewrite argv_split/free: OK, please see 1/2. And this reminds me about set_task_comm()

[PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Oleg Nesterov
On 03/15, Oleg Nesterov wrote: To remind, say, argv_split(poweroff_cmd) can race with sysctl changing this string, in this case it can write to the memory after argv[] array. We can fix this, or we can rewrite argv_split/free: OK, please see 1/2. And this reminds me about set_task_comm()

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Andi Kleen
On Sat, Mar 16, 2013 at 09:23:27PM +0100, Oleg Nesterov wrote: On 03/15, Oleg Nesterov wrote: To remind, say, argv_split(poweroff_cmd) can race with sysctl changing this string, in this case it can write to the memory after argv[] array. We can fix this, or we can rewrite argv_split/free:

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Oleg Nesterov
On 03/16, Andi Kleen wrote: On Sat, Mar 16, 2013 at 09:23:27PM +0100, Oleg Nesterov wrote: On 03/15, Oleg Nesterov wrote: To remind, say, argv_split(poweroff_cmd) can race with sysctl changing this string, in this case it can write to the memory after argv[] array. We can fix

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Andi Kleen
Perhaps rcu can be better, although a global rwsem looks simpler, I dunno. It's a general problem with lots of sysctls. But argv_split() or its usage should be changed anyway, and GFP_KERNEL won't work under rcu_read_lock(). rcu strings has a helper function to copy the string for sleepy

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Oleg Nesterov
On 03/16, Andi Kleen wrote: Perhaps rcu can be better, although a global rwsem looks simpler, I dunno. It's a general problem with lots of sysctls. But argv_split() or its usage should be changed anyway, and GFP_KERNEL won't work under rcu_read_lock(). rcu strings has a helper

Re: [PATCH 0/2] finx argv_split() vs sysctl race

2013-03-16 Thread Andi Kleen
On Sat, Mar 16, 2013 at 10:23:51PM +0100, Oleg Nesterov wrote: On 03/16, Andi Kleen wrote: Perhaps rcu can be better, although a global rwsem looks simpler, I dunno. It's a general problem with lots of sysctls. But argv_split() or its usage should be changed anyway, and