From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Tikhomirov Date: Wed, 12 Aug 2026 18:48:43 +0200 Subject: Re: [Devel] [PATCH vz10 v2] cgroup: use proper lockdep condition for ve_nsproxy dereference In-Reply-To: <20260812164222.173835-1-khorenko@virtuozzo.com> References: <32f00c03-fe20-47a1-b629-0f95f4a6e343@virtuozzo.com> <20260812164222.173835-1-khorenko@virtuozzo.com> Message-ID: List-Id: On 8/12/26 18:42, Konstantin Khorenko wrote: > cgroup_mark_ve_roots(), cgroup_unmark_ve_roots(), cgroup_join_vz_slice() > and cgroup_leave_vz_slice() dereference ve->ve_nsproxy with > rcu_dereference_protected(..., 1), which unconditionally silences lockdep > without naming the actual lock that makes the access safe. > > All four are only called from ve_start_container() and ve_exit_ns(), both > of which run under ve->op_sem write-lock. The pointer is stable there > because ve_grab_context() publishes it via rcu_assign_pointer() and > ve_drop_context() - the only path that clears it - cannot run > concurrently since it also requires op_sem. > > Replace the unconditional "1" with lockdep_is_held(&ve->op_sem) so that > lockdep can actually verify the locking at runtime. This is consistent > with how ve_grab_context(), ve_drop_context(), ve_stop_ns() and > ve_exit_ns() already annotate the same pointer. Store the result in a > local variable to keep the lines readable, and extend the existing > comment to mention op_sem. > > No functional change: the condition is only evaluated under > CONFIG_PROVE_RCU. > > https://virtuozzo.atlassian.net/browse/VSTOR-128317 > > Feature: ve: ve generic structures > Signed-off-by: Konstantin Khorenko Reviewed-by: Pavel Tikhomirov > --- > kernel/cgroup/cgroup.c | 54 ++++++++++++++++++++++++++++++++++-------- > 1 file changed, 44 insertions(+), 10 deletions(-) > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c > index 4db77090f1be1..948a2355df833 100644 > --- a/kernel/cgroup/cgroup.c > +++ b/kernel/cgroup/cgroup.c > @@ -2179,15 +2179,21 @@ static inline bool ve_check_root_cgroups(struct css_set *cset) > int cgroup_mark_ve_roots(struct ve_struct *ve) > { > struct cgrp_cset_link *link; > + struct nsproxy *ve_nsproxy; > struct css_set *cset; > struct cgroup *cgrp; > > /* > - * It's safe to use ve->ve_nsproxy->cgroup_ns->root_cset here without extra > - * locking as we do it from container init at container start after > - * ve_grab_context and only container init can tear those down. > + * It's safe to use ve->ve_nsproxy->cgroup_ns->root_cset here without > + * extra locking as we do it from container init at container start > + * after ve_grab_context and only container init can tear those down. > + * Still ve->op_sem is held here which serializes ve->ve_nsproxy usage > + * against ve_drop_context(), so let lockdep verify the context without > + * external knowledge. > */ > - cset = rcu_dereference_protected(ve->ve_nsproxy, 1)->cgroup_ns->root_cset; > + ve_nsproxy = rcu_dereference_protected(ve->ve_nsproxy, > + lockdep_is_held(&ve->op_sem)); > + cset = ve_nsproxy->cgroup_ns->root_cset; > BUG_ON(!cset); > > spin_lock_irq(&css_set_lock); > @@ -2217,15 +2223,21 @@ int cgroup_mark_ve_roots(struct ve_struct *ve) > void cgroup_unmark_ve_roots(struct ve_struct *ve) > { > struct cgrp_cset_link *link; > + struct nsproxy *ve_nsproxy; > struct css_set *cset; > struct cgroup *cgrp; > > /* > - * It's safe to use ve->ve_nsproxy->cgroup_ns->root_cset here without extra > - * locking as we do it from container init at container start after > - * ve_grab_context and only container init can tear those down. > + * It's safe to use ve->ve_nsproxy->cgroup_ns->root_cset here without > + * extra locking as we do it from container init at container start > + * after ve_grab_context and only container init can tear those down. > + * Still ve->op_sem is held here which serializes ve->ve_nsproxy usage > + * against ve_drop_context(), so let lockdep verify the context without > + * external knowledge. > */ > - cset = rcu_dereference_protected(ve->ve_nsproxy, 1)->cgroup_ns->root_cset; > + ve_nsproxy = rcu_dereference_protected(ve->ve_nsproxy, > + lockdep_is_held(&ve->op_sem)); > + cset = ve_nsproxy->cgroup_ns->root_cset; > BUG_ON(!cset); > > spin_lock_irq(&css_set_lock); > @@ -2247,12 +2259,23 @@ void cgroup_unmark_ve_roots(struct ve_struct *ve) > > int cgroup_join_vz_slice(struct ve_struct *ve) > { > + struct nsproxy *ve_nsproxy; > struct kernfs_node *kn; > struct css_set *cset; > struct cgroup *cgrp; > int ret; > > - cset = rcu_dereference_protected(ve->ve_nsproxy, 1)->cgroup_ns->root_cset; > + /* > + * It's safe to use ve->ve_nsproxy->cgroup_ns->root_cset here without > + * extra locking as we do it from container init at container start > + * after ve_grab_context and only container init can tear those down. > + * Still ve->op_sem is held here which serializes ve->ve_nsproxy usage > + * against ve_drop_context(), so let lockdep verify the context without > + * external knowledge. > + */ > + ve_nsproxy = rcu_dereference_protected(ve->ve_nsproxy, > + lockdep_is_held(&ve->op_sem)); > + cset = ve_nsproxy->cgroup_ns->root_cset; > cgrp = __cset_cgroup_from_root(cset, &cgrp_dfl_root); > > if (!is_virtualized_cgroup(cgrp) || > @@ -2286,11 +2309,22 @@ int cgroup_join_vz_slice(struct ve_struct *ve) > > int cgroup_leave_vz_slice(struct ve_struct *ve) > { > + struct nsproxy *ve_nsproxy; > struct css_set *cset; > struct cgroup *cgrp; > int ret; > > - cset = rcu_dereference_protected(ve->ve_nsproxy, 1)->cgroup_ns->root_cset; > + /* > + * It's safe to use ve->ve_nsproxy->cgroup_ns->root_cset here without > + * extra locking as we do it from container init at container start > + * after ve_grab_context and only container init can tear those down. > + * Still ve->op_sem is held here which serializes ve->ve_nsproxy usage > + * against ve_drop_context(), so let lockdep verify the context without > + * external knowledge. > + */ > + ve_nsproxy = rcu_dereference_protected(ve->ve_nsproxy, > + lockdep_is_held(&ve->op_sem)); > + cset = ve_nsproxy->cgroup_ns->root_cset; > cgrp = __cset_cgroup_from_root(cset, &cgrp_dfl_root); > > if (!is_virtualized_cgroup(cgrp) || -- Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo.