linux-kernel - Re: [PATCH 2/2] locktorture: call percpu_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fe8c274e-efa4-04ec-0d95-d7c49ec4dd83@huawei.com>
Date:   Wed, 23 Sep 2020 10:24:20 +0800
From:   Hou Tao <houtao1@...wei.com>
To:     <paulmck@...nel.org>
CC:     Davidlohr Bueso <dave@...olabs.net>,
        Josh Triplett <josh@...htriplett.org>,
        <linux-kernel@...r.kernel.org>, <rcu@...r.kernel.org>
Subject: Re: [PATCH 2/2] locktorture: call percpu_free_rwsem() to do
 percpu-rwsem cleanup

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 <houtao1@...wei.com>
>> ---
>>  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.