[Devel] [PATCH 1/2] vznetstat: Add protection to venet_acct_set_classes()

[Kirill Tkhai]

On 20.12.2017 11:53, Andrey Ryabinin wrote:
> 
> 
> On 12/19/2017 03:24 PM, Kirill Tkhai wrote:
>> It seems there was no synchronization since the time
>> when ioctls in kernel were serialized via single mutex.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>> ---
>>  kernel/ve/vznetstat/vznetstat.c |   11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/ve/vznetstat/vznetstat.c b/kernel/ve/vznetstat/vznetstat.c
>> index 3a53ce27bde2..a65e05378ff4 100644
>> --- a/kernel/ve/vznetstat/vznetstat.c
>> +++ b/kernel/ve/vznetstat/vznetstat.c
>> @@ -52,6 +52,7 @@ static struct class_info_set *info_v4 = NULL;
>>  #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>  static struct class_info_set *info_v6 = NULL;
>>  #endif
>> +static DEFINE_MUTEX(info_mutex);
>>  
>>  /* v6: flag IPv6 classes or IPv4 */
>>  static int venet_acct_set_classes(const void __user *user_info, int length, int v6)
>> @@ -88,15 +89,17 @@ static int venet_acct_set_classes(const void __user *user_info, int length, int
>>  			goto out_free;
>>  	}
>>  
>> -	rcu_read_lock();
>> +	mutex_lock(&info_mutex);
>>  	if (v6) {
>> -		old = rcu_dereference(info_v6);
>> +		old = rcu_dereference_protected(info_v6,
>> +						lockdep_is_held(&info_mutex));
>>  		rcu_assign_pointer(info_v6, info);
>>  	} else {
>> -		old = rcu_dereference(info_v4);
>> +		old = rcu_dereference_protected(info_v4,
>> +						lockdep_is_held(&info_mutex));
>>  		rcu_assign_pointer(info_v4, info);
> 
> It probably would be easier to simply use xchg() here. Locking wouldn't be needed in that case.
> But as I assume this is not performance sensitive code, so mutex should be fine.
> 
> Acked-by: Andrey Ryabinin <aryabinin at virtuozzo.com>

How you about this?

vznetstat: Add protection to venet_acct_set_classes()
    
It seems there was no synchronization since the time
when ioctls in kernel were serialized via single mutex.

Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
---
diff --git a/kernel/ve/vznetstat/vznetstat.c b/kernel/ve/vznetstat/vznetstat.c
index 3a53ce27bde2..c67ecfeeaf12 100644
--- a/kernel/ve/vznetstat/vznetstat.c
+++ b/kernel/ve/vznetstat/vznetstat.c
@@ -88,15 +88,12 @@ static int venet_acct_set_classes(const void __user *user_info, int length, int
 			goto out_free;
 	}
 
-	rcu_read_lock();
-	if (v6) {
-		old = rcu_dereference(info_v6);
-		rcu_assign_pointer(info_v6, info);
-	} else {
-		old = rcu_dereference(info_v4);
-		rcu_assign_pointer(info_v4, info);
-	}
-	rcu_read_unlock();
+	if (v6)
+		old = xchg(&info_v6, info);
+	else
+		old = xchg(&info_v4, info);
+
+	smp_mb__after_atomic(); /* Implies rcu_assign_pointer() barriers */
 
 	synchronize_net();
 	/* IMPORTANT. I think reset of statistics collected should not be