[1/3] ARM: kprobes: Prevent known test failures stopping other tests running

Message ID 1394556894-18592-2-git-send-email-tixy@linaro.org
State New
Headers show

Commit Message

Jon Medhurst (Tixy) March 11, 2014, 4:54 p.m.
Due to a long-standing issue with Thumb symbol lookup [1] the jprobes
tests fail when built into a kernel compiled as Thumb mode. (They work
fine for ARM mode kernels or for Thumb when built as a loadable module.)

Rather than have this problem terminate testing prematurely lets instead
emit an error message and carry on with the main kprobes tests, delaying
the final failure report until the end.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/063026.html

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
 arch/arm/kernel/kprobes-test.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

David Long March 24, 2014, 3:18 p.m. | #1
On 03/11/14 12:54, Jon Medhurst wrote:
> Due to a long-standing issue with Thumb symbol lookup [1] the jprobes
> tests fail when built into a kernel compiled as Thumb mode. (They work
> fine for ARM mode kernels or for Thumb when built as a loadable module.)
>
> Rather than have this problem terminate testing prematurely lets instead
> emit an error message and carry on with the main kprobes tests, delaying
> the final failure report until the end.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/063026.html
>
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
>   arch/arm/kernel/kprobes-test.c |   10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> index c2fd06b..6de5e94 100644
> --- a/arch/arm/kernel/kprobes-test.c
> +++ b/arch/arm/kernel/kprobes-test.c
> @@ -225,6 +225,7 @@ static int pre_handler_called;
>   static int post_handler_called;
>   static int jprobe_func_called;
>   static int kretprobe_handler_called;
> +static int tests_failed;
>
>   #define FUNC_ARG1 0x12345678
>   #define FUNC_ARG2 0xabcdef
> @@ -461,6 +462,13 @@ static int run_api_tests(long (*func)(long, long))
>
>   	pr_info("    jprobe\n");
>   	ret = test_jprobe(func);
> +#if defined(CONFIG_THUMB2_KERNEL) && !defined(MODULE)
> +	if (ret == -EINVAL) {
> +		pr_err("FAIL: Known longtime bug with jprobe on Thumb kernels\n");
> +		tests_failed = ret;
> +		ret = 0;
> +	}
> +#endif
>   	if (ret < 0)
>   		return ret;
>
> @@ -1671,6 +1679,8 @@ static int __init run_all_tests(void)
>
>   out:
>   	if (ret == 0)
> +		ret = tests_failed;
> +	if (ret == 0)
>   		pr_info("Finished kprobe tests OK\n");
>   	else
>   		pr_err("kprobe tests failed\n");
>

Tixy,

Sorry for the delay in responding.

Because this (correctly) was marked as having no interdependencies it 
got marked as "applied" in the patch-tracker.  That is why I omitted it 
from my v7 patches (although I should have just submitted it separately 
in the first place).  I don't see it in the linux-arm tree though, so I 
guess we do still need it.  If you haven't yet you might want to double 
check that.

