Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree

Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree

[Oleg Nesterov]

On 01/26, Andrew Morton wrote:
>
> From: Neil Horman <nhorman@tuxdriver.com>
>
> What I've done is modify the
> call_usermodehelper() api such that an extra parameter is added, a
> function pointer which will be called by the user helper task, after it
> forks, but before it execs the required process.

Personally I agree, I think this fptr can be useful, not only for coredump.

> This will give the
> caller the opportunity to get a callback in the process's context,
> allowing it to do whatever it needs to to the process in the kernel

in this case it probably needs "void *data" argument, otherwise the
usage is very limited.

Currently only d_coredump() needs this new feature, but please note
that ____call_usermodehelper() was already "uglified" for the coredumping
over the pipe.

If we add sub_info->finit(), then probably we should move the code
under "if (sub_info->stdin)" from ____call_usermodehelper() to
core_pipe_setup() ?

> +/*
> + * This is used as a helper to set up the task that execs
> + * our user space core collector application
> + * Its called in the context of the task thats going to
> + * exec itself to be the helper, so we can modify current here
> + */

very minor nit, perhaps the comment should explain what is the meaning
of the magical rlim_cur = 1 value? It is not immediately obvious we
check cprm.limit == 1 below.

> +void core_pipe_setup(void)
> +{
> +	task_lock(current->group_leader);
> +	current->signal->rlim[RLIMIT_CORE].rlim_cur = 1;
> +	task_unlock(current->group_leader);
> +}

Well, this thread must be the kernel thread and thus it should be
->group_leader and I don't think we really need task_lock() her,
but this is minor and perhaps ->group_leader + task_lock() look
better even if not needed.

Oleg.

Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree

[Oleg Nesterov]

On 01/27, Oleg Nesterov wrote:
>
> Currently only d_coredump() needs this new feature, but please note
> that ____call_usermodehelper() was already "uglified" for the coredumping
> over the pipe.
>
> If we add sub_info->finit(), then probably we should move the code
> under "if (sub_info->stdin)" from ____call_usermodehelper() to
> core_pipe_setup() ?

And, perhaps, we should not change call_usermodehelper() and all its
callers? If the caller needs ->finit() it can customize subprocess_info
like call_usermodehelper_pipe() already does?

To clarify, I don't have a "strong" opinion, I am just asking.

Oleg.

Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree

[Neil Horman]

On Wed, Jan 27, 2010 at 06:58:52PM +0100, Oleg Nesterov wrote:
> On 01/27, Oleg Nesterov wrote:
> >
> > Currently only d_coredump() needs this new feature, but please note
> > that ____call_usermodehelper() was already "uglified" for the coredumping
> > over the pipe.
> >
> > If we add sub_info->finit(), then probably we should move the code
> > under "if (sub_info->stdin)" from ____call_usermodehelper() to
> > core_pipe_setup() ?
> 
> And, perhaps, we should not change call_usermodehelper() and all its
> callers? If the caller needs ->finit() it can customize subprocess_info
> like call_usermodehelper_pipe() already does?
> 
> To clarify, I don't have a "strong" opinion, I am just asking.
> 
I'm not opposed to that, Since Andew has already taken these patches, I'll
tinkier to see how such an implementation change looks, and post some follow on
patches if it seems good.  I'll clean up the comments while I'm at it.

Thanks!
Neil

Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree

[Andrew Morton]

On Wed, 27 Jan 2010 16:22:39 -0500
Neil Horman <nhorman@tuxdriver.com> wrote:

> On Wed, Jan 27, 2010 at 06:58:52PM +0100, Oleg Nesterov wrote:
> > On 01/27, Oleg Nesterov wrote:
> > >
> > > Currently only d_coredump() needs this new feature, but please note
> > > that ____call_usermodehelper() was already "uglified" for the coredumping
> > > over the pipe.
> > >
> > > If we add sub_info->finit(), then probably we should move the code
> > > under "if (sub_info->stdin)" from ____call_usermodehelper() to
> > > core_pipe_setup() ?
> > 
> > And, perhaps, we should not change call_usermodehelper() and all its
> > callers? If the caller needs ->finit() it can customize subprocess_info
> > like call_usermodehelper_pipe() already does?
> > 
> > To clarify, I don't have a "strong" opinion, I am just asking.
> > 
> I'm not opposed to that, Since Andew has already taken these patches, I'll
> tinkier to see how such an implementation change looks, and post some follow on
> patches if it seems good.  I'll clean up the comments while I'm at it.
> 

