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

[Alexander Atanasov]

On 2.02.23 9:21, Pavel Tikhomirov wrote:
> 
> 
> 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.
Ok.

> 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.

I tried to find if it is from a rebase or it was fixed in vz7


>>
>> 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)
>>>
>>
> 

-- 
Regards,
Alexander Atanasov