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

[no-reply@sashiko.virtuozzo.com]

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?

> +
>  #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:

#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?

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

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