[PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker]

arch_validate_hwbkpt_settings() mixes up attribute check and commit into
a single code entity. Therefore the validation may return an error due to
incorrect atributes while still leaving halfway modified architecture
breakpoint data.

This is harmless when we deal with a new breakpoint but it becomes a
problem when we modify an existing breakpoint.

Split attribute parse and commit to fix that. The architecture is
passed a "struct arch_hw_breakpoint" to fill on top of the new attr
and the core takes care about copying the backend data once it's fully
validated. The architectures then need to implement the new API.

Reported-by: Linus Torvalds 
Original-patch-by: Andy Lutomirski 
Signed-off-by: Frederic Weisbecker 
Cc: Linus Torvalds 
Cc: Andy Lutomirski 
Cc: Yoshinori Sato 
Cc: Rich Felker 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Will Deacon 
Cc: Mark Rutland 
Cc: Max Filippov 
Cc: Chris Zankel 
Cc: Catalin Marinas 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Peter Zijlstra 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Joel Fernandes 
---
 kernel/events/hw_breakpoint.c | 57 +++
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6e28d28..314e2a9 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -400,16 +400,35 @@ int dbg_release_bp_slot(struct perf_event *bp)
return 0;
 }
 
-static int validate_hw_breakpoint(struct perf_event *bp)
+#ifndef hw_breakpoint_arch_parse
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+const struct perf_event_attr *attr,
+struct arch_hw_breakpoint *hw)
 {
-   int ret;
+   int err;
 
-   ret = arch_validate_hwbkpt_settings(bp);
-   if (ret)
-   return ret;
+   err = arch_validate_hwbkpt_settings(bp);
+   if (err)
+   return err;
+
+   *hw = bp->hw.info;
+
+   return 0;
+}
+#endif
+
+static int hw_breakpoint_parse(struct perf_event *bp,
+  const struct perf_event_attr *attr,
+  struct arch_hw_breakpoint *hw)
+{
+   int err;
+
+   err = hw_breakpoint_arch_parse(bp, attr, hw);
+   if (err)
+   return err;
 
if (arch_check_bp_in_kernelspace(bp)) {
-   if (bp->attr.exclude_kernel)
+   if (attr->exclude_kernel)
return -EINVAL;
/*
 * Don't let unprivileged users set a breakpoint in the trap
@@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 
 int register_perf_hw_breakpoint(struct perf_event *bp)
 {
-   int ret;
+   struct arch_hw_breakpoint hw;
+   int err;
 
-   ret = reserve_bp_slot(bp);
-   if (ret)
-   return ret;
+   err = reserve_bp_slot(bp);
+   if (err)
+   return err;
 
-   ret = validate_hw_breakpoint(bp);
-
-   /* if arch_validate_hwbkpt_settings() fails then release bp slot */
-   if (ret)
+   err = hw_breakpoint_parse(bp, >attr, );
+   if (err) {
release_bp_slot(bp);
+   return err;
+   }
 
-   return ret;
+   bp->hw.info = hw;
+
+   return 0;
 }
 
 /**
@@ -464,6 +486,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
u64 old_len  = bp->attr.bp_len;
int old_type = bp->attr.bp_type;
bool modify  = attr->bp_type != old_type;
+   struct arch_hw_breakpoint hw;
int err = 0;
 
bp->attr.bp_addr = attr->bp_addr;
@@ -473,7 +496,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
if (check && memcmp(>attr, attr, sizeof(*attr)))
return -EINVAL;
 
-   err = validate_hw_breakpoint(bp);
+   err = hw_breakpoint_parse(bp, attr, );
if (!err && modify)
err = modify_bp_slot(bp, old_type);
 
@@ -484,7 +507,9 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
return err;
}
 
+   bp->hw.info = hw;
bp->attr.disabled = attr->disabled;
+
return 0;
 }
 
-- 
2.7.4

[PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker]

arch_validate_hwbkpt_settings() mixes up attribute check and commit into
a single code entity. Therefore the validation may return an error due to
incorrect atributes while still leaving halfway modified architecture
breakpoint data.

This is harmless when we deal with a new breakpoint but it becomes a
problem when we modify an existing breakpoint.

Split attribute parse and commit to fix that. The architecture is
passed a "struct arch_hw_breakpoint" to fill on top of the new attr
and the core takes care about copying the backend data once it's fully
validated. The architectures then need to implement the new API.

Reported-by: Linus Torvalds 
Original-patch-by: Andy Lutomirski 
Signed-off-by: Frederic Weisbecker 
Cc: Linus Torvalds 
Cc: Andy Lutomirski 
Cc: Yoshinori Sato 
Cc: Rich Felker 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Will Deacon 
Cc: Mark Rutland 
Cc: Max Filippov 
Cc: Chris Zankel 
Cc: Catalin Marinas 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Peter Zijlstra 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Joel Fernandes 
---
 kernel/events/hw_breakpoint.c | 57 +++
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6e28d28..314e2a9 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -400,16 +400,35 @@ int dbg_release_bp_slot(struct perf_event *bp)
return 0;
 }
 
-static int validate_hw_breakpoint(struct perf_event *bp)
+#ifndef hw_breakpoint_arch_parse
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+const struct perf_event_attr *attr,
+struct arch_hw_breakpoint *hw)
 {
-   int ret;
+   int err;
 
-   ret = arch_validate_hwbkpt_settings(bp);
-   if (ret)
-   return ret;
+   err = arch_validate_hwbkpt_settings(bp);
+   if (err)
+   return err;
+
+   *hw = bp->hw.info;
+
+   return 0;
+}
+#endif
+
+static int hw_breakpoint_parse(struct perf_event *bp,
+  const struct perf_event_attr *attr,
+  struct arch_hw_breakpoint *hw)
+{
+   int err;
+
+   err = hw_breakpoint_arch_parse(bp, attr, hw);
+   if (err)
+   return err;
 
if (arch_check_bp_in_kernelspace(bp)) {
-   if (bp->attr.exclude_kernel)
+   if (attr->exclude_kernel)
return -EINVAL;
/*
 * Don't let unprivileged users set a breakpoint in the trap
@@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 
 int register_perf_hw_breakpoint(struct perf_event *bp)
 {
-   int ret;
+   struct arch_hw_breakpoint hw;
+   int err;
 
-   ret = reserve_bp_slot(bp);
-   if (ret)
-   return ret;
+   err = reserve_bp_slot(bp);
+   if (err)
+   return err;
 
-   ret = validate_hw_breakpoint(bp);
-
-   /* if arch_validate_hwbkpt_settings() fails then release bp slot */
-   if (ret)
+   err = hw_breakpoint_parse(bp, >attr, );
+   if (err) {
release_bp_slot(bp);
+   return err;
+   }
 
-   return ret;
+   bp->hw.info = hw;
+
+   return 0;
 }
 
 /**
@@ -464,6 +486,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
u64 old_len  = bp->attr.bp_len;
int old_type = bp->attr.bp_type;
bool modify  = attr->bp_type != old_type;
+   struct arch_hw_breakpoint hw;
int err = 0;
 
bp->attr.bp_addr = attr->bp_addr;
@@ -473,7 +496,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
if (check && memcmp(>attr, attr, sizeof(*attr)))
return -EINVAL;
 
-   err = validate_hw_breakpoint(bp);
+   err = hw_breakpoint_parse(bp, attr, );
if (!err && modify)
err = modify_bp_slot(bp, old_type);
 
@@ -484,7 +507,9 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
return err;
}
 
+   bp->hw.info = hw;
bp->attr.disabled = attr->disabled;
+
return 0;
 }
 
-- 
2.7.4

[PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker]

arch_validate_hwbkpt_settings() mixes up attribute check and commit into
a single code entity. Therefore the validation may return an error due to
incorrect atributes while still leaving halfway modified architecture
breakpoint data.

This is harmless when we deal with a new breakpoint but it becomes a
problem when we modify an existing breakpoint.

Split attribute parse and commit to fix that. The architecture is
passed a "struct arch_hw_breakpoint" to fill on top of the new attr
and the core takes care about copying the backend data once it's fully
validated. The architectures then need to implement the new API.

Reported-by: Linus Torvalds 
Original-patch-by: Andy Lutomirski 
Signed-off-by: Frederic Weisbecker 
Cc: Linus Torvalds 
Cc: Andy Lutomirski 
Cc: Yoshinori Sato 
Cc: Rich Felker 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Will Deacon 
Cc: Mark Rutland 
Cc: Max Filippov 
Cc: Chris Zankel 
Cc: Catalin Marinas 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Peter Zijlstra 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Joel Fernandes 
---
 kernel/events/hw_breakpoint.c | 57 +++
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6e28d28..314e2a9 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -400,16 +400,35 @@ int dbg_release_bp_slot(struct perf_event *bp)
return 0;
 }
 
-static int validate_hw_breakpoint(struct perf_event *bp)
+#ifndef hw_breakpoint_arch_parse
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+const struct perf_event_attr *attr,
+struct arch_hw_breakpoint *hw)
 {
-   int ret;
+   int err;
 
-   ret = arch_validate_hwbkpt_settings(bp);
-   if (ret)
-   return ret;
+   err = arch_validate_hwbkpt_settings(bp);
+   if (err)
+   return err;
+
+   *hw = bp->hw.info;
+
+   return 0;
+}
+#endif
+
+static int hw_breakpoint_parse(struct perf_event *bp,
+  const struct perf_event_attr *attr,
+  struct arch_hw_breakpoint *hw)
+{
+   int err;
+
+   err = hw_breakpoint_arch_parse(bp, attr, hw);
+   if (err)
+   return err;
 
if (arch_check_bp_in_kernelspace(bp)) {
-   if (bp->attr.exclude_kernel)
+   if (attr->exclude_kernel)
return -EINVAL;
/*
 * Don't let unprivileged users set a breakpoint in the trap
@@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 
 int register_perf_hw_breakpoint(struct perf_event *bp)
 {
-   int ret;
+   struct arch_hw_breakpoint hw;
+   int err;
 
-   ret = reserve_bp_slot(bp);
-   if (ret)
-   return ret;
+   err = reserve_bp_slot(bp);
+   if (err)
+   return err;
 
-   ret = validate_hw_breakpoint(bp);
-
-   /* if arch_validate_hwbkpt_settings() fails then release bp slot */
-   if (ret)
+   err = hw_breakpoint_parse(bp, >attr, );
+   if (err) {
release_bp_slot(bp);
+   return err;
+   }
 
