From: Zhang Qiao <[email protected]>
commit 05c7b7a92cc87ff8d7fde189d0fade250697573c upstream.
As previously discussed(https://lkml.org/lkml/2022/1/20/51),
cpuset_attach() is affected with similar cpu hotplug race,
as follow scenario:
cpuset_attach() cpu hotplug
--------------------------- ----------------------
down_write(cpuset_rwsem)
guarantee_online_cpus() // (load cpus_attach)
sched_cpu_deactivate
set_cpu_active()
// will change cpu_active_mask
set_cpus_allowed_ptr(cpus_attach)
__set_cpus_allowed_ptr_locked()
// (if the intersection of cpus_attach and
cpu_active_mask is empty, will return -EINVAL)
up_write(cpuset_rwsem)
To avoid races such as described above, protect cpuset_attach() call
with cpu_hotplug_lock.
Fixes: be367d099270 ("cgroups: let ss->can_attach and ss->attach do whole threadgroups at a time")
Cc: [email protected] # v2.6.32+
Reported-by: Zhao Gongyi <[email protected]>
Signed-off-by: Zhang Qiao <[email protected]>
Acked-by: Waiman Long <[email protected]>
Reviewed-by: Michal Koutný <[email protected]>
Signed-off-by: Tejun Heo <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
kernel/cgroup/cpuset.c | 2 ++
1 file changed, 2 insertions(+)
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1528,6 +1528,7 @@ static void cpuset_attach(struct cgroup_
cgroup_taskset_first(tset, &css);
cs = css_cs(css);
+ cpus_read_lock();
mutex_lock(&cpuset_mutex);
/* prepare for attach */
@@ -1583,6 +1584,7 @@ static void cpuset_attach(struct cgroup_
wake_up(&cpuset_attach_wq);
mutex_unlock(&cpuset_mutex);
+ cpus_read_unlock();
}
/* The various types of files and directories in a cpuset file system */
Hello.
On Mon, Feb 28, 2022 at 06:24:07PM +0100, Greg Kroah-Hartman <[email protected]> wrote:
> [...]
> cpuset_attach() cpu hotplug
> --------------------------- ----------------------
> down_write(cpuset_rwsem)
> guarantee_online_cpus() // (load cpus_attach)
> sched_cpu_deactivate
> set_cpu_active()
> // will change cpu_active_mask
> set_cpus_allowed_ptr(cpus_attach)
> __set_cpus_allowed_ptr_locked()
> // (if the intersection of cpus_attach and
> cpu_active_mask is empty, will return -EINVAL)
> up_write(cpuset_rwsem)
> [...]
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -1528,6 +1528,7 @@ static void cpuset_attach(struct cgroup_
> cgroup_taskset_first(tset, &css);
> cs = css_cs(css);
>
> + cpus_read_lock();
> mutex_lock(&cpuset_mutex);
This backport (and possible older kernels) looks suspicious since it comes
before commit d74b27d63a8b ("cgroup/cpuset: Change cpuset_rwsem and
hotplug lock order") v5.4-rc1~176^2~30 when the locking order was:
cpuset lock, cpus lock.
At the same time it also comes before commit 710da3c8ea7d ("sched/core:
Prevent race condition between cpuset and __sched_setscheduler()")
v5.4-rc1~176^2~27 when neither __sched_setscheduler() cared and this
race is similar. (The swapped locking may still conflict with
rebuild_sched_domains() before d74b27d63a8b.)
Michal
Hello.
In my opinion there are two approaches:
a) drop this backport (given other races present),
b) swap the locks compatible with v4.19 as this patch proposes.
On Mon, Mar 14, 2022 at 05:11:50PM +0800, Zhang Qiao <[email protected]> wrote:
> + /*
> + * It should hold cpus lock because a cpu offline event can
> + * cause set_cpus_allowed_ptr() failed.
> + */
> + cpus_read_lock();
Maybe just a nit, the old kernels before commit c5c63b9a6a2e ("cgroup:
Replace deprecated CPU-hotplug functions.") v5.15-rc1~159^2~5
would be more consistent with get_online_cpus() here (but they're
equivalent functionally so the locking order is correct).
Michal
在 2022/3/14 16:06, Greg Kroah-Hartman 写道:
> On Tue, Mar 08, 2022 at 04:12:32PM +0100, Michal Koutný wrote:
>> Hello.
>>
>> On Mon, Feb 28, 2022 at 06:24:07PM +0100, Greg Kroah-Hartman <[email protected]> wrote:
>>> [...]
>>> cpuset_attach() cpu hotplug
>>> --------------------------- ----------------------
>>> down_write(cpuset_rwsem)
>>> guarantee_online_cpus() // (load cpus_attach)
>>> sched_cpu_deactivate
>>> set_cpu_active()
>>> // will change cpu_active_mask
>>> set_cpus_allowed_ptr(cpus_attach)
>>> __set_cpus_allowed_ptr_locked()
>>> // (if the intersection of cpus_attach and
>>> cpu_active_mask is empty, will return -EINVAL)
>>> up_write(cpuset_rwsem)
>>> [...]
>>> --- a/kernel/cgroup/cpuset.c
>>> +++ b/kernel/cgroup/cpuset.c
>>> @@ -1528,6 +1528,7 @@ static void cpuset_attach(struct cgroup_
>>> cgroup_taskset_first(tset, &css);
>>> cs = css_cs(css);
>>>
>>> + cpus_read_lock();
>>> mutex_lock(&cpuset_mutex);
>>
>> This backport (and possible older kernels) looks suspicious since it comes
>> before commit d74b27d63a8b ("cgroup/cpuset: Change cpuset_rwsem and
>> hotplug lock order") v5.4-rc1~176^2~30 when the locking order was:
>> cpuset lock, cpus lock.
>>
>> At the same time it also comes before commit 710da3c8ea7d ("sched/core:
>> Prevent race condition between cpuset and __sched_setscheduler()")
>> v5.4-rc1~176^2~27 when neither __sched_setscheduler() cared and this
>> race is similar. (The swapped locking may still conflict with
>> rebuild_sched_domains() before d74b27d63a8b.)
>
> Thanks for noticing this. What do you recommend to do to resolve this?
>
> thanks,
>
hi, Please review the following patch to fix it.
thanks.
patch:
[PATCH] cpuset: Fix unsafe lock order between cpuset lock and cpus lock
The backport commit 4eec5fe1c680a ("cgroup/cpuset: Fix a race
between cpuset_attach() and cpu hotplug") looks suspicious since
it comes before commit d74b27d63a8b ("cgroup/cpuset: Change
cpuset_rwsem and hotplug lock order") v5.4-rc1~176^2~30 when
the locking order was: cpuset lock, cpus lock.
Fix it with the correct locking order and reduce the cpus locking
range because only set_cpus_allowed_ptr() needs the protection of
cpus lock.
Fixes: 4eec5fe1c680a ("cgroup/cpuset: Fix a race between cpuset_attach() and cpu hotplug")
Reported-by: Michal Koutný <[email protected]>
Signed-off-by: Zhang Qiao <[email protected]>
---
kernel/cgroup/cpuset.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index d43d25acc..4e1c4232e 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1528,9 +1528,13 @@ static void cpuset_attach(struct cgroup_taskset *tset)
cgroup_taskset_first(tset, &css);
cs = css_cs(css);
- cpus_read_lock();
mutex_lock(&cpuset_mutex);
+ /*
+ * It should hold cpus lock because a cpu offline event can
+ * cause set_cpus_allowed_ptr() failed.
+ */
+ cpus_read_lock();
/* prepare for attach */
if (cs == &top_cpuset)
cpumask_copy(cpus_attach, cpu_possible_mask);
@@ -1549,6 +1553,7 @@ static void cpuset_attach(struct cgroup_taskset *tset)
cpuset_change_task_nodemask(task, &cpuset_attach_nodemask_to);
cpuset_update_task_spread_flag(cs, task);
}
+ cpus_read_unlock();
/*
* Change mm for all threadgroup leaders. This is expensive and may
@@ -1584,7 +1589,6 @@ static void cpuset_attach(struct cgroup_taskset *tset)
wake_up(&cpuset_attach_wq);
mutex_unlock(&cpuset_mutex);
- cpus_read_unlock();
}
/* The various types of files and directories in a cpuset file system */
--
2.18.0
> greg k-h
> .
>
On Tue, Mar 08, 2022 at 04:12:32PM +0100, Michal Koutn? wrote:
> Hello.
>
> On Mon, Feb 28, 2022 at 06:24:07PM +0100, Greg Kroah-Hartman <[email protected]> wrote:
> > [...]
> > cpuset_attach() cpu hotplug
> > --------------------------- ----------------------
> > down_write(cpuset_rwsem)
> > guarantee_online_cpus() // (load cpus_attach)
> > sched_cpu_deactivate
> > set_cpu_active()
> > // will change cpu_active_mask
> > set_cpus_allowed_ptr(cpus_attach)
> > __set_cpus_allowed_ptr_locked()
> > // (if the intersection of cpus_attach and
> > cpu_active_mask is empty, will return -EINVAL)
> > up_write(cpuset_rwsem)
> > [...]
> > --- a/kernel/cgroup/cpuset.c
> > +++ b/kernel/cgroup/cpuset.c
> > @@ -1528,6 +1528,7 @@ static void cpuset_attach(struct cgroup_
> > cgroup_taskset_first(tset, &css);
> > cs = css_cs(css);
> >
> > + cpus_read_lock();
> > mutex_lock(&cpuset_mutex);
>
> This backport (and possible older kernels) looks suspicious since it comes
> before commit d74b27d63a8b ("cgroup/cpuset: Change cpuset_rwsem and
> hotplug lock order") v5.4-rc1~176^2~30 when the locking order was:
> cpuset lock, cpus lock.
>
> At the same time it also comes before commit 710da3c8ea7d ("sched/core:
> Prevent race condition between cpuset and __sched_setscheduler()")
> v5.4-rc1~176^2~27 when neither __sched_setscheduler() cared and this
> race is similar. (The swapped locking may still conflict with
> rebuild_sched_domains() before d74b27d63a8b.)
Thanks for noticing this. What do you recommend to do to resolve this?
thanks,
greg k-h
在 2022/3/16 22:27, Greg Kroah-Hartman 写道:
> On Mon, Mar 14, 2022 at 12:19:41PM +0100, Michal Koutný wrote:
>> Hello.
>>
>> In my opinion there are two approaches:
>> a) drop this backport (given other races present),
>
> I have no problem with that, want to send a revert patch?
>
>> b) swap the locks compatible with v4.19 as this patch proposes.
>>
>> On Mon, Mar 14, 2022 at 05:11:50PM +0800, Zhang Qiao <[email protected]> wrote:
>>> + /*
>>> + * It should hold cpus lock because a cpu offline event can
>>> + * cause set_cpus_allowed_ptr() failed.
>>> + */
>>> + cpus_read_lock();
>>
>> Maybe just a nit, the old kernels before commit c5c63b9a6a2e ("cgroup:
>> Replace deprecated CPU-hotplug functions.") v5.15-rc1~159^2~5
>> would be more consistent with get_online_cpus() here (but they're
>> equivalent functionally so the locking order is correct).
>
> A fixed up patch would also be appreciated :)
>
Fixed up patch as follows, replace cpus_read_lock() with get_online_cpus().
thanks.
--------
[PATCH] cpuset: Fix unsafe lock order between cpuset lock and cpuslock
The backport commit 4eec5fe1c680a ("cgroup/cpuset: Fix a race
between cpuset_attach() and cpu hotplug") looks suspicious since
it comes before commit d74b27d63a8b ("cgroup/cpuset: Change
cpuset_rwsem and hotplug lock order") v5.4-rc1~176^2~30 when
the locking order was: cpuset lock, cpus lock.
Fix it with the correct locking order and reduce the cpus locking
range because only set_cpus_allowed_ptr() needs the protection of
cpus lock.
Fixes: 4eec5fe1c680a ("cgroup/cpuset: Fix a race between cpuset_attach() and cpu hotplug")
Reported-by: Michal Koutný <[email protected]>
Signed-off-by: Zhang Qiao <[email protected]>
---
kernel/cgroup/cpuset.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index d43d25acc..4e1c4232e 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1528,9 +1528,13 @@ static void cpuset_attach(struct cgroup_taskset *tset)
cgroup_taskset_first(tset, &css);
cs = css_cs(css);
- cpus_read_lock();
mutex_lock(&cpuset_mutex);
+ /*
+ * It should hold cpus lock because a cpu offline event can
+ * cause set_cpus_allowed_ptr() failed.
+ */
+ get_online_cpus();
/* prepare for attach */
if (cs == &top_cpuset)
cpumask_copy(cpus_attach, cpu_possible_mask);
@@ -1549,6 +1553,7 @@ static void cpuset_attach(struct cgroup_taskset *tset)
cpuset_change_task_nodemask(task, &cpuset_attach_nodemask_to);
cpuset_update_task_spread_flag(cs, task);
}
+ put_online_cpus();
/*
* Change mm for all threadgroup leaders. This is expensive and may
@@ -1584,7 +1589,6 @@ static void cpuset_attach(struct cgroup_taskset *tset)
wake_up(&cpuset_attach_wq);
mutex_unlock(&cpuset_mutex);
- cpus_read_unlock();
}
/* The various types of files and directories in a cpuset file system */
--
2.18.0
> thanks,
>
> greg k-h
> .
>
On Mon, Mar 14, 2022 at 12:19:41PM +0100, Michal Koutn? wrote:
> Hello.
>
> In my opinion there are two approaches:
> a) drop this backport (given other races present),
I have no problem with that, want to send a revert patch?
> b) swap the locks compatible with v4.19 as this patch proposes.
>
> On Mon, Mar 14, 2022 at 05:11:50PM +0800, Zhang Qiao <[email protected]> wrote:
> > + /*
> > + * It should hold cpus lock because a cpu offline event can
> > + * cause set_cpus_allowed_ptr() failed.
> > + */
> > + cpus_read_lock();
>
> Maybe just a nit, the old kernels before commit c5c63b9a6a2e ("cgroup:
> Replace deprecated CPU-hotplug functions.") v5.15-rc1~159^2~5
> would be more consistent with get_online_cpus() here (but they're
> equivalent functionally so the locking order is correct).
A fixed up patch would also be appreciated :)
thanks,
greg k-h
On Thu, Mar 17, 2022 at 10:41:57AM +0800, Zhang Qiao wrote:
>
>
> 在 2022/3/16 22:27, Greg Kroah-Hartman 写道:
> > On Mon, Mar 14, 2022 at 12:19:41PM +0100, Michal Koutný wrote:
> >> Hello.
> >>
> >> In my opinion there are two approaches:
> >> a) drop this backport (given other races present),
> >
> > I have no problem with that, want to send a revert patch?
> >
> >> b) swap the locks compatible with v4.19 as this patch proposes.
> >>
> >> On Mon, Mar 14, 2022 at 05:11:50PM +0800, Zhang Qiao <[email protected]> wrote:
> >>> + /*
> >>> + * It should hold cpus lock because a cpu offline event can
> >>> + * cause set_cpus_allowed_ptr() failed.
> >>> + */
> >>> + cpus_read_lock();
> >>
> >> Maybe just a nit, the old kernels before commit c5c63b9a6a2e ("cgroup:
> >> Replace deprecated CPU-hotplug functions.") v5.15-rc1~159^2~5
> >> would be more consistent with get_online_cpus() here (but they're
> >> equivalent functionally so the locking order is correct).
> >
> > A fixed up patch would also be appreciated :)
> >
>
> Fixed up patch as follows, replace cpus_read_lock() with get_online_cpus().
>
> thanks.
>
> --------
>
>
> [PATCH] cpuset: Fix unsafe lock order between cpuset lock and cpuslock
>
> The backport commit 4eec5fe1c680a ("cgroup/cpuset: Fix a race
> between cpuset_attach() and cpu hotplug") looks suspicious since
> it comes before commit d74b27d63a8b ("cgroup/cpuset: Change
> cpuset_rwsem and hotplug lock order") v5.4-rc1~176^2~30 when
> the locking order was: cpuset lock, cpus lock.
>
> Fix it with the correct locking order and reduce the cpus locking
> range because only set_cpus_allowed_ptr() needs the protection of
> cpus lock.
>
> Fixes: 4eec5fe1c680a ("cgroup/cpuset: Fix a race between cpuset_attach() and cpu hotplug")
> Reported-by: Michal Koutný <[email protected]>
> Signed-off-by: Zhang Qiao <[email protected]>
> ---
> kernel/cgroup/cpuset.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index d43d25acc..4e1c4232e 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -1528,9 +1528,13 @@ static void cpuset_attach(struct cgroup_taskset *tset)
> cgroup_taskset_first(tset, &css);
> cs = css_cs(css);
>
> - cpus_read_lock();
> mutex_lock(&cpuset_mutex);
>
> + /*
> + * It should hold cpus lock because a cpu offline event can
> + * cause set_cpus_allowed_ptr() failed.
> + */
> + get_online_cpus();
> /* prepare for attach */
> if (cs == &top_cpuset)
> cpumask_copy(cpus_attach, cpu_possible_mask);
> @@ -1549,6 +1553,7 @@ static void cpuset_attach(struct cgroup_taskset *tset)
> cpuset_change_task_nodemask(task, &cpuset_attach_nodemask_to);
> cpuset_update_task_spread_flag(cs, task);
> }
> + put_online_cpus();
>
> /*
> * Change mm for all threadgroup leaders. This is expensive and may
> @@ -1584,7 +1589,6 @@ static void cpuset_attach(struct cgroup_taskset *tset)
> wake_up(&cpuset_attach_wq);
>
> mutex_unlock(&cpuset_mutex);
> - cpus_read_unlock();
> }
>
> /* The various types of files and directories in a cpuset file system */
> --
> 2.18.0
>
>
Argh, whitespace was corrupted :(
I've fixed this up by hand and queued it up...
greg k-h