Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

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

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


oleg at redhat

Apr 19, 2012, 12:20 PM

Post #1 of 13 (139 views)
Permalink
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

On 04/19, Andrew Morton wrote:
>
> From: Konstantin Khlebnikov <khlebnikov [at] openvz>
> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gorcunov at openvz

Apr 19, 2012, 2:00 PM

Post #2 of 13 (140 views)
Permalink
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 [In reply to]

On Thu, Apr 19, 2012 at 09:20:05PM +0200, Oleg Nesterov wrote:
> On 04/19, Andrew Morton wrote:
> >
> > From: Konstantin Khlebnikov <khlebnikov [at] openvz>
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


oleg at redhat

Apr 19, 2012, 2:12 PM

Post #3 of 13 (138 views)
Permalink
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 [In reply to]

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 [at] openvz>
> > > 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gorcunov at openvz

Apr 19, 2012, 2:32 PM

Post #4 of 13 (134 views)
Permalink
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 [In reply to]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


khlebnikov at openvz

Apr 19, 2012, 2:46 PM

Post #5 of 13 (135 views)
Permalink
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 [In reply to]

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 [at] openvz>
>>>> 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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


oleg at redhat

Apr 19, 2012, 2:51 PM

Post #6 of 13 (136 views)
Permalink
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 [In reply to]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gorcunov at openvz

Apr 19, 2012, 3:02 PM

Post #7 of 13 (134 views)
Permalink
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 [In reply to]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


khlebnikov at openvz

Apr 19, 2012, 3:08 PM

Post #8 of 13 (134 views)
Permalink
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 [In reply to]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


oleg at redhat

Apr 19, 2012, 3:09 PM

Post #9 of 13 (134 views)
Permalink
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 [In reply to]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gorcunov at openvz

Apr 19, 2012, 3:16 PM

Post #10 of 13 (136 views)
Permalink
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 [In reply to]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


khlebnikov at openvz

Apr 19, 2012, 3:28 PM

Post #11 of 13 (137 views)
Permalink
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 [In reply to]

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.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


oleg at redhat

Apr 19, 2012, 3:29 PM

Post #12 of 13 (134 views)
Permalink
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 [In reply to]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gorcunov at openvz

Apr 19, 2012, 3:32 PM

Post #13 of 13 (133 views)
Permalink
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 [In reply to]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.