The patch conflicts a bit with Andi's
sysctl-add-call_usermodehelper_cleanup.patch so I dropped v1 of
exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather-than-0.patch

Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree

[nhorman]

On Wed, 27 Jan 2010 16:34:08 -0500, Andrew Morton wrote:
> On Wed, 27 Jan 2010 16:22:39 -0500
> Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > On Wed, Jan 27, 2010 at 06:58:52PM +0100, Oleg Nesterov wrote:
> > > On 01/27, Oleg Nesterov wrote:
> > > >
> > > > Currently only d_coredump() needs this new feature, but please note
> > > > that ____call_usermodehelper() was already "uglified" for the coredumping
> > > > over the pipe.
> > > >> > > > If we add sub_info->finit(), then probably we should move the code
> > > > under "if (sub_info->stdin)" from ____call_usermodehelper() to
> > > > core_pipe_setup() ?
> > > 
> > > And, perhaps, we should not change call_usermodehelper() and all its
> > > callers? If the caller needs ->finit() it can customize subprocess_info
> > > like call_usermodehelper_pipe() already does?
> > > 
> > > To clarify, I don't have a "strong" opinion, I am just asking.
> > > 
> > I'm not opposed to that, Since Andew has already taken these patches, I'll
> > tinkier to see how such an implementation change looks, and post some follow on
> > patches if it seems good.  I'll clean up the comments while I'm at it.
> > 
> 
> The patch conflicts a bit with Andi's
> sysctl-add-call_usermodehelper_cleanup.patch so I dropped v1 of
> exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather-than-0.patch
> 
That's fine.  I'll repost with the cleanups you & oleg noted in a few days

Regards
Neil

Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree

[Neil Horman]

On Wed, Jan 27, 2010 at 06:47:06PM +0100, Oleg Nesterov wrote:
> On 01/26, Andrew Morton wrote:
> >
> > From: Neil Horman <nhorman@tuxdriver.com>
> >
> > What I've done is modify the
> > call_usermodehelper() api such that an extra parameter is added, a
> > function pointer which will be called by the user helper task, after it
> > forks, but before it execs the required process.
> 
> Personally I agree, I think this fptr can be useful, not only for coredump.
> 
> > This will give the
> > caller the opportunity to get a callback in the process's context,
> > allowing it to do whatever it needs to to the process in the kernel
> 
> in this case it probably needs "void *data" argument, otherwise the
> usage is very limited.
> 
I'd thought of that, but I wasn't sure what data would be passed that the caller
wouldn't already be able to glean.  Certainly not opposed to adding something of
that nature though.

> Currently only d_coredump() needs this new feature, but please note
> that ____call_usermodehelper() was already "uglified" for the coredumping
> over the pipe.
> 
> If we add sub_info->finit(), then probably we should move the code
> under "if (sub_info->stdin)" from ____call_usermodehelper() to
> core_pipe_setup() ?
> 
> > +/*
> > + * This is used as a helper to set up the task that execs
> > + * our user space core collector application
> > + * Its called in the context of the task thats going to
> > + * exec itself to be the helper, so we can modify current here
> > + */
> 
> very minor nit, perhaps the comment should explain what is the meaning
> of the magical rlim_cur = 1 value? It is not immediately obvious we
> check cprm.limit == 1 below.
> 
Yeah, Andrew asked me to clean up that comment as well, I'll post a follow on
patch after I tinker with the suggestions in this email and your other note as
well for a bit.

> > +void core_pipe_setup(void)
> > +{
> > +	task_lock(current->group_leader);
> > +	current->signal->rlim[RLIMIT_CORE].rlim_cur = 1;
> > +	task_unlock(current->group_leader);
> > +}
> 
> Well, this thread must be the kernel thread and thus it should be
> ->group_leader and I don't think we really need task_lock() her,
> but this is minor and perhaps ->group_leader + task_lock() look
> better even if not needed.
> 
Perhaps, I wasn't sure, I was just following the code used by the core limit
proc write patch series.

Neil

> Oleg.
> 
>