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

[no-reply@sashiko.virtuozzo.com]

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

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?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022-ovl-use-orig-ve-v1-0-da84c00c8065@virtuozzo.com?part=4

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