-   return ret;
+   bp->hw.info = hw;
+
+   return 0;
 }
 
 /**
@@ -464,6 +486,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
u64 old_len  = bp->attr.bp_len;
int old_type = bp->attr.bp_type;
bool modify  = attr->bp_type != old_type;
+   struct arch_hw_breakpoint hw;
int err = 0;
 
bp->attr.bp_addr = attr->bp_addr;
@@ -473,7 +496,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
if (check && memcmp(>attr, attr, sizeof(*attr)))
return -EINVAL;
 
-   err = validate_hw_breakpoint(bp);
+   err = hw_breakpoint_parse(bp, attr, );
if (!err && modify)
err = modify_bp_slot(bp, old_type);
 
@@ -484,7 +507,9 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
return err;
}
 
+   bp->hw.info = hw;
bp->attr.disabled = attr->disabled;
+
return 0;
 }
 
-- 
2.7.4

[PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker]

arch_validate_hwbkpt_settings() mixes up attribute check and commit into
a single code entity. Therefore the validation may return an error due to
incorrect atributes while still leaving halfway modified architecture
breakpoint data.

This is harmless when we deal with a new breakpoint but it becomes a
problem when we modify an existing breakpoint.

Split attribute parse and commit to fix that. The architecture is
passed a "struct arch_hw_breakpoint" to fill on top of the new attr
and the core takes care about copying the backend data once it's fully
validated. The architectures then need to implement the new API.

Reported-by: Linus Torvalds 
Original-patch-by: Andy Lutomirski 
Signed-off-by: Frederic Weisbecker 
Cc: Linus Torvalds 
Cc: Andy Lutomirski 
Cc: Yoshinori Sato 
Cc: Rich Felker 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Will Deacon 
Cc: Mark Rutland 
Cc: Max Filippov 
Cc: Chris Zankel 
Cc: Catalin Marinas 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Peter Zijlstra 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Joel Fernandes 
---
 kernel/events/hw_breakpoint.c | 57 +++
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6e28d28..314e2a9 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -400,16 +400,35 @@ int dbg_release_bp_slot(struct perf_event *bp)
return 0;
 }
 
-static int validate_hw_breakpoint(struct perf_event *bp)
+#ifndef hw_breakpoint_arch_parse
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+const struct perf_event_attr *attr,
+struct arch_hw_breakpoint *hw)
 {
-   int ret;
+   int err;
 
-   ret = arch_validate_hwbkpt_settings(bp);
-   if (ret)
-   return ret;
+   err = arch_validate_hwbkpt_settings(bp);
+   if (err)
+   return err;
+
+   *hw = bp->hw.info;
+
+   return 0;
+}
+#endif
+
+static int hw_breakpoint_parse(struct perf_event *bp,
+  const struct perf_event_attr *attr,
+  struct arch_hw_breakpoint *hw)
+{
+   int err;
+
+   err = hw_breakpoint_arch_parse(bp, attr, hw);
+   if (err)
+   return err;
 
if (arch_check_bp_in_kernelspace(bp)) {
-   if (bp->attr.exclude_kernel)
+   if (attr->exclude_kernel)
return -EINVAL;
/*
 * Don't let unprivileged users set a breakpoint in the trap
@@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 
 int register_perf_hw_breakpoint(struct perf_event *bp)
 {
-   int ret;
+   struct arch_hw_breakpoint hw;
+   int err;
 
-   ret = reserve_bp_slot(bp);
-   if (ret)
-   return ret;
+   err = reserve_bp_slot(bp);
+   if (err)
+   return err;
 
-   ret = validate_hw_breakpoint(bp);
-
-   /* if arch_validate_hwbkpt_settings() fails then release bp slot */
-   if (ret)
+   err = hw_breakpoint_parse(bp, >attr, );
+   if (err) {
release_bp_slot(bp);
+   return err;
+   }
 
-   return ret;
+   bp->hw.info = hw;
+
+   return 0;
 }
 
 /**
@@ -464,6 +486,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
u64 old_len  = bp->attr.bp_len;
int old_type = bp->attr.bp_type;
bool modify  = attr->bp_type != old_type;
+   struct arch_hw_breakpoint hw;
int err = 0;
 
bp->attr.bp_addr = attr->bp_addr;
@@ -473,7 +496,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
if (check && memcmp(>attr, attr, sizeof(*attr)))
return -EINVAL;
 
-   err = validate_hw_breakpoint(bp);
+   err = hw_breakpoint_parse(bp, attr, );
if (!err && modify)
err = modify_bp_slot(bp, old_type);
 
@@ -484,7 +507,9 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
return err;
}
 
+   bp->hw.info = hw;
bp->attr.disabled = attr->disabled;
+
return 0;
 }
 
-- 
2.7.4

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Michael Ellerman]

Frederic Weisbecker  writes:

> On Thu, May 24, 2018 at 11:56:01AM +1000, Michael Ellerman wrote:
>> Frederic Weisbecker  writes:
>> 
>> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
>> > index 6e28d28..51320c2 100644
>> > --- a/kernel/events/hw_breakpoint.c
>> > +++ b/kernel/events/hw_breakpoint.c
>> > @@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event 
>> > *bp)
>> >  
>> >  int register_perf_hw_breakpoint(struct perf_event *bp)
>> >  {
>> > -  int ret;
>> > +  struct arch_hw_breakpoint hw;
>> > +  int err;
>> >  
>> > -  ret = reserve_bp_slot(bp);
>> > -  if (ret)
>> > -  return ret;
>> > +  err = reserve_bp_slot(bp);
>> > +  if (err)
>> > +  return err;
>> >  
>> > -  ret = validate_hw_breakpoint(bp);
>> > -
>> > -  /* if arch_validate_hwbkpt_settings() fails then release bp slot */
>> > -  if (ret)
>> > +  err = hw_breakpoint_parse(bp, >attr, );
>> 
>> Is there a good reason we pass bp and bp->attr? (I assume so)
>> 
>> That added to the confusion in the existing code I think.
>
> Yes, on breakpoint creation (which is the above function) it's not needed
> but breakpoint modification wants it as we need to pass the attr that are
> to be validated, and those are not yet copied to the breakpoint at this
> stage. This happens in the end of the series.

