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

Submitted by Kirill Tkhai on Dec. 20, 2017, 9:30 a.m.

Details

Message ID 379c2998-784a-3d8a-70f8-3725e81b7c40@virtuozzo.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Kirill Tkhai Dec. 20, 2017, 9:30 a.m.
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@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@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@virtuozzo.com>
---

Patch hide | download patch | download mbox

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

Comments

Andrey Ryabinin Dec. 20, 2017, 9:47 a.m.
On 12/20/2017 12:30 PM, Kirill Tkhai wrote:

>> 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@virtuozzo.com>
> ---


Acked-by: Andrey Ryabinin <aryabinin@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
>