Re: [PATCH v2 1/1] cgroup: replace global percpu_rwsem with signal_struct->group_rwsem when writing cgroup.procs/threads

[escape <escape@linux.alibaba.com>]

Hello,

在 2025/9/5 10:27, Tejun Heo 写道:
> Hello,
>
> On Fri, Sep 05, 2025 at 10:16:30AM +0800, escape wrote:
>>>> +	if (have_favordynmods)
>>>> +		up_read(&tsk->signal->group_rwsem);
>>>>    	percpu_up_read(&cgroup_threadgroup_rwsem);
>>> Hmm... I wonder whether turning on/off the flag is racy. ie. what prevents
>>> have_favordynmods flipping while a task is between change_begin and end?
>> have_favordynmods is read-only after initialization and will not change
>> during runtime.
> I don't think that's necessarily true. favordynmods can also be specified as
> a mount option and mount can race against forks, execs and exits. Also,
> IIRC, cgroup2 doesn't allow remounts but there's nothing preventing someone
> from unmounting and mounting it again with different options.
There are two ways to set favordynmods. The first is through kernel 
config or
the kernel command line parameter cgroup_favordynmods, which sets the 
value of
the variable have_favordynmods and automatically sets the 
CGRP_ROOT_FAVOR_DYNMODS
flag on cgroup_root->flags at mount time. The second is by passing a 
mount option,
which only sets the CGRP_ROOT_FAVOR_DYNMODS flag on cgroup_root->flags 
during mount.

In this patch, the decision whether to use the per-threadgroup rwsem is 
based on
have_favordynmods, not on cgroup_root->flags. An umount & mount affects 
cgroup_root->flags,
but does not change have_favordynmods.

Indeed, there is a minor flaw here: if favordynmods is enabled via a 
mount option rather
than the kernel command line, the per-threadgroup rwsem will not take 
effect.

To avoid the complexity of runtime changes to favordynmods, perhaps a 
better approach
would be to introduce a separate control variable, configured via a 
kernel command line
parameter such as cgroup_migration_lock=[global|thread_group] to 
explicitly govern
this behavior. What do you think about this approach?
>>>> @@ -3010,15 +3008,27 @@ struct task_struct *cgroup_procs_write_start(char *buf, bool threadgroup,
>>>>    	 */
>>>>    	if (tsk->no_cgroup_migration || (tsk->flags & PF_NO_SETAFFINITY)) {
>>>>    		tsk = ERR_PTR(-EINVAL);
>>>> -		goto out_unlock_threadgroup;
>>>> +		goto out_unlock_rcu;
>>>>    	}
>>>> -
>>>>    	get_task_struct(tsk);
>>>> -	goto out_unlock_rcu;
>>>> -out_unlock_threadgroup:
>>>> -	cgroup_attach_unlock(*threadgroup_locked);
>>>> -	*threadgroup_locked = false;
>>>> +	rcu_read_unlock();
>>>> +
>>>> +	/*
>>>> +	 * If we migrate a single thread, we don't care about threadgroup
>>>> +	 * stability. If the thread is `current`, it won't exit(2) under our
>>>> +	 * hands or change PID through exec(2). We exclude
>>>> +	 * cgroup_update_dfl_csses and other cgroup_{proc,thread}s_write
>>>> +	 * callers by cgroup_mutex.
>>>> +	 * Therefore, we can skip the global lock.
>>>> +	 */
>>>> +	lockdep_assert_held(&cgroup_mutex);
>>>> +	*threadgroup_locked = pid || threadgroup;
>>>> +
>>>> +	cgroup_attach_lock(tsk, *threadgroup_locked);
>>> I'm not sure this relocation is safe. What prevents e.g. @tsk changing its
>>> group leader or signal struct before lock is grabbed?
>> When a non-leader thread in a thread group executes the exec system call,
>> the thread group leader is updated, but the signal_struct remains unchanged,
>> so this part is safe.
> But the leader can change, right? So, we can end up in a situation where
> threadgroup is set but the task is not the leader which I think can lead to
> really subtle incorrect behaviors like write succeeding but nothing
> happening when racing against exec.
>
> Thanks.
>
You are right. If the thread group leader changes, the task passed to
cgroup_attach_task may not be the leader, which could lead to errors when
iterating through all threads in while_each_thread.


Similar to commit b78949ebfb56 ("cgroup: simplify double-check locking in
cgroup_attach_proc"), it should check whether the leader has changed after
acquiring the lock, to determine if a retry is needed. I will fix this in
the next version patch.

Thanks.