OK thanks.

cheers

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Michael Ellerman]

Frederic Weisbecker  writes:

> On Thu, May 24, 2018 at 11:56:01AM +1000, Michael Ellerman wrote:
>> Frederic Weisbecker  writes:
>> 
>> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
>> > index 6e28d28..51320c2 100644
>> > --- a/kernel/events/hw_breakpoint.c
>> > +++ b/kernel/events/hw_breakpoint.c
>> > @@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event 
>> > *bp)
>> >  
>> >  int register_perf_hw_breakpoint(struct perf_event *bp)
>> >  {
>> > -  int ret;
>> > +  struct arch_hw_breakpoint hw;
>> > +  int err;
>> >  
>> > -  ret = reserve_bp_slot(bp);
>> > -  if (ret)
>> > -  return ret;
>> > +  err = reserve_bp_slot(bp);
>> > +  if (err)
>> > +  return err;
>> >  
>> > -  ret = validate_hw_breakpoint(bp);
>> > -
>> > -  /* if arch_validate_hwbkpt_settings() fails then release bp slot */
>> > -  if (ret)
>> > +  err = hw_breakpoint_parse(bp, >attr, );
>> 
>> Is there a good reason we pass bp and bp->attr? (I assume so)
>> 
>> That added to the confusion in the existing code I think.
>
> Yes, on breakpoint creation (which is the above function) it's not needed
> but breakpoint modification wants it as we need to pass the attr that are
> to be validated, and those are not yet copied to the breakpoint at this
> stage. This happens in the end of the series.

OK thanks.

cheers

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker]

