[Devel] [PATCH vz9 1/2] ve/cgroups: fix use after free in ve_exit_ns

[Pavel Tikhomirov]

On 02.02.2023 14:34, Alexander Atanasov wrote:
> On 1.02.23 15:06, Pavel Tikhomirov wrote:
>> Nacked-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
>>
>> This is not last reference to ve. Last reference is removed when we 
>> remove cgroup directory with rmdir. One cant remove cgroup until there 
>> is a process in it. ve_exit_ns is called from task in this ve cgroup.
> 
> Then should we fix vz7 do the same?

I don't say that after your change we get bad code, I say that your 
explanation about "last reference" is wrong.

But, yes, I can't find any reason why we need this put under lock. So, 
sorry I was to hasty, we can still apply patch, but please remove the 
commit message part about last reference.

And also probably mention that this change came at some point of rebase 
from vz7 and seems not needed, so let's return to vz7 variant.

> 
> if it is really not the last reference - why does it need to hold the 
> mutex do drop the reference ?
> And i haven't checked how they are destroyed on reboot.
> 
>>
>> On 30.01.2023 13:00, Alexander Atanasov wrote:
>>> Release the lock before dropping the last reference to ve in
>>> ve_exit_ns which can lead to a call to ve_destroy which in turns
>>> does free the ve. In general it is always a bug to drop a reference
>>> of an object with locks held inside of it.
>>>
>>> https://jira.sw.ru/browse/PSBM-144580
>>> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
>>> ---
>>>   kernel/ve/ve.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> i've checked vz7 and it does not have this issue.
>>>
>>> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
>>> index 407d7de6e071..80865161670e 100644
>>> --- a/kernel/ve/ve.c
>>> +++ b/kernel/ve/ve.c
>>> @@ -857,9 +857,11 @@ void ve_exit_ns(struct pid_namespace *pid_ns)
>>>       ve_hook_iterate_fini(VE_SS_CHAIN, ve);
>>>       ve_list_del(ve);
>>>       ve_drop_context(ve);
>>> +    up_write(&ve->op_sem);
>>> +
>>>       printk(KERN_INFO "CT: %s: stopped\n", ve_name(ve));
>>> +
>>>       put_ve(ve); /* from ve_start_container() */
>>> -    up_write(&ve->op_sem);
>>>   }
>>>   u64 ve_get_monotonic(struct ve_struct *ve)
>>
> 

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.