Re: [Devel] [PATCH hci-8.1 2/4] ve: Introduce override_ve/revert_ve helpers

[Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>]

On 10/22/26 17:10, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] ve: override_ve()/revert_ve() !CONFIG_VE stubs hidden inside CONFIG_X86 block
> - [Low] ve: override_ve()/revert_ve() store task_ve without WRITE_ONCE() or task_lock
> --
> 
> commit f94943d7cf0e7cf13a560e724d14ae8e02ad7271
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve: Introduce override_ve/revert_ve helpers
> 
> This commit adds override_ve() and revert_ve() helpers to include/linux/ve.h.
> They temporarily replace current->task_ve, in the same way that
> override_creds() and revert_creds() handle credentials. Stubs for
> CONFIG_VE=n are added as well.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405
> 
>> diff --git a/include/linux/ve.h b/include/linux/ve.h
>> index 717567775f58..157326ef91b9 100644
>> --- a/include/linux/ve.h
>> +++ b/include/linux/ve.h
>> @@ -295,6 +295,19 @@ static inline struct vdso_image *ve_get_vdso_64(struct ve_struct *ve)
>>   	return ve->vdso_64;
>>   }
>>   
>> +static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
>> +{
>> +	struct ve_struct *old_ve = current->task_ve;
>> +
>> +	current->task_ve = new_ve;
>> +	return old_ve;
>> +}
>> +
>> +static inline void revert_ve(struct ve_struct *orig_ve)
>> +{
>> +	current->task_ve = orig_ve;
>> +}
> 
> [Severity: Low]
> Can these plain stores to current->task_ve race with remote readers?
> 
> task_ve() reads the field with READ_ONCE(), and get_task_ve() reads it
> under task_lock:
> 
> include/linux/ve.h:
>      return READ_ONCE(task->task_ve);
>      ...
>      guard(task_lock)(task);
>      return get_ve(task_ve(task));
> 
> override_ve() and revert_ve() use neither WRITE_ONCE() nor task_lock, so
> the lock gives get_ve() no exclusion against them. The existing writer
> follows a different convention:
> 
> kernel/ve/ve_namespace.c:switch_ve_namespace() {
>      scoped_guard(task_lock, p) {
>          old = p->ve_ns;
>          p->ve_ns = new;
>          p->task_ve = new ? new->ve : &ve0;
>      }
> }
> 
> A remote task can hit this window while the overlayfs task is between
> ovl_override_creds() and ovl_revert_creds(). For example, /proc code
> may call task_pid_ve_nr(tsk) or get_task_ve(tsk) on that task. The reader
> can then see creator_ve instead of the task's real VE, while ve_ns still
> refers to the original one. task_pid_ve_nr() could then transiently
> return a wrong or zero pid.
> 
> Should both helpers use WRITE_ONCE(), and should the store be done under
> task_lock so that get_task_ve() cannot observe the transient value?

Will add locking and WRITE_ONCE in v2.

> 
>> +
>>   #else	/* CONFIG_VE */
>>   #include <linux/init_task.h>
>>   #include <net/net_namespace.h>
> 
> [ ... ]
> 
>> @@ -403,6 +416,14 @@ static inline struct vdso_image *ve_get_vdso_64(struct vdso_image *ve)
>>   {
>>   	return (struct vdso_image *)&vdso_image_64;
>>   }
>> +
>> +static inline struct ve_struct *override_ve(struct ve_struct *new_ve)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline void revert_ve(struct ve_struct *orig_ve) {}
>> +
>>   #endif /* CONFIG_X86 */
>>   
>>   #endif	/* CONFIG_VE */
> 
> [Severity: Low]
> Are these CONFIG_VE=n stubs placed correctly? They sit inside the
> CONFIG_X86 block, before the closing line:

Nope, wrong place. Will fix in v2.

> 
> #endif /* CONFIG_X86 */
> 
> With CONFIG_VE=n and CONFIG_X86 unset (for example arm64 or UML),
> override_ve() and revert_ve() are never defined. The CONFIG_VE=y versions
> are defined unconditionally.
> 
> Patch 4/4 of the series calls them from the generic file
> fs/overlayfs/util.c, in ovl_override_creds() and ovl_revert_creds():
> 
>      desc.ve = override_ve(ofs->creator_ve);
>      ...
>      revert_ve(old_creds->ve);
> 
> Would that fail to build on those configurations with an implicit
> declaration error? Neither patch 3 nor patch 4 touches include/linux/ve.h,
> so the series does not fix this later. Could the stubs be moved after the
> closing line of the CONFIG_X86 block?
> 

-- 
Best regards, Riabchun Vladimir
Linux Kernel Developer, Virtuozzo

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel