[patch 3/4] hw watchpoints kernel workaround

[patch 3/4] hw watchpoints kernel workaround

[Jan Kratochvil]

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).


Thanks,
Jan


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.

--- 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.  */
+  if (inf->pid != inferior_pid)
+    {
+      int i;
+
+      /* Workaround some kernel versions reporting EINVAL on setting
+	 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
+	 for the fork-ed child; other kernels copy them from the parent.  */
+
+      ALL_DEBUG_REGISTERS(i)
+	i386_dr_low.set_addr (i, dr_mirror->addr[i]);
+    }
 
   ALL_DEBUG_REGISTERS(i)
     {

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

[Mark Kettenis]

> 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)
>      {
> 

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

[Jan Kratochvil]

note: reordered - this former patch 3/4 is now patch 4/4.

On Mon, 06 Dec 2010 14:45:40 +0100, Mark Kettenis wrote:
> 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.

OK, asked Oleg Nesterov for some hash/version, included them.


> 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?

According to Oleg (I did not know) the two changes went in (probably) by
a single upstream commit.

The EINVAL problem is still present in any upstream 2.6.33+ kernel so far.


> Also, do you know if the problematic behaviour ever was in a kernel
> officially released by Linus?

Oleg says so and I have verified it on a Fedora kernel with disabled utrace,
therefore the ptrace behavior should match the upstream one.
	kernel-2.6.35.6-48.noutrace.fc13
	http://koji.fedoraproject.org/scratch/jkratoch/task_2644776/
	(it will get discarded in a week)


> 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
                                                          ^^^^^^ = Red Hat
> distribute a kernel update to make their kernels conform to either the old
> or the final new behaviour.

I do not post GDB patches dealing with Fedora / Red Hat specific variants for
FSF GDB.


> > 	<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.

OK, changed.


> > --- 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.

linux_nat_iterate_watchpoint_lwps function comment and the two code paths
inside also contain comments about the variables state during detaching
breakpoints from a forked child.

As GDB contained bugs so far and I also do not find this issue so obvious
I wanted to introduce the reader to this problem more than just by wods
"fork-ed child".

I did not want to copy the comments as they will get out of sync and obsolete
in the future otherwise, as discussed in other mails.

Specific suggestion on improvements of these comments is welcome.


> 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.

Done.


> 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.

I + Oleg believe the EINVAL is a clear regression and bug which needs to be
fixed upstream.  Just it is present in Linus Torvalds released kernels for
long enough it is IMO worth to be work arounded by FSF GDB.


> 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.

I agree, fixed.


Thanks,
Jan


gdb/
2010-12-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Workaround Linux kernel Bug - Red Hat Bugzilla #660204.
	* amd64-linux-nat.c (amd64_linux_dr_set_addr): New declaration.
	(amd64_linux_dr_set_control): New variables inferior_pid and inf.
	Pre-set address registers if we are detaching breakpoints now.
	* i386-linux-nat.c (i386_linux_dr_set_addr): New declaration.
	(i386_linux_dr_set_control): New variables inferior_pid and inf.
	Pre-set address registers if we are detaching breakpoints now.
	* i386-nat.c (i386_inferior_data_get): Reset ADDR_PRESET for fork-ed
	child.
	(i386_cleanup_dregs): Reset ADDR_PRESET.
	* i386-nat.h (struct i386_dr_mirror) <addr_preset: New.

--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -310,11 +310,49 @@ amd64_linux_dr_set_control_callback (int tid, void *control_voidp)
   amd64_linux_dr_set (tid, DR_CONTROL, control);
 }
 
+static void amd64_linux_dr_set_addr (int regnum, CORE_ADDR addr);
+
 /* Set DR_CONTROL to ADDR in all LWPs of CURRENT_INFERIOR.  */
 
 static void
 amd64_linux_dr_set_control (unsigned long control)
 {
+  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 for the fork-ed child description.
+     The i386 counterpart is i386_linux_dr_set_control.  */
+  if (inf->pid != inferior_pid)
+    {
+      struct i386_dr_mirror *dr_mirror = i386_dr_mirror_get ();
+      int i;
+
+      /* There were two changes in Linux kernel 2.6.33 by the commit:
+         72f674d203cd230426437cdcf7dd6f681dad8b0d
+
+         (1) After fork/vfork/clone the new task no longer inherits the debug
+	 registers.  It has them zeroed instead.  Either case is OK for GDB as
+	 GDB already registers a fix up by linux_nat_set_new_thread.
+
+	 (2) If you enable a breakpoint by the CONTROL bits you have already
+	 written its ADDRESS.  Otherwise Linux kernel will report EINVAL.
+	 For this case the workaround here ensures that during resetting
+	 (detaching) watchpoints for a fork-ed child we can set CONTROL
+	 arbitrarily as the addresses get pre-set here just to be sure.
+
+	 The second issue is hopefully going to be fixed in Linux kernel:
+	 https://bugzilla.redhat.com/show_bug.cgi?id=660204  */
+
+      if (!dr_mirror->addr_preset)
+	{
+	  dr_mirror->addr_preset = 1;
+
+	  for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR; i++)
+	    amd64_linux_dr_set_addr (i, dr_mirror->addr[i]);
+	}
+    }
+
   linux_nat_iterate_watchpoint_lwps (amd64_linux_dr_set_control_callback,
 				     &control);
 }
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -680,11 +680,31 @@ i386_linux_dr_set_control_callback (int tid, void *control_voidp)
   i386_linux_dr_set (tid, DR_CONTROL, control);
 }
 
+static void i386_linux_dr_set_addr (int regnum, CORE_ADDR addr);
+
 /* Set DR_CONTROL to ADDR in all LWPs of CURRENT_INFERIOR.  */
 
 static void
 i386_linux_dr_set_control (unsigned long control)
 {
+  int inferior_pid = ptid_get_pid (inferior_ptid);
+  struct inferior *inf = current_inferior ();
+
+  /* The amd64 counterpart and description is amd64_linux_dr_set_control.  */
+  if (inf->pid != inferior_pid)
+    {
+      struct i386_dr_mirror *dr_mirror = i386_dr_mirror_get ();
+      int i;
+
+      if (!dr_mirror->addr_preset)
+	{
+	  dr_mirror->addr_preset = 1;
+
+	  for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR; i++)
+	    i386_linux_dr_set_addr (i, dr_mirror->addr[i]);
+	}
+    }
+
   linux_nat_iterate_watchpoint_lwps (i386_linux_dr_set_control_callback,
 				     &control);
 }
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -248,6 +248,7 @@ i386_inferior_data_get (void)
 	  /* Forked processes get a copy of the debug registers.  */
 	  memcpy (&detached_inf_data_local, inf_data,
 		  sizeof (detached_inf_data_local));
+	  detached_inf_data_local.dr_mirror.addr_preset = 0;
 	}
 
       return &detached_inf_data_local;
@@ -276,6 +277,7 @@ i386_cleanup_dregs (void)
       dr_mirror->addr[i] = 0;
       dr_mirror->ref_count[i] = 0;
     }
+  dr_mirror->addr_preset = 0;
   dr_mirror->control = 0;
   dr_mirror->status = 0;
 }
--- a/gdb/i386-nat.h
+++ b/gdb/i386-nat.h
@@ -89,6 +89,9 @@ struct i386_dr_mirror
        control registers separated because they don't hold addresses.  */
     CORE_ADDR addr[DR_NADDR];
 
+    /* All the ADDR hardware registers have been written at least once.  */
+    unsigned addr_preset : 1;
+
     /* Reference counts for each debug register.  */
     int ref_count[DR_NADDR];