* Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree
[not found] <201001262354.o0QNsBiM029772@imap1.linux-foundation.org>
@ 2010-01-27 17:47 ` Oleg Nesterov
2010-01-27 17:58 ` Oleg Nesterov
2010-01-27 21:25 ` Neil Horman
0 siblings, 2 replies; 6+ messages in thread
From: Oleg Nesterov @ 2010-01-27 17:47 UTC (permalink / raw)
To: akpm
Cc: linux-kernel, nhorman, abelay, benh, drbd-dev, gregkh, jmoskovc,
menage, mfasheh, mingo, neilb, shemminger, spock, t.sailer,
takedakn, viro
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree
2010-01-27 17:47 ` + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree Oleg Nesterov
@ 2010-01-27 17:58 ` Oleg Nesterov
2010-01-27 21:22 ` Neil Horman
2010-01-27 21:25 ` Neil Horman
1 sibling, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2010-01-27 17:58 UTC (permalink / raw)
To: akpm
Cc: linux-kernel, nhorman, abelay, benh, drbd-dev, gregkh, jmoskovc,
menage, mfasheh, mingo, neilb, shemminger, spock, t.sailer,
takedakn, viro
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree
2010-01-27 17:58 ` Oleg Nesterov
@ 2010-01-27 21:22 ` Neil Horman
2010-01-27 21:34 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Neil Horman @ 2010-01-27 21:22 UTC (permalink / raw)
To: Oleg Nesterov
Cc: akpm, linux-kernel, abelay, benh, drbd-dev, gregkh, jmoskovc,
menage, mfasheh, mingo, neilb, shemminger, spock, t.sailer,
takedakn, viro
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree
2010-01-27 17:47 ` + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree Oleg Nesterov
2010-01-27 17:58 ` Oleg Nesterov
@ 2010-01-27 21:25 ` Neil Horman
1 sibling, 0 replies; 6+ messages in thread
From: Neil Horman @ 2010-01-27 21:25 UTC (permalink / raw)
To: Oleg Nesterov
Cc: akpm, linux-kernel, abelay, benh, drbd-dev, gregkh, jmoskovc,
menage, mfasheh, mingo, neilb, shemminger, spock, t.sailer,
takedakn, viro
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.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree
2010-01-27 21:22 ` Neil Horman
@ 2010-01-27 21:34 ` Andrew Morton
2010-01-27 23:08 ` nhorman
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2010-01-27 21:34 UTC (permalink / raw)
To: Neil Horman
Cc: Oleg Nesterov, linux-kernel, abelay, benh, drbd-dev, gregkh,
jmoskovc, menage, mfasheh, mingo, neilb, shemminger, spock,
t.sailer, takedakn, viro
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree
2010-01-27 21:34 ` Andrew Morton
@ 2010-01-27 23:08 ` nhorman
0 siblings, 0 replies; 6+ messages in thread
From: nhorman @ 2010-01-27 23:08 UTC (permalink / raw)
To: Andrew Morton, Neil Horman
Cc: Oleg Nesterov, linux-kernel, abelay, benh, drbd-dev, gregkh,
jmoskovc, menage, mfasheh, mingo, neilb, shemminger, spock,
t.sailer, takedakn, viro
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-01-27 23:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201001262354.o0QNsBiM029772@imap1.linux-foundation.org>
2010-01-27 17:47 ` + exec-allow-core_pipe-recursion-check-to-look-for-a-value-of-1-rather -than-0.patch added to -mm tree Oleg Nesterov
2010-01-27 17:58 ` Oleg Nesterov
2010-01-27 21:22 ` Neil Horman
2010-01-27 21:34 ` Andrew Morton
2010-01-27 23:08 ` nhorman
2010-01-27 21:25 ` Neil Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox