diff mbox series

[09/11] tests/tcg: disable pauth for aarch64 gdb tests

Message ID 20230310103123.2118519-10-alex.bennee@linaro.org
State Superseded
Headers show
Series tweaks and fixes for 8.0-rc1 (tests, plugins, docs) | expand

Commit Message

Alex Bennée March 10, 2023, 10:31 a.m. UTC
You need a very new gdb to be able to run with pauth support otherwise
your likely to hit asserts and aborts. Disable pauth for now until we
can properly probe support in gdb.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/Makefile.target | 2 ++
 1 file changed, 2 insertions(+)

Comments

Richard Henderson March 10, 2023, 5:44 p.m. UTC | #1
On 3/10/23 02:31, Alex Bennée wrote:
> You need a very new gdb to be able to run with pauth support otherwise
> your likely to hit asserts and aborts. Disable pauth for now until we
> can properly probe support in gdb.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/tcg/aarch64/Makefile.target | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
> index 9e91a20b0d..8ffde3b0ed 100644
> --- a/tests/tcg/aarch64/Makefile.target
> +++ b/tests/tcg/aarch64/Makefile.target
> @@ -84,6 +84,8 @@ TESTS += sha512-vector
>   ifeq ($(HOST_GDB_SUPPORTS_ARCH),y)
>   GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
>   
> +run-gdbstub-%: QEMU_OPTS=-cpu max,pauth=off
> +
>   run-gdbstub-sysregs: sysregs
>   	$(call run-test, $@, $(GDB_SCRIPT) \
>   		--gdb $(HAVE_GDB_BIN) \
Peter Maydell March 10, 2023, 5:47 p.m. UTC | #2
On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> You need a very new gdb to be able to run with pauth support otherwise
> your likely to hit asserts and aborts. Disable pauth for now until we
> can properly probe support in gdb.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

If it makes gdb fall over, then shouldn't we be disabling
the pauth gdbstub stuff entirely ? Otherwise even if our
tests are fine our users will not be...

-- PMM
Fabiano Rosas March 10, 2023, 6 p.m. UTC | #3
Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> You need a very new gdb to be able to run with pauth support otherwise
>> your likely to hit asserts and aborts. Disable pauth for now until we
>> can properly probe support in gdb.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> If it makes gdb fall over, then shouldn't we be disabling
> the pauth gdbstub stuff entirely ? Otherwise even if our
> tests are fine our users will not be...
>

Have you seem my message on IRC about changing the feature name in the
XML? I think the issue is that we're putting the .xml in a "namespace"
where GDB expects to only find stuff which it has code to
support. Changing from "org.gnu.gdb.aarch64.pauth" to
"org.qemu.aarch64.pauth" made it stop crashing and I can read the
registers just fine.
Peter Maydell March 10, 2023, 6:07 p.m. UTC | #4
On Fri, 10 Mar 2023 at 18:00, Fabiano Rosas <farosas@suse.de> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> You need a very new gdb to be able to run with pauth support otherwise
> >> your likely to hit asserts and aborts. Disable pauth for now until we
> >> can properly probe support in gdb.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >
> > If it makes gdb fall over, then shouldn't we be disabling
> > the pauth gdbstub stuff entirely ? Otherwise even if our
> > tests are fine our users will not be...
> >
>
> Have you seem my message on IRC about changing the feature name in the
> XML? I think the issue is that we're putting the .xml in a "namespace"
> where GDB expects to only find stuff which it has code to
> support. Changing from "org.gnu.gdb.aarch64.pauth" to
> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
> registers just fine.

But then presumably a pauth-aware GDB won't actually know
the values it needs to be able to convert between with-PAC
and without-PAC addresses for backtracing?

Luis, how is this intended to work? Is there some way the
stub can check with the gdb that's connected whether the
gdb is able to cope with the pauth XML, so it can avoid
sending it to a gdb that is going to crash if it sees it ?

thanks
-- PMM
Richard Henderson March 10, 2023, 6:07 p.m. UTC | #5
On 3/10/23 09:47, Peter Maydell wrote:
> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> You need a very new gdb to be able to run with pauth support otherwise
>> your likely to hit asserts and aborts. Disable pauth for now until we
>> can properly probe support in gdb.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> If it makes gdb fall over, then shouldn't we be disabling
> the pauth gdbstub stuff entirely ? Otherwise even if our
> tests are fine our users will not be...

It is, annoyingly, a bug in gdb 12 alone.

Before gdb 12, the pauth extension isn't recognized and so it gets treated as non-special 
registers.  From gdb 13, whatever lead to the internal_error() is fixed and the extension 
works swimmingly.


r~
Alex Bennée March 10, 2023, 6:14 p.m. UTC | #6
(adding some more gdb types to CC)

Fabiano Rosas <farosas@suse.de> writes:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> You need a very new gdb to be able to run with pauth support otherwise
>>> your likely to hit asserts and aborts. Disable pauth for now until we
>>> can properly probe support in gdb.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> If it makes gdb fall over, then shouldn't we be disabling
>> the pauth gdbstub stuff entirely ? Otherwise even if our
>> tests are fine our users will not be...
>>
>
> Have you seem my message on IRC about changing the feature name in the
> XML? I think the issue is that we're putting the .xml in a "namespace"
> where GDB expects to only find stuff which it has code to
> support. Changing from "org.gnu.gdb.aarch64.pauth" to
> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
> registers just fine.

That would work, although I would prefer to probe support so we can use
the official namespace. We went through something similar with SVE
until:

  797920b952 (target/arm: use official org.gnu.gdb.aarch64.sve layout for registers)

which required:

  b1863ccc95 (configure: gate our use of GDB to 8.3.1 or above)

Since then we've introduced:

 ./scripts/probe-gdb-support.py

which given the runes to check for pauth support in gdb could expose a
symbol and we get the best of both worlds. Of course if this keeps
happening we could throw up our hands and just use custom XML for all
the extra register sets.
Fabiano Rosas March 10, 2023, 6:15 p.m. UTC | #7
Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 10 Mar 2023 at 18:00, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >>
>> >> You need a very new gdb to be able to run with pauth support otherwise
>> >> your likely to hit asserts and aborts. Disable pauth for now until we
>> >> can properly probe support in gdb.
>> >>
>> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> >
>> > If it makes gdb fall over, then shouldn't we be disabling
>> > the pauth gdbstub stuff entirely ? Otherwise even if our
>> > tests are fine our users will not be...
>> >
>>
>> Have you seem my message on IRC about changing the feature name in the
>> XML? I think the issue is that we're putting the .xml in a "namespace"
>> where GDB expects to only find stuff which it has code to
>> support. Changing from "org.gnu.gdb.aarch64.pauth" to
>> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
>> registers just fine.
>
> But then presumably a pauth-aware GDB won't actually know
> the values it needs to be able to convert between with-PAC
> and without-PAC addresses for backtracing?
>

Good question. Although that feels to me more like a GDB feature. If we
don't break it even worse by doing that, the QEMU side which is more
about reading the registers should be fine. Note that we already have
other .xml files using a .qemu namespace in the codebase. As I
understand it, gdb simply treats these as extra registers not tied to
any specific feature.
Luis Machado March 13, 2023, 11:16 a.m. UTC | #8
On 3/10/23 18:07, Peter Maydell wrote:
> On Fri, 10 Mar 2023 at 18:00, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>> You need a very new gdb to be able to run with pauth support otherwise
>>>> your likely to hit asserts and aborts. Disable pauth for now until we
>>>> can properly probe support in gdb.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>
>>> If it makes gdb fall over, then shouldn't we be disabling
>>> the pauth gdbstub stuff entirely ? Otherwise even if our
>>> tests are fine our users will not be...
>>>
>>
>> Have you seem my message on IRC about changing the feature name in the
>> XML? I think the issue is that we're putting the .xml in a "namespace"
>> where GDB expects to only find stuff which it has code to
>> support. Changing from "org.gnu.gdb.aarch64.pauth" to
>> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
>> registers just fine.

It might be a better option to emit a pauth feature in the qemu namespace to dodge the crashing bug from older
gdb's (a latent pauth-related bug in gdb that is triggered by having gdb identify that a target supports
pauth and at the same time having a target description containing system registers gdb doesn't
care about).

