[PATCH v2 0/2] cgroup: fix psi monitor for root cgroup

[PATCH v2 0/2] cgroup: fix psi monitor for root cgroup

[Odin Ugedal]

This patchset fixes PSI monitors on the root cgroup, as they currently
only works on the non-root cgroups. Reading works for all, since that
was fixed when adding support for the root PSI files. It also contains
a doc update to reflect the current implementation.

Changes since v1:
 - Added Reviewed-by tag on the original patch (Suren)
 - Updated patch title
 - Added a separate patch to update doc


Odin Ugedal (2):
  cgroup: fix psi monitor for root cgroup
  cgroup: update PSI file description in docs

 Documentation/admin-guide/cgroup-v2.rst | 6 +++---
 kernel/cgroup/cgroup.c                  | 4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.30.0

[PATCH v2 1/2] cgroup: fix psi monitor for root cgroup

[Odin Ugedal]

Fix NULL pointer dereference when adding new psi monitor to the root
cgroup. PSI files for root cgroup was introduced in df5ba5be742 by using
system wide psi struct when reading, but file write/monitor was not
properly fixed. Since the PSI config for the root cgroup isn't
initialized, the current implementation tries to lock a NULL ptr,
resulting in a crash.

Can be triggered by running this as root:
$ tee /sys/fs/cgroup/cpu.pressure <<< "some 10000 1000000"

