All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] arm64: perf: Mark expected switch fall-through
@ 2019-07-26 11:27 ` Anders Roxell
  0 siblings, 0 replies; 15+ messages in thread
From: Anders Roxell @ 2019-07-26 11:27 UTC (permalink / raw)
  To: will, mark.rutland, catalin.marinas
  Cc: stable, Anders Roxell, linux-kernel, linux-arm-kernel

When fall-through warnings was enabled by default, commit d93512ef0f0e
("Makefile: Globally enable fall-through warning"), the following
warnings was starting to show up:

../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
 through [-Wimplicit-fallthrough=]
    if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
       ^
../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
   case 2:
   ^~~~
../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
 through [-Wimplicit-fallthrough=]
    if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
       ^
../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
   default:
   ^~~~~~~

Rework so that the compiler doesn't warn about fall-through. Rework so
the code looks like the arm code. Since the comment in the function
indicates taht this is supposed to behave the same way as arm32 because
it handles 32-bit tasks also.

Cc: stable@vger.kernel.org # v3.16+
Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
 arch/arm64/kernel/hw_breakpoint.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index dceb84520948..ea616adf1cf1 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 		case 0:
 			/* Aligned */
 			break;
-		case 1:
-			/* Allow single byte watchpoint. */
-			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
-				break;
 		case 2:
 			/* Allow halfword watchpoints and breakpoints. */
 			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
 				break;
+			/* Fall through */
+		case 1:
+		case 3:
+			/* Allow single byte watchpoint. */
+			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
+				break;
+			/* Fall through */
 		default:
 			return -EINVAL;
 		}
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 1/3] arm64: perf: Mark expected switch fall-through
@ 2019-07-26 11:27 ` Anders Roxell
  0 siblings, 0 replies; 15+ messages in thread
From: Anders Roxell @ 2019-07-26 11:27 UTC (permalink / raw)
  To: will, mark.rutland, catalin.marinas
  Cc: linux-arm-kernel, linux-kernel, Anders Roxell, stable

When fall-through warnings was enabled by default, commit d93512ef0f0e
("Makefile: Globally enable fall-through warning"), the following
warnings was starting to show up:

../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
 through [-Wimplicit-fallthrough=]
    if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
       ^
../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
   case 2:
   ^~~~
../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
 through [-Wimplicit-fallthrough=]
    if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
       ^
../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
   default:
   ^~~~~~~

Rework so that the compiler doesn't warn about fall-through. Rework so
the code looks like the arm code. Since the comment in the function
indicates taht this is supposed to behave the same way as arm32 because
it handles 32-bit tasks also.

Cc: stable@vger.kernel.org # v3.16+
Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
 arch/arm64/kernel/hw_breakpoint.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index dceb84520948..ea616adf1cf1 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 		case 0:
 			/* Aligned */
 			break;
-		case 1:
-			/* Allow single byte watchpoint. */
-			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
-				break;
 		case 2:
 			/* Allow halfword watchpoints and breakpoints. */
 			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
 				break;
+			/* Fall through */
+		case 1:
+		case 3:
+			/* Allow single byte watchpoint. */
+			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
+				break;
+			/* Fall through */
 		default:
 			return -EINVAL;
 		}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
  2019-07-26 11:27 ` Anders Roxell
@ 2019-07-26 12:10   ` Mark Rutland
  -1 siblings, 0 replies; 15+ messages in thread
From: Mark Rutland @ 2019-07-26 12:10 UTC (permalink / raw)
  To: Anders Roxell
  Cc: Kees Cook, Gustavo A. R. Silva, catalin.marinas, linux-kernel,
	stable, will, linux-arm-kernel

On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
> When fall-through warnings was enabled by default, commit d93512ef0f0e
> ("Makefile: Globally enable fall-through warning"), the following
> warnings was starting to show up:
> 
> ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
> ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
>  through [-Wimplicit-fallthrough=]
>     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
>        ^
> ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
>    case 2:
>    ^~~~
> ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
>  through [-Wimplicit-fallthrough=]
>     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
>        ^
> ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
>    default:
>    ^~~~~~~
> 
> Rework so that the compiler doesn't warn about fall-through. Rework so
> the code looks like the arm code. Since the comment in the function
> indicates taht this is supposed to behave the same way as arm32 because

Typo: s/taht/that/

> it handles 32-bit tasks also.
> 
> Cc: stable@vger.kernel.org # v3.16+
> Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

The patch itself looks fine, but I don't think this needs a CC to
stable, nor does it require that fixes tag, as there's no functional
problem.

If anything, it fixes:

  d93512ef0f0e (" Makefile: Globally enable fall-through warning")

... given the commit message for that patch states:

  Now that all the fall-through warnings have been addressed in the
  kernel, enable the fall-through warning globally.

... and the existence of this patch implies otherwise.