On Thu, May 24, 2018 at 11:56:01AM +1000, Michael Ellerman wrote:
> Frederic Weisbecker  writes:
> 
> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > index 6e28d28..51320c2 100644
> > --- a/kernel/events/hw_breakpoint.c
> > +++ b/kernel/events/hw_breakpoint.c
> > @@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event 
> > *bp)
> >  
> >  int register_perf_hw_breakpoint(struct perf_event *bp)
> >  {
> > -   int ret;
> > +   struct arch_hw_breakpoint hw;
> > +   int err;
> >  
> > -   ret = reserve_bp_slot(bp);
> > -   if (ret)
> > -   return ret;
> > +   err = reserve_bp_slot(bp);
> > +   if (err)
> > +   return err;
> >  
> > -   ret = validate_hw_breakpoint(bp);
> > -
> > -   /* if arch_validate_hwbkpt_settings() fails then release bp slot */
> > -   if (ret)
> > +   err = hw_breakpoint_parse(bp, >attr, );
> 
> Is there a good reason we pass bp and bp->attr? (I assume so)
> 
> That added to the confusion in the existing code I think.

Yes, on breakpoint creation (which is the above function) it's not needed
but breakpoint modification wants it as we need to pass the attr that are
to be validated, and those are not yet copied to the breakpoint at this
stage. This happens in the end of the series.

Thanks.

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker]

On Thu, May 24, 2018 at 11:56:01AM +1000, Michael Ellerman wrote:
> Frederic Weisbecker  writes:
> 
> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > index 6e28d28..51320c2 100644
> > --- a/kernel/events/hw_breakpoint.c
> > +++ b/kernel/events/hw_breakpoint.c
> > @@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event 
> > *bp)
> >  
> >  int register_perf_hw_breakpoint(struct perf_event *bp)
> >  {
> > -   int ret;
> > +   struct arch_hw_breakpoint hw;
> > +   int err;
> >  
> > -   ret = reserve_bp_slot(bp);
> > -   if (ret)
> > -   return ret;
> > +   err = reserve_bp_slot(bp);
> > +   if (err)
> > +   return err;
> >  
> > -   ret = validate_hw_breakpoint(bp);
> > -
> > -   /* if arch_validate_hwbkpt_settings() fails then release bp slot */
> > -   if (ret)
> > +   err = hw_breakpoint_parse(bp, >attr, );
> 
> Is there a good reason we pass bp and bp->attr? (I assume so)
> 
> That added to the confusion in the existing code I think.

Yes, on breakpoint creation (which is the above function) it's not needed
but breakpoint modification wants it as we need to pass the attr that are
to be validated, and those are not yet copied to the breakpoint at this
stage. This happens in the end of the series.

Thanks.

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Michael Ellerman]

