| Message ID | 20220117145608.6781-7-madvenka@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Reorganize the unwinder and implement stack trace reliability checks | expand |
On Mon, Jan 17, 2022 at 08:56:03AM -0600, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > Rename the arguments to unwind() for better consistency. Also, use the > typedef stack_trace_consume_fn for the consume_entry function as it is > already defined in linux/stacktrace.h. Consistency with...? But otherwise: Reviewed-by: Mark Brown <broonie@kernel.org>
On 2/2/22 12:46, Mark Brown wrote: > On Mon, Jan 17, 2022 at 08:56:03AM -0600, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> Rename the arguments to unwind() for better consistency. Also, use the >> typedef stack_trace_consume_fn for the consume_entry function as it is >> already defined in linux/stacktrace.h. > > Consistency with...? But otherwise: Naming consistency. E.g., the name consume_entry is used in a lot of places. This code used to use fn() instead of consume_entry(). arch_stack_walk() names the argument to consume_entry as cookie. This code calls it data instead of cookie. That is all. It is minor in nature. But I thought I might as well make it conform while I am at it. Madhavan
On Wed, Feb 02, 2022 at 06:34:43PM -0600, Madhavan T. Venkataraman wrote: > On 2/2/22 12:46, Mark Brown wrote: > > On Mon, Jan 17, 2022 at 08:56:03AM -0600, madvenka@linux.microsoft.com wrote: > >> Rename the arguments to unwind() for better consistency. Also, use the > >> typedef stack_trace_consume_fn for the consume_entry function as it is > >> already defined in linux/stacktrace.h. > > Consistency with...? But otherwise: > Naming consistency. E.g., the name consume_entry is used in a lot of places. > This code used to use fn() instead of consume_entry(). arch_stack_walk() > names the argument to consume_entry as cookie. This code calls it data > instead of cookie. That is all. It is minor in nature. But I thought I might > as well make it conform while I am at it. The commit message should probably say some of that then.
On 2/3/22 05:30, Mark Brown wrote: > On Wed, Feb 02, 2022 at 06:34:43PM -0600, Madhavan T. Venkataraman wrote: >> On 2/2/22 12:46, Mark Brown wrote: >>> On Mon, Jan 17, 2022 at 08:56:03AM -0600, madvenka@linux.microsoft.com wrote: > >>>> Rename the arguments to unwind() for better consistency. Also, use the >>>> typedef stack_trace_consume_fn for the consume_entry function as it is >>>> already defined in linux/stacktrace.h. > >>> Consistency with...? But otherwise: > >> Naming consistency. E.g., the name consume_entry is used in a lot of places. >> This code used to use fn() instead of consume_entry(). arch_stack_walk() >> names the argument to consume_entry as cookie. This code calls it data >> instead of cookie. That is all. It is minor in nature. But I thought I might >> as well make it conform while I am at it. > > The commit message should probably say some of that then. OK. Will add that to the commit message in the next version. Madhavan
On Mon, Jan 17, 2022 at 08:56:03AM -0600, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > Rename the arguments to unwind() for better consistency. Also, use the > typedef stack_trace_consume_fn for the consume_entry function as it is > already defined in linux/stacktrace.h. > > Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> How about: | arm64: align with common stracktrace naming | | For historical reasons, the naming of parameters and their types in the arm64 | stacktrace code differs from that used in generic code and other | architectures, even though the types are equivalent. | | For consistency and clarity, use the generic names. Either way: Reviewed-by: Mark Rutland <mark.rutland@arm.com> Mark. > --- > arch/arm64/kernel/stacktrace.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index 1b32e55735aa..f772dac78b11 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -181,12 +181,12 @@ static int notrace unwind_next(struct unwind_state *state) > NOKPROBE_SYMBOL(unwind_next); > > static void notrace unwind(struct unwind_state *state, > - bool (*fn)(void *, unsigned long), void *data) > + stack_trace_consume_fn consume_entry, void *cookie) > { > while (1) { > int ret; > > - if (!fn(data, state->pc)) > + if (!consume_entry(cookie, state->pc)) > break; > ret = unwind_next(state); > if (ret < 0) > -- > 2.25.1 >
On 2/15/22 07:39, Mark Rutland wrote: > On Mon, Jan 17, 2022 at 08:56:03AM -0600, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> Rename the arguments to unwind() for better consistency. Also, use the >> typedef stack_trace_consume_fn for the consume_entry function as it is >> already defined in linux/stacktrace.h. >> >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > > How about: > > | arm64: align with common stracktrace naming > | > | For historical reasons, the naming of parameters and their types in the arm64 > | stacktrace code differs from that used in generic code and other > | architectures, even though the types are equivalent. > | > | For consistency and clarity, use the generic names. > Will add this. Madhavan > Either way: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > Mark. > >> --- >> arch/arm64/kernel/stacktrace.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >> index 1b32e55735aa..f772dac78b11 100644 >> --- a/arch/arm64/kernel/stacktrace.c >> +++ b/arch/arm64/kernel/stacktrace.c >> @@ -181,12 +181,12 @@ static int notrace unwind_next(struct unwind_state *state) >> NOKPROBE_SYMBOL(unwind_next); >> >> static void notrace unwind(struct unwind_state *state, >> - bool (*fn)(void *, unsigned long), void *data) >> + stack_trace_consume_fn consume_entry, void *cookie) >> { >> while (1) { >> int ret; >> >> - if (!fn(data, state->pc)) >> + if (!consume_entry(cookie, state->pc)) >> break; >> ret = unwind_next(state); >> if (ret < 0) >> -- >> 2.25.1 >>
Hey Mark Rutland, Mark Brown, Could you please review the rest of the patches in the series when you can? Also, many of the patches have received a Reviewed-By from you both. So, after I send the next version out, can we upstream those ones? Thanks. Madhavan On 2/15/22 07:39, Mark Rutland wrote: > On Mon, Jan 17, 2022 at 08:56:03AM -0600, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> Rename the arguments to unwind() for better consistency. Also, use the >> typedef stack_trace_consume_fn for the consume_entry function as it is >> already defined in linux/stacktrace.h. >> >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > > How about: > > | arm64: align with common stracktrace naming > | > | For historical reasons, the naming of parameters and their types in the arm64 > | stacktrace code differs from that used in generic code and other > | architectures, even though the types are equivalent. > | > | For consistency and clarity, use the generic names. > > Either way: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > Mark. > >> --- >> arch/arm64/kernel/stacktrace.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >> index 1b32e55735aa..f772dac78b11 100644 >> --- a/arch/arm64/kernel/stacktrace.c >> +++ b/arch/arm64/kernel/stacktrace.c >> @@ -181,12 +181,12 @@ static int notrace unwind_next(struct unwind_state *state) >> NOKPROBE_SYMBOL(unwind_next); >> >> static void notrace unwind(struct unwind_state *state, >> - bool (*fn)(void *, unsigned long), void *data) >> + stack_trace_consume_fn consume_entry, void *cookie) >> { >> while (1) { >> int ret; >> >> - if (!fn(data, state->pc)) >> + if (!consume_entry(cookie, state->pc)) >> break; >> ret = unwind_next(state); >> if (ret < 0) >> -- >> 2.25.1 >>
On Mon, Mar 07, 2022 at 10:51:38AM -0600, Madhavan T. Venkataraman wrote: > Hey Mark Rutland, Mark Brown, > > Could you please review the rest of the patches in the series when you can? > Please don't send content free pings. As far as I remember I'd reviewed or was expecting changes based on review or dependent patches for everything that you'd sent. > Also, many of the patches have received a Reviewed-By from you both. So, after I send the next version out, can we upstream those ones? That's more a question for Catalin and Will. If myself and Mark have reviewed patches then we're saying we think those patches are good to go.
On 3/7/22 11:01, Mark Brown wrote: > On Mon, Mar 07, 2022 at 10:51:38AM -0600, Madhavan T. Venkataraman wrote: >> Hey Mark Rutland, Mark Brown, >> >> Could you please review the rest of the patches in the series when you can? >> > > Please don't send content free pings. As far as I remember I'd reviewed > or was expecting changes based on review or dependent patches for > everything that you'd sent. > Indeed you did! Many thanks! It is just that patch 11 that defines "select HAVE_RELIABLE_STACKTRACE" did not receive any comments from you (unless I missed a comment that came from you. That is entirely possible. If I missed it, my bad). Since you suggested that change, I just wanted to make sure that that patch looks OK to you. >> Also, many of the patches have received a Reviewed-By from you both. So, after I send the next version out, can we upstream those ones? > > That's more a question for Catalin and Will. If myself and Mark have > reviewed patches then we're saying we think those patches are good to > go. Got it! Thanks! Madhavan
On Tue, Mar 08, 2022 at 04:00:35PM -0600, Madhavan T. Venkataraman wrote: > It is just that patch 11 that defines "select > HAVE_RELIABLE_STACKTRACE" did not receive any comments from you > (unless I missed a comment that came from you. That is entirely > possible. If I missed it, my bad). Since you suggested that change, I > just wanted to make sure that that patch looks OK to you. I think that's more a question for the livepatch people to be honest - it's not entirely a technical one, there's a bunch of confidence level stuff going on. For example there was some suggestion that people might insist on having objtool support, though there's also substantial pushback on making objtool a requirement for anything from other quarters. I was hoping that posting that patch would provoke some discussion about what exactly is needed but that's not happened thus far.
On 3/9/22 05:47, Mark Brown wrote: > On Tue, Mar 08, 2022 at 04:00:35PM -0600, Madhavan T. Venkataraman wrote: > >> It is just that patch 11 that defines "select >> HAVE_RELIABLE_STACKTRACE" did not receive any comments from you >> (unless I missed a comment that came from you. That is entirely >> possible. If I missed it, my bad). Since you suggested that change, I >> just wanted to make sure that that patch looks OK to you. > > I think that's more a question for the livepatch people to be honest - > it's not entirely a technical one, there's a bunch of confidence level > stuff going on. For example there was some suggestion that people might > insist on having objtool support, though there's also substantial > pushback on making objtool a requirement for anything from other > quarters. I was hoping that posting that patch would provoke some > discussion about what exactly is needed but that's not happened thus > far. Understood. In that case, I will remove that patch because it is not really required for my current work on the unwinder. I will bring this up later in a different patch series where it will trigger a discussion. Thanks. Madhavan
On Wed, 9 Mar 2022, Mark Brown wrote: > On Tue, Mar 08, 2022 at 04:00:35PM -0600, Madhavan T. Venkataraman wrote: > > > It is just that patch 11 that defines "select > > HAVE_RELIABLE_STACKTRACE" did not receive any comments from you > > (unless I missed a comment that came from you. That is entirely > > possible. If I missed it, my bad). Since you suggested that change, I > > just wanted to make sure that that patch looks OK to you. > > I think that's more a question for the livepatch people to be honest - > it's not entirely a technical one, there's a bunch of confidence level > stuff going on. For example there was some suggestion that people might > insist on having objtool support, though there's also substantial > pushback on making objtool a requirement for anything from other > quarters. I was hoping that posting that patch would provoke some > discussion about what exactly is needed but that's not happened thus > far. I think everyone will be happy with HAVE_RELIABLE_STACKTRACE on arm64 as long as there is a guarantee that stack traces are really reliable. My understanding is that there is still some work to be done on arm64 arch side (but I may have misunderstood what Mark R. said elsewhere). And yes, then there is a question of objtool. It is one option but not the only one. There have been proposals of implementing guarantees on a compiler side and leaving objtool for x86_64 only (albeit objtool may bring more features to the table... ORC, arch features checking). Madhavan also mentioned that he enhanced objtool and he planned to submit it eventually (https://lore.kernel.org/all/1a0e19db-a7f8-4c8e-0163-398fcd364d54@linux.microsoft.com/T/#u), so maybe arm64 maintainers could decide on a future direction based on that? Regards Miroslav
On 3/10/22 02:33, Miroslav Benes wrote: > On Wed, 9 Mar 2022, Mark Brown wrote: > >> On Tue, Mar 08, 2022 at 04:00:35PM -0600, Madhavan T. Venkataraman wrote: >> >>> It is just that patch 11 that defines "select >>> HAVE_RELIABLE_STACKTRACE" did not receive any comments from you >>> (unless I missed a comment that came from you. That is entirely >>> possible. If I missed it, my bad). Since you suggested that change, I >>> just wanted to make sure that that patch looks OK to you. >> >> I think that's more a question for the livepatch people to be honest - >> it's not entirely a technical one, there's a bunch of confidence level >> stuff going on. For example there was some suggestion that people might >> insist on having objtool support, though there's also substantial >> pushback on making objtool a requirement for anything from other >> quarters. I was hoping that posting that patch would provoke some >> discussion about what exactly is needed but that's not happened thus >> far. > > I think everyone will be happy with HAVE_RELIABLE_STACKTRACE on arm64 as > long as there is a guarantee that stack traces are really reliable. My > understanding is that there is still some work to be done on arm64 arch > side (but I may have misunderstood what Mark R. said elsewhere). And yes, > then there is a question of objtool. It is one option but not the only > one. There have been proposals of implementing guarantees on a compiler > side and leaving objtool for x86_64 only (albeit objtool may bring more > features to the table... ORC, arch features checking). > > Madhavan also mentioned that he enhanced objtool and he planned to submit > it eventually > (https://lore.kernel.org/all/1a0e19db-a7f8-4c8e-0163-398fcd364d54@linux.microsoft.com/T/#u), > so maybe arm64 maintainers could decide on a future direction based on > that? > Yes. I am working on that right now. Hope to send it out soon. Madhavan
On Wed, Mar 09, 2022 at 11:47:38AM +0000, Mark Brown wrote: > On Tue, Mar 08, 2022 at 04:00:35PM -0600, Madhavan T. Venkataraman wrote: > > > It is just that patch 11 that defines "select > > HAVE_RELIABLE_STACKTRACE" did not receive any comments from you > > (unless I missed a comment that came from you. That is entirely > > possible. If I missed it, my bad). Since you suggested that change, I > > just wanted to make sure that that patch looks OK to you. > > I think that's more a question for the livepatch people to be honest - > it's not entirely a technical one, there's a bunch of confidence level > stuff going on. For example there was some suggestion that people might > insist on having objtool support, though there's also substantial > pushback on making objtool a requirement for anything from other > quarters. I was hoping that posting that patch would provoke some > discussion about what exactly is needed but that's not happened thus > far. That patch has HAVE_RELIABLE_STACKTRACE depending on STACK_VALIDATION, which doesn't exist on arm64. So it didn't seem controversial enough to warrant discussion ;-)
On Mon, Mar 07, 2022 at 10:51:38AM -0600, Madhavan T. Venkataraman wrote: > Hey Mark Rutland, Mark Brown, > > Could you please review the rest of the patches in the series when you can? Sorry, I was expecting a new version with some of my comments addressed, in case that had effects on subsequent patches. > Also, many of the patches have received a Reviewed-By from you both. > So, after I send the next version out, can we upstream those ones? I would very much like to upstream the ones I have given a Reviewed-by. Given those were conditional on some adjustments (e.g. actually filling out comments), do you mind if I pick those into a series now? Then, once that's picked, you can rebase the rest atop, and we can review that. Thanks, Mark. > On 2/15/22 07:39, Mark Rutland wrote: > > On Mon, Jan 17, 2022 at 08:56:03AM -0600, madvenka@linux.microsoft.com wrote: > >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > >> > >> Rename the arguments to unwind() for better consistency. Also, use the > >> typedef stack_trace_consume_fn for the consume_entry function as it is > >> already defined in linux/stacktrace.h. > >> > >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > > > > How about: > > > > | arm64: align with common stracktrace naming > > | > > | For historical reasons, the naming of parameters and their types in the arm64 > > | stacktrace code differs from that used in generic code and other > > | architectures, even though the types are equivalent. > > | > > | For consistency and clarity, use the generic names. > > > > Either way: > > > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > > > Mark. > > > >> --- > >> arch/arm64/kernel/stacktrace.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > >> index 1b32e55735aa..f772dac78b11 100644 > >> --- a/arch/arm64/kernel/stacktrace.c > >> +++ b/arch/arm64/kernel/stacktrace.c > >> @@ -181,12 +181,12 @@ static int notrace unwind_next(struct unwind_state *state) > >> NOKPROBE_SYMBOL(unwind_next); > >> > >> static void notrace unwind(struct unwind_state *state, > >> - bool (*fn)(void *, unsigned long), void *data) > >> + stack_trace_consume_fn consume_entry, void *cookie) > >> { > >> while (1) { > >> int ret; > >> > >> - if (!fn(data, state->pc)) > >> + if (!consume_entry(cookie, state->pc)) > >> break; > >> ret = unwind_next(state); > >> if (ret < 0) > >> -- > >> 2.25.1 > >>
On Fri, Apr 08, 2022 at 03:44:34PM +0100, Mark Rutland wrote: > On Mon, Mar 07, 2022 at 10:51:38AM -0600, Madhavan T. Venkataraman wrote: > > Hey Mark Rutland, Mark Brown, > > > > Could you please review the rest of the patches in the series when you can? > > Sorry, I was expecting a new version with some of my comments > addressed, in case that had effects on subsequent patches. > > > Also, many of the patches have received a Reviewed-By from you both. > > So, after I send the next version out, can we upstream those ones? > > I would very much like to upstream the ones I have given a Reviewed-by. > > Given those were conditional on some adjustments (e.g. actually filling > out comments), do you mind if I pick those into a series now? FWIW, I've picked up the set I'm trivially happy with, rebased that on v5.18-rc1, and put that on a branch with a couple of other cleanups: https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/stacktrace/cleanups I'll send that out on Monday if there are no objections. Thanks, Mark.
On 4/8/22 09:44, Mark Rutland wrote: > On Mon, Mar 07, 2022 at 10:51:38AM -0600, Madhavan T. Venkataraman wrote: >> Hey Mark Rutland, Mark Brown, >> >> Could you please review the rest of the patches in the series when you can? > > Sorry, I was expecting a new version with some of my comments > addressed, in case that had effects on subsequent patches. > Yes. I realized that. I am actually working on the next version addressing the comments I have received. >> Also, many of the patches have received a Reviewed-By from you both. >> So, after I send the next version out, can we upstream those ones? > > I would very much like to upstream the ones I have given a Reviewed-by. > > Given those were conditional on some adjustments (e.g. actually filling > out comments), do you mind if I pick those into a series now? > > Then, once that's picked, you can rebase the rest atop, and we can > review that. > That would be great! Thanks! Madhavan
On 4/8/22 12:58, Mark Rutland wrote: > On Fri, Apr 08, 2022 at 03:44:34PM +0100, Mark Rutland wrote: >> On Mon, Mar 07, 2022 at 10:51:38AM -0600, Madhavan T. Venkataraman wrote: >>> Hey Mark Rutland, Mark Brown, >>> >>> Could you please review the rest of the patches in the series when you can? >> >> Sorry, I was expecting a new version with some of my comments >> addressed, in case that had effects on subsequent patches. >> >>> Also, many of the patches have received a Reviewed-By from you both. >>> So, after I send the next version out, can we upstream those ones? >> >> I would very much like to upstream the ones I have given a Reviewed-by. >> >> Given those were conditional on some adjustments (e.g. actually filling >> out comments), do you mind if I pick those into a series now? > > FWIW, I've picked up the set I'm trivially happy with, rebased that on > v5.18-rc1, and put that on a branch with a couple of other cleanups: > > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/stacktrace/cleanups > > I'll send that out on Monday if there are no objections. > > Thanks, > Mark. LGTM. Madhavan
On 4/8/22 09:44, Mark Rutland wrote: > On Mon, Mar 07, 2022 at 10:51:38AM -0600, Madhavan T. Venkataraman wrote: >> Hey Mark Rutland, Mark Brown, >> >> Could you please review the rest of the patches in the series when you can? > > Sorry, I was expecting a new version with some of my comments > addressed, in case that had effects on subsequent patches. > >> Also, many of the patches have received a Reviewed-By from you both. >> So, after I send the next version out, can we upstream those ones? > > I would very much like to upstream the ones I have given a Reviewed-by. > > Given those were conditional on some adjustments (e.g. actually filling > out comments), do you mind if I pick those into a series now? > > Then, once that's picked, you can rebase the rest atop, and we can > review that. > So, do you want me to address the comments so far and send the next version? I can do it ASAP. Madhavan
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 1b32e55735aa..f772dac78b11 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -181,12 +181,12 @@ static int notrace unwind_next(struct unwind_state *state) NOKPROBE_SYMBOL(unwind_next); static void notrace unwind(struct unwind_state *state, - bool (*fn)(void *, unsigned long), void *data) + stack_trace_consume_fn consume_entry, void *cookie) { while (1) { int ret; - if (!fn(data, state->pc)) + if (!consume_entry(cookie, state->pc)) break; ret = unwind_next(state); if (ret < 0)