Re: [Devel] [PATCH hci-8.1 4/4] ve/overlayfs: Override VE together with creds

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

On 10/22/26 17:17, no-reply@sashiko.virtuozzo.com wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Low] overlayfs: typo 'inforamtion' in new struct ovl_creds_desc comment

Will fix in v2.

> - [Low] ve: override_ve()/revert_ve() store current->task_ve without WRITE_ONCE or task_lock
> - [Low] ve: override_ve()/revert_ve() !CONFIG_VE stubs are x86-only; overlayfs now needs them

Already reported, will fix.

> --
> 
> commit f580a7811856a41f48d8fc37ebc70368ebd6d989
> Author: Vladimir Riabchun <vladimir.riabchun@virtuozzo.com>
> 
> ve/overlayfs: Override VE together with creds
> 
> This patch changes ovl_override_creds() to return a struct ovl_creds_desc
> holding both the old cred and the old VE. It adds ovl_revert_creds() to
> restore both, and converts all overlayfs callers. The aim is to make
> ve_capable checks behave correctly when a host user accesses an overlayfs
> created inside a container.
> 
> Link: https://virtuozzo.atlassian.net/browse/VSTOR-142405
> 
>> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> --- a/fs/overlayfs/overlayfs.h
>> +++ b/fs/overlayfs/overlayfs.h
> 
> [ ... ]
> 
>> +/* Credential inforamtion used by overlay */
>> +struct ovl_creds_desc {
>> +	const struct cred *cred;
>> +	struct ve_struct *ve;
>> +};
> 
> [Severity: Low]
> This isn't a bug, but there is a typo in the new comment: "inforamtion"
> should be "information".
> 
> The commit message also has typos: "swich" should be "switch", and
> "Stroing" should be "Storing".
> 
>> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
>> --- a/fs/overlayfs/util.c
>> +++ b/fs/overlayfs/util.c
> 
> [ ... ]
> 
>> +struct ovl_creds_desc ovl_override_creds(struct super_block *sb)
>>   {
>>   	struct ovl_fs *ofs = OVL_FS(sb);
>> +	struct ovl_creds_desc desc;
>>   
>> -	return override_creds(ofs->creator_cred);
>> +	desc.cred = override_creds(ofs->creator_cred);
>> +	desc.ve = override_ve(ofs->creator_ve);
>> +
>> +	return desc;
>> +}
>> +
>> +void ovl_revert_creds(struct ovl_creds_desc *old_creds)
>> +{
>> +	revert_ve(old_creds->ve);
>> +	revert_creds(old_creds->cred);
>>   }
> 
> [Severity: Low]
> Will this build when CONFIG_VE is disabled and CONFIG_X86 is not set, for
> example on UML or on non-x86 architectures?
> 
> In include/linux/ve.h the !CONFIG_VE stubs for override_ve() and
> revert_ve() sit inside the "#ifdef CONFIG_X86" block of the !CONFIG_VE
> branch:
> 
>      #ifdef CONFIG_X86
>      ...
>      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 */
> 
> ovl_override_creds() and ovl_revert_creds() now call these helpers
> unconditionally. fs/overlayfs/Kconfig has no dependency on VE or X86, and
> CONFIG_VE is a bool that can be turned off.
> 
> With CONFIG_OVERLAY_FS enabled, CONFIG_VE off and CONFIG_X86 unset, the
> helpers have no declaration, so fs/overlayfs/util.c would fail with an
> implicit declaration error. Should the stubs be moved outside the
> CONFIG_X86 block?
> 
> [Severity: Low]
> The helpers called here, override_ve() and revert_ve() in
> include/linux/ve.h, store current->task_ve with a plain write:
> 
>      struct ve_struct *old_ve = current->task_ve;
> 
>      current->task_ve = new_ve;
>      return old_ve;
> 
> Remote readers such as get_task_ve() take task_lock() and use READ_ONCE()
> on task_ve. The writers take neither task_lock nor use WRITE_ONCE().
> 
> Before this patch nothing outside ve.h used these helpers. Now every
> ovl_override_creds() caller, for example ovl_lookup(), ovl_iterate() and
> ovl_copy_up_flags(), flips task_ve for the whole duration of operations that
> can sleep. During that window a remote get_task_ve() caller, for example
> sched_show_task() or update_sched_lat(), can race with the store. KCSAN
> would flag this. The remote reader can also see the creator VE instead of
> the task's own VE.
> 
> Would it make sense to use WRITE_ONCE() in override_ve() and revert_ve(),
> and possibly take task_lock() as well?
> 

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

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