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

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

[Oleg Nesterov]

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.

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

[Cyrill Gorcunov]

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

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

[Oleg Nesterov]

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.

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

[Cyrill Gorcunov]

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

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

[Konstantin Khlebnikov]

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.

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

[Cyrill Gorcunov]

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

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

[Oleg Nesterov]

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.

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

[Konstantin Khlebnikov]

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

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

[Oleg Nesterov]

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.

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

[Cyrill Gorcunov]

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

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

[Oleg Nesterov]

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.

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

[Konstantin Khlebnikov]

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

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

[Cyrill Gorcunov]

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