All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] arm64: kgdb: handle read-only text / modules
@ 2016-09-20 10:03 ` AKASHI Takahiro
  0 siblings, 0 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2016-09-20 10:03 UTC (permalink / raw)
  To: catalin.marinas, will.deacon, jason.wessel
  Cc: linux-arm-kernel, kgdb-bugreport, stable, AKASHI Takahiro

Handle read-only cases (CONFIG_DEBUG_RODATA/CONFIG_DEBUG_SET_MODULE_RONX)
by using aarch64_insn_write() instead of probe_kernel_write().
See how this works:
    commit 2f896d586610 ("arm64: use fixmap for text patching")

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: <stable@vger.kernel.org> # 4.0-
---
 arch/arm64/kernel/kgdb.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 6732a27..133cfe3 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -382,3 +382,23 @@ struct kgdb_arch arch_kgdb_ops = {
 		KGDB_DYN_BRK_INS_BYTE(3),
 	}
 };
+
+int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
+{
+	int err;
+
+	BUILD_BUG_ON(AARCH64_INSN_SIZE != BREAK_INSTR_SIZE);
+
+	err = aarch64_insn_read((void *)bpt->bpt_addr, (u32 *)bpt->saved_instr);
+	if (err)
+		return err;
+
+	return aarch64_insn_write((void *)bpt->bpt_addr,
+			(u32)AARCH64_BREAK_KGDB_DYN_DBG);
+}
+
+int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
+{
+	return aarch64_insn_write((void *)bpt->bpt_addr,
+			*(u32 *)bpt->saved_instr);
+}
-- 
2.10.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] arm64: kgdb: handle read-only text / modules
@ 2016-09-20 10:03 ` AKASHI Takahiro
  0 siblings, 0 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2016-09-20 10:03 UTC (permalink / raw)
  To: linux-arm-kernel

Handle read-only cases (CONFIG_DEBUG_RODATA/CONFIG_DEBUG_SET_MODULE_RONX)
by using aarch64_insn_write() instead of probe_kernel_write().
See how this works:
    commit 2f896d586610 ("arm64: use fixmap for text patching")

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: <stable@vger.kernel.org> # 4.0-
---
 arch/arm64/kernel/kgdb.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 6732a27..133cfe3 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -382,3 +382,23 @@ struct kgdb_arch arch_kgdb_ops = {
 		KGDB_DYN_BRK_INS_BYTE(3),
 	}
 };
+
+int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
+{
+	int err;
+
+	BUILD_BUG_ON(AARCH64_INSN_SIZE != BREAK_INSTR_SIZE);
+
+	err = aarch64_insn_read((void *)bpt->bpt_addr, (u32 *)bpt->saved_instr);
+	if (err)
+		return err;
+
+	return aarch64_insn_write((void *)bpt->bpt_addr,
+			(u32)AARCH64_BREAK_KGDB_DYN_DBG);
+}
+
+int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
+{
+	return aarch64_insn_write((void *)bpt->bpt_addr,
+			*(u32 *)bpt->saved_instr);
+}
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: kgdb: handle read-only text / modules
  2016-09-20 10:03 ` AKASHI Takahiro
@ 2016-09-20 10:33   ` Mark Rutland
  -1 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2016-09-20 10:33 UTC (permalink / raw)
  To: AKASHI Takahiro
  Cc: catalin.marinas, will.deacon, jason.wessel, kgdb-bugreport,
	stable, linux-arm-kernel, labbott

On Tue, Sep 20, 2016 at 07:03:21PM +0900, AKASHI Takahiro wrote:
> Handle read-only cases (CONFIG_DEBUG_RODATA/CONFIG_DEBUG_SET_MODULE_RONX)
> by using aarch64_insn_write() instead of probe_kernel_write().
> See how this works:
>     commit 2f896d586610 ("arm64: use fixmap for text patching")
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Jason Wessel <jason.wessel@windriver.com>
> Cc: <stable@vger.kernel.org> # 4.0-

We had SET_MODULE_RONX in v3.17, and we had KGDB before that, so we need
something for v3.17+.

> ---
>  arch/arm64/kernel/kgdb.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 6732a27..133cfe3 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -382,3 +382,23 @@ struct kgdb_arch arch_kgdb_ops = {
>  		KGDB_DYN_BRK_INS_BYTE(3),
>  	}
>  };
> +int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> +{
> +	int err;
> +
> +	BUILD_BUG_ON(AARCH64_INSN_SIZE != BREAK_INSTR_SIZE);
> +
> +	err = aarch64_insn_read((void *)bpt->bpt_addr, (u32 *)bpt->saved_instr);
> +	if (err)
> +		return err;
> +
> +	return aarch64_insn_write((void *)bpt->bpt_addr,
> +			(u32)AARCH64_BREAK_KGDB_DYN_DBG);
> +}

This changes the endianness of saved_instr (on BE), but it looks like
that's handed as an opaque token by the core code anyway, so that should
be fine.