IIUC that patch isn't even in mainline yet, but given this is simple I
imagine that Will and Catalin might be happy to pick this up for the
next rc.

Thanks,
Mark.

> ---
>  arch/arm64/kernel/hw_breakpoint.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index dceb84520948..ea616adf1cf1 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>  		case 0:
>  			/* Aligned */
>  			break;
> -		case 1:
> -			/* Allow single byte watchpoint. */
> -			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> -				break;
>  		case 2:
>  			/* Allow halfword watchpoints and breakpoints. */
>  			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
>  				break;
> +			/* Fall through */
> +		case 1:
> +		case 3:
> +			/* Allow single byte watchpoint. */
> +			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> +				break;
> +			/* Fall through */
>  		default:
>  			return -EINVAL;
>  		}
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
@ 2019-07-26 12:10   ` Mark Rutland
  0 siblings, 0 replies; 15+ messages in thread
From: Mark Rutland @ 2019-07-26 12:10 UTC (permalink / raw)
  To: Anders Roxell
  Cc: will, catalin.marinas, linux-arm-kernel, linux-kernel, stable,
	Gustavo A. R. Silva, Kees Cook

On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
> When fall-through warnings was enabled by default, commit d93512ef0f0e
> ("Makefile: Globally enable fall-through warning"), the following
> warnings was starting to show up:
> 
> ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
> ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
>  through [-Wimplicit-fallthrough=]
>     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
>        ^
> ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
>    case 2:
>    ^~~~
> ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
>  through [-Wimplicit-fallthrough=]
>     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
>        ^
> ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
>    default:
>    ^~~~~~~
> 
> Rework so that the compiler doesn't warn about fall-through. Rework so
> the code looks like the arm code. Since the comment in the function
> indicates taht this is supposed to behave the same way as arm32 because

Typo: s/taht/that/

> it handles 32-bit tasks also.
> 
> Cc: stable@vger.kernel.org # v3.16+
> Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>

The patch itself looks fine, but I don't think this needs a CC to
stable, nor does it require that fixes tag, as there's no functional
problem.

If anything, it fixes:

  d93512ef0f0e (" Makefile: Globally enable fall-through warning")

... given the commit message for that patch states:

  Now that all the fall-through warnings have been addressed in the
  kernel, enable the fall-through warning globally.

... and the existence of this patch implies otherwise.

IIUC that patch isn't even in mainline yet, but given this is simple I
imagine that Will and Catalin might be happy to pick this up for the
next rc.

Thanks,
Mark.

> ---
>  arch/arm64/kernel/hw_breakpoint.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index dceb84520948..ea616adf1cf1 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>  		case 0:
>  			/* Aligned */
>  			break;
> -		case 1:
> -			/* Allow single byte watchpoint. */
> -			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> -				break;
>  		case 2:
>  			/* Allow halfword watchpoints and breakpoints. */
>  			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
>  				break;
> +			/* Fall through */
> +		case 1:
> +		case 3:
> +			/* Allow single byte watchpoint. */
> +			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> +				break;
> +			/* Fall through */
>  		default:
>  			return -EINVAL;
>  		}
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
  2019-07-26 12:10   ` Mark Rutland
@ 2019-07-26 12:13     ` Mark Rutland
  -1 siblings, 0 replies; 15+ messages in thread
From: Mark Rutland @ 2019-07-26 12:13 UTC (permalink / raw)
  To: Anders Roxell
  Cc: Kees Cook, Gustavo A. R. Silva, catalin.marinas, linux-kernel,
	stable, will, linux-arm-kernel

On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
> On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
> > When fall-through warnings was enabled by default, commit d93512ef0f0e
> > ("Makefile: Globally enable fall-through warning"), the following
> > warnings was starting to show up:
> > 
> > ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
> > ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
> >  through [-Wimplicit-fallthrough=]
> >     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> >        ^
> > ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
> >    case 2:
> >    ^~~~
> > ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
> >  through [-Wimplicit-fallthrough=]
> >     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
> >        ^
> > ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
> >    default:
> >    ^~~~~~~
> > 
> > Rework so that the compiler doesn't warn about fall-through. Rework so
> > the code looks like the arm code. Since the comment in the function
> > indicates taht this is supposed to behave the same way as arm32 because
> 
> Typo: s/taht/that/
> 
> > it handles 32-bit tasks also.
> > 
> > Cc: stable@vger.kernel.org # v3.16+
> > Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
> > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> 
> The patch itself looks fine, but I don't think this needs a CC to
> stable, nor does it require that fixes tag, as there's no functional
> problem.

Hmm... I now see I spoke too soon, and this is making the 1-byte
breakpoint work at a 3-byte offset.

Given that:

Acked-by: Mark Rutland <mark.rutland@arm.com>

... and the fixes and stable tags are appropriate for that portion of
the patch.

Sorry for the noise.

Thanks,
Mark.


> > ---
> >  arch/arm64/kernel/hw_breakpoint.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> > index dceb84520948..ea616adf1cf1 100644
> > --- a/arch/arm64/kernel/hw_breakpoint.c
> > +++ b/arch/arm64/kernel/hw_breakpoint.c
> > @@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
> >  		case 0:
> >  			/* Aligned */
> >  			break;
> > -		case 1:
> > -			/* Allow single byte watchpoint. */
> > -			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> > -				break;
> >  		case 2:
> >  			/* Allow halfword watchpoints and breakpoints. */
> >  			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
> >  				break;
> > +			/* Fall through */
> > +		case 1:
> > +		case 3:
> > +			/* Allow single byte watchpoint. */
> > +			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> > +				break;
> > +			/* Fall through */
> >  		default:
> >  			return -EINVAL;
> >  		}
> > -- 
> > 2.20.1
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
@ 2019-07-26 12:13     ` Mark Rutland
  0 siblings, 0 replies; 15+ messages in thread
