Skip to contentSkip to conversation

cgroup/cpuset: Invalidate remote partition on housekeeping conflict

PATCH In review Guopeng Zhang 3 participants 5 messages Sep 29 – Oct 12

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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-4

B 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.

Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")

---

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
index 1fcec89a28b9..d0a45241c070 100644
@@ -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
reviewer
#1 lore raw
Quoted 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-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-4

B 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.

Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")

---

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 +14 -0
index 1fcec89a28b9..d0a45241c070 100644
@@ -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
#2 lore raw

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-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-4

B 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.

Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")

---

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 +0 -0
index 1fcec89a28b9..d0a45241c070 100644
@@ -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
+ */

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))) +        return PERR_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 update

kernel/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

reviewer
#3 lore raw
Quoted 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-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-4

B 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.

Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")

---

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 +17 -0
index 1fcec89a28b9..d0a45241c070 100644
@@ -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 returns PERR_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-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.

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,
Guopeng

Quoted 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
maintainer
#4 lore raw
Quoted 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-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-4

B 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.

Fixes f62a5d39368e ("cgroup/cpuset: Remove remote_partition_check() & make update_cpumasks_hier() handle remote partition")

---

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 +0 -0
index 1fcec89a28b9..d0a45241c070 100644
@@ -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
+ */

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:

Quoted 9 lines

     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.

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_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.

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