[PATCH] sched: psi: fix unprivileged polling against cgroups

[PATCH] sched: psi: fix unprivileged polling against cgroups

[Johannes Weiner]

519fabc7aaba ("psi: remove 500ms min window size limitation for
triggers") breaks unprivileged psi polling on cgroups.

Historically, we had a privilege check for polling in the open() of a
pressure file in /proc, but were erroneously missing it for the open()
of cgroup pressure files.

When unprivileged polling was introduced in d82caa273565 ("sched/psi:
Allow unprivileged polling of N*2s period"), it needed to filter
privileges depending on the exact polling parameters, and as such
moved the CAP_SYS_RESOURCE check from the proc open() callback to
psi_trigger_create(). Both the proc files as well as cgroup files go
through this during write(). This implicitly added the missing check
for privileges required for HT polling for cgroups.

When 519fabc7aaba ("psi: remove 500ms min window size limitation for
triggers") followed right after to remove further restrictions on the
RT polling window, it incorrectly assumed the cgroup privilege check
was still missing and added it to the cgroup open(), mirroring what we
used to do for proc files in the past.

As a result, unprivileged poll requests that would be supported now
get rejected when opening the cgroup pressure file for writing.

Remove the cgroup open() check. psi_trigger_create() handles it.

Fixes: 519fabc7aaba ("psi: remove 500ms min window size limitation for triggers")
Cc: stable@vger.kernel.org # 6.5+
Reported-by: Luca Boccassi <bluca@debian.org>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 kernel/cgroup/cgroup.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index f11488b18ceb..2069ee98da60 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3879,14 +3879,6 @@ static __poll_t cgroup_pressure_poll(struct kernfs_open_file *of,
 	return psi_trigger_poll(&ctx->psi.trigger, of->file, pt);
 }
 
-static int cgroup_pressure_open(struct kernfs_open_file *of)
-{
-	if (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE))
-		return -EPERM;
-
-	return 0;
-}
-
 static void cgroup_pressure_release(struct kernfs_open_file *of)
 {
 	struct cgroup_file_ctx *ctx = of->priv;
@@ -5287,7 +5279,6 @@ static struct cftype cgroup_psi_files[] = {
 	{
 		.name = "io.pressure",
 		.file_offset = offsetof(struct cgroup, psi_files[PSI_IO]),
-		.open = cgroup_pressure_open,
 		.seq_show = cgroup_io_pressure_show,
 		.write = cgroup_io_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5296,7 +5287,6 @@ static struct cftype cgroup_psi_files[] = {
 	{
 		.name = "memory.pressure",
 		.file_offset = offsetof(struct cgroup, psi_files[PSI_MEM]),
-		.open = cgroup_pressure_open,
 		.seq_show = cgroup_memory_pressure_show,
 		.write = cgroup_memory_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5305,7 +5295,6 @@ static struct cftype cgroup_psi_files[] = {
 	{
 		.name = "cpu.pressure",
 		.file_offset = offsetof(struct cgroup, psi_files[PSI_CPU]),
-		.open = cgroup_pressure_open,
 		.seq_show = cgroup_cpu_pressure_show,
 		.write = cgroup_cpu_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5315,7 +5304,6 @@ static struct cftype cgroup_psi_files[] = {
 	{
 		.name = "irq.pressure",
 		.file_offset = offsetof(struct cgroup, psi_files[PSI_IRQ]),
-		.open = cgroup_pressure_open,
 		.seq_show = cgroup_irq_pressure_show,
 		.write = cgroup_irq_pressure_write,
 		.poll = cgroup_pressure_poll,
-- 
2.42.0

Re: [PATCH] sched: psi: fix unprivileged polling against cgroups

[Luca Boccassi]

On Thu, 26 Oct 2023 at 17:41, Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> 519fabc7aaba ("psi: remove 500ms min window size limitation for
> triggers") breaks unprivileged psi polling on cgroups.
>
> Historically, we had a privilege check for polling in the open() of a
> pressure file in /proc, but were erroneously missing it for the open()
> of cgroup pressure files.
>
> When unprivileged polling was introduced in d82caa273565 ("sched/psi:
> Allow unprivileged polling of N*2s period"), it needed to filter
> privileges depending on the exact polling parameters, and as such
> moved the CAP_SYS_RESOURCE check from the proc open() callback to
> psi_trigger_create(). Both the proc files as well as cgroup files go
> through this during write(). This implicitly added the missing check
> for privileges required for HT polling for cgroups.
>
> When 519fabc7aaba ("psi: remove 500ms min window size limitation for
> triggers") followed right after to remove further restrictions on the
> RT polling window, it incorrectly assumed the cgroup privilege check
> was still missing and added it to the cgroup open(), mirroring what we
> used to do for proc files in the past.
>
> As a result, unprivileged poll requests that would be supported now
> get rejected when opening the cgroup pressure file for writing.
>
> Remove the cgroup open() check. psi_trigger_create() handles it.
>
> Fixes: 519fabc7aaba ("psi: remove 500ms min window size limitation for triggers")
> Cc: stable@vger.kernel.org # 6.5+
> Reported-by: Luca Boccassi <bluca@debian.org>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Luca Boccassi <bluca@debian.org>

Thank you very much for the quick fix - this was reported originally
on the systemd bug tracker by Daniel Black (I do not have an email
address):

https://github.com/systemd/systemd/issues/29723

It is very important for systemd services to be able to do this
without capabilities, as using capabilities means in turn user
namespaces cannot be used (PrivateUsers=yes in systemd parlance).

Kind regards,
Luca Boccassi

Re: [PATCH] sched: psi: fix unprivileged polling against cgroups

[Suren Baghdasaryan]

On Thu, Oct 26, 2023 at 9:49 AM Luca Boccassi <bluca@debian.org> wrote:
>
> On Thu, 26 Oct 2023 at 17:41, Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > 519fabc7aaba ("psi: remove 500ms min window size limitation for
> > triggers") breaks unprivileged psi polling on cgroups.
> >
> > Historically, we had a privilege check for polling in the open() of a
> > pressure file in /proc, but were erroneously missing it for the open()
> > of cgroup pressure files.
> >
> > When unprivileged polling was introduced in d82caa273565 ("sched/psi:
> > Allow unprivileged polling of N*2s period"), it needed to filter
> > privileges depending on the exact polling parameters, and as such
> > moved the CAP_SYS_RESOURCE check from the proc open() callback to
> > psi_trigger_create(). Both the proc files as well as cgroup files go
> > through this during write(). This implicitly added the missing check
> > for privileges required for HT polling for cgroups.
> >
> > When 519fabc7aaba ("psi: remove 500ms min window size limitation for
> > triggers") followed right after to remove further restrictions on the
> > RT polling window, it incorrectly assumed the cgroup privilege check
> > was still missing and added it to the cgroup open(), mirroring what we
> > used to do for proc files in the past.
> >
> > As a result, unprivileged poll requests that would be supported now
> > get rejected when opening the cgroup pressure file for writing.

Ah, I see the problem. In our discussion
https://lore.kernel.org/all/ZADj4YX4uftK%2FFrh@cmpxchg.org/ we decided
to have the check in open() to fail early but we never considered
unprivileged processes which only poll and never create any triggers.
Makes sense.

> >
> > Remove the cgroup open() check. psi_trigger_create() handles it.
> >
> > Fixes: 519fabc7aaba ("psi: remove 500ms min window size limitation for triggers")
> > Cc: stable@vger.kernel.org # 6.5+
> > Reported-by: Luca Boccassi <bluca@debian.org>
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>
> Acked-by: Luca Boccassi <bluca@debian.org>

Acked-by: Suren Baghdasaryan <surenb@google.com>

>
> Thank you very much for the quick fix - this was reported originally
> on the systemd bug tracker by Daniel Black (I do not have an email
> address):
>
> https://github.com/systemd/systemd/issues/29723
>
> It is very important for systemd services to be able to do this
> without capabilities, as using capabilities means in turn user
> namespaces cannot be used (PrivateUsers=yes in systemd parlance).
>
> Kind regards,
> Luca Boccassi

Re: [PATCH] sched: psi: fix unprivileged polling against cgroups

[Johannes Weiner]

On Thu, Oct 26, 2023 at 09:55:23AM -0700, Suren Baghdasaryan wrote:
> On Thu, Oct 26, 2023 at 9:49 AM Luca Boccassi <bluca@debian.org> wrote:
> >
> > On Thu, 26 Oct 2023 at 17:41, Johannes Weiner <hannes@cmpxchg.org> wrote:
> > >
> > > 519fabc7aaba ("psi: remove 500ms min window size limitation for
> > > triggers") breaks unprivileged psi polling on cgroups.
> > >
> > > Historically, we had a privilege check for polling in the open() of a
> > > pressure file in /proc, but were erroneously missing it for the open()
> > > of cgroup pressure files.
> > >
> > > When unprivileged polling was introduced in d82caa273565 ("sched/psi:
> > > Allow unprivileged polling of N*2s period"), it needed to filter
> > > privileges depending on the exact polling parameters, and as such
> > > moved the CAP_SYS_RESOURCE check from the proc open() callback to
> > > psi_trigger_create(). Both the proc files as well as cgroup files go
> > > through this during write(). This implicitly added the missing check
> > > for privileges required for HT polling for cgroups.
> > >
> > > When 519fabc7aaba ("psi: remove 500ms min window size limitation for
> > > triggers") followed right after to remove further restrictions on the
> > > RT polling window, it incorrectly assumed the cgroup privilege check
> > > was still missing and added it to the cgroup open(), mirroring what we
> > > used to do for proc files in the past.
> > >
> > > As a result, unprivileged poll requests that would be supported now
> > > get rejected when opening the cgroup pressure file for writing.
> 
> Ah, I see the problem. In our discussion
> https://lore.kernel.org/all/ZADj4YX4uftK%2FFrh@cmpxchg.org/ we decided
> to have the check in open() to fail early but we never considered
> unprivileged processes which only poll and never create any triggers.
> Makes sense.

Yeah, the two patches just ended up clashing. We made that open()
decision before unprivileged polling was merged, then ended up merging
it before the window patch.

Thanks!

Re: [PATCH] sched: psi: fix unprivileged polling against cgroups

[Peter Zijlstra]

+cc tj because cgroup

On Thu, Oct 26, 2023 at 12:41:14PM -0400, Johannes Weiner wrote:
> 519fabc7aaba ("psi: remove 500ms min window size limitation for
> triggers") breaks unprivileged psi polling on cgroups.
> 
> Historically, we had a privilege check for polling in the open() of a
> pressure file in /proc, but were erroneously missing it for the open()
> of cgroup pressure files.
> 
> When unprivileged polling was introduced in d82caa273565 ("sched/psi:
> Allow unprivileged polling of N*2s period"), it needed to filter
> privileges depending on the exact polling parameters, and as such
> moved the CAP_SYS_RESOURCE check from the proc open() callback to
> psi_trigger_create(). Both the proc files as well as cgroup files go
> through this during write(). This implicitly added the missing check
> for privileges required for HT polling for cgroups.
> 
> When 519fabc7aaba ("psi: remove 500ms min window size limitation for
> triggers") followed right after to remove further restrictions on the
> RT polling window, it incorrectly assumed the cgroup privilege check
> was still missing and added it to the cgroup open(), mirroring what we
> used to do for proc files in the past.
> 
> As a result, unprivileged poll requests that would be supported now
> get rejected when opening the cgroup pressure file for writing.
> 
> Remove the cgroup open() check. psi_trigger_create() handles it.
> 
> Fixes: 519fabc7aaba ("psi: remove 500ms min window size limitation for triggers")
> Cc: stable@vger.kernel.org # 6.5+
> Reported-by: Luca Boccassi <bluca@debian.org>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Since merge window is upon is, I've queued this with the intent to stick
into sched/urgent after rc1.

> ---
>  kernel/cgroup/cgroup.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index f11488b18ceb..2069ee98da60 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -3879,14 +3879,6 @@ static __poll_t cgroup_pressure_poll(struct kernfs_open_file *of,
>  	return psi_trigger_poll(&ctx->psi.trigger, of->file, pt);
>  }
>  
> -static int cgroup_pressure_open(struct kernfs_open_file *of)
> -{
> -	if (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE))
> -		return -EPERM;
> -
> -	return 0;
> -}
> -
>  static void cgroup_pressure_release(struct kernfs_open_file *of)
>  {
>  	struct cgroup_file_ctx *ctx = of->priv;
> @@ -5287,7 +5279,6 @@ static struct cftype cgroup_psi_files[] = {
>  	{
>  		.name = "io.pressure",
>  		.file_offset = offsetof(struct cgroup, psi_files[PSI_IO]),
> -		.open = cgroup_pressure_open,
>  		.seq_show = cgroup_io_pressure_show,
>  		.write = cgroup_io_pressure_write,
>  		.poll = cgroup_pressure_poll,
> @@ -5296,7 +5287,6 @@ static struct cftype cgroup_psi_files[] = {
>  	{
>  		.name = "memory.pressure",
>  		.file_offset = offsetof(struct cgroup, psi_files[PSI_MEM]),
> -		.open = cgroup_pressure_open,
>  		.seq_show = cgroup_memory_pressure_show,
>  		.write = cgroup_memory_pressure_write,
>  		.poll = cgroup_pressure_poll,
> @@ -5305,7 +5295,6 @@ static struct cftype cgroup_psi_files[] = {
>  	{
>  		.name = "cpu.pressure",
>  		.file_offset = offsetof(struct cgroup, psi_files[PSI_CPU]),
> -		.open = cgroup_pressure_open,
>  		.seq_show = cgroup_cpu_pressure_show,
>  		.write = cgroup_cpu_pressure_write,
>  		.poll = cgroup_pressure_poll,
> @@ -5315,7 +5304,6 @@ static struct cftype cgroup_psi_files[] = {
>  	{
>  		.name = "irq.pressure",
>  		.file_offset = offsetof(struct cgroup, psi_files[PSI_IRQ]),
> -		.open = cgroup_pressure_open,
>  		.seq_show = cgroup_irq_pressure_show,
>  		.write = cgroup_irq_pressure_write,
>  		.poll = cgroup_pressure_poll,
> -- 
> 2.42.0
> 

[tip: sched/urgent] sched: psi: fix unprivileged polling against cgroups

[tip-bot2 for Johannes Weiner]

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     8b39d20eceeda6c4eb23df1497f9ed2fffdc8f69
Gitweb:        https://git.kernel.org/tip/8b39d20eceeda6c4eb23df1497f9ed2fffdc8f69
Author:        Johannes Weiner <hannes@cmpxchg.org>
AuthorDate:    Thu, 26 Oct 2023 12:41:14 -04:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 14 Nov 2023 22:27:00 +01:00

sched: psi: fix unprivileged polling against cgroups

519fabc7aaba ("psi: remove 500ms min window size limitation for
triggers") breaks unprivileged psi polling on cgroups.

Historically, we had a privilege check for polling in the open() of a
pressure file in /proc, but were erroneously missing it for the open()
of cgroup pressure files.

When unprivileged polling was introduced in d82caa273565 ("sched/psi:
Allow unprivileged polling of N*2s period"), it needed to filter
privileges depending on the exact polling parameters, and as such
moved the CAP_SYS_RESOURCE check from the proc open() callback to
psi_trigger_create(). Both the proc files as well as cgroup files go
through this during write(). This implicitly added the missing check
for privileges required for HT polling for cgroups.

When 519fabc7aaba ("psi: remove 500ms min window size limitation for
triggers") followed right after to remove further restrictions on the
RT polling window, it incorrectly assumed the cgroup privilege check
was still missing and added it to the cgroup open(), mirroring what we
used to do for proc files in the past.

As a result, unprivileged poll requests that would be supported now
get rejected when opening the cgroup pressure file for writing.

Remove the cgroup open() check. psi_trigger_create() handles it.

Fixes: 519fabc7aaba ("psi: remove 500ms min window size limitation for triggers")
Reported-by: Luca Boccassi <bluca@debian.org>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Luca Boccassi <bluca@debian.org>
Acked-by: Suren Baghdasaryan <surenb@google.com>
Cc: stable@vger.kernel.org # 6.5+
Link: https://lore.kernel.org/r/20231026164114.2488682-1-hannes@cmpxchg.org
---
 kernel/cgroup/cgroup.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 1d5b9de..4b9ff41 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3885,14 +3885,6 @@ static __poll_t cgroup_pressure_poll(struct kernfs_open_file *of,
 	return psi_trigger_poll(&ctx->psi.trigger, of->file, pt);
 }
 
-static int cgroup_pressure_open(struct kernfs_open_file *of)
-{
-	if (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE))
-		return -EPERM;
-
-	return 0;
-}
-
 static void cgroup_pressure_release(struct kernfs_open_file *of)
 {
 	struct cgroup_file_ctx *ctx = of->priv;
@@ -5299,7 +5291,6 @@ static struct cftype cgroup_psi_files[] = {
 	{
 		.name = "io.pressure",
 		.file_offset = offsetof(struct cgroup, psi_files[PSI_IO]),
-		.open = cgroup_pressure_open,
 		.seq_show = cgroup_io_pressure_show,
 		.write = cgroup_io_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5308,7 +5299,6 @@ static struct cftype cgroup_psi_files[] = {
 	{
 		.name = "memory.pressure",
 		.file_offset = offsetof(struct cgroup, psi_files[PSI_MEM]),
-		.open = cgroup_pressure_open,
 		.seq_show = cgroup_memory_pressure_show,
 		.write = cgroup_memory_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5317,7 +5307,6 @@ static struct cftype cgroup_psi_files[] = {
 	{
 		.name = "cpu.pressure",
 		.file_offset = offsetof(struct cgroup, psi_files[PSI_CPU]),
-		.open = cgroup_pressure_open,
 		.seq_show = cgroup_cpu_pressure_show,
 		.write = cgroup_cpu_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5327,7 +5316,6 @@ static struct cftype cgroup_psi_files[] = {
 	{
 		.name = "irq.pressure",
 		.file_offset = offsetof(struct cgroup, psi_files[PSI_IRQ]),
-		.open = cgroup_pressure_open,
 		.seq_show = cgroup_irq_pressure_show,
 		.write = cgroup_irq_pressure_write,
 		.poll = cgroup_pressure_poll,

Re: [PATCH] sched: psi: fix unprivileged polling against cgroups

[Daniel Black]

Thank you,

Reported-by: Daniel Black <daniel@mariadb.org>