Frederic Weisbecker  writes:

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 6e28d28..51320c2 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
>  
>  int register_perf_hw_breakpoint(struct perf_event *bp)
>  {
> - int ret;
> + struct arch_hw_breakpoint hw;
> + int err;
>  
> - ret = reserve_bp_slot(bp);
> - if (ret)
> - return ret;
> + err = reserve_bp_slot(bp);
> + if (err)
> + return err;
>  
> - ret = validate_hw_breakpoint(bp);
> -
> - /* if arch_validate_hwbkpt_settings() fails then release bp slot */
> - if (ret)
> + err = hw_breakpoint_parse(bp, >attr, );

Is there a good reason we pass bp and bp->attr? (I assume so)

That added to the confusion in the existing code I think.

cheers

Re: [PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Michael Ellerman]

Frederic Weisbecker  writes:

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 6e28d28..51320c2 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
>  
>  int register_perf_hw_breakpoint(struct perf_event *bp)
>  {
> - int ret;
> + struct arch_hw_breakpoint hw;
> + int err;
>  
> - ret = reserve_bp_slot(bp);
> - if (ret)
> - return ret;
> + err = reserve_bp_slot(bp);
> + if (err)
> + return err;
>  
> - ret = validate_hw_breakpoint(bp);
> -
> - /* if arch_validate_hwbkpt_settings() fails then release bp slot */
> - if (ret)
> + err = hw_breakpoint_parse(bp, >attr, );

Is there a good reason we pass bp and bp->attr? (I assume so)

That added to the confusion in the existing code I think.

cheers

[PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker]

arch_validate_hwbkpt_settings() mixes up attribute check and commit into
a single code entity. Therefore the validation may return an error due to
incorrect atributes while still leaving halfway modified architecture
breakpoint data.

This is harmless when we deal with a new breakpoint but it becomes a
problem when we modify an existing breakpoint.

Split attribute parse and commit to fix that. The architecture is
passed a "struct arch_hw_breakpoint" to fill on top of the new attr
and the core takes care about copying the backend data once it's fully
validated. The architectures then need to implement the new API.

Reported-by: Linus Torvalds 
Original-patch-by: Andy Lutomirski 
Signed-off-by: Frederic Weisbecker 
Cc: Linus Torvalds 
Cc: Andy Lutomirski 
Cc: Yoshinori Sato 
Cc: Rich Felker 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Will Deacon 
Cc: Mark Rutland 
Cc: Max Filippov 
Cc: Chris Zankel 
Cc: Catalin Marinas 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Peter Zijlstra 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Joel Fernandes 
---
 kernel/events/hw_breakpoint.c | 57 +++
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6e28d28..51320c2 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -400,16 +400,35 @@ int dbg_release_bp_slot(struct perf_event *bp)
return 0;
 }
 
-static int validate_hw_breakpoint(struct perf_event *bp)
+#ifndef hw_breakpoint_arch_parse
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+struct perf_event_attr *attr,
+struct arch_hw_breakpoint *hw)
 {
-   int ret;
+   int err;
 
-   ret = arch_validate_hwbkpt_settings(bp);
-   if (ret)
-   return ret;
+   err = arch_validate_hwbkpt_settings(bp);
+   if (err)
+   return err;
+
+   *hw = bp->hw.info;
+
+   return 0;
+}
+#endif
+
+static int hw_breakpoint_parse(struct perf_event *bp,
+  struct perf_event_attr *attr,
+  struct arch_hw_breakpoint *hw)
+{
+   int err;
+
+   err = hw_breakpoint_arch_parse(bp, attr, hw);
+   if (err)
+   return err;
 
if (arch_check_bp_in_kernelspace(bp)) {
-   if (bp->attr.exclude_kernel)
+   if (attr->exclude_kernel)
return -EINVAL;
/*
 * Don't let unprivileged users set a breakpoint in the trap
@@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 
 int register_perf_hw_breakpoint(struct perf_event *bp)
 {
-   int ret;
+   struct arch_hw_breakpoint hw;
+   int err;
 
-   ret = reserve_bp_slot(bp);
-   if (ret)
-   return ret;
+   err = reserve_bp_slot(bp);
+   if (err)
+   return err;
 
-   ret = validate_hw_breakpoint(bp);
-
-   /* if arch_validate_hwbkpt_settings() fails then release bp slot */
-   if (ret)
+   err = hw_breakpoint_parse(bp, >attr, );
+   if (err) {
release_bp_slot(bp);
+   return err;
+   }
 
-   return ret;
+   bp->hw.info = hw;
+
+   return 0;
 }
 
 /**
@@ -464,6 +486,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
u64 old_len  = bp->attr.bp_len;
int old_type = bp->attr.bp_type;
bool modify  = attr->bp_type != old_type;
+   struct arch_hw_breakpoint hw;
int err = 0;
 
bp->attr.bp_addr = attr->bp_addr;
@@ -473,7 +496,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
if (check && memcmp(>attr, attr, sizeof(*attr)))
return -EINVAL;
 
-   err = validate_hw_breakpoint(bp);
+   err = hw_breakpoint_parse(bp, attr, );
if (!err && modify)
err = modify_bp_slot(bp, old_type);
 
@@ -484,7 +507,9 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
return err;
}
 
+   bp->hw.info = hw;
bp->attr.disabled = attr->disabled;
+
return 0;
 }
 
-- 
2.7.4

[PATCH 01/12] perf/breakpoint: Split attribute parse and commit

[Frederic Weisbecker]

arch_validate_hwbkpt_settings() mixes up attribute check and commit into
a single code entity. Therefore the validation may return an error due to
incorrect atributes while still leaving halfway modified architecture
breakpoint data.

This is harmless when we deal with a new breakpoint but it becomes a
problem when we modify an existing breakpoint.

Split attribute parse and commit to fix that. The architecture is
passed a "struct arch_hw_breakpoint" to fill on top of the new attr
and the core takes care about copying the backend data once it's fully
validated. The architectures then need to implement the new API.

Reported-by: Linus Torvalds 
Original-patch-by: Andy Lutomirski 
Signed-off-by: Frederic Weisbecker 
Cc: Linus Torvalds 
Cc: Andy Lutomirski 
Cc: Yoshinori Sato 
Cc: Rich Felker 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Will Deacon 
Cc: Mark Rutland 
Cc: Max Filippov 
Cc: Chris Zankel 
Cc: Catalin Marinas 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: Peter Zijlstra 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Joel Fernandes 
---
 kernel/events/hw_breakpoint.c | 57 +++
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 6e28d28..51320c2 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -400,16 +400,35 @@ int dbg_release_bp_slot(struct perf_event *bp)
return 0;
 }
 
-static int validate_hw_breakpoint(struct perf_event *bp)
+#ifndef hw_breakpoint_arch_parse
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+struct perf_event_attr *attr,
+struct arch_hw_breakpoint *hw)
 {
-   int ret;
+   int err;
 
-   ret = arch_validate_hwbkpt_settings(bp);
-   if (ret)
-   return ret;
+   err = arch_validate_hwbkpt_settings(bp);
+   if (err)
+   return err;
+
+   *hw = bp->hw.info;
+
+   return 0;
+}
+#endif
+
+static int hw_breakpoint_parse(struct perf_event *bp,
+  struct perf_event_attr *attr,
+  struct arch_hw_breakpoint *hw)
+{
+   int err;
+
+   err = hw_breakpoint_arch_parse(bp, attr, hw);
+   if (err)
+   return err;
 
if (arch_check_bp_in_kernelspace(bp)) {
-   if (bp->attr.exclude_kernel)
+   if (attr->exclude_kernel)
return -EINVAL;
/*
 * Don't let unprivileged users set a breakpoint in the trap
@@ -424,19 +443,22 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 
 int register_perf_hw_breakpoint(struct perf_event *bp)
 {
-   int ret;
+   struct arch_hw_breakpoint hw;
+   int err;
 
-   ret = reserve_bp_slot(bp);
-   if (ret)
-   return ret;
+   err = reserve_bp_slot(bp);
+   if (err)
+   return err;
 
-   ret = validate_hw_breakpoint(bp);
-
-   /* if arch_validate_hwbkpt_settings() fails then release bp slot */
-   if (ret)
+   err = hw_breakpoint_parse(bp, >attr, );
+   if (err) {
release_bp_slot(bp);
+   return err;
+   }
 
-   return ret;
+   bp->hw.info = hw;
+
+   return 0;
 }
 
 /**
@@ -464,6 +486,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
u64 old_len  = bp->attr.bp_len;
int old_type = bp->attr.bp_type;
bool modify  = attr->bp_type != old_type;
+   struct arch_hw_breakpoint hw;
int err = 0;
 
bp->attr.bp_addr = attr->bp_addr;
@@ -473,7 +496,7 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
if (check && memcmp(>attr, attr, sizeof(*attr)))
return -EINVAL;
 
-   err = validate_hw_breakpoint(bp);
+   err = hw_breakpoint_parse(bp, attr, );
if (!err && modify)
err = modify_bp_slot(bp, old_type);
 
@@ -484,7 +507,9 @@ modify_user_hw_breakpoint_check(struct perf_event *bp, 
struct perf_event_attr *a
return err;
}
 
+   bp->hw.info = hw;
bp->attr.disabled = attr->disabled;
+
return 0;
 }
 
-- 
2.7.4