When do percpu-rwsem writer lock torture, the RCU callback
rcu_sync_func() may still be pending after locktorture module
is removed, and it will lead to the following Oops:
BUG: unable to handle page fault for address: ffffffffc00eb920
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 6500a067 P4D 6500a067 PUD 6500c067 PMD 13a36c067 PTE 800000013691c163
Oops: 0000 [#1] PREEMPT SMP
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.9.0-rc5+ #4
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
RIP: 0010:rcu_cblist_dequeue+0x12/0x30
Call Trace:
<IRQ>
rcu_core+0x1b1/0x860
__do_softirq+0xfe/0x326
asm_call_on_stack+0x12/0x20
</IRQ>
do_softirq_own_stack+0x5f/0x80
irq_exit_rcu+0xaf/0xc0
sysvec_apic_timer_interrupt+0x2e/0xb0
asm_sysvec_apic_timer_interrupt+0x12/0x20
Fix it by adding an exit hook in lock_torture_ops and
use it to call percpu_free_rwsem() for percpu rwsem torture
before the module is removed, so we can ensure rcu_sync_func()
completes before module exits.
Also needs to call exit hook if lock_torture_init() fails half-way,
so use ctx->cur_ops != NULL to signal that init hook has been called.
Signed-off-by: Hou Tao <[email protected]>
---
kernel/locking/locktorture.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index bebdf98e6cd78..e91033e9b6f95 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -74,6 +74,7 @@ static void lock_torture_cleanup(void);
*/
struct lock_torture_ops {
void (*init)(void);
+ void (*exit)(void);
int (*writelock)(void);
void (*write_delay)(struct torture_random_state *trsp);
void (*task_boost)(struct torture_random_state *trsp);
@@ -571,6 +572,11 @@ void torture_percpu_rwsem_init(void)
BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
}
+static void torture_percpu_rwsem_exit(void)
+{
+ percpu_free_rwsem(&pcpu_rwsem);
+}
+
static int torture_percpu_rwsem_down_write(void) __acquires(pcpu_rwsem)
{
percpu_down_write(&pcpu_rwsem);
@@ -595,6 +601,7 @@ static void torture_percpu_rwsem_up_read(void) __releases(pcpu_rwsem)
static struct lock_torture_ops percpu_rwsem_lock_ops = {
.init = torture_percpu_rwsem_init,
+ .exit = torture_percpu_rwsem_exit,
.writelock = torture_percpu_rwsem_down_write,
.write_delay = torture_rwsem_write_delay,
.task_boost = torture_boost_dummy,
@@ -786,9 +793,10 @@ static void lock_torture_cleanup(void)
/*
* Indicates early cleanup, meaning that the test has not run,
- * such as when passing bogus args when loading the module. As
- * such, only perform the underlying torture-specific cleanups,
- * and avoid anything related to locktorture.
+ * such as when passing bogus args when loading the module.
+ * However cxt->cur_ops.init() may have been invoked, so beside
+ * perform the underlying torture-specific cleanups, cur_ops.exit()
+ * will be invoked if needed.
*/
if (!cxt.lwsa && !cxt.lrsa)
goto end;
@@ -828,6 +836,12 @@ static void lock_torture_cleanup(void)
cxt.lrsa = NULL;
end:
+ /* If init() has been called, then do exit() accordingly */
+ if (cxt.cur_ops) {
+ if (cxt.cur_ops->exit)
+ cxt.cur_ops->exit();
+ cxt.cur_ops = NULL;
+ }
torture_cleanup_end();
}
@@ -835,6 +849,7 @@ static int __init lock_torture_init(void)
{
int i, j;
int firsterr = 0;
+ struct lock_torture_ops *cur_ops;
static struct lock_torture_ops *torture_ops[] = {
&lock_busted_ops,
&spin_lock_ops, &spin_lock_irq_ops,
@@ -853,8 +868,8 @@ static int __init lock_torture_init(void)
/* Process args and tell the world that the torturer is on the job. */
for (i = 0; i < ARRAY_SIZE(torture_ops); i++) {
- cxt.cur_ops = torture_ops[i];
- if (strcmp(torture_type, cxt.cur_ops->name) == 0)
+ cur_ops = torture_ops[i];
+ if (strcmp(torture_type, cur_ops->name) == 0)
break;
}
if (i == ARRAY_SIZE(torture_ops)) {
@@ -869,12 +884,13 @@ static int __init lock_torture_init(void)
}
if (nwriters_stress == 0 &&
- (!cxt.cur_ops->readlock || nreaders_stress == 0)) {
+ (!cur_ops->readlock || nreaders_stress == 0)) {
pr_alert("lock-torture: must run at least one locking thread\n");
firsterr = -EINVAL;
goto unwind;
}
+ cxt.cur_ops = cur_ops;
if (cxt.cur_ops->init)
cxt.cur_ops->init();
--
2.25.0.4.g0ad7144999
On Thu, Sep 17, 2020 at 09:59:10PM +0800, Hou Tao wrote:
> When do percpu-rwsem writer lock torture, the RCU callback
> rcu_sync_func() may still be pending after locktorture module
> is removed, and it will lead to the following Oops:
>
> BUG: unable to handle page fault for address: ffffffffc00eb920
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 6500a067 P4D 6500a067 PUD 6500c067 PMD 13a36c067 PTE 800000013691c163
> Oops: 0000 [#1] PREEMPT SMP
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.9.0-rc5+ #4
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> RIP: 0010:rcu_cblist_dequeue+0x12/0x30
> Call Trace:
> <IRQ>
> rcu_core+0x1b1/0x860
> __do_softirq+0xfe/0x326
> asm_call_on_stack+0x12/0x20
> </IRQ>
> do_softirq_own_stack+0x5f/0x80
> irq_exit_rcu+0xaf/0xc0
> sysvec_apic_timer_interrupt+0x2e/0xb0
> asm_sysvec_apic_timer_interrupt+0x12/0x20
>
> Fix it by adding an exit hook in lock_torture_ops and
> use it to call percpu_free_rwsem() for percpu rwsem torture
> before the module is removed, so we can ensure rcu_sync_func()
> completes before module exits.
>
> Also needs to call exit hook if lock_torture_init() fails half-way,
> so use ctx->cur_ops != NULL to signal that init hook has been called.
Good catch, but please see below for comments and questions.
> Signed-off-by: Hou Tao <[email protected]>
> ---
> kernel/locking/locktorture.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> index bebdf98e6cd78..e91033e9b6f95 100644
> --- a/kernel/locking/locktorture.c
> +++ b/kernel/locking/locktorture.c
> @@ -74,6 +74,7 @@ static void lock_torture_cleanup(void);
> */
> struct lock_torture_ops {
> void (*init)(void);
> + void (*exit)(void);
This is fine, but why not also add a flag to the lock_torture_cxt
structure that is set when the ->init() function is called? Perhaps
something like this in lock_torture_init():
if (cxt.cur_ops->init) {
cxt.cur_ops->init();
cxt.initcalled = true;
}
> int (*writelock)(void);
> void (*write_delay)(struct torture_random_state *trsp);
> void (*task_boost)(struct torture_random_state *trsp);
> @@ -571,6 +572,11 @@ void torture_percpu_rwsem_init(void)
> BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
> }
>
> +static void torture_percpu_rwsem_exit(void)
> +{
> + percpu_free_rwsem(&pcpu_rwsem);
> +}
> +
> static int torture_percpu_rwsem_down_write(void) __acquires(pcpu_rwsem)
> {
> percpu_down_write(&pcpu_rwsem);
> @@ -595,6 +601,7 @@ static void torture_percpu_rwsem_up_read(void) __releases(pcpu_rwsem)
>
> static struct lock_torture_ops percpu_rwsem_lock_ops = {
> .init = torture_percpu_rwsem_init,
> + .exit = torture_percpu_rwsem_exit,
> .writelock = torture_percpu_rwsem_down_write,
> .write_delay = torture_rwsem_write_delay,
> .task_boost = torture_boost_dummy,
> @@ -786,9 +793,10 @@ static void lock_torture_cleanup(void)
>
> /*
> * Indicates early cleanup, meaning that the test has not run,
> - * such as when passing bogus args when loading the module. As
> - * such, only perform the underlying torture-specific cleanups,
> - * and avoid anything related to locktorture.
> + * such as when passing bogus args when loading the module.
> + * However cxt->cur_ops.init() may have been invoked, so beside
> + * perform the underlying torture-specific cleanups, cur_ops.exit()
> + * will be invoked if needed.
> */
> if (!cxt.lwsa && !cxt.lrsa)
> goto end;
> @@ -828,6 +836,12 @@ static void lock_torture_cleanup(void)
> cxt.lrsa = NULL;
>
> end:
> + /* If init() has been called, then do exit() accordingly */
> + if (cxt.cur_ops) {
> + if (cxt.cur_ops->exit)
> + cxt.cur_ops->exit();
> + cxt.cur_ops = NULL;
> + }
The above can then be:
if (cxt.initcalled && cxt.cur_ops->exit)
cxt.cur_ops->exit();
Maybe you also need to clear cxt.initcalled at this point, but I don't
immediately see why that would be needed.
> torture_cleanup_end();
> }
>
> @@ -835,6 +849,7 @@ static int __init lock_torture_init(void)
> {
> int i, j;
> int firsterr = 0;
> + struct lock_torture_ops *cur_ops;
And then you don't need this extra pointer. Not that this pointer is bad
in and of itself, but using (!cxt.cur_ops) to indicate that the ->init()
function has not been called is an accident waiting to happen.
And the changes below are no longer needed.
Or am I missing something subtle?
Thanx, Paul
> static struct lock_torture_ops *torture_ops[] = {
> &lock_busted_ops,
> &spin_lock_ops, &spin_lock_irq_ops,
> @@ -853,8 +868,8 @@ static int __init lock_torture_init(void)
>
> /* Process args and tell the world that the torturer is on the job. */
> for (i = 0; i < ARRAY_SIZE(torture_ops); i++) {
> - cxt.cur_ops = torture_ops[i];
> - if (strcmp(torture_type, cxt.cur_ops->name) == 0)
> + cur_ops = torture_ops[i];
> + if (strcmp(torture_type, cur_ops->name) == 0)
> break;
> }
> if (i == ARRAY_SIZE(torture_ops)) {
> @@ -869,12 +884,13 @@ static int __init lock_torture_init(void)
> }
>
> if (nwriters_stress == 0 &&
> - (!cxt.cur_ops->readlock || nreaders_stress == 0)) {
> + (!cur_ops->readlock || nreaders_stress == 0)) {
> pr_alert("lock-torture: must run at least one locking thread\n");
> firsterr = -EINVAL;
> goto unwind;
> }
>
> + cxt.cur_ops = cur_ops;
> if (cxt.cur_ops->init)
> cxt.cur_ops->init();
>
> --
> 2.25.0.4.g0ad7144999
>
Hi Paul,
> On 2020/9/23 7:24, Paul E. McKenney wrote:
snip
>> Fix it by adding an exit hook in lock_torture_ops and
>> use it to call percpu_free_rwsem() for percpu rwsem torture
>> before the module is removed, so we can ensure rcu_sync_func()
>> completes before module exits.
>>
>> Also needs to call exit hook if lock_torture_init() fails half-way,
>> so use ctx->cur_ops != NULL to signal that init hook has been called.
>
> Good catch, but please see below for comments and questions.
>
>> Signed-off-by: Hou Tao <[email protected]>
>> ---
>> kernel/locking/locktorture.c | 28 ++++++++++++++++++++++------
>> 1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>> index bebdf98e6cd78..e91033e9b6f95 100644
>> --- a/kernel/locking/locktorture.c
>> +++ b/kernel/locking/locktorture.c
>> @@ -74,6 +74,7 @@ static void lock_torture_cleanup(void);
>> */
>> struct lock_torture_ops {
>> void (*init)(void);
>> + void (*exit)(void);
>
> This is fine, but why not also add a flag to the lock_torture_cxt
> structure that is set when the ->init() function is called? Perhaps
> something like this in lock_torture_init():
>
> if (cxt.cur_ops->init) {
> cxt.cur_ops->init();
> cxt.initcalled = true;
> }
>
You are right. Add a new field to indicate the init hook has been
called is much better than reusing ctx->cur_ops != NULL to do that.
>> int (*writelock)(void);
>> void (*write_delay)(struct torture_random_state *trsp);
>> void (*task_boost)(struct torture_random_state *trsp);
>> @@ -571,6 +572,11 @@ void torture_percpu_rwsem_init(void)
>> BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
>> }
>>
>> +static void torture_percpu_rwsem_exit(void)
>> +{
>> + percpu_free_rwsem(&pcpu_rwsem);
>> +}
>> +
snip
>> @@ -828,6 +836,12 @@ static void lock_torture_cleanup(void)
>> cxt.lrsa = NULL;
>>
>> end:
>> + /* If init() has been called, then do exit() accordingly */
>> + if (cxt.cur_ops) {
>> + if (cxt.cur_ops->exit)
>> + cxt.cur_ops->exit();
>> + cxt.cur_ops = NULL;
>> + }
>
> The above can then be:
>
> if (cxt.initcalled && cxt.cur_ops->exit)
> cxt.cur_ops->exit();
>
> Maybe you also need to clear cxt.initcalled at this point, but I don't
> immediately see why that would be needed.
>
Because we are doing cleanup, so I think reset initcalled to false is OK
after the cleanup is done.
>> torture_cleanup_end();
>> }
>>
>> @@ -835,6 +849,7 @@ static int __init lock_torture_init(void)
>> {
>> int i, j;
>> int firsterr = 0;
>> + struct lock_torture_ops *cur_ops;
>
> And then you don't need this extra pointer. Not that this pointer is bad
> in and of itself, but using (!cxt.cur_ops) to indicate that the ->init()
> function has not been called is an accident waiting to happen.
>
> And the changes below are no longer needed.
>
> Or am I missing something subtle?
>
Thanks for your suggestion. Will send v2.
Thanks.
On Wed, Sep 23, 2020 at 10:24:20AM +0800, Hou Tao wrote:
> Hi Paul,
>
> > On 2020/9/23 7:24, Paul E. McKenney wrote:
> snip
>
> >> Fix it by adding an exit hook in lock_torture_ops and
> >> use it to call percpu_free_rwsem() for percpu rwsem torture
> >> before the module is removed, so we can ensure rcu_sync_func()
> >> completes before module exits.
> >>
> >> Also needs to call exit hook if lock_torture_init() fails half-way,
> >> so use ctx->cur_ops != NULL to signal that init hook has been called.
> >
> > Good catch, but please see below for comments and questions.
> >
> >> Signed-off-by: Hou Tao <[email protected]>
> >> ---
> >> kernel/locking/locktorture.c | 28 ++++++++++++++++++++++------
> >> 1 file changed, 22 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> >> index bebdf98e6cd78..e91033e9b6f95 100644
> >> --- a/kernel/locking/locktorture.c
> >> +++ b/kernel/locking/locktorture.c
> >> @@ -74,6 +74,7 @@ static void lock_torture_cleanup(void);
> >> */
> >> struct lock_torture_ops {
> >> void (*init)(void);
> >> + void (*exit)(void);
> >
> > This is fine, but why not also add a flag to the lock_torture_cxt
> > structure that is set when the ->init() function is called? Perhaps
> > something like this in lock_torture_init():
> >
> > if (cxt.cur_ops->init) {
> > cxt.cur_ops->init();
> > cxt.initcalled = true;
> > }
> >
>
> You are right. Add a new field to indicate the init hook has been
> called is much better than reusing ctx->cur_ops != NULL to do that.
>
> >> int (*writelock)(void);
> >> void (*write_delay)(struct torture_random_state *trsp);
> >> void (*task_boost)(struct torture_random_state *trsp);
> >> @@ -571,6 +572,11 @@ void torture_percpu_rwsem_init(void)
> >> BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
> >> }
> >>
> >> +static void torture_percpu_rwsem_exit(void)
> >> +{
> >> + percpu_free_rwsem(&pcpu_rwsem);
> >> +}
> >> +
> snip
>
> >> @@ -828,6 +836,12 @@ static void lock_torture_cleanup(void)
> >> cxt.lrsa = NULL;
> >>
> >> end:
> >> + /* If init() has been called, then do exit() accordingly */
> >> + if (cxt.cur_ops) {
> >> + if (cxt.cur_ops->exit)
> >> + cxt.cur_ops->exit();
> >> + cxt.cur_ops = NULL;
> >> + }
> >
> > The above can then be:
> >
> > if (cxt.initcalled && cxt.cur_ops->exit)
> > cxt.cur_ops->exit();
> >
> > Maybe you also need to clear cxt.initcalled at this point, but I don't
> > immediately see why that would be needed.
> >
> Because we are doing cleanup, so I think reset initcalled to false is OK
> after the cleanup is done.
Maybe best to try it both ways and see how each really works?
We might each have our opinions, but the computer's opinion is the one
that really counts. ;-)
> >> torture_cleanup_end();
> >> }
> >>
> >> @@ -835,6 +849,7 @@ static int __init lock_torture_init(void)
> >> {
> >> int i, j;
> >> int firsterr = 0;
> >> + struct lock_torture_ops *cur_ops;
> >
> > And then you don't need this extra pointer. Not that this pointer is bad
> > in and of itself, but using (!cxt.cur_ops) to indicate that the ->init()
> > function has not been called is an accident waiting to happen.
> >
> > And the changes below are no longer needed.
> >
> > Or am I missing something subtle?
> >
> Thanks for your suggestion. Will send v2.
Looking forward to seeing it!
Thanx, Paul
When do percpu-rwsem writer lock torture, the RCU callback
rcu_sync_func() may still be pending after locktorture module
is removed, and it will lead to the following Oops:
BUG: unable to handle page fault for address: ffffffffc00eb920
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 6500a067 P4D 6500a067 PUD 6500c067 PMD 13a36c067 PTE 800000013691c163
Oops: 0000 [#1] PREEMPT SMP
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.9.0-rc5+ #4
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
RIP: 0010:rcu_cblist_dequeue+0x12/0x30
Call Trace:
<IRQ>
rcu_core+0x1b1/0x860
__do_softirq+0xfe/0x326
asm_call_on_stack+0x12/0x20
</IRQ>
do_softirq_own_stack+0x5f/0x80
irq_exit_rcu+0xaf/0xc0
sysvec_apic_timer_interrupt+0x2e/0xb0
asm_sysvec_apic_timer_interrupt+0x12/0x20
Fix it by adding an exit hook in lock_torture_ops and
use it to call percpu_free_rwsem() for percpu rwsem torture
before the module is removed, so we can ensure rcu_sync_func()
completes before module exits.
Also needs to call exit hook if lock_torture_init() fails half-way,
so add init_called field in lock_torture_cxt to indicate that
init hook has been called.
Signed-off-by: Hou Tao <[email protected]>
---
v2: add init_called field in lock_torture_cxt instead of reusing
cxt->cur_ops for error handling
kernel/locking/locktorture.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index bebdf98e6cd78..1fbbcf76f495b 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -74,6 +74,7 @@ static void lock_torture_cleanup(void);
*/
struct lock_torture_ops {
void (*init)(void);
+ void (*exit)(void);
int (*writelock)(void);
void (*write_delay)(struct torture_random_state *trsp);
void (*task_boost)(struct torture_random_state *trsp);
@@ -90,12 +91,13 @@ struct lock_torture_cxt {
int nrealwriters_stress;
int nrealreaders_stress;
bool debug_lock;
+ bool init_called;
atomic_t n_lock_torture_errors;
struct lock_torture_ops *cur_ops;
struct lock_stress_stats *lwsa; /* writer statistics */
struct lock_stress_stats *lrsa; /* reader statistics */
};
-static struct lock_torture_cxt cxt = { 0, 0, false,
+static struct lock_torture_cxt cxt = { 0, 0, false, false,
ATOMIC_INIT(0),
NULL, NULL};
/*
@@ -571,6 +573,11 @@ void torture_percpu_rwsem_init(void)
BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
}
+static void torture_percpu_rwsem_exit(void)
+{
+ percpu_free_rwsem(&pcpu_rwsem);
+}
+
static int torture_percpu_rwsem_down_write(void) __acquires(pcpu_rwsem)
{
percpu_down_write(&pcpu_rwsem);
@@ -595,6 +602,7 @@ static void torture_percpu_rwsem_up_read(void) __releases(pcpu_rwsem)
static struct lock_torture_ops percpu_rwsem_lock_ops = {
.init = torture_percpu_rwsem_init,
+ .exit = torture_percpu_rwsem_exit,
.writelock = torture_percpu_rwsem_down_write,
.write_delay = torture_rwsem_write_delay,
.task_boost = torture_boost_dummy,
@@ -786,9 +794,10 @@ static void lock_torture_cleanup(void)
/*
* Indicates early cleanup, meaning that the test has not run,
- * such as when passing bogus args when loading the module. As
- * such, only perform the underlying torture-specific cleanups,
- * and avoid anything related to locktorture.
+ * such as when passing bogus args when loading the module.
+ * However cxt->cur_ops.init() may have been invoked, so beside
+ * perform the underlying torture-specific cleanups, cur_ops.exit()
+ * will be invoked if needed.
*/
if (!cxt.lwsa && !cxt.lrsa)
goto end;
@@ -828,6 +837,11 @@ static void lock_torture_cleanup(void)
cxt.lrsa = NULL;
end:
+ if (cxt.init_called) {
+ if (cxt.cur_ops->exit)
+ cxt.cur_ops->exit();
+ cxt.init_called = false;
+ }
torture_cleanup_end();
}
@@ -875,8 +889,10 @@ static int __init lock_torture_init(void)
goto unwind;
}
- if (cxt.cur_ops->init)
+ if (cxt.cur_ops->init) {
cxt.cur_ops->init();
+ cxt.init_called = true;
+ }
if (nwriters_stress >= 0)
cxt.nrealwriters_stress = nwriters_stress;
--
2.25.0.4.g0ad7144999
On Thu, Sep 24, 2020 at 10:18:54PM +0800, Hou Tao wrote:
> When do percpu-rwsem writer lock torture, the RCU callback
> rcu_sync_func() may still be pending after locktorture module
> is removed, and it will lead to the following Oops:
>
> BUG: unable to handle page fault for address: ffffffffc00eb920
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 6500a067 P4D 6500a067 PUD 6500c067 PMD 13a36c067 PTE 800000013691c163
> Oops: 0000 [#1] PREEMPT SMP
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.9.0-rc5+ #4
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
> RIP: 0010:rcu_cblist_dequeue+0x12/0x30
> Call Trace:
> <IRQ>
> rcu_core+0x1b1/0x860
> __do_softirq+0xfe/0x326
> asm_call_on_stack+0x12/0x20
> </IRQ>
> do_softirq_own_stack+0x5f/0x80
> irq_exit_rcu+0xaf/0xc0
> sysvec_apic_timer_interrupt+0x2e/0xb0
> asm_sysvec_apic_timer_interrupt+0x12/0x20
>
> Fix it by adding an exit hook in lock_torture_ops and
> use it to call percpu_free_rwsem() for percpu rwsem torture
> before the module is removed, so we can ensure rcu_sync_func()
> completes before module exits.
>
> Also needs to call exit hook if lock_torture_init() fails half-way,
> so add init_called field in lock_torture_cxt to indicate that
> init hook has been called.
>
> Signed-off-by: Hou Tao <[email protected]>
Nice, thank you! I have queued this for v5.11.
As always, I could not resist reworking the commit log as below, and
as always, please check to make sure that I didn't mess anything up.
Thanx, Paul
------------------------------------------------------------------------
commit f794b408207152995aefdf41ee9a3f5c974f622a
Author: Hou Tao <[email protected]>
Date: Thu Sep 24 22:18:54 2020 +0800
locktorture: Invoke percpu_free_rwsem() to do percpu-rwsem cleanup
When executing the LOCK06 locktorture scenario featuring percpu-rwsem,
the RCU callback rcu_sync_func() may still be pending after locktorture
module is removed. This can in turn lead to the following Oops:
BUG: unable to handle page fault for address: ffffffffc00eb920
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 6500a067 P4D 6500a067 PUD 6500c067 PMD 13a36c067 PTE 800000013691c163
Oops: 0000 [#1] PREEMPT SMP
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.9.0-rc5+ #4
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
RIP: 0010:rcu_cblist_dequeue+0x12/0x30
Call Trace:
<IRQ>
rcu_core+0x1b1/0x860
__do_softirq+0xfe/0x326
asm_call_on_stack+0x12/0x20
</IRQ>
do_softirq_own_stack+0x5f/0x80
irq_exit_rcu+0xaf/0xc0
sysvec_apic_timer_interrupt+0x2e/0xb0
asm_sysvec_apic_timer_interrupt+0x12/0x20
This commit avoids tis problem by adding an exit hook in lock_torture_ops
and using it to call percpu_free_rwsem() for percpu rwsem torture during
the module-cleanup function, thus ensuring that rcu_sync_func() completes
before module exits.
It is also necessary to call the exit hook if lock_torture_init()
fails half-way, so this commit also adds an ->init_called field in
lock_torture_cxt to indicate that exit hook, if present, must be called.
Signed-off-by: Hou Tao <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 79fbd97..fd838ce 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -76,6 +76,7 @@ static void lock_torture_cleanup(void);
*/
struct lock_torture_ops {
void (*init)(void);
+ void (*exit)(void);
int (*writelock)(void);
void (*write_delay)(struct torture_random_state *trsp);
void (*task_boost)(struct torture_random_state *trsp);
@@ -92,12 +93,13 @@ struct lock_torture_cxt {
int nrealwriters_stress;
int nrealreaders_stress;
bool debug_lock;
+ bool init_called;
atomic_t n_lock_torture_errors;
struct lock_torture_ops *cur_ops;
struct lock_stress_stats *lwsa; /* writer statistics */
struct lock_stress_stats *lrsa; /* reader statistics */
};
-static struct lock_torture_cxt cxt = { 0, 0, false,
+static struct lock_torture_cxt cxt = { 0, 0, false, false,
ATOMIC_INIT(0),
NULL, NULL};
/*
@@ -573,6 +575,11 @@ static void torture_percpu_rwsem_init(void)
BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
}
+static void torture_percpu_rwsem_exit(void)
+{
+ percpu_free_rwsem(&pcpu_rwsem);
+}
+
static int torture_percpu_rwsem_down_write(void) __acquires(pcpu_rwsem)
{
percpu_down_write(&pcpu_rwsem);
@@ -597,6 +604,7 @@ static void torture_percpu_rwsem_up_read(void) __releases(pcpu_rwsem)
static struct lock_torture_ops percpu_rwsem_lock_ops = {
.init = torture_percpu_rwsem_init,
+ .exit = torture_percpu_rwsem_exit,
.writelock = torture_percpu_rwsem_down_write,
.write_delay = torture_rwsem_write_delay,
.task_boost = torture_boost_dummy,
@@ -789,9 +797,10 @@ static void lock_torture_cleanup(void)
/*
* Indicates early cleanup, meaning that the test has not run,
- * such as when passing bogus args when loading the module. As
- * such, only perform the underlying torture-specific cleanups,
- * and avoid anything related to locktorture.
+ * such as when passing bogus args when loading the module.
+ * However cxt->cur_ops.init() may have been invoked, so beside
+ * perform the underlying torture-specific cleanups, cur_ops.exit()
+ * will be invoked if needed.
*/
if (!cxt.lwsa && !cxt.lrsa)
goto end;
@@ -831,6 +840,11 @@ static void lock_torture_cleanup(void)
cxt.lrsa = NULL;
end:
+ if (cxt.init_called) {
+ if (cxt.cur_ops->exit)
+ cxt.cur_ops->exit();
+ cxt.init_called = false;
+ }
torture_cleanup_end();
}
@@ -878,8 +892,10 @@ static int __init lock_torture_init(void)
goto unwind;
}
- if (cxt.cur_ops->init)
+ if (cxt.cur_ops->init) {
cxt.cur_ops->init();
+ cxt.init_called = true;
+ }
if (nwriters_stress >= 0)
cxt.nrealwriters_stress = nwriters_stress;