Re: + prctl-pr_set_mm-introduce-pr_set_mm_map-operation-v3.patch added to -mm tree

Cyrill Gorcunov
2014-08-23T14:18:25Z

On Sat, Aug 23, 2014 at 03:30:01PM +0200, Oleg Nesterov wrote:
> forgot to mention,
>
> On 08/23, Oleg Nesterov wrote:
> >
> > On 08/23, Cyrill Gorcunov wrote:
> >
> > > Looks like I need
> > > to use cred_guard_mutex instead of task_lock here, no?
> >
> > Please don't. First of all, it can't help because proc_pid_auxv() doesn't hold
> > this lock. It does mm_access() which drops this lock after return. And to remind,
> > we are going to remove mm_access/lock_trace from sys_read() paths in proc.
>
> Besides, it can't help anyway. cred_guard_mutex is per-process (not per-thread),
> suppose that a vfork()'ed child does prctl() while another thread reads the
> parent's /proc/pid/auxv.

Then either I need to use some other lock (not sure which one) either leave it
completely unlocked mentionin in the man page such lockless behaviour. Thoughts?

> Cyrill, I am sorry, but I am starting to think that this patch should be
> dropped and replaced by another version. Or do you think it would be better
> to send the fixes on top?

It's not a problem to drop this particular patch (together with all fixes on
top) one and replace it with new version (this looks like a better idea than
drowning lkml with series of small patches). I rather need to understand what
exactly should be done in new version. So from your previous email

| > Stricktly speaking yes, but don't forget we might need to update
| > exe::file as well which requires lock to be taken.
|
| For reading? I see prctl_set_mm_exe_file_locked() in this patch, probably
| this function was added by another patch. But, if this function calls
| set_mm_exe_file() (I guess it does?) then down_read() is not enough?
| set_mm_exe_file() can race with itself.

yes, for reading, look in set_mm_exe_file we lookup for vma which should
be not present when we change the link, and yes, because of read-only lock
this call can race but only one caller success there because we allow
to change exe link only once.

| But for what? Ignoring the (I think buggy) check in do_shmat() ->start_stack
| is simply unused, we only report it via /proc/. The same for, say, mm->start_code.

that't the good question if this check in do_shmat is buggy or not, why do
you think it's a bug there?

Oleg, letme summarize all the concerns maybe there would be a way to handle
them gracefully

1) How code flows for now (with all fixes on top of current Andrew's queued patches)

- obtain struct prctl_mm_map from userspace
- copy saved_auxv from userspace
- down-read mmap_sem
- validate all the data passed from userspace
- we need a reference to stack-vma for RLIMIT_STACK check (this is doable,
as you said, but until we drop the RLIMIT_STACK from do_shmat I would
prefer it to be here)
- we need to be sure that start_brk, brk, arg_start, arg_end, env_start, env_end
really point to existing VMAs, strictly speaking the probgram can unmap
all own VMAs except executable one and continue running without problem
but this is not that practical I think and at first iteration I prefer
more severe tests here on VMAs
- setup new mm::exe_file (we need to be sure the old exe_file is unmapped
so mmap_sem read-lock is needed)
- update mm::saved_auxv with new values
- finally setup new members to struct mm_struct
- up-read mmap_sem

2) The qustion is do we really need that read-lock would be taken for all this
time? And my answer is yes because of how I implement the checks for start_brk
and etc.

Oleg, check please if I undersnad you correctly, you propose

- drop off mmap_sem completely
- don't verify for RLIMIT_STACK
- drop off task_lock when updating mm::saved_auxv but still invent
how to prevent update/read race

right?