Hey, Oleg.
On Tue, Nov 17, 2015 at 08:34:19PM +0100, Oleg Nesterov wrote:
> On 11/16, Tejun Heo wrote:
> >
> > *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS ***
> > *** MACROS MAY CONTAIN MALICIOUS CODE ***
> > *** Open only if you can verify and trust the sender ***
> > *** Please contact [email protected] if you have questions or concerns **
>
> Hmm, [email protected] doesn't like you. But I dared to open and nothing
> happened so far. although perhaps you already own my machine.
lol no idea what's going on there but dude you gotta clean up the
browsing history.
> > Hmmm... It's nasty tho. We're breaking a guaranteed userland behavior
>
> Perhaps you are right, but I am wondering if it was ever guaranteed.
>
> What actually annoys me is that now I am almost sure that it was me
> who asked you to hide this from user-space, and today I see no reason
> for this hack.
>
> > I'd be a lot more comfortable stating
> > that cgroup freezer is currently broken rather than diddling with
> > subtle ptrace semantics.
>
> OK, lets keep this JOBCTL_TRAPPING_BIT.
>
> But still I would like to know what Pedro thinks...
>
> Anyway, wait_on_bit(TASK_UNINTERRUPTIBLE) doesn't look good. Do you
> see any problem with the change below? Yes, the comment is not clear,
> it should be updated, the tracee can clear this bit too.
>
> And perhaps we can change get_task_state() until freezer gets another state,
>
> --- x/fs/proc/array.c
> +++ x/fs/proc/array.c
> @@ -126,6 +126,9 @@ static inline const char *get_task_state
> {
> unsigned int state = (tsk->state | tsk->exit_state) &
> TASK_REPORT;
>
> + if (tsk->flags & PF_FROZEN)
> + return "D (frozen)";
> +
> BUILD_BUG_ON(1 + ilog2(TASK_REPORT) !=
> ARRAY_SIZE(task_state_array)-1);
>
> return task_state_array[fls(state)];
Hmm... the only nit is that we'll eventually want to share "T
(stopped)" or do "T (frozen)" and switching down the road could be a
bit confusing. It shouldn't be a big deal tho. I think I'm mostly
reluctant to accomodate the broken behavior of cgroup freezer.
> --- x/kernel/ptrace.c
> +++ x/kernel/ptrace.c
> @@ -364,8 +364,13 @@ unlock_creds:
> mutex_unlock(&task->signal->cred_guard_mutex);
> out:
> if (!retval) {
> - wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
> - TASK_UNINTERRUPTIBLE);
> + if (wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
> + TASK_KILLABLE))
> + /*
> + * We will clear JOBCTL_TRAPPING in __ptrace_unlink(),
> + * until then nobody can trace this task anyway.
> + */
> + retval = -EINTR;
Yeah, this looks good to me.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
