Message-ID: <20110510180811.GB32637@redhat.com>
Date:	Tue, 10 May 2011 20:08:11 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	jan.kratochvil@...hat.com, vda.linux@...glemail.com,
	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org, indan@....nu
Subject: Re: [PATCH 04/11] ptrace: implement PTRACE_INTERRUPT

On 05/10, Tejun Heo wrote:
>
> > And, you know, I am not sure this is very clear. What if we change the
> > rules so that PTRACE_SEIZE always reports PTRACE_EVENT_INTERRUPT like
> > PTRACE_INTERRUPT? This looks more symmetrical and simple to me. IOW,
> > ptrace(PTRACE_SEIZE) simply implies the implicit PTRACE_INTERRUPT.
>
> Actually, that was my initial implementation but as we don't guarantee
> that the first trap would be an INTERRUPT one,

Yes, and ptrace(PTRACE_INTERRUPT) doesn't guarantee this too,

> so it seemed nicer to
> avoid double trapping after SEIZE.

Why? We can simply say that ptrace(PTRACE_SEIZE) == attach + INTERRUPT,
the rules are the same.

Hmm. Suddenly I got lost. Perhaps instead JOBCTL_TRAP_INTERRUPT should
be cleared on any trap too, like SEIZE.

Again, agaian, I don't have the strong opinion, I am asking.

> Maybe we can make sure that
> INTERRUPT is always the trap to be taken

I don't know. Personally, I do not think this will make the API much
better.

> but it would be a tad bit
> more complex than the current implementation.

Yes.

> > Oh. And this reminds me the previous discussion. Should PTRACE_SEIZE
> > stop the tracee? Perhaps it should only attach or do PTRACE_INTERRUPT
> > depending on flags? Personally I think it should stop...
> >
> > To clarify, personally I do not know. Jan, Denys, all, please comment.
> > If PTRACE_SEIZE doesn't stop the tracee, then we should probably pass
> > more options.
>
> I'm rather strongly against not trapping and somebody would have to
> give me _very_ convincing rationales to change my mind on the subject.

Yes, I agree.

> > Either way, these changes do not handle the auto-attach case correctly.
> > tracehook_report_clone() shouldn't send SIGSTOP unconditionally, we
> > should check PT_SEIZED. And perhaps we can do auto-attach better, but
> > right now this is off-topic.
>
> Oh yeah, that's something I meant to ask you but forgot.  The SEIZE
> behaviors are not complete after this patchset and thus there's no
> patch to remove the DEVEL flag yet. ISTR you telling me there are two
> or three conditions where the reporting/stopping isn't transparent in
> a previous mail, which I can't find anymore.

Hmm. No, I can't recall anything related...

> One apparently is clone.
> What were the others?

clone() is special because (I think) auto-attach should obviously
inherit PT_SEIZED. ptrace_init_task() copies PT_SEIZED correctly, so
we should only change tracehook_report_clone().

Another special (and nasty!) case is PTRACE_TRACEME. I do not know
how often it is used, but probabaly it is important enough. At least,
iirc, it is used by strace. Probably we need PTRACE_SEIZEME as well.

> > Well. Perhaps PTRACE_INTERRUPT should return -EALREADY or something if
> > JOBCTL_TRAP_INTERRUPT is already set? Again, again, it is not that I think
> > this is really useful. But since we are going to add the new API it is
> > better to discuss every detail.
>
> At first I made PTRACE_INTERRUPT noop if it was already in INTERRUPT
> trap but then changed my mind because guaranteeing that there will be
> at least one PTRACE_EVENT_INTERRUPT trap after PTRACE_INTERRUPT seems
> better than having subtle behavior difference which might be nicer in
> limited use cases.

No, I meant -EALREADY if JOBCTL_TRAP_INTERRUPT is pending set but not
yet reported. But anyway, this is really minor.

Oleg.

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

Powered by blists - more mailing lists