>
> But then presumably a pauth-aware GDB won't actually know
> the values it needs to be able to convert between with-PAC
> and without-PAC addresses for backtracing?
>
> Luis, how is this intended to work? Is there some way the
> stub can check with the gdb that's connected whether the
> gdb is able to cope with the pauth XML, so it can avoid
> sending it to a gdb that is going to crash if it sees it ?

There isn't a probing mechanism unfortunately, and gdb isn't supposed to crash in this case.

With the changes from commit 6d0020873deb2f2c4e0965dc2ebf227bc1db3140, gdb now unmasks signed
addresses using the additional pauth registers. If gdb doesn't detect the pauth feature, it will
still mask out the top bits using a default mask of 0xff80000000000000.

While that should be enough for user addresses, it won't help with "kernel" addresses (when the VA select bit is 1).

To dodge the crashing bug of older gdb's, I can adjust gdb to also look for the pauth registers in the qemu namespace and
document that accordingly.

>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Peter Maydell March 13, 2023, 11:22 a.m. UTC | #9
On Fri, 10 Mar 2023 at 18:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> (adding some more gdb types to CC)
>
> Fabiano Rosas <farosas@suse.de> writes:
>
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> >> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>>
> >>> You need a very new gdb to be able to run with pauth support otherwise
> >>> your likely to hit asserts and aborts. Disable pauth for now until we
> >>> can properly probe support in gdb.
> >>>
> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >>
> >> If it makes gdb fall over, then shouldn't we be disabling
> >> the pauth gdbstub stuff entirely ? Otherwise even if our
> >> tests are fine our users will not be...
> >>
> >
> > Have you seem my message on IRC about changing the feature name in the
> > XML? I think the issue is that we're putting the .xml in a "namespace"
> > where GDB expects to only find stuff which it has code to
> > support. Changing from "org.gnu.gdb.aarch64.pauth" to
> > "org.qemu.aarch64.pauth" made it stop crashing and I can read the
> > registers just fine.
>
> That would work, although I would prefer to probe support so we can use
> the official namespace.

