Re: [patch 3/4] hw watchpoints kernel workaround

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Mon, 6 Dec 2010 12:14:06 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> Hi,
> 
> there has been a change in upstream Linux kernels behavior.  Formerly newly
> created processes/threads inherited the debug registers content.  Very
> recently the new processes/threads get the debug registers cleared.
> 
> ptrace: watchpoint-fork (DR fork() inheritance)
> https://bugzilla.redhat.com/show_bug.cgi?id=660003
> http://sources.redhat.com/cgi-bin/cvsweb.cgi/tests/ptrace-tests/tests/Attic/watchpoint-fork.c?rev=1.3&cvsroot=systemtap
> if $? == 0 then it is old-style kernel - debug registers get inherited
> if $? == 1 then it is new-style kernel - debug registers get zeroed
> 
> In fact so far I do not know it would cause any problem.  It rather
> workarounds a GDB bug described in [patch 2/4] (b).  For threads GDB is
> already ensuring the right debug registers content:
> 	linux_nat_set_new_thread (t, amd64_linux_new_thread);
> 
> But together with the second change in upstream Linux kernels behavior:
> 
> ptrace: watchpoint-zeroaddr: EINVAL on PTRACE_POKEUSER of DR7
> https://bugzilla.redhat.com/show_bug.cgi?id=660204
> http://sources.redhat.com/cgi-bin/cvsweb.cgi/tests/ptrace-tests/tests/watchpoint-zeroaddr.c?rev=1.1&cvsroot=systemtap
> if $? == 0 then it is old-style kernel - anything is allowed
> if $? == 1 then it is new-style kernel - EINVAL given on invalid DRs state
> 
> there is a problem GDB tries to remove watchpoints from registers where are
> already zeroes (due to the first kernel change/bug).  And despite GDB "safely"
> tries to unset control register first and only then reset the address
> registers still for 2+ watchpoints it causes the kernel EINVAL.
> 
> I tried to make GDB working with any combination of these two
> behavior changes present as such kernels are already released in the
> wild.
> 
> Therefore here is the workaround.  It is needed only on the
> new-style kernels.  (scope: RHEL-6 is old-style, updated Fedora 13
> is already new-style)
> 
> BTW these are upstream Linux kernel behavior changes unrelated to utrace.
> I do not know exact kernel version/commit of the change(s).

That's too bad.  Please do at least mention some kernel version
numbers, such that 10 years from now we have an idea when the
behaviour changed.

Not sure I understand the issue correctly.  You mention two changes.
So is there a period in between those changes where things are broken,
and are things fixed now?  Or is the EINVAL behaviour there to stay?

Also, do you know if the problematic behaviour ever was in a kernel
officially released by Linus?  If not, I'm inclined to suggest that
the GDB should only have to deal with the old behaviour and the final
new behaviour, and that RedHat should distribute a kernel update to
make their kernels conform to either the old or the final new
behaviour.

Some comments inline below.

Cheers,

Mark

> gdb/
> 2010-12-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Workaround Linux kernel Bug - Red Hat Bugzilla #660204.
> 	* i386-nat.c (i386_remove_aligned_watchpoint): New variables
> 	inferior_pid and inf.
> 	<inf->pid != inferior_pid>: New.

The <...> bit in the ChangeLog is odd.  I don't think that's an
acceptable way to describe changes according to the coding standards.

> --- a/gdb/i386-nat.c
> +++ b/gdb/i386-nat.c
> @@ -453,6 +453,24 @@ i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
>  {
>    struct dr_mirror *dr_mirror = dr_mirror_get ();
>    int i, retval = -1;
> +  int inferior_pid = ptid_get_pid (inferior_ptid);
> +  struct inferior *inf = current_inferior ();
> +
> +  /* Are we detaching breakpoints from a fork-ed child?
> +     See linux_nat_iterate_watchpoint_lwps.  */

It's not abvious to me what this "See
linux_nat_iterate_watchpoint_lwps" is referring to.

> +  if (inf->pid != inferior_pid)
> +    {
> +      int i;
> +
> +      /* Workaround some kernel versions reporting EINVAL on setting

It's the Linux kernel you're talking about here, so:

"Work around some Linux kernel versions that report EINVAL..."

> +	 DR_CONTROL with still unset (and thus zero) DR_*ADDR registers.
> +	 See: https://bugzilla.redhat.com/show_bug.cgi?id=660204
> +	 It can happen as some kernel versions reset all DR registers to zero

"This can happen as..."

> +	 for the fork-ed child; other kernels copy them from the parent.  */

But I think you'd better rewrite that comment completely since it
would really help if you could explain what old kernels did and what
new kernels do and that you need to reset the breakpoints in the
fork-ed child to deal with the new behaviour.

Also, if the EINVAL behaviour is there to stay, don't say "Work
around" but simly say "Deal with the fact that the Linux kernel
behaviour changed", or something like that.

It doesn't seem inconceivable that other (non-Linux) kernels also need
this bit of code, so i386-nat.c is an appropriate place for it.

> +
> +      ALL_DEBUG_REGISTERS(i)
> +	i386_dr_low.set_addr (i, dr_mirror->addr[i]);
> +    }
>  
>    ALL_DEBUG_REGISTERS(i)
>      {
>