| Message ID | 20190430081844.22597-2-mkoutny@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reduce mmap_sem usage for args manipulation | expand |
On 30.04.2019 11:18, Michal Koutný wrote: > The commit a3b609ef9f8b ("proc read mm's {arg,env}_{start,end} with mmap > semaphore taken.") added synchronization of reading argument/environment > boundaries under mmap_sem. Later commit 88aa7cc688d4 ("mm: introduce > arg_lock to protect arg_start|end and env_start|end in mm_struct") > avoided the coarse use of mmap_sem in similar situations. > > get_cmdline can also use arg_lock instead of mmap_sem when it reads the > boundaries. > > Fixes: 88aa7cc688d4 ("mm: introduce arg_lock to protect arg_start|end and env_start|end in mm_struct") > Cc: Yang Shi <yang.shi@linux.alibaba.com> > Cc: Mateusz Guzik <mguzik@redhat.com> > Signed-off-by: Michal Koutný <mkoutny@suse.com> > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > mm/util.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/util.c b/mm/util.c > index 43a2984bccaa..5cf0e84a0823 100644 > --- a/mm/util.c > +++ b/mm/util.c > @@ -758,12 +758,12 @@ int get_cmdline(struct task_struct *task, char *buffer, int buflen) > if (!mm->arg_end) > goto out_mm; /* Shh! No looking before we're done */ > > - down_read(&mm->mmap_sem); > + spin_lock(&mm->arg_lock); > arg_start = mm->arg_start; > arg_end = mm->arg_end; > env_start = mm->env_start; > env_end = mm->env_end; > - up_read(&mm->mmap_sem); > + spin_unlock(&mm->arg_lock); > > len = arg_end - arg_start; This looks OK for me. But speaking about existing code it's a secret for me, why we ignore arg_lock in binfmt code, e.g. in load_elf_binary().
On Tue, Apr 30, 2019 at 12:09:57PM +0300, Kirill Tkhai wrote: > > This looks OK for me. > > But speaking about existing code it's a secret for me, why we ignore arg_lock > in binfmt code, e.g. in load_elf_binary(). Well, strictly speaking we probably should but you know setup of the @arg_start by kernel's elf loader doesn't cause any side effects as far as I can tell (its been working this lockless way for years, mmap_sem is taken later in the loader code). Though for consistency sake we probably should set it up under the spinlock.
On 30.04.2019 12:38, Cyrill Gorcunov wrote: > On Tue, Apr 30, 2019 at 12:09:57PM +0300, Kirill Tkhai wrote: >> >> This looks OK for me. >> >> But speaking about existing code it's a secret for me, why we ignore arg_lock >> in binfmt code, e.g. in load_elf_binary(). > > Well, strictly speaking we probably should but you know setup of > the @arg_start by kernel's elf loader doesn't cause any side > effects as far as I can tell (its been working this lockless > way for years, mmap_sem is taken later in the loader code). > Though for consistency sake we probably should set it up > under the spinlock. Ok, so elf loader doesn't change these parameters. Thanks for the explanation.
On Tue, Apr 30, 2019 at 12:53:51PM +0300, Kirill Tkhai wrote: > > > > Well, strictly speaking we probably should but you know setup of > > the @arg_start by kernel's elf loader doesn't cause any side > > effects as far as I can tell (its been working this lockless > > way for years, mmap_sem is taken later in the loader code). > > Though for consistency sake we probably should set it up > > under the spinlock. > > Ok, so elf loader doesn't change these parameters. > Thanks for the explanation. It setups these parameters unconditionally. I need to revisit this moment. Technically (if only I'm not missing something obvious) we might have a race here with prctl setting up new params, but this should be harmless since most of them (except stack setup) are purely informative data.
On Tue, Apr 30, 2019 at 01:45:17PM +0300, Cyrill Gorcunov <gorcunov@gmail.com> wrote: > It setups these parameters unconditionally. I need to revisit > this moment. Technically (if only I'm not missing something > obvious) we might have a race here with prctl setting up new > params, but this should be harmless since most of them (except > stack setup) are purely informative data. FTR, when I reviewed that usage, I noticed it was missing the synchronization. My understanding was that the mm_struct isn't yet shared at this moment. I can see some of the operations take place after flush_old_exec (where current->mm = mm_struct), so potentially it is shared since then. OTOH, I guess there aren't concurrent parties that could access the field at this stage of exec. My 2 cents, Michal
On Tue, Apr 30, 2019 at 12:56:10PM +0200, Michal Koutný wrote: > On Tue, Apr 30, 2019 at 01:45:17PM +0300, Cyrill Gorcunov <gorcunov@gmail.com> wrote: > > It setups these parameters unconditionally. I need to revisit > > this moment. Technically (if only I'm not missing something > > obvious) we might have a race here with prctl setting up new > > params, but this should be harmless since most of them (except > > stack setup) are purely informative data. > > FTR, when I reviewed that usage, I noticed it was missing the > synchronization. My understanding was that the mm_struct isn't yet > shared at this moment. I can see some of the operations take place after > flush_old_exec (where current->mm = mm_struct), so potentially it is > shared since then. OTOH, I guess there aren't concurrent parties that > could access the field at this stage of exec. Just revisited this code -- we're either executing prctl, either execve. Since both operates with current task we're safe.
diff --git a/mm/util.c b/mm/util.c index 43a2984bccaa..5cf0e84a0823 100644 --- a/mm/util.c +++ b/mm/util.c @@ -758,12 +758,12 @@ int get_cmdline(struct task_struct *task, char *buffer, int buflen) if (!mm->arg_end) goto out_mm; /* Shh! No looking before we're done */ - down_read(&mm->mmap_sem); + spin_lock(&mm->arg_lock); arg_start = mm->arg_start; arg_end = mm->arg_end; env_start = mm->env_start; env_end = mm->env_end; - up_read(&mm->mmap_sem); + spin_unlock(&mm->arg_lock); len = arg_end - arg_start;
The commit a3b609ef9f8b ("proc read mm's {arg,env}_{start,end} with mmap semaphore taken.") added synchronization of reading argument/environment boundaries under mmap_sem. Later commit 88aa7cc688d4 ("mm: introduce arg_lock to protect arg_start|end and env_start|end in mm_struct") avoided the coarse use of mmap_sem in similar situations. get_cmdline can also use arg_lock instead of mmap_sem when it reads the boundaries. Fixes: 88aa7cc688d4 ("mm: introduce arg_lock to protect arg_start|end and env_start|end in mm_struct") Cc: Yang Shi <yang.shi@linux.alibaba.com> Cc: Mateusz Guzik <mguzik@redhat.com> Signed-off-by: Michal Koutný <mkoutny@suse.com> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- mm/util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)