Signed-off-by: Odin Ugedal <odin@uged.al>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
 kernel/cgroup/cgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 613845769103..1ea995f801ec 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3564,6 +3564,7 @@ static ssize_t cgroup_pressure_write(struct kernfs_open_file *of, char *buf,
 {
 	struct psi_trigger *new;
 	struct cgroup *cgrp;
+	struct psi_group *psi;
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
 	if (!cgrp)
@@ -3572,7 +3573,8 @@ static ssize_t cgroup_pressure_write(struct kernfs_open_file *of, char *buf,
 	cgroup_get(cgrp);
 	cgroup_kn_unlock(of->kn);
 
-	new = psi_trigger_create(&cgrp->psi, buf, nbytes, res);
+	psi = cgroup_ino(cgrp) == 1 ? &psi_system : &cgrp->psi;
+	new = psi_trigger_create(psi, buf, nbytes, res);
 	if (IS_ERR(new)) {
 		cgroup_put(cgrp);
 		return PTR_ERR(new);
-- 
2.30.0

Re: [PATCH v2 1/2] cgroup: fix psi monitor for root cgroup

[Johannes Weiner]

On Sat, Jan 16, 2021 at 06:36:33PM +0100, Odin Ugedal wrote:
> Fix NULL pointer dereference when adding new psi monitor to the root
> cgroup. PSI files for root cgroup was introduced in df5ba5be742 by using
> system wide psi struct when reading, but file write/monitor was not
> properly fixed. Since the PSI config for the root cgroup isn't
> initialized, the current implementation tries to lock a NULL ptr,
> resulting in a crash.
> 
> Can be triggered by running this as root:
> $ tee /sys/fs/cgroup/cpu.pressure <<< "some 10000 1000000"
> 
> Signed-off-by: Odin Ugedal <odin@uged.al>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>

Fixes: df5ba5be7425 ("kernel/sched/psi.c: expose pressure metrics on root cgroup")
Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Since this is a userspace-triggerable NULL ptr crash, we should
probably also

Cc: stable@vger.kernel.org # 5.2+

[PATCH v2 2/2] cgroup: update PSI file description in docs

[Odin Ugedal]

Update PSI file description in cgroup-v2 docs to reflect the current
implementation.

Signed-off-by: Odin Ugedal <odin@uged.al>
---
 Documentation/admin-guide/cgroup-v2.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 63521cd36ce5..f638c9d3d9f2 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -1029,7 +1029,7 @@ All time durations are in microseconds.
 	one number is written, $MAX is updated.
 
   cpu.pressure
-	A read-only nested-key file which exists on non-root cgroups.
+	A read-only nested-keyed file.
 
 	Shows pressure stall information for CPU. See
 	:ref:`Documentation/accounting/psi.rst <psi>` for details.
@@ -1475,7 +1475,7 @@ PAGE_SIZE multiple when read back.
 	reduces the impact on the workload and memory management.
 
   memory.pressure
-	A read-only nested-key file which exists on non-root cgroups.
+	A read-only nested-keyed file.
 
 	Shows pressure stall information for memory. See
 	:ref:`Documentation/accounting/psi.rst <psi>` for details.
@@ -1714,7 +1714,7 @@ IO Interface Files
 	  8:16 rbps=2097152 wbps=max riops=max wiops=max
 
   io.pressure
-	A read-only nested-key file which exists on non-root cgroups.
+	A read-only nested-keyed file.
 
 	Shows pressure stall information for IO. See
 	:ref:`Documentation/accounting/psi.rst <psi>` for details.
-- 
2.30.0

Re: [PATCH v2 2/2] cgroup: update PSI file description in docs

[Johannes Weiner]

On Sat, Jan 16, 2021 at 06:36:34PM +0100, Odin Ugedal wrote:
> Update PSI file description in cgroup-v2 docs to reflect the current
> implementation.
> 
> Signed-off-by: Odin Ugedal <odin@uged.al>
> ---
>  Documentation/admin-guide/cgroup-v2.rst | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> index 63521cd36ce5..f638c9d3d9f2 100644
> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -1029,7 +1029,7 @@ All time durations are in microseconds.
>  	one number is written, $MAX is updated.
>  
>    cpu.pressure
> -	A read-only nested-key file which exists on non-root cgroups.
> +	A read-only nested-keyed file.

Could you please also change the 'read-only' to 'read-write'?

With that, please feel free to add:
Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v2 2/2] cgroup: update PSI file description in docs

[Tejun Heo]

On Tue, Jan 19, 2021 at 11:56:18AM -0500, Johannes Weiner wrote:
> On Sat, Jan 16, 2021 at 06:36:34PM +0100, Odin Ugedal wrote:
> > Update PSI file description in cgroup-v2 docs to reflect the current
> > implementation.
> > 
> > Signed-off-by: Odin Ugedal <odin@uged.al>
> > ---
> >  Documentation/admin-guide/cgroup-v2.rst | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
> > index 63521cd36ce5..f638c9d3d9f2 100644
> > --- a/Documentation/admin-guide/cgroup-v2.rst
> > +++ b/Documentation/admin-guide/cgroup-v2.rst
> > @@ -1029,7 +1029,7 @@ All time durations are in microseconds.
> >  	one number is written, $MAX is updated.
> >  
> >    cpu.pressure
> > -	A read-only nested-key file which exists on non-root cgroups.
> > +	A read-only nested-keyed file.
> 
> Could you please also change the 'read-only' to 'read-write'?
> 
> With that, please feel free to add:
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Applied w/ the suggested change.

Thanks.

-- 
tejun

Re: [PATCH v2 0/2] cgroup: fix psi monitor for root cgroup

[Dan Schatzberg]

> This patchset fixes PSI monitors on the root cgroup, as they currently
> only works on the non-root cgroups. Reading works for all, since that
> was fixed when adding support for the root PSI files. It also contains
> a doc update to reflect the current implementation.
> 
> Changes since v1:
> - Added Reviewed-by tag on the original patch (Suren)
> - Updated patch title
> - Added a separate patch to update doc
>
>
> Odin Ugedal (2):
>  cgroup: fix psi monitor for root cgroup
>  cgroup: update PSI file description in docs
>
> Documentation/admin-guide/cgroup-v2.rst | 6 +++---
> kernel/cgroup/cgroup.c                  | 4 +++-
> 2 files changed, 6 insertions(+), 4 deletions(-)

Both patches look good.

Acked-by: Dan Schatzberg <dschatzberg@fb.com>

Re: [PATCH v2 0/2] cgroup: fix psi monitor for root cgroup

[Tejun Heo]

On Sat, Jan 16, 2021 at 06:36:32PM +0100, Odin Ugedal wrote:
> This patchset fixes PSI monitors on the root cgroup, as they currently
> only works on the non-root cgroups. Reading works for all, since that
> was fixed when adding support for the root PSI files. It also contains
> a doc update to reflect the current implementation.

Applied to cgroup/for-5.11-fixes with acks and stable tag added.

Thanks.

-- 
tejun