I don't think there's a way to probe for this problem. I spoke
to Luis about this, and apparently it's a bug in how gdb handles
the pauth XML description (fixed in gdb commit 1ba3a3222039eb25).
A gdb without any pauth support at all will be fine; a gdb with
the bug will report that it has pauth support but will crash
if you feed it the whole set of XML that QEMU has; a gdb
with the bug fixed will also report pauth support but won't crash.
(The bug only manifests if the full XML includes registers that GDB
doesn't care about, like the system registers; if the stub sends
only registers GDB knows about then it won't crash.)

Luis and I came up with two options:

(1) leave QEMU outputting the pauth xml as-is, and tell people
whose gdb 12 crashes that they should upgrade to a newer gdb

(2) make QEMU output the pauth info under a different XML namespace,
and tell people who need backtraces when pauth is enabled
that they should upgrade to a newer gdb

Neither of these feel great, but on balance I guess 2 is better?

Luis: I think that rather than doing (2) with a QEMU namespace,
we should define a gdb namespace for this. That makes it clear
that this is still a gdb-upstream-sanctioned way of exposing
the pauth registers.

thanks
-- PMM
Luis Machado March 13, 2023, 11:44 a.m. UTC | #10
On 3/13/23 11:22, Peter Maydell via Gdb wrote:
> On Fri, 10 Mar 2023 at 18:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> (adding some more gdb types to CC)
>>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>>
>>>>> You need a very new gdb to be able to run with pauth support otherwise
>>>>> your likely to hit asserts and aborts. Disable pauth for now until we
>>>>> can properly probe support in gdb.
>>>>>
>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>
>>>> If it makes gdb fall over, then shouldn't we be disabling
>>>> the pauth gdbstub stuff entirely ? Otherwise even if our
>>>> tests are fine our users will not be...
>>>>
>>>
>>> Have you seem my message on IRC about changing the feature name in the
>>> XML? I think the issue is that we're putting the .xml in a "namespace"
>>> where GDB expects to only find stuff which it has code to
>>> support. Changing from "org.gnu.gdb.aarch64.pauth" to
>>> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
>>> registers just fine.
>>
>> That would work, although I would prefer to probe support so we can use
>> the official namespace.
>
> I don't think there's a way to probe for this problem. I spoke
> to Luis about this, and apparently it's a bug in how gdb handles
> the pauth XML description (fixed in gdb commit 1ba3a3222039eb25).
> A gdb without any pauth support at all will be fine; a gdb with
> the bug will report that it has pauth support but will crash
> if you feed it the whole set of XML that QEMU has; a gdb
> with the bug fixed will also report pauth support but won't crash.
> (The bug only manifests if the full XML includes registers that GDB
> doesn't care about, like the system registers; if the stub sends
> only registers GDB knows about then it won't crash.)
>
> Luis and I came up with two options:
>
> (1) leave QEMU outputting the pauth xml as-is, and tell people
> whose gdb 12 crashes that they should upgrade to a newer gdb
>
> (2) make QEMU output the pauth info under a different XML namespace,
> and tell people who need backtraces when pauth is enabled
> that they should upgrade to a newer gdb
>
> Neither of these feel great, but on balance I guess 2 is better?
>
> Luis: I think that rather than doing (2) with a QEMU namespace,
> we should define a gdb namespace for this. That makes it clear
> that this is still a gdb-upstream-sanctioned way of exposing
> the pauth registers.

That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.

We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
"org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.

FYI, I've pushed a better documentation for the arm/aarch64 xml descriptions here:

https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=d7001b29e9f256dfc60acb481d9df8f91f2ee623
https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=e0994165d1b8469dfc27b09b62ac74862d535812

>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Richard Henderson March 13, 2023, 7:21 p.m. UTC | #11
On 3/13/23 04:44, Luis Machado wrote:
>> Luis: I think that rather than doing (2) with a QEMU namespace,
>> we should define a gdb namespace for this. That makes it clear
>> that this is still a gdb-upstream-sanctioned way of exposing
>> the pauth registers.
> 
> That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
> 
> We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop 
> using the original
> "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant 
> pauth_v2.

What if we leave the original two registers, pauth_[cd]mask, in org.gnu.gdb.aarch64.pauth 
and move the new *_high registers into a different feature?  That would maximize the set 
of gdb version for which the original user-only support is functional.


r~
Peter Maydell March 13, 2023, 9:35 p.m. UTC | #12
On Mon, 13 Mar 2023 at 19:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/13/23 04:44, Luis Machado wrote:
> >> Luis: I think that rather than doing (2) with a QEMU namespace,
> >> we should define a gdb namespace for this. That makes it clear
> >> that this is still a gdb-upstream-sanctioned way of exposing
> >> the pauth registers.
> >
> > That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
> >
> > We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop
> > using the original
> > "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant
> > pauth_v2.
>
> What if we leave the original two registers, pauth_[cd]mask, in org.gnu.gdb.aarch64.pauth
> and move the new *_high registers into a different feature?  That would maximize the set
> of gdb version for which the original user-only support is functional.

If that avoids the gdb crash, sure. But I had the impression from
Luis' description of it that that would not help (i.e. that it was
the not-used-by-gdb registers in other XML sections like sysregs
that resulted in it getting confused about the register number
for its internal pauth-related register).

thanks
-- PMM
Luis Machado March 14, 2023, 8:20 a.m. UTC | #13
On 3/13/23 19:21, Richard Henderson wrote:
> On 3/13/23 04:44, Luis Machado wrote:
>>> Luis: I think that rather than doing (2) with a QEMU namespace,
>>> we should define a gdb namespace for this. That makes it clear
>>> that this is still a gdb-upstream-sanctioned way of exposing
>>> the pauth registers.
>>
>> That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
>>
>> We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
>> "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.
>
> What if we leave the original two registers, pauth_[cd]mask, in org.gnu.gdb.aarch64.pauth and move the new *_high registers into a different feature?  That would maximize the set of gdb version for which the original user-only support is functional.

 From what I recall from the gdb bug, I don't think that will help. gdb will detect pauth support, will add the ra_sign_state pseudo-register internally with the incorrect numbering and will also see the additional system registers from QEMU, resulting in a crash.

>
>
> r~
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Luis Machado March 15, 2023, 9:50 a.m. UTC | #14
Hi,

On 3/13/23 11:44, Luis Machado wrote:
> On 3/13/23 11:22, Peter Maydell via Gdb wrote:
>> On Fri, 10 Mar 2023 at 18:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>>
>>> (adding some more gdb types to CC)
>>>
>>> Fabiano Rosas <farosas@suse.de> writes:
>>>
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>>>
>>>>>> You need a very new gdb to be able to run with pauth support otherwise
>>>>>> your likely to hit asserts and aborts. Disable pauth for now until we
>>>>>> can properly probe support in gdb.
>>>>>>
>>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>
>>>>> If it makes gdb fall over, then shouldn't we be disabling
>>>>> the pauth gdbstub stuff entirely ? Otherwise even if our
>>>>> tests are fine our users will not be...
>>>>>
>>>>
>>>> Have you seem my message on IRC about changing the feature name in the
>>>> XML? I think the issue is that we're putting the .xml in a "namespace"
>>>> where GDB expects to only find stuff which it has code to
>>>> support. Changing from "org.gnu.gdb.aarch64.pauth" to
>>>> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
>>>> registers just fine.
>>>
>>> That would work, although I would prefer to probe support so we can use
>>> the official namespace.
>>
>> I don't think there's a way to probe for this problem. I spoke
>> to Luis about this, and apparently it's a bug in how gdb handles
>> the pauth XML description (fixed in gdb commit 1ba3a3222039eb25).
>> A gdb without any pauth support at all will be fine; a gdb with
>> the bug will report that it has pauth support but will crash
>> if you feed it the whole set of XML that QEMU has; a gdb
>> with the bug fixed will also report pauth support but won't crash.
>> (The bug only manifests if the full XML includes registers that GDB
>> doesn't care about, like the system registers; if the stub sends
>> only registers GDB knows about then it won't crash.)
>>
>> Luis and I came up with two options:
>>
>> (1) leave QEMU outputting the pauth xml as-is, and tell people
>> whose gdb 12 crashes that they should upgrade to a newer gdb
>>
>> (2) make QEMU output the pauth info under a different XML namespace,
>> and tell people who need backtraces when pauth is enabled
>> that they should upgrade to a newer gdb
>>
>> Neither of these feel great, but on balance I guess 2 is better?
>>
>> Luis: I think that rather than doing (2) with a QEMU namespace,
>> we should define a gdb namespace for this. That makes it clear
>> that this is still a gdb-upstream-sanctioned way of exposing
>> the pauth registers.
>
> That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
>
> We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
> "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.
>
> FYI, I've pushed a better documentation for the arm/aarch64 xml descriptions here:
>
> https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=d7001b29e9f256dfc60acb481d9df8f91f2ee623
> https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=e0994165d1b8469dfc27b09b62ac74862d535812
>
>>
>> thanks
>> -- PMM
>

Just an update on this. I had a chat with Richard Henderson yesterday, and it might actually be easier and more convenient to backport
fixes to older gdb versions (at least gdb-12 and gdb-11, but gdb-10 and gdb-9 are also affected). This will ensure those won't crash when
they connect to a qemu that advertises the pauth feature.

It also means we won't need qemu-side changes. My understanding is that we're close to the 8.0.0 release, and the code is already in place.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Peter Maydell March 17, 2023, 4:37 p.m. UTC | #15
On Wed, 15 Mar 2023 at 09:51, Luis Machado <luis.machado@arm.com> wrote:
> On 3/13/23 11:44, Luis Machado wrote:
> > On 3/13/23 11:22, Peter Maydell via Gdb wrote:
> >> Luis and I came up with two options:
> >>
> >> (1) leave QEMU outputting the pauth xml as-is, and tell people
> >> whose gdb 12 crashes that they should upgrade to a newer gdb
> >>
> >> (2) make QEMU output the pauth info under a different XML namespace,
> >> and tell people who need backtraces when pauth is enabled
> >> that they should upgrade to a newer gdb
> >>
> >> Neither of these feel great, but on balance I guess 2 is better?
> >>
> >> Luis: I think that rather than doing (2) with a QEMU namespace,
> >> we should define a gdb namespace for this. That makes it clear
> >> that this is still a gdb-upstream-sanctioned way of exposing
> >> the pauth registers.
> >
> > That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
> >
> > We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
> > "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.
> >
> > FYI, I've pushed a better documentation for the arm/aarch64 xml descriptions here:
> >
> > https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=d7001b29e9f256dfc60acb481d9df8f91f2ee623
> > https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=e0994165d1b8469dfc27b09b62ac74862d535812

> Just an update on this. I had a chat with Richard Henderson yesterday, and it might actually be easier and more convenient to backport
> fixes to older gdb versions (at least gdb-12 and gdb-11, but gdb-10 and gdb-9 are also affected). This will ensure those won't crash when
> they connect to a qemu that advertises the pauth feature.
>
> It also means we won't need qemu-side changes. My understanding is that we're close to the 8.0.0 release, and the code is already in place.

Having run into this problem in another couple of situations, one of
which involved gdb 10, I think I'm increasingly favouring option
2 here. The affected gdbs seem to be quite widely deployed, and
the bug results in crashes even for users who didn't really
care about pauth. So I'd rather we didn't release a QEMU 8.0
which crashes these affected deployed gdbs.

So:
 (a) if on the gdb side you can define (within the next week) a
suitable new XML name you want QEMU to expose, we can commit a
change to switch to that before we do the 8.0 release
 (b) if that's too tight a timescale, we can commit a patch which
just stops QEMU from exposing the pauth.xml, and we can come up
with a better solution after 8.0 releases

In fact, I think I'm going to submit a patch to do (b) for
now and we can follow up with a patch for (a) if we want.

thanks
-- PMM
Luis Machado March 17, 2023, 4:55 p.m. UTC | #16
On 3/17/23 16:37, Peter Maydell wrote:
> On Wed, 15 Mar 2023 at 09:51, Luis Machado <luis.machado@arm.com> wrote:
>> On 3/13/23 11:44, Luis Machado wrote:
>>> On 3/13/23 11:22, Peter Maydell via Gdb wrote:
>>>> Luis and I came up with two options:
>>>>
>>>> (1) leave QEMU outputting the pauth xml as-is, and tell people
>>>> whose gdb 12 crashes that they should upgrade to a newer gdb
>>>>
>>>> (2) make QEMU output the pauth info under a different XML namespace,
>>>> and tell people who need backtraces when pauth is enabled
>>>> that they should upgrade to a newer gdb
>>>>
>>>> Neither of these feel great, but on balance I guess 2 is better?
>>>>
>>>> Luis: I think that rather than doing (2) with a QEMU namespace,
>>>> we should define a gdb namespace for this. That makes it clear
>>>> that this is still a gdb-upstream-sanctioned way of exposing
>>>> the pauth registers.
>>>
>>> That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
>>>
>>> We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
>>> "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.
>>>
>>> FYI, I've pushed a better documentation for the arm/aarch64 xml descriptions here:
>>>
>>> https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=d7001b29e9f256dfc60acb481d9df8f91f2ee623
>>> https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=e0994165d1b8469dfc27b09b62ac74862d535812
>
>> Just an update on this. I had a chat with Richard Henderson yesterday, and it might actually be easier and more convenient to backport
>> fixes to older gdb versions (at least gdb-12 and gdb-11, but gdb-10 and gdb-9 are also affected). This will ensure those won't crash when
>> they connect to a qemu that advertises the pauth feature.
>>
>> It also means we won't need qemu-side changes. My understanding is that we're close to the 8.0.0 release, and the code is already in place.
>
> Having run into this problem in another couple of situations, one of
> which involved gdb 10, I think I'm increasingly favouring option
> 2 here. The affected gdbs seem to be quite widely deployed, and
> the bug results in crashes even for users who didn't really
> care about pauth. So I'd rather we didn't release a QEMU 8.0
> which crashes these affected deployed gdbs.
>

Are the affected gdb's packaged by distros? If so, a backport the distros can pick up
will solve this in a quick package update.

If we decide qemu should now emit a different xml for pauth, it will fix the crashes, but it also
means older gdb's (9/10/11/12) will not be able to backtrace properly through pauth-signed frames.

Maybe that's a reasonable drawback for qemu users?

If someone decides to implement a debugging stub that reports pauth (fast models, for example), it will
also crash gdb, so I still plan to do the backport anyway.

> So:
>   (a) if on the gdb side you can define (within the next week) a
> suitable new XML name you want QEMU to expose, we can commit a
> change to switch to that before we do the 8.0 release

pauth_v2 sounds about as good as any other for me.

>   (b) if that's too tight a timescale, we can commit a patch which
> just stops QEMU from exposing the pauth.xml, and we can come up
> with a better solution after 8.0 releases
>
> In fact, I think I'm going to submit a patch to do (b) for
> now and we can follow up with a patch for (a) if we want.
>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Peter Maydell March 17, 2023, 5:07 p.m. UTC | #17
On Fri, 17 Mar 2023 at 16:55, Luis Machado <luis.machado@arm.com> wrote:
> On 3/17/23 16:37, Peter Maydell wrote:
> > Having run into this problem in another couple of situations, one of
> > which involved gdb 10, I think I'm increasingly favouring option
> > 2 here. The affected gdbs seem to be quite widely deployed, and
> > the bug results in crashes even for users who didn't really
> > care about pauth. So I'd rather we didn't release a QEMU 8.0
> > which crashes these affected deployed gdbs.
> >
>
> Are the affected gdb's packaged by distros? If so, a backport the distros can pick up
> will solve this in a quick package update.

Yes, it's exactly because these gdbs are distro-packaged
that I don't want QEMU to make them crash. I think it's
going to take a long time for the fix to go into gdb branches
and gdb to make a point release and distros to pick up that
point release, and in the meantime that's a lot of crashing
gdb bug reports that we're going to have to field.

> If we decide qemu should now emit a different xml for pauth, it will fix the crashes, but it also
> means older gdb's (9/10/11/12) will not be able to backtrace properly through pauth-signed frames.
>
> Maybe that's a reasonable drawback for qemu users?

"No backtrace through pauth frames" is the situation we've
been in ever since we implemented pauth in 2019, so I think
that's fine. It's not regressing something that used to work.

> If someone decides to implement a debugging stub that reports pauth (fast models, for example), it will
> also crash gdb, so I still plan to do the backport anyway.

If you're backporting the fix, you could also backport the
(hopefully tiny) change that says "treat pauth_v2 the same
way we do pauth", and then users with an updated older
gdb will also get working backtraces.

thanks
-- PMM
Luis Machado March 17, 2023, 5:12 p.m. UTC | #18
On 3/17/23 17:07, Peter Maydell wrote:
> On Fri, 17 Mar 2023 at 16:55, Luis Machado <luis.machado@arm.com> wrote:
>> On 3/17/23 16:37, Peter Maydell wrote:
>>> Having run into this problem in another couple of situations, one of
>>> which involved gdb 10, I think I'm increasingly favouring option
>>> 2 here. The affected gdbs seem to be quite widely deployed, and
>>> the bug results in crashes even for users who didn't really
>>> care about pauth. So I'd rather we didn't release a QEMU 8.0
>>> which crashes these affected deployed gdbs.
>>>
>>
>> Are the affected gdb's packaged by distros? If so, a backport the distros can pick up
>> will solve this in a quick package update.
>
> Yes, it's exactly because these gdbs are distro-packaged
> that I don't want QEMU to make them crash. I think it's
> going to take a long time for the fix to go into gdb branches
> and gdb to make a point release and distros to pick up that
> point release, and in the meantime that's a lot of crashing
> gdb bug reports that we're going to have to field.

Just to clarify, there won't be any point releases for gdb's 9/10/11/12.  We might have a bug fix
release for gdb 13 though (which isn't affected).

>
>> If we decide qemu should now emit a different xml for pauth, it will fix the crashes, but it also
>> means older gdb's (9/10/11/12) will not be able to backtrace properly through pauth-signed frames.
>>
>> Maybe that's a reasonable drawback for qemu users?
>
> "No backtrace through pauth frames" is the situation we've
> been in ever since we implemented pauth in 2019, so I think
> that's fine. It's not regressing something that used to work.
>

Fair enough.

>> If someone decides to implement a debugging stub that reports pauth (fast models, for example), it will
>> also crash gdb, so I still plan to do the backport anyway.
>
> If you're backporting the fix, you could also backport the
> (hopefully tiny) change that says "treat pauth_v2 the same
> way we do pauth", and then users with an updated older
> gdb will also get working backtraces.

I can negotiate that as well, though it borders being a new feature.

>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Luis Machado March 17, 2023, 5:16 p.m. UTC | #19
On 3/17/23 17:12, Luis Machado wrote:
> On 3/17/23 17:07, Peter Maydell wrote:
>> On Fri, 17 Mar 2023 at 16:55, Luis Machado <luis.machado@arm.com> wrote:
>>> On 3/17/23 16:37, Peter Maydell wrote:
>>>> Having run into this problem in another couple of situations, one of
>>>> which involved gdb 10, I think I'm increasingly favouring option
>>>> 2 here. The affected gdbs seem to be quite widely deployed, and
>>>> the bug results in crashes even for users who didn't really
>>>> care about pauth. So I'd rather we didn't release a QEMU 8.0
>>>> which crashes these affected deployed gdbs.
>>>>
>>>
>>> Are the affected gdb's packaged by distros? If so, a backport the distros can pick up
>>> will solve this in a quick package update.
>>
>> Yes, it's exactly because these gdbs are distro-packaged
>> that I don't want QEMU to make them crash. I think it's
>> going to take a long time for the fix to go into gdb branches
>> and gdb to make a point release and distros to pick up that
>> point release, and in the meantime that's a lot of crashing
>> gdb bug reports that we're going to have to field.
>
> Just to clarify, there won't be any point releases for gdb's 9/10/11/12.  We might have a bug fix
> release for gdb 13 though (which isn't affected).
>

Just to complement, my plan is to make the backports available (via stable branch commits) so distro package
maintainers can pick those up easily. No new releases will be made for older gdb's, so the package maintainers
can pick the backport up as soon as they are pushed. There won't be waiting for a new release of gdb.

>>
>>> If we decide qemu should now emit a different xml for pauth, it will fix the crashes, but it also
>>> means older gdb's (9/10/11/12) will not be able to backtrace properly through pauth-signed frames.
>>>
>>> Maybe that's a reasonable drawback for qemu users?
>>
>> "No backtrace through pauth frames" is the situation we've
>> been in ever since we implemented pauth in 2019, so I think
>> that's fine. It's not regressing something that used to work.
>>
>
> Fair enough.
>
>>> If someone decides to implement a debugging stub that reports pauth (fast models, for example), it will
>>> also crash gdb, so I still plan to do the backport anyway.
>>
>> If you're backporting the fix, you could also backport the
>> (hopefully tiny) change that says "treat pauth_v2 the same
>> way we do pauth", and then users with an updated older
>> gdb will also get working backtraces.
>
> I can negotiate that as well, though it borders being a new feature.
>
>>
>> thanks
>> -- PMM
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff mbox series

Patch

diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 9e91a20b0d..8ffde3b0ed 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -84,6 +84,8 @@  TESTS += sha512-vector
 ifeq ($(HOST_GDB_SUPPORTS_ARCH),y)
 GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
 
+run-gdbstub-%: QEMU_OPTS=-cpu max,pauth=off
+
 run-gdbstub-sysregs: sysregs
 	$(call run-test, $@, $(GDB_SCRIPT) \
 		--gdb $(HAVE_GDB_BIN) \