I'm happy ack'ing this (if that's useful to you :-).

-dl
Jon Medhurst (Tixy) March 24, 2014, 4:49 p.m. | #2
On Mon, 2014-03-24 at 11:18 -0400, David Long wrote:
> On 03/11/14 12:54, Jon Medhurst wrote:
> > Due to a long-standing issue with Thumb symbol lookup [1] the jprobes
> > tests fail when built into a kernel compiled as Thumb mode. (They work
> > fine for ARM mode kernels or for Thumb when built as a loadable module.)
> >
> > Rather than have this problem terminate testing prematurely lets instead
> > emit an error message and carry on with the main kprobes tests, delaying
> > the final failure report until the end.
> >
> > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/063026.html
> >
> > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > ---
> >   arch/arm/kernel/kprobes-test.c |   10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
> > index c2fd06b..6de5e94 100644
> > --- a/arch/arm/kernel/kprobes-test.c
> > +++ b/arch/arm/kernel/kprobes-test.c
> > @@ -225,6 +225,7 @@ static int pre_handler_called;
> >   static int post_handler_called;
> >   static int jprobe_func_called;
> >   static int kretprobe_handler_called;
> > +static int tests_failed;
> >
> >   #define FUNC_ARG1 0x12345678
> >   #define FUNC_ARG2 0xabcdef
> > @@ -461,6 +462,13 @@ static int run_api_tests(long (*func)(long, long))
> >
> >   	pr_info("    jprobe\n");
> >   	ret = test_jprobe(func);
> > +#if defined(CONFIG_THUMB2_KERNEL) && !defined(MODULE)
> > +	if (ret == -EINVAL) {
> > +		pr_err("FAIL: Known longtime bug with jprobe on Thumb kernels\n");
> > +		tests_failed = ret;
> > +		ret = 0;
> > +	}
> > +#endif
> >   	if (ret < 0)
> >   		return ret;
> >
> > @@ -1671,6 +1679,8 @@ static int __init run_all_tests(void)
> >
> >   out:
> >   	if (ret == 0)
> > +		ret = tests_failed;
> > +	if (ret == 0)
> >   		pr_info("Finished kprobe tests OK\n");
> >   	else
> >   		pr_err("kprobe tests failed\n");
> >
[...]
> Because this (correctly) was marked as having no interdependencies it 
> got marked as "applied" in the patch-tracker.  That is why I omitted it 
> from my v7 patches.

Yes, I now see this in Russell's patch tracker (with a less verbose
commit message) as patch 7978, and it's marked as "Applied to git-curr
(uprobes branch)". Perhaps it got dropped along with the rest of the
uprobes branch when the config problems were discovered, Russell?

I have two other kprobes test fixes in this patches series, and was
planning on sending a pull request, or submitting to the patch tracker,
after the merge window has closed, so they can head into 3.16. (I posted
these a bit late for 3.15 and as the bugs they fix have been there since
day one I didn't think it warranted expediting).
Russell King - ARM Linux March 24, 2014, 4:56 p.m. | #3
On Mon, Mar 24, 2014 at 04:49:05PM +0000, Jon Medhurst (Tixy) wrote:
> Yes, I now see this in Russell's patch tracker (with a less verbose
> commit message) as patch 7978, and it's marked as "Applied to git-curr
> (uprobes branch)". Perhaps it got dropped along with the rest of the
> uprobes branch when the config problems were discovered, Russell?

If it was applied to the uprobes branch, and it wasn't part of David's
uprobes git pull request, then yes, it doesn't exist anymore.
David Long March 24, 2014, 6:34 p.m. | #4
On 03/24/14 12:56, Russell King - ARM Linux wrote:
> On Mon, Mar 24, 2014 at 04:49:05PM +0000, Jon Medhurst (Tixy) wrote:
>> Yes, I now see this in Russell's patch tracker (with a less verbose
>> commit message) as patch 7978, and it's marked as "Applied to git-curr
>> (uprobes branch)". Perhaps it got dropped along with the rest of the
>> uprobes branch when the config problems were discovered, Russell?
>
> If it was applied to the uprobes branch, and it wasn't part of David's
> uprobes git pull request, then yes, it doesn't exist anymore.
>

OK.  Just to be absolutely clear:  this doesn't create any new problem, 
but we do then need it in Tixy's new patchset in order to fix this older 
bug.

-dl
Russell King - ARM Linux March 25, 2014, 2:02 p.m. | #5
On Mon, Mar 24, 2014 at 02:34:01PM -0400, David Long wrote:
> On 03/24/14 12:56, Russell King - ARM Linux wrote:
>> On Mon, Mar 24, 2014 at 04:49:05PM +0000, Jon Medhurst (Tixy) wrote:
>>> Yes, I now see this in Russell's patch tracker (with a less verbose
>>> commit message) as patch 7978, and it's marked as "Applied to git-curr
>>> (uprobes branch)". Perhaps it got dropped along with the rest of the
>>> uprobes branch when the config problems were discovered, Russell?
>>
>> If it was applied to the uprobes branch, and it wasn't part of David's
>> uprobes git pull request, then yes, it doesn't exist anymore.
>>
>
> OK.  Just to be absolutely clear:  this doesn't create any new problem,  
> but we do then need it in Tixy's new patchset in order to fix this older  
> bug.