From: Mark Rutland @ 2019-07-26 12:13 UTC (permalink / raw)
  To: Anders Roxell
  Cc: will, catalin.marinas, linux-arm-kernel, linux-kernel, stable,
	Gustavo A. R. Silva, Kees Cook

On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
> On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
> > When fall-through warnings was enabled by default, commit d93512ef0f0e
> > ("Makefile: Globally enable fall-through warning"), the following
> > warnings was starting to show up:
> > 
> > ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
> > ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
> >  through [-Wimplicit-fallthrough=]
> >     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> >        ^
> > ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
> >    case 2:
> >    ^~~~
> > ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
> >  through [-Wimplicit-fallthrough=]
> >     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
> >        ^
> > ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
> >    default:
> >    ^~~~~~~
> > 
> > Rework so that the compiler doesn't warn about fall-through. Rework so
> > the code looks like the arm code. Since the comment in the function
> > indicates taht this is supposed to behave the same way as arm32 because
> 
> Typo: s/taht/that/
> 
> > it handles 32-bit tasks also.
> > 
> > Cc: stable@vger.kernel.org # v3.16+
> > Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
> > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> 
> The patch itself looks fine, but I don't think this needs a CC to
> stable, nor does it require that fixes tag, as there's no functional
> problem.

Hmm... I now see I spoke too soon, and this is making the 1-byte
breakpoint work at a 3-byte offset.

Given that:

Acked-by: Mark Rutland <mark.rutland@arm.com>

... and the fixes and stable tags are appropriate for that portion of
the patch.

Sorry for the noise.

Thanks,
Mark.


> > ---
> >  arch/arm64/kernel/hw_breakpoint.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> > index dceb84520948..ea616adf1cf1 100644
> > --- a/arch/arm64/kernel/hw_breakpoint.c
> > +++ b/arch/arm64/kernel/hw_breakpoint.c
> > @@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
> >  		case 0:
> >  			/* Aligned */
> >  			break;
> > -		case 1:
> > -			/* Allow single byte watchpoint. */
> > -			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> > -				break;
> >  		case 2:
> >  			/* Allow halfword watchpoints and breakpoints. */
> >  			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
> >  				break;
> > +			/* Fall through */
> > +		case 1:
> > +		case 3:
> > +			/* Allow single byte watchpoint. */
> > +			if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> > +				break;
> > +			/* Fall through */
> >  		default:
> >  			return -EINVAL;
> >  		}
> > -- 
> > 2.20.1
> > 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
  2019-07-26 12:13     ` Mark Rutland
@ 2019-07-26 12:27       ` Will Deacon
  -1 siblings, 0 replies; 15+ messages in thread
From: Will Deacon @ 2019-07-26 12:27 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Anders Roxell, Kees Cook, Gustavo A. R. Silva, catalin.marinas,
	linux-kernel, stable, linux-arm-kernel

On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
> On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
> > On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
> > > When fall-through warnings was enabled by default, commit d93512ef0f0e
> > > ("Makefile: Globally enable fall-through warning"), the following
> > > warnings was starting to show up:
> > > 
> > > ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
> > > ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
> > >  through [-Wimplicit-fallthrough=]
> > >     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> > >        ^
> > > ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
> > >    case 2:
> > >    ^~~~
> > > ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
> > >  through [-Wimplicit-fallthrough=]
> > >     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
> > >        ^
> > > ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
> > >    default:
> > >    ^~~~~~~
> > > 
> > > Rework so that the compiler doesn't warn about fall-through. Rework so
> > > the code looks like the arm code. Since the comment in the function
> > > indicates taht this is supposed to behave the same way as arm32 because
> > 
> > Typo: s/taht/that/
> > 
> > > it handles 32-bit tasks also.
> > > 
> > > Cc: stable@vger.kernel.org # v3.16+
> > > Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
> > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > 
> > The patch itself looks fine, but I don't think this needs a CC to
> > stable, nor does it require that fixes tag, as there's no functional
> > problem.
> 
> Hmm... I now see I spoke too soon, and this is making the 1-byte
> breakpoint work at a 3-byte offset.

