* + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal.patch added to -mm tree
@ 2012-04-19 18:52 akpm
2012-04-19 19:20 ` + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch " Oleg Nesterov
0 siblings, 1 reply; 14+ messages in thread
From: akpm @ 2012-04-19 18:52 UTC (permalink / raw)
To: mm-commits
Cc: khlebnikov, gorcunov, keescook, kosaki.motohiro, matthltc, oleg,
tj, xemul
The patch titled
Subject: c/r: prctl: update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal
has been added to the -mm tree. Its filename is
c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal.patch
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/SubmitChecklist when testing your code ***
The -mm tree is included into linux-next and is updated
there every 3-4 working days
------------------------------------------------------
From: Konstantin Khlebnikov <khlebnikov@openvz.org>
Subject: c/r: prctl: update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal
[ fix for "c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-v2" from mm tree ]
After removing mm->num_exe_file_vmas kernel keeps mm->exe_file until final
mmput(), it never becomes NULL while task is alive.
We can check for other mapped files in mm instead of checking
mm->num_exe_file_vmas, and mark mm with flag MMF_EXE_FILE_CHANGED in order
to forbid second changing of mm->exe_file.
Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Matt Helsley <matthltc@us.ibm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Pavel Emelyanov <xemul@parallels.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
include/linux/sched.h | 1 +
kernel/sys.c | 31 +++++++++++++++++++------------
2 files changed, 20 insertions(+), 12 deletions(-)
diff -puN include/linux/sched.h~c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal include/linux/sched.h
--- a/include/linux/sched.h~c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal
+++ a/include/linux/sched.h
@@ -437,6 +437,7 @@ extern int get_dumpable(struct mm_struct
/* leave room for more dump flags */
#define MMF_VM_MERGEABLE 16 /* KSM may merge identical pages */
#define MMF_VM_HUGEPAGE 17 /* set when VM_HUGEPAGE is set on vma */
+#define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
#define MMF_INIT_MASK (MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
diff -puN kernel/sys.c~c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal kernel/sys.c
--- a/kernel/sys.c~c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal
+++ a/kernel/sys.c
@@ -1714,17 +1714,11 @@ static bool vma_flags_mismatch(struct vm
static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
{
+ struct vm_area_struct *vma;
struct file *exe_file;
struct dentry *dentry;
int err;
- /*
- * Setting new mm::exe_file is only allowed when no VM_EXECUTABLE vma's
- * remain. So perform a quick test first.
- */
- if (mm->num_exe_file_vmas)
- return -EBUSY;
-
exe_file = fget(fd);
if (!exe_file)
return -EBADF;
@@ -1745,17 +1739,30 @@ static int prctl_set_mm_exe_file(struct
if (err)
goto exit;
+ down_write(&mm->mmap_sem);
+
+ /*
+ * Forbid mm->exe_file change if there are mapped other files.
+ */
+ err = -EBUSY;
+ for (vma = mm->mmap; vma; vma = vma->vm_next) {
+ if (vma->vm_file && !path_equal(&vma->vm_file->f_path,
+ &exe_file->f_path))
+ goto exit_unlock;
+ }
+
/*
* The symlink can be changed only once, just to disallow arbitrary
* transitions malicious software might bring in. This means one
* could make a snapshot over all processes running and monitor
* /proc/pid/exe changes to notice unusual activity if needed.
*/
- down_write(&mm->mmap_sem);
- if (likely(!mm->exe_file))
- set_mm_exe_file(mm, exe_file);
- else
- err = -EBUSY;
+ err = -EPERM;
+ if (test_and_set_bit(MMF_EXE_FILE_CHANGED, &mm->flags))
+ goto exit_unlock;
+
+ set_mm_exe_file(mm, exe_file);
+exit_unlock:
up_write(&mm->mmap_sem);
exit:
_
Subject: Subject: c/r: prctl: update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal
Patches currently in -mm which might be from khlebnikov@openvz.org are
mm-hugetlb-fix-warning-in-alloc_huge_page-dequeue_huge_page_vma.patch
linux-next.patch
mm-correctly-synchronize-rss-counters-at-exit-exec.patch
mm-correctly-synchronize-rss-counters-at-exit-exec-fix.patch
mm-correctly-synchronize-rss-counters-at-exit-exec-set-task-exit-code-before-complete_vfork_done.patch
mm-remove-swap-token-code.patch
mm-vmscan-remove-lumpy-reclaim.patch
mm-vmscan-do-not-stall-on-writeback-during-memory-compaction.patch
mm-vmscan-remove-reclaim_mode_t.patch
mm-memcg-scanning_global_lru-means-mem_cgroup_disabled.patch
mm-memcg-move-reclaim_stat-into-lruvec.patch
mm-push-lru-index-into-shrink_active_list.patch
mm-push-lru-index-into-shrink_active_list-fix.patch
mm-mark-mm-inline-functions-as-__always_inline.patch
mm-remove-lru-type-checks-from-__isolate_lru_page.patch
mm-memcg-kill-mem_cgroup_lru_del.patch
mm-memcg-use-vm_swappiness-from-target-memory-cgroup.patch
fork-call-complete_vfork_done-after-clearing-child_tid-and-flushing-rss-counters.patch
c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal.patch
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 18:52 + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal.patch added to -mm tree akpm
@ 2012-04-19 19:20 ` Oleg Nesterov
2012-04-19 21:00 ` Cyrill Gorcunov
0 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2012-04-19 19:20 UTC (permalink / raw)
To: akpm
Cc: khlebnikov, gorcunov, keescook, kosaki.motohiro, matthltc, tj,
xemul, linux-kernel
On 04/19, Andrew Morton wrote:
>
> From: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Subject: c/r: prctl: update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal
>
> [ fix for "c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-v2" from mm tree ]
>
> After removing mm->num_exe_file_vmas kernel keeps mm->exe_file until final
> mmput(), it never becomes NULL while task is alive.
>
> We can check for other mapped files in mm instead of checking
> mm->num_exe_file_vmas, and mark mm with flag MMF_EXE_FILE_CHANGED in order
> to forbid second changing of mm->exe_file.
I lost the track a long ago.
Just one question, what does this "forbid second changing" actually mean?
> * The symlink can be changed only once, just to disallow arbitrary
> * transitions malicious software might bring in. This means one
> * could make a snapshot over all processes running and monitor
> * /proc/pid/exe changes to notice unusual activity if needed.
> */
> - down_write(&mm->mmap_sem);
> - if (likely(!mm->exe_file))
> - set_mm_exe_file(mm, exe_file);
> - else
> - err = -EBUSY;
> + err = -EPERM;
> + if (test_and_set_bit(MMF_EXE_FILE_CHANGED, &mm->flags))
> + goto exit_unlock;
> +
> + set_mm_exe_file(mm, exe_file);
> +exit_unlock:
OK, I am not arguing, but looking at this code I suspect that you
also want to forbid the second change after fork too?
If yes, then you probably need to include MMF_EXE_FILE_CHANGED in
MMF_INIT_MASK.
But at the same time, then it should be probably cleared somewhere
in bprm_mm_init() paths.
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 19:20 ` + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch " Oleg Nesterov
@ 2012-04-19 21:00 ` Cyrill Gorcunov
2012-04-19 21:12 ` Oleg Nesterov
0 siblings, 1 reply; 14+ messages in thread
From: Cyrill Gorcunov @ 2012-04-19 21:00 UTC (permalink / raw)
To: Oleg Nesterov
Cc: akpm, khlebnikov, keescook, kosaki.motohiro, matthltc, tj, xemul,
linux-kernel
On Thu, Apr 19, 2012 at 09:20:05PM +0200, Oleg Nesterov wrote:
> On 04/19, Andrew Morton wrote:
> >
> > From: Konstantin Khlebnikov <khlebnikov@openvz.org>
> > Subject: c/r: prctl: update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal
> >
> > [ fix for "c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-v2" from mm tree ]
> >
> > After removing mm->num_exe_file_vmas kernel keeps mm->exe_file until final
> > mmput(), it never becomes NULL while task is alive.
> >
> > We can check for other mapped files in mm instead of checking
> > mm->num_exe_file_vmas, and mark mm with flag MMF_EXE_FILE_CHANGED in order
> > to forbid second changing of mm->exe_file.
>
> I lost the track a long ago.
>
> Just one question, what does this "forbid second changing" actually mean?
Heh :) Oleg, it was actually your idea to make this feature "one-shot".
Once exe-file changed to a new value, it can't be changed again. The
reason was to bring at least minimum disturbance in sysadmins life.
Cyrill
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 21:00 ` Cyrill Gorcunov
@ 2012-04-19 21:12 ` Oleg Nesterov
2012-04-19 21:32 ` Cyrill Gorcunov
2012-04-19 21:46 ` Konstantin Khlebnikov
0 siblings, 2 replies; 14+ messages in thread
From: Oleg Nesterov @ 2012-04-19 21:12 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: akpm, khlebnikov, keescook, kosaki.motohiro, matthltc, tj, xemul,
linux-kernel
On 04/20, Cyrill Gorcunov wrote:
>
> On Thu, Apr 19, 2012 at 09:20:05PM +0200, Oleg Nesterov wrote:
> > On 04/19, Andrew Morton wrote:
> > >
> > > From: Konstantin Khlebnikov <khlebnikov@openvz.org>
> > > Subject: c/r: prctl: update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal
> > >
> > > [ fix for "c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-v2" from mm tree ]
> > >
> > > After removing mm->num_exe_file_vmas kernel keeps mm->exe_file until final
> > > mmput(), it never becomes NULL while task is alive.
> > >
> > > We can check for other mapped files in mm instead of checking
> > > mm->num_exe_file_vmas, and mark mm with flag MMF_EXE_FILE_CHANGED in order
> > > to forbid second changing of mm->exe_file.
> >
> > I lost the track a long ago.
> >
> > Just one question, what does this "forbid second changing" actually mean?
>
> Heh :) Oleg, it was actually your idea to make this feature "one-shot".
Heh, no ;)
IIRC, I only asked you what do you actually want,
Just one note for the record, prctl_set_mm_exe_file() does
if (mm->num_exe_file_vmas)
return -EBUSY;
We could do
if (mm->exe_file)
return -EBUSY;
This way "because this feature is a special to C/R" becomes
really true. IOW, you can't do PR_SET_MM_EXE_FILE twice.
I am fine either way, just I want to ensure you really want
the current version.
and only because it was documented as "feature is a special to C/R".
> Once exe-file changed to a new value, it can't be changed again. The
> reason was to bring at least minimum disturbance in sysadmins life.
You misunderstood. I am not arguing with "one-shot", I do not really
care.
My question is: unless I missed something "it can't be changed again"
is not actually true. A task does PR_SET_MM_EXE_FILE, then it forks
the new child. The child can do PR_SET_MM_EXE_FILE again. Is this
by design?
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 21:12 ` Oleg Nesterov
@ 2012-04-19 21:32 ` Cyrill Gorcunov
2012-04-19 22:08 ` Konstantin Khlebnikov
2012-04-19 21:46 ` Konstantin Khlebnikov
1 sibling, 1 reply; 14+ messages in thread
From: Cyrill Gorcunov @ 2012-04-19 21:32 UTC (permalink / raw)
To: Oleg Nesterov
Cc: akpm, khlebnikov, keescook, kosaki.motohiro, matthltc, tj, xemul,
linux-kernel
On Thu, Apr 19, 2012 at 11:12:16PM +0200, Oleg Nesterov wrote:
> > Heh :) Oleg, it was actually your idea to make this feature "one-shot".
>
> Heh, no ;)
>
> IIRC, I only asked you what do you actually want,
>
> Just one note for the record, prctl_set_mm_exe_file() does
>
> if (mm->num_exe_file_vmas)
> return -EBUSY;
>
> We could do
>
> if (mm->exe_file)
> return -EBUSY;
>
> This way "because this feature is a special to C/R" becomes
> really true. IOW, you can't do PR_SET_MM_EXE_FILE twice.
>
> I am fine either way, just I want to ensure you really want
> the current version.
>
> and only because it was documented as "feature is a special to C/R".
ok, ubedil :)
> > Once exe-file changed to a new value, it can't be changed again. The
> > reason was to bring at least minimum disturbance in sysadmins life.
>
> You misunderstood. I am not arguing with "one-shot", I do not really
> care.
>
> My question is: unless I missed something "it can't be changed again"
> is not actually true. A task does PR_SET_MM_EXE_FILE, then it forks
> the new child. The child can do PR_SET_MM_EXE_FILE again. Is this
> by design?
Hmm, not sure, Konstantin?
Cyrill
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 21:12 ` Oleg Nesterov
2012-04-19 21:32 ` Cyrill Gorcunov
@ 2012-04-19 21:46 ` Konstantin Khlebnikov
2012-04-19 21:51 ` Oleg Nesterov
1 sibling, 1 reply; 14+ messages in thread
From: Konstantin Khlebnikov @ 2012-04-19 21:46 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Cyrill Gorcunov, akpm, keescook, kosaki.motohiro, matthltc, tj,
Pavel Emelianov, linux-kernel
Oleg Nesterov wrote:
> On 04/20, Cyrill Gorcunov wrote:
>>
>> On Thu, Apr 19, 2012 at 09:20:05PM +0200, Oleg Nesterov wrote:
>>> On 04/19, Andrew Morton wrote:
>>>>
>>>> From: Konstantin Khlebnikov<khlebnikov@openvz.org>
>>>> Subject: c/r: prctl: update prctl_set_mm_exe_file() after mm->num_exe_file_vmas removal
>>>>
>>>> [ fix for "c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-v2" from mm tree ]
>>>>
>>>> After removing mm->num_exe_file_vmas kernel keeps mm->exe_file until final
>>>> mmput(), it never becomes NULL while task is alive.
>>>>
>>>> We can check for other mapped files in mm instead of checking
>>>> mm->num_exe_file_vmas, and mark mm with flag MMF_EXE_FILE_CHANGED in order
>>>> to forbid second changing of mm->exe_file.
>>>
>>> I lost the track a long ago.
>>>
>>> Just one question, what does this "forbid second changing" actually mean?
>>
>> Heh :) Oleg, it was actually your idea to make this feature "one-shot".
>
> Heh, no ;)
>
> IIRC, I only asked you what do you actually want,
>
> Just one note for the record, prctl_set_mm_exe_file() does
>
> if (mm->num_exe_file_vmas)
> return -EBUSY;
>
> We could do
>
> if (mm->exe_file)
> return -EBUSY;
>
> This way "because this feature is a special to C/R" becomes
> really true. IOW, you can't do PR_SET_MM_EXE_FILE twice.
>
> I am fine either way, just I want to ensure you really want
> the current version.
>
> and only because it was documented as "feature is a special to C/R".
>
>> Once exe-file changed to a new value, it can't be changed again. The
>> reason was to bring at least minimum disturbance in sysadmins life.
>
> You misunderstood. I am not arguing with "one-shot", I do not really
> care.
>
> My question is: unless I missed something "it can't be changed again"
> is not actually true. A task does PR_SET_MM_EXE_FILE, then it forks
> the new child. The child can do PR_SET_MM_EXE_FILE again. Is this
> by design?
>
> Oleg.
>
I found more weird case: child thread (with CLONE_THREAD and without CLONE_VM)
changes its exe_file...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 21:46 ` Konstantin Khlebnikov
@ 2012-04-19 21:51 ` Oleg Nesterov
2012-04-19 22:02 ` Cyrill Gorcunov
0 siblings, 1 reply; 14+ messages in thread
From: Oleg Nesterov @ 2012-04-19 21:51 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Cyrill Gorcunov, akpm, keescook, kosaki.motohiro, matthltc, tj,
Pavel Emelianov, linux-kernel
On 04/20, Konstantin Khlebnikov wrote:
>
> Oleg Nesterov wrote:
>>
>> You misunderstood. I am not arguing with "one-shot", I do not really
>> care.
>>
>> My question is: unless I missed something "it can't be changed again"
>> is not actually true. A task does PR_SET_MM_EXE_FILE, then it forks
>> the new child. The child can do PR_SET_MM_EXE_FILE again. Is this
>> by design?
>>
>> Oleg.
>>
>
> I found more weird case: child thread (with CLONE_THREAD and without CLONE_VM)
> changes its exe_file...
No. copy_process() does:
if ((clone_flags & CLONE_THREAD) && !(clone_flags & CLONE_SIGHAND))
return ERR_PTR(-EINVAL);
if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM))
return ERR_PTR(-EINVAL);
IOW, CLONE_THREAD => CLONE_SIGHAND => CLONE_VM
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 21:51 ` Oleg Nesterov
@ 2012-04-19 22:02 ` Cyrill Gorcunov
2012-04-19 22:09 ` Oleg Nesterov
0 siblings, 1 reply; 14+ messages in thread
From: Cyrill Gorcunov @ 2012-04-19 22:02 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Konstantin Khlebnikov, akpm, keescook, kosaki.motohiro, matthltc,
tj, Pavel Emelianov, linux-kernel
On Thu, Apr 19, 2012 at 11:51:09PM +0200, Oleg Nesterov wrote:
> On 04/20, Konstantin Khlebnikov wrote:
> >
> > Oleg Nesterov wrote:
> >>
> >> You misunderstood. I am not arguing with "one-shot", I do not really
> >> care.
> >>
> >> My question is: unless I missed something "it can't be changed again"
> >> is not actually true. A task does PR_SET_MM_EXE_FILE, then it forks
> >> the new child. The child can do PR_SET_MM_EXE_FILE again. Is this
> >> by design?
> >>
> >> Oleg.
> >>
> >
> > I found more weird case: child thread (with CLONE_THREAD and without CLONE_VM)
> > changes its exe_file...
>
> No. copy_process() does:
>
> if ((clone_flags & CLONE_THREAD) && !(clone_flags & CLONE_SIGHAND))
> return ERR_PTR(-EINVAL);
>
> if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM))
> return ERR_PTR(-EINVAL);
>
> IOW, CLONE_THREAD => CLONE_SIGHAND => CLONE_VM
Guys, while I more-less agree with Matt about single-shot behaviour
[ let me copy my and his email
>> With mm->exe_file this prctl option become a one-shot
>> only, and while at moment our user-space tool can perfectly
>> live with that I thought that there is no strict need to
>> limit the option this way from the very beginning.
>>
> As far as backward compatibility, isn't it better to lift that restriction
> later rather than add it? I think the latter would very likely "break"
> things whereas the former would not.
>
> I also prefer that restriction because it establishes a bound on how
> frequently the symlink can change. Keeping it a one-shot deal makes the
> values that show up in tools like top more reliable for admins.
]
I guess maybe it's time to drop one-shot requirement and as result
we can drop MMF_EXE_FILE_CHANGED bit completely, making overall code
simplier?
Our tool can live with any behaviour (and one shot and multishot,
doesn't matter).
Cyrill
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 21:32 ` Cyrill Gorcunov
@ 2012-04-19 22:08 ` Konstantin Khlebnikov
2012-04-19 22:16 ` Cyrill Gorcunov
2012-04-19 22:29 ` Oleg Nesterov
0 siblings, 2 replies; 14+ messages in thread
From: Konstantin Khlebnikov @ 2012-04-19 22:08 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Oleg Nesterov, akpm, keescook, kosaki.motohiro, matthltc, tj,
Pavel Emelianov, linux-kernel
Cyrill Gorcunov wrote:
> On Thu, Apr 19, 2012 at 11:12:16PM +0200, Oleg Nesterov wrote:
>>> Heh :) Oleg, it was actually your idea to make this feature "one-shot".
>>
>> Heh, no ;)
>>
>> IIRC, I only asked you what do you actually want,
>>
>> Just one note for the record, prctl_set_mm_exe_file() does
>>
>> if (mm->num_exe_file_vmas)
>> return -EBUSY;
>>
>> We could do
>>
>> if (mm->exe_file)
>> return -EBUSY;
>>
>> This way "because this feature is a special to C/R" becomes
>> really true. IOW, you can't do PR_SET_MM_EXE_FILE twice.
>>
>> I am fine either way, just I want to ensure you really want
>> the current version.
>>
>> and only because it was documented as "feature is a special to C/R".
>
> ok, ubedil :)
>
>>> Once exe-file changed to a new value, it can't be changed again. The
>>> reason was to bring at least minimum disturbance in sysadmins life.
>>
>> You misunderstood. I am not arguing with "one-shot", I do not really
>> care.
>>
>> My question is: unless I missed something "it can't be changed again"
>> is not actually true. A task does PR_SET_MM_EXE_FILE, then it forks
>> the new child. The child can do PR_SET_MM_EXE_FILE again. Is this
>> by design?
>
> Hmm, not sure, Konstantin?
Why not? It has new pid, why it cannot change exe_file? Actually I don't care too.
But even if we include this bit into MMF_INIT_MASK we cannot forbid exe-file change
in childs tasks which was forked before exe-file change in parent.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 22:02 ` Cyrill Gorcunov
@ 2012-04-19 22:09 ` Oleg Nesterov
2012-04-19 22:28 ` Konstantin Khlebnikov
2012-04-19 22:32 ` Cyrill Gorcunov
0 siblings, 2 replies; 14+ messages in thread
From: Oleg Nesterov @ 2012-04-19 22:09 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Konstantin Khlebnikov, akpm, keescook, kosaki.motohiro, matthltc,
tj, Pavel Emelianov, linux-kernel
On 04/20, Cyrill Gorcunov wrote:
>
> Guys, while I more-less agree with Matt about single-shot behaviour
>
> [ let me copy my and his email
>
> >> With mm->exe_file this prctl option become a one-shot
> >> only, and while at moment our user-space tool can perfectly
> >> live with that I thought that there is no strict need to
> >> limit the option this way from the very beginning.
> >>
> > As far as backward compatibility, isn't it better to lift that restriction
> > later rather than add it? I think the latter would very likely "break"
> > things whereas the former would not.
> >
> > I also prefer that restriction because it establishes a bound on how
> > frequently the symlink can change. Keeping it a one-shot deal makes the
> > values that show up in tools like top more reliable for admins.
> ]
>
> I guess maybe it's time to drop one-shot requirement and as result
> we can drop MMF_EXE_FILE_CHANGED bit completely,
Plus perhaps we can remove this for_each_vma check?
> making overall code
> simplier?
Personally I'd certainly prefer this ;)
But let me repeat to avoid the confusion. I am fine either way,
I am not going to discuss this again unless I see something which
looks technically wrong. And the current MMF_EXE_FILE_CHANGED
doesn't look right even if the problem is minor.
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 22:08 ` Konstantin Khlebnikov
@ 2012-04-19 22:16 ` Cyrill Gorcunov
2012-04-19 22:29 ` Oleg Nesterov
1 sibling, 0 replies; 14+ messages in thread
From: Cyrill Gorcunov @ 2012-04-19 22:16 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Oleg Nesterov, akpm, keescook, kosaki.motohiro, matthltc, tj,
Pavel Emelianov, linux-kernel
On Fri, Apr 20, 2012 at 02:08:43AM +0400, Konstantin Khlebnikov wrote:
> >>My question is: unless I missed something "it can't be changed again"
> >>is not actually true. A task does PR_SET_MM_EXE_FILE, then it forks
> >>the new child. The child can do PR_SET_MM_EXE_FILE again. Is this
> >>by design?
> >
> >Hmm, not sure, Konstantin?
>
> Why not? It has new pid, why it cannot change exe_file? Actually I don't care too.
> But even if we include this bit into MMF_INIT_MASK we cannot forbid exe-file change
> in childs tasks which was forked before exe-file change in parent.
OK, I see, thanks.
Cyrill
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 22:09 ` Oleg Nesterov
@ 2012-04-19 22:28 ` Konstantin Khlebnikov
2012-04-19 22:32 ` Cyrill Gorcunov
1 sibling, 0 replies; 14+ messages in thread
From: Konstantin Khlebnikov @ 2012-04-19 22:28 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Cyrill Gorcunov, akpm, keescook, kosaki.motohiro, matthltc, tj,
Pavel Emelianov, linux-kernel
Oleg Nesterov wrote:
> On 04/20, Cyrill Gorcunov wrote:
>>
>> Guys, while I more-less agree with Matt about single-shot behaviour
>>
>> [ let me copy my and his email
>>
>> >> With mm->exe_file this prctl option become a one-shot
>> >> only, and while at moment our user-space tool can perfectly
>> >> live with that I thought that there is no strict need to
>> >> limit the option this way from the very beginning.
>> >>
>> > As far as backward compatibility, isn't it better to lift that restriction
>> > later rather than add it? I think the latter would very likely "break"
>> > things whereas the former would not.
>> >
>> > I also prefer that restriction because it establishes a bound on how
>> > frequently the symlink can change. Keeping it a one-shot deal makes the
>> > values that show up in tools like top more reliable for admins.
>> ]
>>
>> I guess maybe it's time to drop one-shot requirement and as result
>> we can drop MMF_EXE_FILE_CHANGED bit completely,
>
> Plus perhaps we can remove this for_each_vma check?
>
>> making overall code
>> simplier?
>
> Personally I'd certainly prefer this ;)
>
>
>
> But let me repeat to avoid the confusion. I am fine either way,
> I am not going to discuss this again unless I see something which
> looks technically wrong. And the current MMF_EXE_FILE_CHANGED
> doesn't look right even if the problem is minor.
Yeah, whole this protection does not protect anything and can be easily bypassed.
For example task can re-execute itself and change exe-file again and again.
>
> Oleg.
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 22:08 ` Konstantin Khlebnikov
2012-04-19 22:16 ` Cyrill Gorcunov
@ 2012-04-19 22:29 ` Oleg Nesterov
1 sibling, 0 replies; 14+ messages in thread
From: Oleg Nesterov @ 2012-04-19 22:29 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Cyrill Gorcunov, akpm, keescook, kosaki.motohiro, matthltc, tj,
Pavel Emelianov, linux-kernel
On 04/20, Konstantin Khlebnikov wrote:
>
>>> My question is: unless I missed something "it can't be changed again"
>>> is not actually true. A task does PR_SET_MM_EXE_FILE, then it forks
>>> the new child. The child can do PR_SET_MM_EXE_FILE again. Is this
>>> by design?
>>
>> Hmm, not sure, Konstantin?
>
> Why not? It has new pid, why it cannot change exe_file?
OK, if you do not see a problem - I agree with this patch.
I don't really understand what PR_SET_MM_EXE_FILE buys us if
parent/child can have different /proc/pid/exe links without
exec, but
> Actually I don't care too.
same here ;)
and at least this addresses the "establishes a bound on how
frequently the symlink can change" from Matt.
Oleg.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch added to -mm tree
2012-04-19 22:09 ` Oleg Nesterov
2012-04-19 22:28 ` Konstantin Khlebnikov
@ 2012-04-19 22:32 ` Cyrill Gorcunov
1 sibling, 0 replies; 14+ messages in thread
From: Cyrill Gorcunov @ 2012-04-19 22:32 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Konstantin Khlebnikov, akpm, keescook, kosaki.motohiro, matthltc,
tj, Pavel Emelianov, linux-kernel
On Fri, Apr 20, 2012 at 12:09:19AM +0200, Oleg Nesterov wrote:
> >
> > I guess maybe it's time to drop one-shot requirement and as result
> > we can drop MMF_EXE_FILE_CHANGED bit completely,
>
> Plus perhaps we can remove this for_each_vma check?
>
> > making overall code simplier?
>
> Personally I'd certainly prefer this ;)
>
> But let me repeat to avoid the confusion. I am fine either way,
> I am not going to discuss this again unless I see something which
> looks technically wrong. And the current MMF_EXE_FILE_CHANGED
> doesn't look right even if the problem is minor.
So if there no stong agrues against, lets rip all together --
and for_each_vma and MMF_EXE_FILE_CHANGED bits, finally making
code simplier.
Cyrill
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-04-19 23:02 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-19 18:52 + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm-num_exe_file_vmas-removal.patch added to -mm tree akpm
2012-04-19 19:20 ` + c-r-prctl-add-ability-to-set-new-mm_struct-exe_file-update-after-mm- num_exe_file_vmas-removal.patch " Oleg Nesterov
2012-04-19 21:00 ` Cyrill Gorcunov
2012-04-19 21:12 ` Oleg Nesterov
2012-04-19 21:32 ` Cyrill Gorcunov
2012-04-19 22:08 ` Konstantin Khlebnikov
2012-04-19 22:16 ` Cyrill Gorcunov
2012-04-19 22:29 ` Oleg Nesterov
2012-04-19 21:46 ` Konstantin Khlebnikov
2012-04-19 21:51 ` Oleg Nesterov
2012-04-19 22:02 ` Cyrill Gorcunov
2012-04-19 22:09 ` Oleg Nesterov
2012-04-19 22:28 ` Konstantin Khlebnikov
2012-04-19 22:32 ` Cyrill Gorcunov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.