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

[Oleg Nesterov <oleg@redhat.com>]

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 sure why the simple
> > rwsem is worse.
>
> The reason I did it originally like that was that some of the sysctls weren't
> as "slow path" as power off.

OK, in this case rwsem is not fine, I agree.

> > > > To me 1/2 looks as a simplification anyway, but I won't argue if we
> > > > decide to add rcu/locking and avoid this patch.
> > >
> > > Ok I'll revisit.
> >
> > OK, but do you agree with 1/2?
>
> It doesn't solve the race alone because when the 0 byte can move it's
> not safe to run kstrndup() in parallel.

I don't think this is possible... To simplify, lets consider
poweroff_cmd[POWEROFF_CMD_PATH_LEN]. proc_dostring() or whatever can
never overwrite the last byte == 0, otherwise it will exceed this array
later to write the final zero.

But this doesn't matter,

> Ok given the n and that it
> force terminates it could only lead to some junk at the end.

Yes, kstrndup(max) is always safe even if the memory is not null
terminated.

> But it seems like a useful small optimization, although I don't know
> if it's used in any non slow paths.

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).

"fix the race" only means "make it safe" of course, a string in between
is unlikely useful.

> I assume you audited all callers that they comprehend that they need
> to free differently now.

Yes, /bin/grep shows that all callers use argv_free().

Oleg.