I still don't think it's quite right though, since it forbids a 2-byte
watchpoint on a byte-aligned address.

I think the arm64 code matches what we had on 32-bit prior to
d968d2b801d8 ("ARM: 7497/1: hw_breakpoint: allow single-byte watchpoints
on all addresses"), so we should have one patch bringing us up to speed
with that change, and then another annotating the fallthroughs.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
@ 2019-07-26 12:27       ` Will Deacon
  0 siblings, 0 replies; 15+ messages in thread
From: Will Deacon @ 2019-07-26 12:27 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Anders Roxell, catalin.marinas, linux-arm-kernel, linux-kernel,
	stable, Gustavo A. R. Silva, Kees Cook

On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
> On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
> > On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
> > > When fall-through warnings was enabled by default, commit d93512ef0f0e
> > > ("Makefile: Globally enable fall-through warning"), the following
> > > warnings was starting to show up:
> > > 
> > > ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
> > > ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
> > >  through [-Wimplicit-fallthrough=]
> > >     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> > >        ^
> > > ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
> > >    case 2:
> > >    ^~~~
> > > ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
> > >  through [-Wimplicit-fallthrough=]
> > >     if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
> > >        ^
> > > ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
> > >    default:
> > >    ^~~~~~~
> > > 
> > > Rework so that the compiler doesn't warn about fall-through. Rework so
> > > the code looks like the arm code. Since the comment in the function
> > > indicates taht this is supposed to behave the same way as arm32 because
> > 
> > Typo: s/taht/that/
> > 
> > > it handles 32-bit tasks also.
> > > 
> > > Cc: stable@vger.kernel.org # v3.16+
> > > Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
> > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > 
> > The patch itself looks fine, but I don't think this needs a CC to
> > stable, nor does it require that fixes tag, as there's no functional
> > problem.
> 
> Hmm... I now see I spoke too soon, and this is making the 1-byte
> breakpoint work at a 3-byte offset.

I still don't think it's quite right though, since it forbids a 2-byte
watchpoint on a byte-aligned address.

I think the arm64 code matches what we had on 32-bit prior to
d968d2b801d8 ("ARM: 7497/1: hw_breakpoint: allow single-byte watchpoints
on all addresses"), so we should have one patch bringing us up to speed
with that change, and then another annotating the fallthroughs.

Will

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
  2019-07-26 12:27       ` Will Deacon
@ 2019-07-26 12:38         ` Robin Murphy
  -1 siblings, 0 replies; 15+ messages in thread
From: Robin Murphy @ 2019-07-26 12:38 UTC (permalink / raw)
  To: Will Deacon, Mark Rutland
  Cc: Anders Roxell, Kees Cook, Gustavo A. R. Silva, catalin.marinas,
	linux-kernel, stable, linux-arm-kernel

On 26/07/2019 13:27, Will Deacon wrote:
> On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
>> On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
>>> On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
>>>> When fall-through warnings was enabled by default, commit d93512ef0f0e
>>>> ("Makefile: Globally enable fall-through warning"), the following
>>>> warnings was starting to show up:
>>>>
>>>> ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
>>>> ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
>>>>   through [-Wimplicit-fallthrough=]
>>>>      if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
>>>>         ^
>>>> ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
>>>>     case 2:
>>>>     ^~~~
>>>> ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
>>>>   through [-Wimplicit-fallthrough=]
>>>>      if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
>>>>         ^
>>>> ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
>>>>     default:
>>>>     ^~~~~~~
>>>>
>>>> Rework so that the compiler doesn't warn about fall-through. Rework so
>>>> the code looks like the arm code. Since the comment in the function
>>>> indicates taht this is supposed to behave the same way as arm32 because
>>>
>>> Typo: s/taht/that/
>>>
>>>> it handles 32-bit tasks also.
>>>>
>>>> Cc: stable@vger.kernel.org # v3.16+
>>>> Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
>>>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>>>
>>> The patch itself looks fine, but I don't think this needs a CC to
>>> stable, nor does it require that fixes tag, as there's no functional
>>> problem.
>>
>> Hmm... I now see I spoke too soon, and this is making the 1-byte
>> breakpoint work at a 3-byte offset.
> 
> I still don't think it's quite right though, since it forbids a 2-byte
> watchpoint on a byte-aligned address.

Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.

Not that I know anything about this code, but it does start to look like 
it might want rewriting without the offending switch statement anyway. 
At a glance, it looks like the intended semantic might boil down to:

	if (hw->ctrl.len > offset)
		return -EINVAL;

Robin.

> I think the arm64 code matches what we had on 32-bit prior to
> d968d2b801d8 ("ARM: 7497/1: hw_breakpoint: allow single-byte watchpoints
> on all addresses"), so we should have one patch bringing us up to speed
> with that change, and then another annotating the fallthroughs.
> 
> Will
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
@ 2019-07-26 12:38         ` Robin Murphy
  0 siblings, 0 replies; 15+ messages in thread
From: Robin Murphy @ 2019-07-26 12:38 UTC (permalink / raw)
  To: Will Deacon, Mark Rutland
  Cc: Anders Roxell, Kees Cook, Gustavo A. R. Silva, catalin.marinas,
	linux-kernel, stable, linux-arm-kernel

On 26/07/2019 13:27, Will Deacon wrote:
> On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
>> On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
>>> On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
>>>> When fall-through warnings was enabled by default, commit d93512ef0f0e
>>>> ("Makefile: Globally enable fall-through warning"), the following
>>>> warnings was starting to show up:
>>>>
>>>> ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
>>>> ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
>>>>   through [-Wimplicit-fallthrough=]
>>>>      if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
>>>>         ^
>>>> ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
>>>>     case 2:
>>>>     ^~~~
>>>> ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
>>>>   through [-Wimplicit-fallthrough=]
>>>>      if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
>>>>         ^
>>>> ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
>>>>     default:
>>>>     ^~~~~~~
>>>>
>>>> Rework so that the compiler doesn't warn about fall-through. Rework so
>>>> the code looks like the arm code. Since the comment in the function
>>>> indicates taht this is supposed to behave the same way as arm32 because
>>>
>>> Typo: s/taht/that/
>>>
>>>> it handles 32-bit tasks also.
>>>>
>>>> Cc: stable@vger.kernel.org # v3.16+
>>>> Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
>>>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>>>
>>> The patch itself looks fine, but I don't think this needs a CC to
>>> stable, nor does it require that fixes tag, as there's no functional
>>> problem.
>>
>> Hmm... I now see I spoke too soon, and this is making the 1-byte
>> breakpoint work at a 3-byte offset.
> 
> I still don't think it's quite right though, since it forbids a 2-byte
> watchpoint on a byte-aligned address.

Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.

Not that I know anything about this code, but it does start to look like 
it might want rewriting without the offending switch statement anyway. 
At a glance, it looks like the intended semantic might boil down to:

	if (hw->ctrl.len > offset)
		return -EINVAL;

Robin.

> I think the arm64 code matches what we had on 32-bit prior to
> d968d2b801d8 ("ARM: 7497/1: hw_breakpoint: allow single-byte watchpoints
> on all addresses"), so we should have one patch bringing us up to speed
> with that change, and then another annotating the fallthroughs.
> 
> Will
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
  2019-07-26 12:38         ` Robin Murphy
@ 2019-07-26 13:05           ` Will Deacon
  -1 siblings, 0 replies; 15+ messages in thread
From: Will Deacon @ 2019-07-26 13:05 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Mark Rutland, Anders Roxell, Kees Cook, Gustavo A. R. Silva,
	catalin.marinas, linux-kernel, stable, linux-arm-kernel

On Fri, Jul 26, 2019 at 01:38:24PM +0100, Robin Murphy wrote:
> On 26/07/2019 13:27, Will Deacon wrote:
> > On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
> > > On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
> > > > On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
> > > > > When fall-through warnings was enabled by default, commit d93512ef0f0e
> > > > > ("Makefile: Globally enable fall-through warning"), the following
> > > > > warnings was starting to show up:
> > > > > 
> > > > > ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
> > > > > ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
> > > > >   through [-Wimplicit-fallthrough=]
> > > > >      if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> > > > >         ^
> > > > > ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
> > > > >     case 2:
> > > > >     ^~~~
> > > > > ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
> > > > >   through [-Wimplicit-fallthrough=]
> > > > >      if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
> > > > >         ^
> > > > > ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
> > > > >     default:
> > > > >     ^~~~~~~
> > > > > 
> > > > > Rework so that the compiler doesn't warn about fall-through. Rework so
> > > > > the code looks like the arm code. Since the comment in the function
> > > > > indicates taht this is supposed to behave the same way as arm32 because
> > > > 
> > > > Typo: s/taht/that/
> > > > 
> > > > > it handles 32-bit tasks also.
> > > > > 
> > > > > Cc: stable@vger.kernel.org # v3.16+
> > > > > Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
> > > > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > > > 
> > > > The patch itself looks fine, but I don't think this needs a CC to
> > > > stable, nor does it require that fixes tag, as there's no functional
> > > > problem.
> > > 
> > > Hmm... I now see I spoke too soon, and this is making the 1-byte
> > > breakpoint work at a 3-byte offset.
> > 
> > I still don't think it's quite right though, since it forbids a 2-byte
> > watchpoint on a byte-aligned address.
> 
> Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.
> 
> Not that I know anything about this code, but it does start to look like it
> might want rewriting without the offending switch statement anyway. At a
> glance, it looks like the intended semantic might boil down to:
> 
> 	if (hw->ctrl.len > offset)
> 		return -EINVAL;

Given that it's compat code, I think it's worth staying as close to the
arch/arm/ implementation as we can. Also, beware that the
ARM_BREAKPOINT_LEN_* definitions are masks because of the BAS fields in
the debug architecture.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
@ 2019-07-26 13:05           ` Will Deacon
  0 siblings, 0 replies; 15+ messages in thread
From: Will Deacon @ 2019-07-26 13:05 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Mark Rutland, Anders Roxell, Kees Cook, Gustavo A. R. Silva,
	catalin.marinas, linux-kernel, stable, linux-arm-kernel

On Fri, Jul 26, 2019 at 01:38:24PM +0100, Robin Murphy wrote:
> On 26/07/2019 13:27, Will Deacon wrote:
> > On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
> > > On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
> > > > On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
> > > > > When fall-through warnings was enabled by default, commit d93512ef0f0e
> > > > > ("Makefile: Globally enable fall-through warning"), the following
> > > > > warnings was starting to show up:
> > > > > 
> > > > > ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
> > > > > ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
> > > > >   through [-Wimplicit-fallthrough=]
> > > > >      if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
> > > > >         ^
> > > > > ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
> > > > >     case 2:
> > > > >     ^~~~
> > > > > ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
> > > > >   through [-Wimplicit-fallthrough=]
> > > > >      if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
> > > > >         ^
> > > > > ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
> > > > >     default:
> > > > >     ^~~~~~~
> > > > > 
> > > > > Rework so that the compiler doesn't warn about fall-through. Rework so
> > > > > the code looks like the arm code. Since the comment in the function
> > > > > indicates taht this is supposed to behave the same way as arm32 because
> > > > 
> > > > Typo: s/taht/that/
> > > > 
> > > > > it handles 32-bit tasks also.
> > > > > 
> > > > > Cc: stable@vger.kernel.org # v3.16+
> > > > > Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
> > > > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> > > > 
> > > > The patch itself looks fine, but I don't think this needs a CC to
> > > > stable, nor does it require that fixes tag, as there's no functional
> > > > problem.
> > > 
> > > Hmm... I now see I spoke too soon, and this is making the 1-byte
> > > breakpoint work at a 3-byte offset.
> > 
> > I still don't think it's quite right though, since it forbids a 2-byte
> > watchpoint on a byte-aligned address.
> 
> Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.
> 
> Not that I know anything about this code, but it does start to look like it
> might want rewriting without the offending switch statement anyway. At a
> glance, it looks like the intended semantic might boil down to:
> 
> 	if (hw->ctrl.len > offset)
> 		return -EINVAL;

Given that it's compat code, I think it's worth staying as close to the
arch/arm/ implementation as we can. Also, beware that the
ARM_BREAKPOINT_LEN_* definitions are masks because of the BAS fields in
the debug architecture.

Will

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
  2019-07-26 13:05           ` Will Deacon
@ 2019-07-26 13:28             ` Robin Murphy
  -1 siblings, 0 replies; 15+ messages in thread
From: Robin Murphy @ 2019-07-26 13:28 UTC (permalink / raw)
  To: Will Deacon
  Cc: Mark Rutland, Anders Roxell, Kees Cook, Gustavo A. R. Silva,
	catalin.marinas, linux-kernel, stable, linux-arm-kernel

On 26/07/2019 14:05, Will Deacon wrote:
> On Fri, Jul 26, 2019 at 01:38:24PM +0100, Robin Murphy wrote:
>> On 26/07/2019 13:27, Will Deacon wrote:
>>> On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
>>>> On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
>>>>> On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
>>>>>> When fall-through warnings was enabled by default, commit d93512ef0f0e
>>>>>> ("Makefile: Globally enable fall-through warning"), the following
>>>>>> warnings was starting to show up:
>>>>>>
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
>>>>>>    through [-Wimplicit-fallthrough=]
>>>>>>       if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
>>>>>>          ^
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
>>>>>>      case 2:
>>>>>>      ^~~~
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
>>>>>>    through [-Wimplicit-fallthrough=]
>>>>>>       if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
>>>>>>          ^
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
>>>>>>      default:
>>>>>>      ^~~~~~~
>>>>>>
>>>>>> Rework so that the compiler doesn't warn about fall-through. Rework so
>>>>>> the code looks like the arm code. Since the comment in the function
>>>>>> indicates taht this is supposed to behave the same way as arm32 because
>>>>>
>>>>> Typo: s/taht/that/
>>>>>
>>>>>> it handles 32-bit tasks also.
>>>>>>
>>>>>> Cc: stable@vger.kernel.org # v3.16+
>>>>>> Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
>>>>>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>>>>>
>>>>> The patch itself looks fine, but I don't think this needs a CC to
>>>>> stable, nor does it require that fixes tag, as there's no functional
>>>>> problem.
>>>>
>>>> Hmm... I now see I spoke too soon, and this is making the 1-byte
>>>> breakpoint work at a 3-byte offset.
>>>
>>> I still don't think it's quite right though, since it forbids a 2-byte
>>> watchpoint on a byte-aligned address.
>>
>> Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.

[and of course, what I missed was that that's the case the fallthrough 
serves... yuck indeed]

>> Not that I know anything about this code, but it does start to look like it
>> might want rewriting without the offending switch statement anyway. At a
>> glance, it looks like the intended semantic might boil down to:
>>
>> 	if (hw->ctrl.len > offset)
>> 		return -EINVAL;
> 
> Given that it's compat code, I think it's worth staying as close to the
> arch/arm/ implementation as we can.

Right, I also misread the accompanying arch/arm/ patch and got the 
impression that 32-bit also had a problem such that any fix would happen 
in parallel - on closer inspection the current arch/arm/ code does 
actually seem to make sense, even if it is horribly subtle.

> Also, beware that the
> ARM_BREAKPOINT_LEN_* definitions are masks because of the BAS fields in
> the debug architecture.

Fun... Clearly it's a bit too Friday for me to be useful here, so 
apologies for the confusion :)

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
@ 2019-07-26 13:28             ` Robin Murphy
  0 siblings, 0 replies; 15+ messages in thread
From: Robin Murphy @ 2019-07-26 13:28 UTC (permalink / raw)
  To: Will Deacon
  Cc: Mark Rutland, Anders Roxell, Kees Cook, Gustavo A. R. Silva,
	catalin.marinas, linux-kernel, stable, linux-arm-kernel

On 26/07/2019 14:05, Will Deacon wrote:
> On Fri, Jul 26, 2019 at 01:38:24PM +0100, Robin Murphy wrote:
>> On 26/07/2019 13:27, Will Deacon wrote:
>>> On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
>>>> On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
>>>>> On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
>>>>>> When fall-through warnings was enabled by default, commit d93512ef0f0e
>>>>>> ("Makefile: Globally enable fall-through warning"), the following
>>>>>> warnings was starting to show up:
>>>>>>
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’:
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall
>>>>>>    through [-Wimplicit-fallthrough=]
>>>>>>       if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)
>>>>>>          ^
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here
>>>>>>      case 2:
>>>>>>      ^~~~
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall
>>>>>>    through [-Wimplicit-fallthrough=]
>>>>>>       if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2)
>>>>>>          ^
>>>>>> ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here
>>>>>>      default:
>>>>>>      ^~~~~~~
>>>>>>
>>>>>> Rework so that the compiler doesn't warn about fall-through. Rework so
>>>>>> the code looks like the arm code. Since the comment in the function
>>>>>> indicates taht this is supposed to behave the same way as arm32 because
>>>>>
>>>>> Typo: s/taht/that/
>>>>>
>>>>>> it handles 32-bit tasks also.
>>>>>>
>>>>>> Cc: stable@vger.kernel.org # v3.16+
>>>>>> Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code")
>>>>>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
>>>>>
>>>>> The patch itself looks fine, but I don't think this needs a CC to
>>>>> stable, nor does it require that fixes tag, as there's no functional
>>>>> problem.
>>>>
>>>> Hmm... I now see I spoke too soon, and this is making the 1-byte
>>>> breakpoint work at a 3-byte offset.
>>>
>>> I still don't think it's quite right though, since it forbids a 2-byte
>>> watchpoint on a byte-aligned address.
>>
>> Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.

