cgroup/cpuset: Invalidate remote partition on housekeeping conflict
Subsystems (2)
PATCH v3 1/1 cgroup/cpuset: Invalidate remote partition on housekeeping conflict
From: Guopeng Zhang <[email protected]>
Widening an ancestor's exclusive CPU mask can add a boot-isolated CPU to a valid remote partition root without touching the partition's own control files. The partition then load balances that CPU, silently defeating isolcpus=domain for it.
This can be reproduced on a 32-CPU system booted with isolcpus=domain,4:
cd /sys/fs/cgroup
echo +cpuset > cgroup.subtree_control
mkdir -p A/B
echo +cpuset > A/cgroup.subtree_control
echo 2-4 > A/cpuset.cpus
echo 2-3 > A/cpuset.cpus.exclusive
echo 2-4 > A/B/cpuset.cpus
echo 2-4 > A/B/cpuset.cpus.exclusive
echo root > A/B/cpuset.cpus.partition
cat A/B/cpuset.cpus.effective # 2-3
echo 2-4 > A/cpuset.cpus.exclusive
cat A/B/cpuset.cpus.partition # root
cat A/B/cpuset.cpus.effective # 2-4
The last write returns 0 and leaves the hierarchy in this state:
root (cpuset.cpus.effective=0-1,5-31)
|
\-- A (member): cpuset.cpus=2-4
| cpuset.cpus.exclusive=2-4
\-- B (root, remote): cpuset.cpus=2-4
cpuset.cpus.effective=2-4B is a remote partition: it takes its CPUs directly from the root cpuset, and A only passes its exclusive list down. Before the last write, that list is 2-3, so B holds 2-3 and CPU 4 stays in the root cpuset as a boot-isolated CPU. The write widens A's exclusive list to 2-4, which additionally grants CPU 4 to B. Nothing rejects the grant: B remains a valid root partition, and CPU 4 is still listed in cpuset.cpus.isolated while sitting in a load-balanced partition.
remote_partition_enable() already rejects such grants through prstate_housekeeping_conflict(). remote_cpus_update(), which applies ancestor changes to a remote partition, does not.
Both paths also open-code remote partition validation. Move those checks into validate_remote_partition(), and check the resulting effective exclusive CPU mask for a housekeeping conflict there. The existing prs_err path then invalidates the remote partition instead of assigning it a boot-isolated CPU.
---
Changes since v2:
- Pass the enable/update mode explicitly to validate_remote_partition() instead of deriving it from cs->remote_partition, which can remain set after a failed partition state transition, as suggested by Tejun.
Changes since v1:
- Consolidate remote partition validation in a common helper, as suggested by Ridong.
- Check housekeeping conflicts against the resulting effective exclusive CPU mask.
- Rebase onto cgroup/for-7.3-fixes.
kernel/cgroup/cpuset.c | 69 ++++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 22 deletions(-)
kernel/cgroup/cpuset.c +47 -22
@@ -1561,6 +1561,43 @@ static inline bool is_local_partition(struct cpuset *cs)return is_partition_valid (cs) && !is_remote_partition (cs); } +/** +* validate_remote_partition - Validate a remote partition CPU change +* @prs: partition root state to validate +* @updating: true for an update, false for an enable +* @excpus: resulting effective exclusive CPU mask +* @addcpus: exclusive CPUs to be added +* @delcpus: exclusive CPUs to be deleted, can be NULL +* +* Return: PERR_NONE if valid, otherwise an appropriate error code +*/ +static enum prs_errcode +validate_remote_partition (int prs,bool updating, +struct cpumask *excpus, +struct cpumask *addcpus, +struct cpumask *delcpus) +{ +if (!capable (CAP_SYS_ADMIN )) +return PERR_ACCESS ; + +if (!updating && + (!cpumask_intersects (excpus, cpu_active_mask) || +cpumask_subset (top_cpuset.effective_cpus, addcpus))) +return PERR_INVCPUS ; + +if (cpumask_intersects (addcpus, subpartitions_cpus) || + (updating && +cpumask_subset (top_cpuset.effective_cpus, addcpus))) +return PERR_NOCPUS ; + +if ((prs ==PRS_ISOLATED && + !isolated_cpus_can_update (addcpus, delcpus)) || +prstate_housekeeping_conflict (prs, excpus)) +return PERR_HKEEPING ; + +return PERR_NONE ; +} +/* * remote_partition_enable - Enable current cpuset as a remote partition root * @cs: the cpuset to update
@@ -1574,11 +1611,7 @@ static inline bool is_local_partition(struct cpuset *cs)static int remote_partition_enable (struct cpuset *cs,int new_prs,struct tmpmasks *tmp) { -/* -* The user must have sysadmin privilege. -*/ -if (!capable (CAP_SYS_ADMIN )) -return PERR_ACCESS ; +enum prs_errcode err;/* * The requested exclusive_cpus must not be allocated to other
@@ -1591,15 +1624,10 @@ static int remote_partition_enable(struct cpuset *cs, int new_prs,* above it or remote partition root underneath it is not allowed. */ compute_excpus (cs, tmp->new_cpus); -if (!cpumask_intersects (tmp->new_cpus, cpu_active_mask) || -cpumask_subset (top_cpuset.effective_cpus, tmp->new_cpus)) -return PERR_INVCPUS ; -if (cpumask_intersects (tmp->new_cpus, subpartitions_cpus)) -return PERR_NOCPUS ; -if (((new_prs ==PRS_ISOLATED ) && - !isolated_cpus_can_update (tmp->new_cpus,NULL )) || -prstate_housekeeping_conflict (new_prs, tmp->new_cpus)) -return PERR_HKEEPING ; + err =validate_remote_partition (new_prs, false, tmp->new_cpus, + tmp->new_cpus,NULL ); +if (err) +return err;spin_lock_irq (&callback_lock);partition_xcpus_add (new_prs,NULL , tmp->new_cpus);
@@ -1672,6 +1700,7 @@ static void remote_partition_disable(struct cpuset *cs, struct tmpmasks *tmp)static void remote_cpus_update (struct cpuset *cs,struct cpumask *xcpus,struct cpumask *excpus,struct tmpmasks *tmp) { +enum prs_errcode err;bool adding, deleting;int prs = cs->partition_root_state;
@@ -1695,14 +1724,10 @@ static void remote_cpus_update(struct cpuset *cs, struct cpumask *xcpus,*/ if (adding) {WARN_ON_ONCE (cpumask_intersects (tmp->addmask, subpartitions_cpus)); -if (!capable (CAP_SYS_ADMIN )) -WRITE_ONCE (cs->prs_err,PERR_ACCESS ); -else if (cpumask_intersects (tmp->addmask, subpartitions_cpus) || -cpumask_subset (top_cpuset.effective_cpus, tmp->addmask)) -WRITE_ONCE (cs->prs_err,PERR_NOCPUS ); -else if ((prs ==PRS_ISOLATED ) && - !isolated_cpus_can_update (tmp->addmask, tmp->delmask)) -WRITE_ONCE (cs->prs_err,PERR_HKEEPING ); + err =validate_remote_partition (prs, true, excpus, tmp->addmask, + tmp->delmask); +if (err) +WRITE_ONCE (cs->prs_err, err);if (cs->prs_err) goto invalidate; }
base-commit: 31c88350b7dd1522792f726f79607f31bb55c50f
Signature · 1 line
2.43.0
Ridong Chen
reviewerQuoted 97 lines · Guopeng Zhang
From: Guopeng Zhang <[email protected]>
Widening an ancestor's exclusive CPU mask can add a boot-isolated CPU to a valid remote partition root without touching the partition's own control files. The partition then load balances that CPU, silently defeating isolcpus=domain for it.
This can be reproduced on a 32-CPU system booted with isolcpus=domain,4:
cd
/sys/fs/cgroup
echo +cpuset > cgroup.subtree_control
mkdir -p A/B
echo +cpuset > A/cgroup.subtree_control
echo 2-4 > A/cpuset.cpus
echo 2-3 > A/cpuset.cpus.exclusive
echo 2-4 > A/B/cpuset.cpus
echo 2-4 > A/B/cpuset.cpus.exclusive
echo root > A/B/cpuset.cpus.partition
cat A/B/cpuset.cpus.effective # 2-3
echo 2-4 > A/cpuset.cpus.exclusive
cat A/B/cpuset.cpus.partition # root
cat A/B/cpuset.cpus.effective # 2-4The last write returns 0 and leaves the hierarchy in this state:
root (cpuset.cpus.effective=0-1,5-31) | \-- A (member): cpuset.cpus=2-4 | cpuset.cpus.exclusive=2-4 \-- B (root, remote): cpuset.cpus=2-4 cpuset.cpus.effective=2-4B is a remote partition: it takes its CPUs directly from the root cpuset, and A only passes its exclusive list down. Before the last write, that list is 2-3, so B holds 2-3 and CPU 4 stays in the root cpuset as a boot-isolated CPU. The write widens A's exclusive list to 2-4, which additionally grants CPU 4 to B. Nothing rejects the grant: B remains a valid root partition, and CPU 4 is still listed in cpuset.cpus.isolated while sitting in a load-balanced partition.
remote_partition_enable()already rejects such grants throughprstate_housekeeping_conflict().remote_cpus_update(), which applies ancestor changes to a remote partition, does not.Both paths also open-code remote partition validation. Move those checks into
validate_remote_partition(), and check the resulting effective exclusive CPU mask for a housekeeping conflict there. The existingprs_errpath then invalidates the remote partition instead of assigning it a boot-isolated CPU.Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")Suggested-by Ridong Chen <[email protected]>Suggested-by Tejun Heo <[email protected]>Reviewed-by Ridong Chen <[email protected]>Reviewed-by Waiman Long <[email protected]>Signed-off-by Guopeng Zhang <[email protected]>---
Changes since v2:
- Pass the enable/update mode explicitly tovalidate_remote_partition()instead of deriving it fromcs->remote_partition, which can remain set after a failed partition state transition, as suggested by Tejun.Changes since v1:
- Consolidate remote partition validation in a common helper, as suggested by Ridong.
- Check housekeeping conflicts against the resulting effective exclusive CPU mask.
- Rebase onto cgroup/for-7.3-fixes.kernel/cgroup/cpuset.c | 69 ++++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 22 deletions(-)kernel/cgroup/cpuset.c +14
-0@@ -1561,6 +1561,43 @@ static inline bool is_local_partition(struct cpuset *cs)return is_partition_valid (cs) && !is_remote_partition (cs); } +/** +* validate_remote_partition - Validate a remote partition CPU change +* @prs: partition root state to validate +* @updating: true for an update, false for an enable +* @excpus: resulting effective exclusive CPU mask +* @addcpus: exclusive CPUs to be added +* @delcpus: exclusive CPUs to be deleted, can be NULL +* +* Return: PERR_NONE if valid, otherwise an appropriate error code +*/ +static enum prs_errcode +validate_remote_partition(int prs, bool updating, + struct cpumask *excpus, + struct cpumask *addcpus,
kernel/cgroup/cpuset.c static inline bool is_local_partition(struct cpuset *cs) · line 1587
Quoted 10 lines
kernel/cgroup/cpuset.c +10
-0+ struct cpumask *delcpus) +{ +if (!capable (CAP_SYS_ADMIN )) +return PERR_ACCESS ; + +if (!updating && + (!cpumask_intersects (excpus, cpu_active_mask) || +cpumask_subset (top_cpuset.effective_cpus, addcpus))) +return PERR_INVCPUS ; +
Do we really need the updating flag? I mean, whether or not it's an updating partition, it should still be an invalid remote partition, right? So why do we have to distinguish between updating and not updating?
Quoted 78 lines
kernel/cgroup/cpuset.c +23
-22+if (cpumask_intersects (addcpus, subpartitions_cpus) || + (updating && +cpumask_subset (top_cpuset.effective_cpus, addcpus))) +return PERR_NOCPUS ; + +if ((prs ==PRS_ISOLATED && + !isolated_cpus_can_update (addcpus, delcpus)) || +prstate_housekeeping_conflict (prs, excpus)) +return PERR_HKEEPING ; + +return PERR_NONE ; +} +/* * remote_partition_enable - Enable current cpuset as a remote partition root * @cs: the cpuset to update @@ -1574,11 +1611,7 @@ static inline bool is_local_partition(struct cpuset *cs)static int remote_partition_enable (struct cpuset *cs,int new_prs,struct tmpmasks *tmp) { -/* -* The user must have sysadmin privilege. -*/ -if (!capable (CAP_SYS_ADMIN )) -return PERR_ACCESS ; +enum prs_errcode err;/* * The requested exclusive_cpus must not be allocated to other @@ -1591,15 +1624,10 @@ static int remote_partition_enable(struct cpuset *cs, int new_prs,* above it or remote partition root underneath it is not allowed. */ compute_excpus (cs, tmp->new_cpus); -if (!cpumask_intersects (tmp->new_cpus, cpu_active_mask) || -cpumask_subset (top_cpuset.effective_cpus, tmp->new_cpus)) -return PERR_INVCPUS ; -if (cpumask_intersects (tmp->new_cpus, subpartitions_cpus)) -return PERR_NOCPUS ; -if (((new_prs ==PRS_ISOLATED ) && - !isolated_cpus_can_update (tmp->new_cpus,NULL )) || -prstate_housekeeping_conflict (new_prs, tmp->new_cpus)) -return PERR_HKEEPING ; + err =validate_remote_partition (new_prs, false, tmp->new_cpus, + tmp->new_cpus,NULL ); +if (err) +return err;spin_lock_irq (&callback_lock);partition_xcpus_add (new_prs,NULL , tmp->new_cpus);@@ -1672,6 +1700,7 @@ static void remote_partition_disable(struct cpuset *cs, struct tmpmasks *tmp)static void remote_cpus_update (struct cpuset *cs,struct cpumask *xcpus,struct cpumask *excpus,struct tmpmasks *tmp) { +enum prs_errcode err;bool adding, deleting;int prs = cs->partition_root_state;@@ -1695,14 +1724,10 @@ static void remote_cpus_update(struct cpuset *cs, struct cpumask *xcpus,*/ if (adding) {WARN_ON_ONCE (cpumask_intersects (tmp->addmask, subpartitions_cpus)); -if (!capable (CAP_SYS_ADMIN )) -WRITE_ONCE (cs->prs_err,PERR_ACCESS ); -else if (cpumask_intersects (tmp->addmask, subpartitions_cpus) || -cpumask_subset (top_cpuset.effective_cpus, tmp->addmask)) -WRITE_ONCE (cs->prs_err,PERR_NOCPUS ); -else if ((prs ==PRS_ISOLATED ) && - !isolated_cpus_can_update (tmp->addmask, tmp->delmask)) -WRITE_ONCE (cs->prs_err,PERR_HKEEPING ); + err =validate_remote_partition (prs, true, excpus, tmp->addmask, + tmp->delmask); +if (err) +WRITE_ONCE (cs->prs_err, err);if (cs->prs_err) goto invalidate; }base-commit:
31c88350b7dd1522792f726f79607f31bb55c50f
Signature · 2 lines
Best regards Ridong
Hi Ridong,
Quoted 103 lines · Ridong Chen
Quoted 100 lines · Guopeng Zhang
From: Guopeng Zhang <[email protected]>
Widening an ancestor's exclusive CPU mask can add a boot-isolated CPU to a valid remote partition root without touching the partition's own control files. The partition then load balances that CPU, silently defeating isolcpus=domain for it.
This can be reproduced on a 32-CPU system booted with isolcpus=domain,4:
cd
/sys/fs/cgroup
echo +cpuset > cgroup.subtree_control mkdir -p A/B
echo +cpuset > A/cgroup.subtree_control echo 2-4 > A/cpuset.cpus
echo 2-3 > A/cpuset.cpus.exclusive
echo 2-4 > A/B/cpuset.cpus
echo 2-4 > A/B/cpuset.cpus.exclusive
echo root > A/B/cpuset.cpus.partition
cat A/B/cpuset.cpus.effective # 2-3 echo 2-4 > A/cpuset.cpus.exclusive
cat A/B/cpuset.cpus.partition # root cat A/B/cpuset.cpus.effective # 2-4The last write returns 0 and leaves the hierarchy in this state:
root (cpuset.cpus.effective=0-1,5-31)
|
\-- A (member): cpuset.cpus=2-4 | cpuset.cpus.exclusive=2-4 \-- B (root, remote): cpuset.cpus=2-4 cpuset.cpus.effective=2-4B is a remote partition: it takes its CPUs directly from the root cpuset, and A only passes its exclusive list down. Before the last write, that list is 2-3, so B holds 2-3 and CPU 4 stays in the root cpuset as a boot-isolated CPU. The write widens A's exclusive list to 2-4, which additionally grants CPU 4 to B. Nothing rejects the grant: B remains a valid root partition, and CPU 4 is still listed in cpuset.cpus.isolated while sitting in a load-balanced partition.
remote_partition_enable()already rejects such grants throughprstate_housekeeping_conflict().remote_cpus_update(), which applies ancestor changes to a remote partition, does not.Both paths also open-code remote partition validation. Move those checks into
validate_remote_partition(), and check the resulting effective exclusive CPU mask for a housekeeping conflict there. The existingprs_errpath then invalidates the remote partition instead of assigning it a boot-isolated CPU.Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")Suggested-by Ridong Chen <[email protected]>Suggested-by Tejun Heo <[email protected]>Reviewed-by Ridong Chen <[email protected]>Reviewed-by Waiman Long <[email protected]>Signed-off-by Guopeng Zhang <[email protected]>---
Changes since v2:
- Pass the enable/update mode explicitly tovalidate_remote_partition()instead of deriving it fromcs->remote_partition, which can remain set after a failed partition state transition, as suggested by Tejun.Changes since v1:
- Consolidate remote partition validation in a common helper, as suggested by Ridong.
- Check housekeeping conflicts against the resulting effective exclusive CPU mask.
- Rebase onto cgroup/for-7.3-fixes.
kernel/cgroup/cpuset.c| 69 ++++++++++++++++++++++++++++--------------1 file changed, 47 insertions(+), 22 deletions(-)kernel/cgroup/cpuset.c +0
-0@@ -1561,6 +1561,43 @@ static inline bool is_local_partition(struct cpuset *cs) return is_partition_valid(cs) && !is_remote_partition(cs); }+/**
+ *validate_remote_partition- Validate a remote partition CPU change
+ * @prs: partition root state to validate
+ * @updating: true for an update, false for an enable
+ * @excpus: resulting effective exclusive CPU mask
+ * @addcpus: exclusive CPUs to be added
+ * @delcpus: exclusive CPUs to be deleted, can be NULL
+ *
+ * Return:PERR_NONEif valid, otherwise an appropriate error code
+ */kernel/cgroup/cpuset.c +8
-0+static enum prs_errcode +validate_remote_partition (int prs,bool updating, + struct cpumask *excpus, + struct cpumask *addcpus, + struct cpumask *delcpus) +{ + if (!capable(CAP_SYS_ADMIN)) + return PERR_ACCESS;
Quoted 9 lines
kernel/cgroup/cpuset.c static inline bool is_local_partition(struct cpuset *cs) · line 1587
Quoted 6 lines
kernel/cgroup/cpuset.c +6
-0+ + if (!updating && + (!cpumask_intersects(excpus, cpu_active_mask) || + cpumask_subset(top_cpuset.effective_cpus, addcpus))) + return PERR_INVCPUS; +Do we really need the updating flag? I mean, whether or not it's an updating partition, it should still be an invalid remote partition, right? So why do we have to distinguish between updating and not updating?
Both paths do invalidate the remote partition when validation fails, but the flag was added when factoring out this helper to preserve the different semantics of the two paths.
For example, suppose the top cpuset has only CPU 4 left:
top_cpuset.effective_cpus = 4
When enabling a new remote partition:
addcpus = 4
cpumask_subset(top_cpuset.effective_cpus, addcpus) is true, and the existing enable path returns PERR_INVCPUS.
Now suppose an existing remote partition owns CPUs 2-3 and is updated to add CPU 4:
old excpus = 2-3
addcpus = 4
new excpus = 2-4
The same cpumask_subset() condition is true, but the existing update path returns PERR_NOCPUS. The active CPU check also differs: the enable path requires the resulting mask to contain at least one active CPU, while the update path only checks whether the resulting mask is empty.
If we want to remove the flag, I think the cleaner approach is to keep the checks specific to each path in their respective callers and leave only the shared checks in validate_remote_partition().
Thanks,
Guopeng
Quoted 76 lines
Quoted 75 lines
+ if (
cpumask_intersects(addcpus, subpartitions_cpus)|| + (updating &&
+cpumask_subset(top_cpuset.effective_cpus, addcpus))) + returnPERR_NOCPUS;kernel/cgroup/cpuset.c +9
-0+ + if ((prs == PRS_ISOLATED && + !isolated_cpus_can_update(addcpus, delcpus)) || + prstate_housekeeping_conflict(prs, excpus)) + return PERR_HKEEPING; + + return PERR_NONE; +} +/*
*remote_partition_enable- Enable current cpuset as a remote partition root
* @cs: the cpuset to updatekernel/cgroup/cpuset.c +6
-14@@ -1574,11 +1611,7 @@ static inline bool is_local_partition(struct cpuset *cs) static int remote_partition_enable(struct cpuset *cs, int new_prs, struct tmpmasks *tmp) { - /* - * The user must have sysadmin privilege. - */ - if (!capable(CAP_SYS_ADMIN)) - return PERR_ACCESS; + enum prs_errcode err; /** The requested exclusive_cpus must not be allocated to other @@ -1591,15 +1624,10 @@ static int remote_partition_enable(struct cpuset *cs, int new_prs,* above it or remote partition root underneath it is not allowed. */ compute_excpus(cs, tmp->new_cpus); - if (!cpumask_intersects(tmp->new_cpus, cpu_active_mask) || - cpumask_subset(top_cpuset.effective_cpus, tmp->new_cpus)) - return PERR_INVCPUS; - if (cpumask_intersects(tmp->new_cpus, subpartitions_cpus)) - return PERR_NOCPUS; - if (((new_prs == PRS_ISOLATED) && - !isolated_cpus_can_update(tmp->new_cpus, NULL)) || - prstate_housekeeping_conflict(new_prs, tmp->new_cpus)) - return PERR_HKEEPING; + err = validate_remote_partition(new_prs, false, tmp->new_cpus, + tmp->new_cpus, NULL); + if (err) + return err; spin_lock_irq(&callback_lock); partition_xcpus_add(new_prs, NULL, tmp->new_cpus);@@ -1672,6 +1700,7 @@ static void remote_partition_disable(struct cpuset *cs, struct tmpmasks *tmp) static void remote_cpus_update(struct cpuset *cs, struct cpumask *xcpus, struct cpumask *excpus, struct tmpmasks *tmp) { + enum prs_errcode err; bool adding, deleting; int prs = cs->partition_root_state; @@ -1695,14 +1724,10 @@ static void remote_cpus_update(struct cpuset *cs, struct cpumask *xcpus,*/
if (adding) {
WARN_ON_ONCE(cpumask_intersects(tmp->addmask, subpartitions_cpus)); - if (!capable(CAP_SYS_ADMIN))
-WRITE_ONCE(cs->prs_err, PERR_ACCESS);
- else if (cpumask_intersects(tmp->addmask, subpartitions_cpus)|| -cpumask_subset(top_cpuset.effective_cpus, tmp->addmask)) -WRITE_ONCE(cs->prs_err, PERR_NOCPUS);
- else if ((prs ==PRS_ISOLATED) &&
- !isolated_cpus_can_update(tmp->addmask,tmp->delmask)) -WRITE_ONCE(cs->prs_err, PERR_HKEEPING);
+ err =validate_remote_partition(prs, true, excpus, tmp->addmask, + tmp->delmask);
+ if (err)
+WRITE_ONCE(cs->prs_err, err);
if (cs->prs_err)
goto invalidate;
}base-commit:
31c88350b7dd1522792f726f79607f31bb55c50f
Ridong Chen
reviewerQuoted 134 lines · Guopeng Zhang
Hi Ridong,
Quoted 103 lines · Ridong Chen
Quoted 100 lines · Guopeng Zhang
From: Guopeng Zhang <[email protected]>
Widening an ancestor's exclusive CPU mask can add a boot-isolated CPU to a valid remote partition root without touching the partition's own control files. The partition then load balances that CPU, silently defeating isolcpus=domain for it.
This can be reproduced on a 32-CPU system booted with isolcpus=domain,4:
cd
/sys/fs/cgroup
echo +cpuset > cgroup.subtree_control mkdir -p A/B
echo +cpuset > A/cgroup.subtree_control echo 2-4 > A/cpuset.cpus
echo 2-3 > A/cpuset.cpus.exclusive
echo 2-4 > A/B/cpuset.cpus
echo 2-4 > A/B/cpuset.cpus.exclusive
echo root > A/B/cpuset.cpus.partition
cat A/B/cpuset.cpus.effective # 2-3 echo 2-4 > A/cpuset.cpus.exclusive
cat A/B/cpuset.cpus.partition # root cat A/B/cpuset.cpus.effective # 2-4The last write returns 0 and leaves the hierarchy in this state:
root (cpuset.cpus.effective=0-1,5-31)
|
\-- A (member): cpuset.cpus=2-4 | cpuset.cpus.exclusive=2-4 \-- B (root, remote): cpuset.cpus=2-4 cpuset.cpus.effective=2-4B is a remote partition: it takes its CPUs directly from the root cpuset, and A only passes its exclusive list down. Before the last write, that list is 2-3, so B holds 2-3 and CPU 4 stays in the root cpuset as a boot-isolated CPU. The write widens A's exclusive list to 2-4, which additionally grants CPU 4 to B. Nothing rejects the grant: B remains a valid root partition, and CPU 4 is still listed in cpuset.cpus.isolated while sitting in a load-balanced partition.
remote_partition_enable()already rejects such grants throughprstate_housekeeping_conflict().remote_cpus_update(), which applies ancestor changes to a remote partition, does not.Both paths also open-code remote partition validation. Move those checks into
validate_remote_partition(), and check the resulting effective exclusive CPU mask for a housekeeping conflict there. The existingprs_errpath then invalidates the remote partition instead of assigning it a boot-isolated CPU.Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")Suggested-by Ridong Chen <[email protected]>Suggested-by Tejun Heo <[email protected]>Reviewed-by Ridong Chen <[email protected]>Reviewed-by Waiman Long <[email protected]>Signed-off-by Guopeng Zhang <[email protected]>---
Changes since v2:
- Pass the enable/update mode explicitly tovalidate_remote_partition()instead of deriving it fromcs->remote_partition, which can remain set after a failed partition state transition, as suggested by Tejun.Changes since v1:
- Consolidate remote partition validation in a common helper, as suggested by Ridong.
- Check housekeeping conflicts against the resulting effective exclusive CPU mask.
- Rebase onto cgroup/for-7.3-fixes.kernel/cgroup/cpuset.c | 69 ++++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 22 deletions(-)kernel/cgroup/cpuset.c +17
-0@@ -1561,6 +1561,43 @@ static inline bool is_local_partition(struct cpuset *cs) return is_partition_valid(cs) && !is_remote_partition(cs); } +/** +* validate_remote_partition - Validate a remote partition CPU change +* @prs: partition root state to validate +* @updating: true for an update, false for an enable +* @excpus: resulting effective exclusive CPU mask +* @addcpus: exclusive CPUs to be added +* @delcpus: exclusive CPUs to be deleted, can be NULL +* +* Return: PERR_NONE if valid, otherwise an appropriate error code +*/ +static enum prs_errcode +validate_remote_partition (int prs,bool updating, + struct cpumask *excpus, + struct cpumask *addcpus, + struct cpumask *delcpus) +{ + if (!capable(CAP_SYS_ADMIN)) + return PERR_ACCESS;Quoted 9 lines
kernel/cgroup/cpuset.c static inline bool is_local_partition(struct cpuset *cs) · line 1587
Quoted 6 lines
kernel/cgroup/cpuset.c +6
-0+ + if (!updating && + (!cpumask_intersects(excpus, cpu_active_mask) || + cpumask_subset(top_cpuset.effective_cpus, addcpus))) + return PERR_INVCPUS; +Do we really need the updating flag? I mean, whether or not it's an updating partition, it should still be an invalid remote partition, right? So why do we have to distinguish between updating and not updating?
Both paths do invalidate the remote partition when validation fails, but the flag was added when factoring out this helper to preserve the different semantics of the two paths.
For example, suppose the top cpuset has only CPU 4 left:
top_cpuset.effective_cpus = 4
When enabling a new remote partition:
addcpus = 4
cpumask_subset(top_cpuset.effective_cpus, addcpus)is true, and the existing enable path returnsPERR_INVCPUS.Now suppose an existing remote partition owns CPUs 2-3 and is updated to add CPU 4:
Quoted 9 lines
old excpus = 2-3
addcpus = 4
new excpus = 2-4The same
cpumask_subset()condition is true, but the existing update path returnsPERR_NOCPUS. The active CPU check also differs: the enable path requires the resulting mask to contain at least one active CPU, while the update path only checks whether the resulting mask is empty.
What we want to do is to unify the remote partition check. I think enabling and updating should return the same error when the same condition check fails.
So I think we may change it 'a little' if it is reasonable. In my opinion, returning PERR_NOCPUS when the top_cpuset.effective_cpus becomes empty is reasonable. And I do not want to see some special case, which makes the code complicated.
I would like to see what Longman's opinion is.
Quoted 84 lines
If we want to remove the flag, I think the cleaner approach is to keep the checks specific to each path in their respective callers and leave only the shared checks in
validate_remote_partition().Thanks,
GuopengQuoted 76 lines
Quoted 75 lines
kernel/cgroup/cpuset.c +19
-14+ if (cpumask_intersects(addcpus, subpartitions_cpus) || + (updating && + cpumask_subset(top_cpuset.effective_cpus, addcpus))) + return PERR_NOCPUS; + + if ((prs == PRS_ISOLATED && + !isolated_cpus_can_update(addcpus, delcpus)) || + prstate_housekeeping_conflict(prs, excpus)) + return PERR_HKEEPING; + + return PERR_NONE; +} + /** remote_partition_enable - Enable current cpuset as a remote partition root * @cs: the cpuset to update @@ -1574,11 +1611,7 @@ static inline bool is_local_partition(struct cpuset *cs) static int remote_partition_enable(struct cpuset *cs, int new_prs, struct tmpmasks *tmp) { - /* - * The user must have sysadmin privilege. - */ - if (!capable(CAP_SYS_ADMIN)) - return PERR_ACCESS; + enum prs_errcode err; /** The requested exclusive_cpus must not be allocated to other @@ -1591,15 +1624,10 @@ static int remote_partition_enable(struct cpuset *cs, int new_prs,* above it or remote partition root underneath it is not allowed. */ compute_excpus(cs, tmp->new_cpus); - if (!cpumask_intersects(tmp->new_cpus, cpu_active_mask) || - cpumask_subset(top_cpuset.effective_cpus, tmp->new_cpus)) - return PERR_INVCPUS; - if (cpumask_intersects(tmp->new_cpus, subpartitions_cpus)) - return PERR_NOCPUS; - if (((new_prs == PRS_ISOLATED) && - !isolated_cpus_can_update(tmp->new_cpus, NULL)) || - prstate_housekeeping_conflict(new_prs, tmp->new_cpus)) - return PERR_HKEEPING; + err = validate_remote_partition(new_prs, false, tmp->new_cpus, + tmp->new_cpus, NULL); + if (err) + return err; spin_lock_irq(&callback_lock); partition_xcpus_add(new_prs, NULL, tmp->new_cpus);@@ -1672,6 +1700,7 @@ static void remote_partition_disable(struct cpuset *cs, struct tmpmasks *tmp) static void remote_cpus_update(struct cpuset *cs, struct cpumask *xcpus, struct cpumask *excpus, struct tmpmasks *tmp) { + enum prs_errcode err; bool adding, deleting; int prs = cs->partition_root_state; @@ -1695,14 +1724,10 @@ static void remote_cpus_update(struct cpuset *cs, struct cpumask *xcpus,*/
if (adding) {WARN_ON_ONCE(cpumask_intersects(tmp->addmask, subpartitions_cpus));
- if (!capable(CAP_SYS_ADMIN))
-WRITE_ONCE(cs->prs_err, PERR_ACCESS);
- else if (cpumask_intersects(tmp->addmask, subpartitions_cpus)|| -cpumask_subset(top_cpuset.effective_cpus, tmp->addmask)) -WRITE_ONCE(cs->prs_err, PERR_NOCPUS);
- else if ((prs ==PRS_ISOLATED) &&
- !isolated_cpus_can_update(tmp->addmask,tmp->delmask)) -WRITE_ONCE(cs->prs_err, PERR_HKEEPING);
+ err =validate_remote_partition(prs, true, excpus, tmp->addmask, + tmp->delmask);
+ if (err)
+WRITE_ONCE(cs->prs_err, err);
if (cs->prs_err)
goto invalidate;
}base-commit:
31c88350b7dd1522792f726f79607f31bb55c50f
Signature · 2 lines
Best regards Ridong
Waiman Long
maintainerQuoted 151 lines · Ridong Chen
Quoted 138 lines · Guopeng Zhang
Hi Ridong,
Quoted 107 lines · Ridong Chen
Quoted 104 lines · Guopeng Zhang
From: Guopeng Zhang <[email protected]>
Widening an ancestor's exclusive CPU mask can add a boot-isolated CPU to a valid remote partition root without touching the partition's own control files. The partition then load balances that CPU, silently defeating isolcpus=domain for it.
This can be reproduced on a 32-CPU system booted with isolcpus=domain,4:
cd
/sys/fs/cgroup
echo +cpuset > cgroup.subtree_control mkdir -p A/B
echo +cpuset > A/cgroup.subtree_control echo 2-4 > A/cpuset.cpus
echo 2-3 > A/cpuset.cpus.exclusive
echo 2-4 > A/B/cpuset.cpus
echo 2-4 > A/B/cpuset.cpus.exclusive
echo root > A/B/cpuset.cpus.partition
cat A/B/cpuset.cpus.effective # 2-3 echo 2-4 > A/cpuset.cpus.exclusive
cat A/B/cpuset.cpus.partition # root cat A/B/cpuset.cpus.effective # 2-4The last write returns 0 and leaves the hierarchy in this state:
root (cpuset.cpus.effective=0-1,5-31)
|
\-- A (member): cpuset.cpus=2-4 | cpuset.cpus.exclusive=2-4 \-- B (root, remote): cpuset.cpus=2-4 cpuset.cpus.effective=2-4B is a remote partition: it takes its CPUs directly from the root cpuset, and A only passes its exclusive list down. Before the last write, that list is 2-3, so B holds 2-3 and CPU 4 stays in the root cpuset as a boot-isolated CPU. The write widens A's exclusive list to 2-4, which additionally grants CPU 4 to B. Nothing rejects the grant: B remains a valid root partition, and CPU 4 is still listed in cpuset.cpus.isolated while sitting in a load-balanced partition.
remote_partition_enable()already rejects such grants throughprstate_housekeeping_conflict().remote_cpus_update(), which applies ancestor changes to a remote partition, does not.Both paths also open-code remote partition validation. Move those checks into
validate_remote_partition(), and check the resulting effective exclusive CPU mask for a housekeeping conflict there. The existingprs_errpath then invalidates the remote partition instead of assigning it a boot-isolated CPU.Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")Suggested-by Ridong Chen <[email protected]>Suggested-by Tejun Heo <[email protected]>Reviewed-by Ridong Chen <[email protected]>Reviewed-by Waiman Long <[email protected]>Signed-off-by Guopeng Zhang <[email protected]>---
Changes since v2:
- Pass the enable/update mode explicitly tovalidate_remote_partition()
instead of deriving it fromcs->remote_partition, which can remain set after a failed partition state transition, as suggested by Tejun.Changes since v1:
- Consolidate remote partition validation in a common helper, as suggested by Ridong.
- Check housekeeping conflicts against the resulting effective exclusive
CPU mask.
- Rebase onto cgroup/for-7.3-fixes.
kernel/cgroup/cpuset.c| 69
++++++++++++++++++++++++++++--------------1 file changed, 47 insertions(+), 22 deletions(-)kernel/cgroup/cpuset.c +0
-0@@ -1561,6 +1561,43 @@ static inline bool is_local_partition(struct cpuset *cs) return is_partition_valid(cs) && !is_remote_partition(cs); }+/**
+ *validate_remote_partition- Validate a remote partition CPU change
+ * @prs: partition root state to validate
+ * @updating: true for an update, false for an enable
+ * @excpus: resulting effective exclusive CPU mask
+ * @addcpus: exclusive CPUs to be added
+ * @delcpus: exclusive CPUs to be deleted, can be NULL
+ *
+ * Return:PERR_NONEif valid, otherwise an appropriate error code
+ */kernel/cgroup/cpuset.c +8
-0+static enum prs_errcode +validate_remote_partition (int prs,bool updating, + struct cpumask *excpus, + struct cpumask *addcpus, + struct cpumask *delcpus) +{ + if (!capable(CAP_SYS_ADMIN)) + return PERR_ACCESS;Quoted 9 lines
kernel/cgroup/cpuset.c static inline bool is_local_partition(struct cpuset *cs) · line 1587
Quoted 6 lines
kernel/cgroup/cpuset.c +6
-0+ + if (!updating && + (!cpumask_intersects(excpus, cpu_active_mask) || + cpumask_subset(top_cpuset.effective_cpus, addcpus))) + return PERR_INVCPUS; +Do we really need the updating flag? I mean, whether or not it's an updating partition, it should still be an invalid remote partition, right? So why do we have to distinguish between updating and not updating?
Both paths do invalidate the remote partition when validation fails, but the flag was added when factoring out this helper to preserve the different semantics of the two paths.
For example, suppose the top cpuset has only CPU 4 left:
top_cpuset.effective_cpus = 4
When enabling a new remote partition:
addcpus = 4
cpumask_subset(top_cpuset.effective_cpus, addcpus)is true, and the existing enable path returnsPERR_INVCPUS.Now suppose an existing remote partition owns CPUs 2-3 and is updated to add CPU 4:
Quoted 9 lines
old excpus = 2-3
addcpus = 4
new excpus = 2-4The same
cpumask_subset()condition is true, but the existing update path returnsPERR_NOCPUS. The active CPU check also differs: the enable path requires the resulting mask to contain at least one active CPU, while the update path only checks whether the resulting mask is empty.
Quoted 5 lines
What we want to do is to unify the remote partition check. I think enabling and updating should return the same error when the same condition check fails.
So I think we may change it 'a little' if it is reasonable. In my opinion, returning
PERR_NOCPUSwhen the top_cpuset.effective_cpus becomes empty is reasonable. And I do not want to see some special case, which makes the code complicated.I would like to see what Longman's opinion is.
There are slight inconsistency in the type of sanity check done in different code paths. So I like the idea of extract them out into a common helper to make them consistent. As for the special updating flag, I think we should remove it as long as it won't cause a false positive. BTW, I am going to post a v3 version of my cpuset patch that will combine the addmask & delmask into a single new_cpus mask that should make the elimination of the updating flag possible.
Cheers,
Longman