Okay, so I'll just wait for Tixy's patch set then?
David Long March 25, 2014, 2:08 p.m. | #6
On 03/25/14 10:02, Russell King - ARM Linux wrote:
> On Mon, Mar 24, 2014 at 02:34:01PM -0400, David Long wrote:
>> On 03/24/14 12:56, Russell King - ARM Linux wrote:
>>> On Mon, Mar 24, 2014 at 04:49:05PM +0000, Jon Medhurst (Tixy) wrote:
>>>> Yes, I now see this in Russell's patch tracker (with a less verbose
>>>> commit message) as patch 7978, and it's marked as "Applied to git-curr
>>>> (uprobes branch)". Perhaps it got dropped along with the rest of the
>>>> uprobes branch when the config problems were discovered, Russell?
>>>
>>> If it was applied to the uprobes branch, and it wasn't part of David's
>>> uprobes git pull request, then yes, it doesn't exist anymore.
>>>
>>
>> OK.  Just to be absolutely clear:  this doesn't create any new problem,
>> but we do then need it in Tixy's new patchset in order to fix this older
>> bug.
>
> Okay, so I'll just wait for Tixy's patch set then?
>

You don't have to delay anything, just eventually (e.g.: v3.16) take 
Tixy's patch as  a separate patch fixing this old bug.  The uprobes 
stuff is fine just as it is.  There are no interdependencies.

-dl
Jon Medhurst (Tixy) March 25, 2014, 2:20 p.m. | #7
On Tue, 2014-03-25 at 10:08 -0400, David Long wrote:
> On 03/25/14 10:02, Russell King - ARM Linux wrote:
> > On Mon, Mar 24, 2014 at 02:34:01PM -0400, David Long wrote:
> >> On 03/24/14 12:56, Russell King - ARM Linux wrote:
> >>> On Mon, Mar 24, 2014 at 04:49:05PM +0000, Jon Medhurst (Tixy) wrote:
> >>>> Yes, I now see this in Russell's patch tracker (with a less verbose
> >>>> commit message) as patch 7978, and it's marked as "Applied to git-curr
> >>>> (uprobes branch)". Perhaps it got dropped along with the rest of the
> >>>> uprobes branch when the config problems were discovered, Russell?
> >>>
> >>> If it was applied to the uprobes branch, and it wasn't part of David's
> >>> uprobes git pull request, then yes, it doesn't exist anymore.
> >>>
> >>
> >> OK.  Just to be absolutely clear:  this doesn't create any new problem,
> >> but we do then need it in Tixy's new patchset in order to fix this older
> >> bug.
> >
> > Okay, so I'll just wait for Tixy's patch set then?
> >
> 
> You don't have to delay anything, just eventually (e.g.: v3.16) take 
> Tixy's patch as  a separate patch fixing this old bug.  The uprobes 
> stuff is fine just as it is.  There are no interdependencies.

In case you are talking at crossed purposes. The uprobes code which
Russell has already pulled into his tree, and is in linux-next, is fine,
nothing need changing, and is safe to go into 3.15.

The 3 patches I posted, which this is in reply to, are for fixes for
minor bugs in kprobes which have been there for 3 years, and can wait
for 3.16.

Patch

diff --git a/arch/arm/kernel/kprobes-test.c b/arch/arm/kernel/kprobes-test.c
index c2fd06b..6de5e94 100644
--- a/arch/arm/kernel/kprobes-test.c
+++ b/arch/arm/kernel/kprobes-test.c
@@ -225,6 +225,7 @@  static int pre_handler_called;
 static int post_handler_called;
 static int jprobe_func_called;
 static int kretprobe_handler_called;
+static int tests_failed;
 
 #define FUNC_ARG1 0x12345678
 #define FUNC_ARG2 0xabcdef
@@ -461,6 +462,13 @@  static int run_api_tests(long (*func)(long, long))
 
 	pr_info("    jprobe\n");
 	ret = test_jprobe(func);
+#if defined(CONFIG_THUMB2_KERNEL) && !defined(MODULE)
+	if (ret == -EINVAL) {
+		pr_err("FAIL: Known longtime bug with jprobe on Thumb kernels\n");
+		tests_failed = ret;
+		ret = 0;
+	}
+#endif
 	if (ret < 0)
 		return ret;
 
@@ -1671,6 +1679,8 @@  static int __init run_all_tests(void)
 
 out:
 	if (ret == 0)
+		ret = tests_failed;
+	if (ret == 0)
 		pr_info("Finished kprobe tests OK\n");
 	else
 		pr_err("kprobe tests failed\n");