[and of course, what I missed was that that's the case the fallthrough 
serves... yuck indeed]

>> Not that I know anything about this code, but it does start to look like it
>> might want rewriting without the offending switch statement anyway. At a
>> glance, it looks like the intended semantic might boil down to:
>>
>> 	if (hw->ctrl.len > offset)
>> 		return -EINVAL;
> 
> Given that it's compat code, I think it's worth staying as close to the
> arch/arm/ implementation as we can.

Right, I also misread the accompanying arch/arm/ patch and got the 
impression that 32-bit also had a problem such that any fix would happen 
in parallel - on closer inspection the current arch/arm/ code does 
actually seem to make sense, even if it is horribly subtle.

> Also, beware that the
> ARM_BREAKPOINT_LEN_* definitions are masks because of the BAS fields in
> the debug architecture.

Fun... Clearly it's a bit too Friday for me to be useful here, so 
apologies for the confusion :)

Robin.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/3] arm64: perf: Mark expected switch fall-through
  2019-07-26 11:27 ` Anders Roxell
  (?)
  (?)
@ 2019-07-26 14:17 ` Sasha Levin
  -1 siblings, 0 replies; 15+ messages in thread
From: Sasha Levin @ 2019-07-26 14:17 UTC (permalink / raw)
  To: Sasha Levin, Anders Roxell, will, mark.rutland, catalin.marinas
  Cc: stable, linux-kernel, linux-arm-kernel

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 6ee33c2712fc ARM: hw_breakpoint: correct and simplify alignment fixup code.

