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

[Kirill Tkhai]

On 20.12.2017 12:24, Kirill Tkhai wrote:
> 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..92541dbc2a3f 100644
--- a/kernel/ve/vznetstat/vznetstat.c
+++ b/kernel/ve/vznetstat/vznetstat.c
@@ -88,15 +88,11 @@ 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);
+	/* xchg() implies rcu_assign_pointer() barriers */
 
 	synchronize_net();
 	/* IMPORTANT. I think reset of statistics collected should not be