This also renders arch_kgdb_ops.gdb_bpt_instr unused. Can/should we get
rid of that?

> +int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> +{
> +	return aarch64_insn_write((void *)bpt->bpt_addr,
> +			*(u32 *)bpt->saved_instr);
> +}

We also need a few additional includes:

<asm/debug-monitors.h> # for BREAK_INSTR_SIZE, AARCH64_BREAK_KGDB_DYN_DBG
<asm/insn.h> # for AARCH64_INSN_SIZE, insn_{read,write}
<linux/bug.h> # for BUILD_BUG_ON()

I take it that we're protected against nesting within
aarch64_insn_write(), so that we can't deadlock on patch_lock?

Other than that, this looks good to me.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] arm64: kgdb: handle read-only text / modules
@ 2016-09-20 10:33   ` Mark Rutland
  0 siblings, 0 replies; 6+ messages in thread
From: Mark Rutland @ 2016-09-20 10:33 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Sep 20, 2016 at 07:03:21PM +0900, AKASHI Takahiro wrote:
> Handle read-only cases (CONFIG_DEBUG_RODATA/CONFIG_DEBUG_SET_MODULE_RONX)
> by using aarch64_insn_write() instead of probe_kernel_write().
> See how this works:
>     commit 2f896d586610 ("arm64: use fixmap for text patching")
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Jason Wessel <jason.wessel@windriver.com>
> Cc: <stable@vger.kernel.org> # 4.0-

We had SET_MODULE_RONX in v3.17, and we had KGDB before that, so we need
something for v3.17+.

> ---
>  arch/arm64/kernel/kgdb.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 6732a27..133cfe3 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -382,3 +382,23 @@ struct kgdb_arch arch_kgdb_ops = {
>  		KGDB_DYN_BRK_INS_BYTE(3),
>  	}
>  };
> +int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> +{
> +	int err;
> +
> +	BUILD_BUG_ON(AARCH64_INSN_SIZE != BREAK_INSTR_SIZE);
> +
> +	err = aarch64_insn_read((void *)bpt->bpt_addr, (u32 *)bpt->saved_instr);
> +	if (err)
> +		return err;
> +
> +	return aarch64_insn_write((void *)bpt->bpt_addr,
> +			(u32)AARCH64_BREAK_KGDB_DYN_DBG);
> +}

This changes the endianness of saved_instr (on BE), but it looks like
that's handed as an opaque token by the core code anyway, so that should
be fine.

This also renders arch_kgdb_ops.gdb_bpt_instr unused. Can/should we get
rid of that?

> +int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> +{
> +	return aarch64_insn_write((void *)bpt->bpt_addr,
> +			*(u32 *)bpt->saved_instr);
> +}

We also need a few additional includes:

<asm/debug-monitors.h> # for BREAK_INSTR_SIZE, AARCH64_BREAK_KGDB_DYN_DBG
<asm/insn.h> # for AARCH64_INSN_SIZE, insn_{read,write}
<linux/bug.h> # for BUILD_BUG_ON()

I take it that we're protected against nesting within
aarch64_insn_write(), so that we can't deadlock on patch_lock?

Other than that, this looks good to me.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] arm64: kgdb: handle read-only text / modules
  2016-09-20 10:33   ` Mark Rutland
@ 2016-09-21  7:14     ` AKASHI Takahiro
  -1 siblings, 0 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2016-09-21  7:14 UTC (permalink / raw)
  To: Mark Rutland
  Cc: catalin.marinas, will.deacon, jason.wessel, kgdb-bugreport,
	stable, linux-arm-kernel, labbott

On Tue, Sep 20, 2016 at 11:33:34AM +0100, Mark Rutland wrote:
> On Tue, Sep 20, 2016 at 07:03:21PM +0900, AKASHI Takahiro wrote:
> > Handle read-only cases (CONFIG_DEBUG_RODATA/CONFIG_DEBUG_SET_MODULE_RONX)
> > by using aarch64_insn_write() instead of probe_kernel_write().
> > See how this works:
> >     commit 2f896d586610 ("arm64: use fixmap for text patching")
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Jason Wessel <jason.wessel@windriver.com>
> > Cc: <stable@vger.kernel.org> # 4.0-
> 
> We had SET_MODULE_RONX in v3.17, and we had KGDB before that, so we need
> something for v3.17+.

Right, but 3.18+ :)
Unfortunately, the patch ("arm64: use fixmap for text patching")
is merged only in v4.0 or later. So it is also a pre-requisite.

> > ---
> >  arch/arm64/kernel/kgdb.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> > index 6732a27..133cfe3 100644
> > --- a/arch/arm64/kernel/kgdb.c
> > +++ b/arch/arm64/kernel/kgdb.c
> > @@ -382,3 +382,23 @@ struct kgdb_arch arch_kgdb_ops = {
> >  		KGDB_DYN_BRK_INS_BYTE(3),
> >  	}
> >  };
> > +int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> > +{
> > +	int err;
> > +
> > +	BUILD_BUG_ON(AARCH64_INSN_SIZE != BREAK_INSTR_SIZE);
> > +
> > +	err = aarch64_insn_read((void *)bpt->bpt_addr, (u32 *)bpt->saved_instr);
> > +	if (err)
> > +		return err;
> > +
> > +	return aarch64_insn_write((void *)bpt->bpt_addr,
> > +			(u32)AARCH64_BREAK_KGDB_DYN_DBG);
> > +}
> 
> This changes the endianness of saved_instr (on BE), but it looks like
> that's handed as an opaque token by the core code anyway, so that should
> be fine.
> 
> This also renders arch_kgdb_ops.gdb_bpt_instr unused. Can/should we get
> rid of that?

Yes, we can. But arch_kgdb_ops is still needed for compiling anyway.

> > +int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> > +{
> > +	return aarch64_insn_write((void *)bpt->bpt_addr,
> > +			*(u32 *)bpt->saved_instr);
> > +}
> 
> We also need a few additional includes:
> 
> <asm/debug-monitors.h> # for BREAK_INSTR_SIZE, AARCH64_BREAK_KGDB_DYN_DBG
> <asm/insn.h> # for AARCH64_INSN_SIZE, insn_{read,write}
> <linux/bug.h> # for BUILD_BUG_ON()

Added.

> I take it that we're protected against nesting within
> aarch64_insn_write(), so that we can't deadlock on patch_lock?
> 
> Other than that, this looks good to me.

Thanks,
-Takahiro AKASHI

> Thanks,
> Mark.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] arm64: kgdb: handle read-only text / modules
@ 2016-09-21  7:14     ` AKASHI Takahiro
  0 siblings, 0 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2016-09-21  7:14 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Sep 20, 2016 at 11:33:34AM +0100, Mark Rutland wrote:
> On Tue, Sep 20, 2016 at 07:03:21PM +0900, AKASHI Takahiro wrote:
> > Handle read-only cases (CONFIG_DEBUG_RODATA/CONFIG_DEBUG_SET_MODULE_RONX)
> > by using aarch64_insn_write() instead of probe_kernel_write().
> > See how this works:
> >     commit 2f896d586610 ("arm64: use fixmap for text patching")
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Jason Wessel <jason.wessel@windriver.com>
> > Cc: <stable@vger.kernel.org> # 4.0-
> 
> We had SET_MODULE_RONX in v3.17, and we had KGDB before that, so we need
> something for v3.17+.

Right, but 3.18+ :)
Unfortunately, the patch ("arm64: use fixmap for text patching")
is merged only in v4.0 or later. So it is also a pre-requisite.

> > ---
> >  arch/arm64/kernel/kgdb.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> > index 6732a27..133cfe3 100644
> > --- a/arch/arm64/kernel/kgdb.c
> > +++ b/arch/arm64/kernel/kgdb.c
> > @@ -382,3 +382,23 @@ struct kgdb_arch arch_kgdb_ops = {
> >  		KGDB_DYN_BRK_INS_BYTE(3),
> >  	}
> >  };
> > +int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> > +{
> > +	int err;
> > +
> > +	BUILD_BUG_ON(AARCH64_INSN_SIZE != BREAK_INSTR_SIZE);
> > +
> > +	err = aarch64_insn_read((void *)bpt->bpt_addr, (u32 *)bpt->saved_instr);
> > +	if (err)
> > +		return err;
> > +
> > +	return aarch64_insn_write((void *)bpt->bpt_addr,
> > +			(u32)AARCH64_BREAK_KGDB_DYN_DBG);
> > +}
> 
> This changes the endianness of saved_instr (on BE), but it looks like
> that's handed as an opaque token by the core code anyway, so that should
> be fine.
> 
> This also renders arch_kgdb_ops.gdb_bpt_instr unused. Can/should we get
> rid of that?

Yes, we can. But arch_kgdb_ops is still needed for compiling anyway.

> > +int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> > +{
> > +	return aarch64_insn_write((void *)bpt->bpt_addr,
> > +			*(u32 *)bpt->saved_instr);
> > +}
> 
> We also need a few additional includes:
> 
> <asm/debug-monitors.h> # for BREAK_INSTR_SIZE, AARCH64_BREAK_KGDB_DYN_DBG
> <asm/insn.h> # for AARCH64_INSN_SIZE, insn_{read,write}
> <linux/bug.h> # for BUILD_BUG_ON()

Added.

> I take it that we're protected against nesting within
> aarch64_insn_write(), so that we can't deadlock on patch_lock?
> 
> Other than that, this looks good to me.

Thanks,
-Takahiro AKASHI

> Thanks,
> Mark.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-21  7:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-20 10:03 [PATCH] arm64: kgdb: handle read-only text / modules AKASHI Takahiro
2016-09-20 10:03 ` AKASHI Takahiro
2016-09-20 10:33 ` Mark Rutland
2016-09-20 10:33   ` Mark Rutland
2016-09-21  7:14   ` AKASHI Takahiro
2016-09-21  7:14     ` AKASHI Takahiro

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.