The bot has tested the following trees: v5.2.2, v5.1.19, v4.19.60, v4.14.134, v4.9.186, v4.4.186.

v5.2.2: Build OK!
v5.1.19: Build OK!
v4.19.60: Build OK!
v4.14.134: Failed to apply! Possible dependencies:
    18ff57b22061 ("hw_breakpoint: Factor out __modify_user_hw_breakpoint() function")
    195bce73bd10 ("signal/sh: Use force_sig_fault in hw_breakpoint_handler")
    705feaf321c3 ("hw_breakpoint: Add perf_event_attr fields check in __modify_user_hw_breakpoint()")
    8c449753a681 ("perf/arch/arm64: Implement hw_breakpoint_arch_parse()")
    8e983ff9ac02 ("perf/hw_breakpoint: Pass arch breakpoint struct to arch_check_bp_in_kernelspace()")
    9a4903dde2c8 ("perf/hw_breakpoint: Split attribute parse and commit")
    ea6a9d530c17 ("hw_breakpoint: Add modify_bp_slot() function")

v4.9.186: Failed to apply! Possible dependencies:
    0ddb8e0b784b ("arm64: Allow hw watchpoint of length 3,5,6 and 7")
    18ff57b22061 ("hw_breakpoint: Factor out __modify_user_hw_breakpoint() function")
    195bce73bd10 ("signal/sh: Use force_sig_fault in hw_breakpoint_handler")
    705feaf321c3 ("hw_breakpoint: Add perf_event_attr fields check in __modify_user_hw_breakpoint()")
    8c449753a681 ("perf/arch/arm64: Implement hw_breakpoint_arch_parse()")
    8e983ff9ac02 ("perf/hw_breakpoint: Pass arch breakpoint struct to arch_check_bp_in_kernelspace()")
    9a4903dde2c8 ("perf/hw_breakpoint: Split attribute parse and commit")
    b08fb180bb88 ("arm64: Allow hw watchpoint at varied offset from base address")
    ea6a9d530c17 ("hw_breakpoint: Add modify_bp_slot() function")

v4.4.186: Failed to apply! Possible dependencies:
    0ddb8e0b784b ("arm64: Allow hw watchpoint of length 3,5,6 and 7")
    18ff57b22061 ("hw_breakpoint: Factor out __modify_user_hw_breakpoint() function")
    195bce73bd10 ("signal/sh: Use force_sig_fault in hw_breakpoint_handler")
    6ec7026ac01f ("xtensa: use context structure for debug exceptions")
    705feaf321c3 ("hw_breakpoint: Add perf_event_attr fields check in __modify_user_hw_breakpoint()")
    8c449753a681 ("perf/arch/arm64: Implement hw_breakpoint_arch_parse()")
    8e983ff9ac02 ("perf/hw_breakpoint: Pass arch breakpoint struct to arch_check_bp_in_kernelspace()")
    9a4903dde2c8 ("perf/hw_breakpoint: Split attribute parse and commit")
    b08fb180bb88 ("arm64: Allow hw watchpoint at varied offset from base address")
    c91e02bd9702 ("xtensa: support hardware breakpoints/watchpoints")
    ea6a9d530c17 ("hw_breakpoint: Add modify_bp_slot() function")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

--
Thanks,
Sasha

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-07-26 14:17 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-26 11:27 [PATCH 1/3] arm64: perf: Mark expected switch fall-through Anders Roxell
2019-07-26 11:27 ` Anders Roxell
2019-07-26 12:10 ` Mark Rutland
2019-07-26 12:10   ` Mark Rutland
2019-07-26 12:13   ` Mark Rutland
2019-07-26 12:13     ` Mark Rutland
2019-07-26 12:27     ` Will Deacon
2019-07-26 12:27       ` Will Deacon
2019-07-26 12:38       ` Robin Murphy
2019-07-26 12:38         ` Robin Murphy
2019-07-26 13:05         ` Will Deacon
2019-07-26 13:05           ` Will Deacon
2019-07-26 13:28           ` Robin Murphy
2019-07-26 13:28             ` Robin Murphy
2019-07-26 14:17 ` Sasha Levin

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.