[PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Suren Baghdasaryan]

Current 500ms min window size for psi triggers limits polling interval
to 50ms to prevent polling threads from using too much cpu bandwidth by
polling too frequently. However the number of cgroups with triggers is
unlimited, so this protection can be defeated by creating multiple
cgroups with psi triggers (triggers in each cgroup are served by a single
"psimon" kernel thread).
Instead of limiting min polling period, which also limits the latency of
psi events, it's better to limit psi trigger creation to authorized users
only, like we do for system-wide psi triggers (/proc/pressure/* files can
be written only by processes with CAP_SYS_RESOURCE capability). This also
makes access rules for cgroup psi files consistent with system-wide ones.
Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
remove the psi window min size limitation.

Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 kernel/cgroup/cgroup.c | 10 ++++++++++
 kernel/sched/psi.c     |  4 +---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 935e8121b21e..b600a6baaeca 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3867,6 +3867,12 @@ 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)
+{
+	return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
+		-EPERM : 0;
+}
+
 static void cgroup_pressure_release(struct kernfs_open_file *of)
 {
 	struct cgroup_file_ctx *ctx = of->priv;
@@ -5266,6 +5272,7 @@ 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,
@@ -5274,6 +5281,7 @@ 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,
@@ -5282,6 +5290,7 @@ 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,
@@ -5291,6 +5300,7 @@ 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,
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 02e011cabe91..9c02b27052bb 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -160,7 +160,6 @@ __setup("psi=", setup_psi);
 #define EXP_300s	2034		/* 1/exp(2s/300s) */
 
 /* PSI trigger definitions */
-#define WINDOW_MIN_US 500000	/* Min window size is 500ms */
 #define WINDOW_MAX_US 10000000	/* Max window size is 10s */
 #define UPDATES_PER_WINDOW 10	/* 10 updates per window */
 
@@ -1278,8 +1277,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
 	if (state >= PSI_NONIDLE)
 		return ERR_PTR(-EINVAL);
 
-	if (window_us < WINDOW_MIN_US ||
-		window_us > WINDOW_MAX_US)
+	if (window_us <= 0 || window_us > WINDOW_MAX_US)
 		return ERR_PTR(-EINVAL);
 
 	/* Check threshold */
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

[PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Suren Baghdasaryan]

Current 500ms min window size for psi triggers limits polling interval
to 50ms to prevent polling threads from using too much cpu bandwidth by
polling too frequently. However the number of cgroups with triggers is
unlimited, so this protection can be defeated by creating multiple
cgroups with psi triggers (triggers in each cgroup are served by a single
"psimon" kernel thread).
Instead of limiting min polling period, which also limits the latency of
psi events, it's better to limit psi trigger creation to authorized users
only, like we do for system-wide psi triggers (/proc/pressure/* files can
be written only by processes with CAP_SYS_RESOURCE capability). This also
makes access rules for cgroup psi files consistent with system-wide ones.
Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
remove the psi window min size limitation.

Suggested-by: Sudarshan Rajagopalan <quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org/
Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup/cgroup.c | 10 ++++++++++
 kernel/sched/psi.c     |  4 +---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 935e8121b21e..b600a6baaeca 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3867,6 +3867,12 @@ 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)
+{
+	return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
+		-EPERM : 0;
+}
+
 static void cgroup_pressure_release(struct kernfs_open_file *of)
 {
 	struct cgroup_file_ctx *ctx = of->priv;
@@ -5266,6 +5272,7 @@ 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,
@@ -5274,6 +5281,7 @@ 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,
@@ -5282,6 +5290,7 @@ 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,
@@ -5291,6 +5300,7 @@ 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,
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 02e011cabe91..9c02b27052bb 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -160,7 +160,6 @@ __setup("psi=", setup_psi);
 #define EXP_300s	2034		/* 1/exp(2s/300s) */
 
 /* PSI trigger definitions */
-#define WINDOW_MIN_US 500000	/* Min window size is 500ms */
 #define WINDOW_MAX_US 10000000	/* Max window size is 10s */
 #define UPDATES_PER_WINDOW 10	/* 10 updates per window */
 
@@ -1278,8 +1277,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
 	if (state >= PSI_NONIDLE)
 		return ERR_PTR(-EINVAL);
 
-	if (window_us < WINDOW_MIN_US ||
-		window_us > WINDOW_MAX_US)
+	if (window_us <= 0 || window_us > WINDOW_MAX_US)
 		return ERR_PTR(-EINVAL);
 
 	/* Check threshold */
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Johannes Weiner]

On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> Current 500ms min window size for psi triggers limits polling interval
> to 50ms to prevent polling threads from using too much cpu bandwidth by
> polling too frequently. However the number of cgroups with triggers is
> unlimited, so this protection can be defeated by creating multiple
> cgroups with psi triggers (triggers in each cgroup are served by a single
> "psimon" kernel thread).
> Instead of limiting min polling period, which also limits the latency of
> psi events, it's better to limit psi trigger creation to authorized users
> only, like we do for system-wide psi triggers (/proc/pressure/* files can
> be written only by processes with CAP_SYS_RESOURCE capability). This also
> makes access rules for cgroup psi files consistent with system-wide ones.
> Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> remove the psi window min size limitation.
> 
> Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
> Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  kernel/cgroup/cgroup.c | 10 ++++++++++
>  kernel/sched/psi.c     |  4 +---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 935e8121b21e..b600a6baaeca 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -3867,6 +3867,12 @@ 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)
> +{
> +	return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> +		-EPERM : 0;
> +}

I agree with the change, but it's a bit unfortunate that this check is
duplicated between system and cgroup.

What do you think about psi_trigger_create() taking the file and
checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Johannes Weiner]

On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> Current 500ms min window size for psi triggers limits polling interval
> to 50ms to prevent polling threads from using too much cpu bandwidth by
> polling too frequently. However the number of cgroups with triggers is
> unlimited, so this protection can be defeated by creating multiple
> cgroups with psi triggers (triggers in each cgroup are served by a single
> "psimon" kernel thread).
> Instead of limiting min polling period, which also limits the latency of
> psi events, it's better to limit psi trigger creation to authorized users
> only, like we do for system-wide psi triggers (/proc/pressure/* files can
> be written only by processes with CAP_SYS_RESOURCE capability). This also
> makes access rules for cgroup psi files consistent with system-wide ones.
> Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> remove the psi window min size limitation.
> 
> Suggested-by: Sudarshan Rajagopalan <quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
> Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org/
> Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  kernel/cgroup/cgroup.c | 10 ++++++++++
>  kernel/sched/psi.c     |  4 +---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 935e8121b21e..b600a6baaeca 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -3867,6 +3867,12 @@ 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)
> +{
> +	return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> +		-EPERM : 0;
> +}

I agree with the change, but it's a bit unfortunate that this check is
duplicated between system and cgroup.

What do you think about psi_trigger_create() taking the file and
checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Suren Baghdasaryan]

On Wed, Mar 1, 2023 at 12:07 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> > Current 500ms min window size for psi triggers limits polling interval
> > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > polling too frequently. However the number of cgroups with triggers is
> > unlimited, so this protection can be defeated by creating multiple
> > cgroups with psi triggers (triggers in each cgroup are served by a single
> > "psimon" kernel thread).
> > Instead of limiting min polling period, which also limits the latency of
> > psi events, it's better to limit psi trigger creation to authorized users
> > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > makes access rules for cgroup psi files consistent with system-wide ones.
> > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > remove the psi window min size limitation.
> >
> > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
> > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >  kernel/cgroup/cgroup.c | 10 ++++++++++
> >  kernel/sched/psi.c     |  4 +---
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > index 935e8121b21e..b600a6baaeca 100644
> > --- a/kernel/cgroup/cgroup.c
> > +++ b/kernel/cgroup/cgroup.c
> > @@ -3867,6 +3867,12 @@ 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)
> > +{
> > +     return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> > +             -EPERM : 0;
> > +}
>
> I agree with the change, but it's a bit unfortunate that this check is
> duplicated between system and cgroup.
>
> What do you think about psi_trigger_create() taking the file and
> checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?

That's definitely doable and we don't even need to pass file to
psi_trigger_create() since it's called only when we write to the file.
However by moving the capability check into psi_trigger_create() we
also postpone the check until write() instead of failing early in
open(). I always assumed failing early is preferable but if
consolidating the code here makes more sense then I can make the
switch. Please let me know if you still prefer to move the check.

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Suren Baghdasaryan]

On Wed, Mar 1, 2023 at 12:07 PM Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> wrote:
>
> On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> > Current 500ms min window size for psi triggers limits polling interval
> > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > polling too frequently. However the number of cgroups with triggers is
> > unlimited, so this protection can be defeated by creating multiple
> > cgroups with psi triggers (triggers in each cgroup are served by a single
> > "psimon" kernel thread).
> > Instead of limiting min polling period, which also limits the latency of
> > psi events, it's better to limit psi trigger creation to authorized users
> > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > makes access rules for cgroup psi files consistent with system-wide ones.
> > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > remove the psi window min size limitation.
> >
> > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
> > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > ---
> >  kernel/cgroup/cgroup.c | 10 ++++++++++
> >  kernel/sched/psi.c     |  4 +---
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > index 935e8121b21e..b600a6baaeca 100644
> > --- a/kernel/cgroup/cgroup.c
> > +++ b/kernel/cgroup/cgroup.c
> > @@ -3867,6 +3867,12 @@ 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)
> > +{
> > +     return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> > +             -EPERM : 0;
> > +}
>
> I agree with the change, but it's a bit unfortunate that this check is
> duplicated between system and cgroup.
>
> What do you think about psi_trigger_create() taking the file and
> checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?

That's definitely doable and we don't even need to pass file to
psi_trigger_create() since it's called only when we write to the file.
However by moving the capability check into psi_trigger_create() we
also postpone the check until write() instead of failing early in
open(). I always assumed failing early is preferable but if
consolidating the code here makes more sense then I can make the
switch. Please let me know if you still prefer to move the check.

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Kalesh Singh]

On Wed, Mar 1, 2023 at 12:48 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Mar 1, 2023 at 12:07 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> > > Current 500ms min window size for psi triggers limits polling interval
> > > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > > polling too frequently. However the number of cgroups with triggers is
> > > unlimited, so this protection can be defeated by creating multiple
> > > cgroups with psi triggers (triggers in each cgroup are served by a single
> > > "psimon" kernel thread).
> > > Instead of limiting min polling period, which also limits the latency of
> > > psi events, it's better to limit psi trigger creation to authorized users
> > > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > > makes access rules for cgroup psi files consistent with system-wide ones.
> > > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > > remove the psi window min size limitation.
> > >
> > > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
> > > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > ---
> > >  kernel/cgroup/cgroup.c | 10 ++++++++++
> > >  kernel/sched/psi.c     |  4 +---
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > index 935e8121b21e..b600a6baaeca 100644
> > > --- a/kernel/cgroup/cgroup.c
> > > +++ b/kernel/cgroup/cgroup.c
> > > @@ -3867,6 +3867,12 @@ 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)
> > > +{
> > > +     return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> > > +             -EPERM : 0;
> > > +}
> >
> > I agree with the change, but it's a bit unfortunate that this check is
> > duplicated between system and cgroup.
> >
> > What do you think about psi_trigger_create() taking the file and
> > checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?
>
> That's definitely doable and we don't even need to pass file to
> psi_trigger_create() since it's called only when we write to the file.
> However by moving the capability check into psi_trigger_create() we
> also postpone the check until write() instead of failing early in
> open(). I always assumed failing early is preferable but if
> consolidating the code here makes more sense then I can make the
> switch. Please let me know if you still prefer to move the check.
>

I always assumed the convention is to check the credentials on open()
per Linus comment here:

https://lore.kernel.org/r/CAHk-=whDkekE8n2LdPiKHeTdRnV--ys0V0nPZ76oPaE0fn-d+g@mail.gmail.com/#t

--Kalesh

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Kalesh Singh]

On Wed, Mar 1, 2023 at 12:48 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Mar 1, 2023 at 12:07 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> > > Current 500ms min window size for psi triggers limits polling interval
> > > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > > polling too frequently. However the number of cgroups with triggers is
> > > unlimited, so this protection can be defeated by creating multiple
> > > cgroups with psi triggers (triggers in each cgroup are served by a single
> > > "psimon" kernel thread).
> > > Instead of limiting min polling period, which also limits the latency of
> > > psi events, it's better to limit psi trigger creation to authorized users
> > > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > > makes access rules for cgroup psi files consistent with system-wide ones.
> > > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > > remove the psi window min size limitation.
> > >
> > > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
> > > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > > Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  kernel/cgroup/cgroup.c | 10 ++++++++++
> > >  kernel/sched/psi.c     |  4 +---
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > index 935e8121b21e..b600a6baaeca 100644
> > > --- a/kernel/cgroup/cgroup.c
> > > +++ b/kernel/cgroup/cgroup.c
> > > @@ -3867,6 +3867,12 @@ 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)
> > > +{
> > > +     return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> > > +             -EPERM : 0;
> > > +}
> >
> > I agree with the change, but it's a bit unfortunate that this check is
> > duplicated between system and cgroup.
> >
> > What do you think about psi_trigger_create() taking the file and
> > checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?
>
> That's definitely doable and we don't even need to pass file to
> psi_trigger_create() since it's called only when we write to the file.
> However by moving the capability check into psi_trigger_create() we
> also postpone the check until write() instead of failing early in
> open(). I always assumed failing early is preferable but if
> consolidating the code here makes more sense then I can make the
> switch. Please let me know if you still prefer to move the check.
>

I always assumed the convention is to check the credentials on open()
per Linus comment here:

https://lore.kernel.org/r/CAHk-=whDkekE8n2LdPiKHeTdRnV--ys0V0nPZ76oPaE0fn-d+g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/#t

--Kalesh

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Johannes Weiner]

On Wed, Mar 01, 2023 at 12:48:38PM -0800, Suren Baghdasaryan wrote:
> On Wed, Mar 1, 2023 at 12:07 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> > > Current 500ms min window size for psi triggers limits polling interval
> > > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > > polling too frequently. However the number of cgroups with triggers is
> > > unlimited, so this protection can be defeated by creating multiple
> > > cgroups with psi triggers (triggers in each cgroup are served by a single
> > > "psimon" kernel thread).
> > > Instead of limiting min polling period, which also limits the latency of
> > > psi events, it's better to limit psi trigger creation to authorized users
> > > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > > makes access rules for cgroup psi files consistent with system-wide ones.
> > > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > > remove the psi window min size limitation.
> > >
> > > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
> > > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > ---
> > >  kernel/cgroup/cgroup.c | 10 ++++++++++
> > >  kernel/sched/psi.c     |  4 +---
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > index 935e8121b21e..b600a6baaeca 100644
> > > --- a/kernel/cgroup/cgroup.c
> > > +++ b/kernel/cgroup/cgroup.c
> > > @@ -3867,6 +3867,12 @@ 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)
> > > +{
> > > +     return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> > > +             -EPERM : 0;
> > > +}
> >
> > I agree with the change, but it's a bit unfortunate that this check is
> > duplicated between system and cgroup.
> >
> > What do you think about psi_trigger_create() taking the file and
> > checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?
> 
> That's definitely doable and we don't even need to pass file to
> psi_trigger_create() since it's called only when we write to the file.
> However by moving the capability check into psi_trigger_create() we
> also postpone the check until write() instead of failing early in
> open(). I always assumed failing early is preferable but if
> consolidating the code here makes more sense then I can make the
> switch. Please let me know if you still prefer to move the check.

Just for context, a person on our team is working on allowing
unprivileged polls with windows that are multiples of 2s, which can be
triggered from the regular aggregator threads. This should be useful
for container delegation, and also for the desktop monitor app usecase
that Chris Down brought up some time ago. At that point, everybody can
open the file for write, and permissions are checked against the
trigger parameters.

So I don't think it's a big deal to check this particular permission
at write time. But if you prefer we can also merge your patch as-is
and do the refactor as part of the other series.

Your call. In either case, please feel free to add

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Johannes Weiner]

On Wed, Mar 01, 2023 at 12:48:38PM -0800, Suren Baghdasaryan wrote:
> On Wed, Mar 1, 2023 at 12:07 PM Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> wrote:
> >
> > On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> > > Current 500ms min window size for psi triggers limits polling interval
> > > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > > polling too frequently. However the number of cgroups with triggers is
> > > unlimited, so this protection can be defeated by creating multiple
> > > cgroups with psi triggers (triggers in each cgroup are served by a single
> > > "psimon" kernel thread).
> > > Instead of limiting min polling period, which also limits the latency of
> > > psi events, it's better to limit psi trigger creation to authorized users
> > > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > > makes access rules for cgroup psi files consistent with system-wide ones.
> > > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > > remove the psi window min size limitation.
> > >
> > > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
> > > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org/
> > > Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  kernel/cgroup/cgroup.c | 10 ++++++++++
> > >  kernel/sched/psi.c     |  4 +---
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > index 935e8121b21e..b600a6baaeca 100644
> > > --- a/kernel/cgroup/cgroup.c
> > > +++ b/kernel/cgroup/cgroup.c
> > > @@ -3867,6 +3867,12 @@ 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)
> > > +{
> > > +     return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> > > +             -EPERM : 0;
> > > +}
> >
> > I agree with the change, but it's a bit unfortunate that this check is
> > duplicated between system and cgroup.
> >
> > What do you think about psi_trigger_create() taking the file and
> > checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?
> 
> That's definitely doable and we don't even need to pass file to
> psi_trigger_create() since it's called only when we write to the file.
> However by moving the capability check into psi_trigger_create() we
> also postpone the check until write() instead of failing early in
> open(). I always assumed failing early is preferable but if
> consolidating the code here makes more sense then I can make the
> switch. Please let me know if you still prefer to move the check.

Just for context, a person on our team is working on allowing
unprivileged polls with windows that are multiples of 2s, which can be
triggered from the regular aggregator threads. This should be useful
for container delegation, and also for the desktop monitor app usecase
that Chris Down brought up some time ago. At that point, everybody can
open the file for write, and permissions are checked against the
trigger parameters.

So I don't think it's a big deal to check this particular permission
at write time. But if you prefer we can also merge your patch as-is
and do the refactor as part of the other series.

Your call. In either case, please feel free to add

Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Suren Baghdasaryan]

On Thu, Mar 2, 2023 at 7:30 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Wed, Mar 01, 2023 at 12:48:38PM -0800, Suren Baghdasaryan wrote:
> > On Wed, Mar 1, 2023 at 12:07 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > >
> > > On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> > > > Current 500ms min window size for psi triggers limits polling interval
> > > > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > > > polling too frequently. However the number of cgroups with triggers is
> > > > unlimited, so this protection can be defeated by creating multiple
> > > > cgroups with psi triggers (triggers in each cgroup are served by a single
> > > > "psimon" kernel thread).
> > > > Instead of limiting min polling period, which also limits the latency of
> > > > psi events, it's better to limit psi trigger creation to authorized users
> > > > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > > > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > > > makes access rules for cgroup psi files consistent with system-wide ones.
> > > > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > > > remove the psi window min size limitation.
> > > >
> > > > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
> > > > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > > ---
> > > >  kernel/cgroup/cgroup.c | 10 ++++++++++
> > > >  kernel/sched/psi.c     |  4 +---
> > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > > index 935e8121b21e..b600a6baaeca 100644
> > > > --- a/kernel/cgroup/cgroup.c
> > > > +++ b/kernel/cgroup/cgroup.c
> > > > @@ -3867,6 +3867,12 @@ 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)
> > > > +{
> > > > +     return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> > > > +             -EPERM : 0;
> > > > +}
> > >
> > > I agree with the change, but it's a bit unfortunate that this check is
> > > duplicated between system and cgroup.
> > >
> > > What do you think about psi_trigger_create() taking the file and
> > > checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?
> >
> > That's definitely doable and we don't even need to pass file to
> > psi_trigger_create() since it's called only when we write to the file.
> > However by moving the capability check into psi_trigger_create() we
> > also postpone the check until write() instead of failing early in
> > open(). I always assumed failing early is preferable but if
> > consolidating the code here makes more sense then I can make the
> > switch. Please let me know if you still prefer to move the check.
>
> Just for context, a person on our team is working on allowing
> unprivileged polls with windows that are multiples of 2s, which can be
> triggered from the regular aggregator threads. This should be useful
> for container delegation, and also for the desktop monitor app usecase
> that Chris Down brought up some time ago. At that point, everybody can
> open the file for write, and permissions are checked against the
> trigger parameters.
>
> So I don't think it's a big deal to check this particular permission
> at write time. But if you prefer we can also merge your patch as-is
> and do the refactor as part of the other series.

Let's roll this check without additional changes and then consolidate
the checking inside psi_trigger_create() in a separate patch. If
anybody objects to the late permission check we will just revert that
last change without affecting anything else.

>
> Your call. In either case, please feel free to add
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Thanks! Will post the final patch with Ack's later today. Originally
it was purely cgroup-related change but now it's more of a PSI change.
Therefore Peter's tree will probably be the right place for it.

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Suren Baghdasaryan]

On Thu, Mar 2, 2023 at 7:30 AM Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> wrote:
>
> On Wed, Mar 01, 2023 at 12:48:38PM -0800, Suren Baghdasaryan wrote:
> > On Wed, Mar 1, 2023 at 12:07 PM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > >
> > > On Wed, Mar 01, 2023 at 11:34:03AM -0800, Suren Baghdasaryan wrote:
> > > > Current 500ms min window size for psi triggers limits polling interval
> > > > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > > > polling too frequently. However the number of cgroups with triggers is
> > > > unlimited, so this protection can be defeated by creating multiple
> > > > cgroups with psi triggers (triggers in each cgroup are served by a single
> > > > "psimon" kernel thread).
> > > > Instead of limiting min polling period, which also limits the latency of
> > > > psi events, it's better to limit psi trigger creation to authorized users
> > > > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > > > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > > > makes access rules for cgroup psi files consistent with system-wide ones.
> > > > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > > > remove the psi window min size limitation.
> > > >
> > > > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
> > > > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > > > Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  kernel/cgroup/cgroup.c | 10 ++++++++++
> > > >  kernel/sched/psi.c     |  4 +---
> > > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > > > index 935e8121b21e..b600a6baaeca 100644
> > > > --- a/kernel/cgroup/cgroup.c
> > > > +++ b/kernel/cgroup/cgroup.c
> > > > @@ -3867,6 +3867,12 @@ 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)
> > > > +{
> > > > +     return (of->file->f_mode & FMODE_WRITE && !capable(CAP_SYS_RESOURCE)) ?
> > > > +             -EPERM : 0;
> > > > +}
> > >
> > > I agree with the change, but it's a bit unfortunate that this check is
> > > duplicated between system and cgroup.
> > >
> > > What do you think about psi_trigger_create() taking the file and
> > > checking FMODE_WRITE and CAP_SYS_RESOURCE against file->f_cred?
> >
> > That's definitely doable and we don't even need to pass file to
> > psi_trigger_create() since it's called only when we write to the file.
> > However by moving the capability check into psi_trigger_create() we
> > also postpone the check until write() instead of failing early in
> > open(). I always assumed failing early is preferable but if
> > consolidating the code here makes more sense then I can make the
> > switch. Please let me know if you still prefer to move the check.
>
> Just for context, a person on our team is working on allowing
> unprivileged polls with windows that are multiples of 2s, which can be
> triggered from the regular aggregator threads. This should be useful
> for container delegation, and also for the desktop monitor app usecase
> that Chris Down brought up some time ago. At that point, everybody can
> open the file for write, and permissions are checked against the
> trigger parameters.
>
> So I don't think it's a big deal to check this particular permission
> at write time. But if you prefer we can also merge your patch as-is
> and do the refactor as part of the other series.

Let's roll this check without additional changes and then consolidate
the checking inside psi_trigger_create() in a separate patch. If
anybody objects to the late permission check we will just revert that
last change without affecting anything else.

>
> Your call. In either case, please feel free to add
>
> Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>

Thanks! Will post the final patch with Ack's later today. Originally
it was purely cgroup-related change but now it's more of a PSI change.
Therefore Peter's tree will probably be the right place for it.

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Michal Hocko]

On Thu 02-03-23 08:13:54, Suren Baghdasaryan wrote:
[...]
> Let's roll this check without additional changes and then consolidate
> the checking inside psi_trigger_create() in a separate patch. If
> anybody objects to the late permission check we will just revert that
> last change without affecting anything else.

Permissions checks at write time are problematic because userspace
cannot drop privileges. Also I think it would be an antipattern for how
we do this in general. 
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Michal Hocko]

On Thu 02-03-23 08:13:54, Suren Baghdasaryan wrote:
[...]
> Let's roll this check without additional changes and then consolidate
> the checking inside psi_trigger_create() in a separate patch. If
> anybody objects to the late permission check we will just revert that
> last change without affecting anything else.

Permissions checks at write time are problematic because userspace
cannot drop privileges. Also I think it would be an antipattern for how
we do this in general. 
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Johannes Weiner]

On Thu, Mar 02, 2023 at 06:41:47PM +0100, Michal Hocko wrote:
> On Thu 02-03-23 08:13:54, Suren Baghdasaryan wrote:
> [...]
> > Let's roll this check without additional changes and then consolidate
> > the checking inside psi_trigger_create() in a separate patch. If
> > anybody objects to the late permission check we will just revert that
> > last change without affecting anything else.
> 
> Permissions checks at write time are problematic because userspace
> cannot drop privileges. Also I think it would be an antipattern for how
> we do this in general. 

The permissions can be checked against opener privileges through
file->f_cred. This allows dropping privileges, as well as passing the
fd to a trusted but unprivileged process to delegate trigger setup.

I agree with keeping it in open() for now. But it will matter when we
distinguish between privileged and unprivileged trigger parameters.

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Johannes Weiner]

On Thu, Mar 02, 2023 at 06:41:47PM +0100, Michal Hocko wrote:
> On Thu 02-03-23 08:13:54, Suren Baghdasaryan wrote:
> [...]
> > Let's roll this check without additional changes and then consolidate
> > the checking inside psi_trigger_create() in a separate patch. If
> > anybody objects to the late permission check we will just revert that
> > last change without affecting anything else.
> 
> Permissions checks at write time are problematic because userspace
> cannot drop privileges. Also I think it would be an antipattern for how
> we do this in general. 

The permissions can be checked against opener privileges through
file->f_cred. This allows dropping privileges, as well as passing the
fd to a trusted but unprivileged process to delegate trigger setup.

I agree with keeping it in open() for now. But it will matter when we
distinguish between privileged and unprivileged trigger parameters.

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Michal Hocko]

On Wed 01-03-23 11:34:03, Suren Baghdasaryan wrote:
> Current 500ms min window size for psi triggers limits polling interval
> to 50ms to prevent polling threads from using too much cpu bandwidth by
> polling too frequently. However the number of cgroups with triggers is
> unlimited, so this protection can be defeated by creating multiple
> cgroups with psi triggers (triggers in each cgroup are served by a single
> "psimon" kernel thread).
> Instead of limiting min polling period, which also limits the latency of
> psi events, it's better to limit psi trigger creation to authorized users
> only, like we do for system-wide psi triggers (/proc/pressure/* files can
> be written only by processes with CAP_SYS_RESOURCE capability). This also
> makes access rules for cgroup psi files consistent with system-wide ones.
> Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> remove the psi window min size limitation.
> 
> Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
> Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>

Acked-by: Michal Hocko <mhocko@suse.com>

with this to fix
[...]
> @@ -1278,8 +1277,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
>  	if (state >= PSI_NONIDLE)
>  		return ERR_PTR(-EINVAL);
>  
> -	if (window_us < WINDOW_MIN_US ||
> -		window_us > WINDOW_MAX_US)
> +	if (window_us <= 0 || window_us > WINDOW_MAX_US)
>  		return ERR_PTR(-EINVAL);

window_us is u32 sp the check for <= 0 doesn't make any sense.

>  
>  	/* Check threshold */
> -- 
> 2.40.0.rc0.216.gc4246ad0f0-goog

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Suren Baghdasaryan]

On Thu, Mar 2, 2023 at 5:58 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Wed 01-03-23 11:34:03, Suren Baghdasaryan wrote:
> > Current 500ms min window size for psi triggers limits polling interval
> > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > polling too frequently. However the number of cgroups with triggers is
> > unlimited, so this protection can be defeated by creating multiple
> > cgroups with psi triggers (triggers in each cgroup are served by a single
> > "psimon" kernel thread).
> > Instead of limiting min polling period, which also limits the latency of
> > psi events, it's better to limit psi trigger creation to authorized users
> > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > makes access rules for cgroup psi files consistent with system-wide ones.
> > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > remove the psi window min size limitation.
> >
> > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
> > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> with this to fix
> [...]
> > @@ -1278,8 +1277,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
> >       if (state >= PSI_NONIDLE)
> >               return ERR_PTR(-EINVAL);
> >
> > -     if (window_us < WINDOW_MIN_US ||
> > -             window_us > WINDOW_MAX_US)
> > +     if (window_us <= 0 || window_us > WINDOW_MAX_US)
> >               return ERR_PTR(-EINVAL);
>
> window_us is u32 sp the check for <= 0 doesn't make any sense.

Completely missed that. Will change to == 0 and post the new version.
window_us is later multiplied by NSEC_PER_USEC and then divided by
UPDATES_PER_WINDOW(10), so even the smallest value of 1 still results
in a positive poll_min_period. I think we should be fine with that
check.
Thanks!

>
> >
> >       /* Check threshold */
> > --
> > 2.40.0.rc0.216.gc4246ad0f0-goog
>
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Suren Baghdasaryan]

On Thu, Mar 2, 2023 at 5:58 AM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
>
> On Wed 01-03-23 11:34:03, Suren Baghdasaryan wrote:
> > Current 500ms min window size for psi triggers limits polling interval
> > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > polling too frequently. However the number of cgroups with triggers is
> > unlimited, so this protection can be defeated by creating multiple
> > cgroups with psi triggers (triggers in each cgroup are served by a single
> > "psimon" kernel thread).
> > Instead of limiting min polling period, which also limits the latency of
> > psi events, it's better to limit psi trigger creation to authorized users
> > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > makes access rules for cgroup psi files consistent with system-wide ones.
> > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > remove the psi window min size limitation.
> >
> > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
> > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org/
> > Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> Acked-by: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>
>
> with this to fix
> [...]
> > @@ -1278,8 +1277,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
> >       if (state >= PSI_NONIDLE)
> >               return ERR_PTR(-EINVAL);
> >
> > -     if (window_us < WINDOW_MIN_US ||
> > -             window_us > WINDOW_MAX_US)
> > +     if (window_us <= 0 || window_us > WINDOW_MAX_US)
> >               return ERR_PTR(-EINVAL);
>
> window_us is u32 sp the check for <= 0 doesn't make any sense.

Completely missed that. Will change to == 0 and post the new version.
window_us is later multiplied by NSEC_PER_USEC and then divided by
UPDATES_PER_WINDOW(10), so even the smallest value of 1 still results
in a positive poll_min_period. I think we should be fine with that
check.
Thanks!

>
> >
> >       /* Check threshold */
> > --
> > 2.40.0.rc0.216.gc4246ad0f0-goog
>
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Michal Hocko]

On Thu 02-03-23 08:08:38, Suren Baghdasaryan wrote:
> On Thu, Mar 2, 2023 at 5:58 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Wed 01-03-23 11:34:03, Suren Baghdasaryan wrote:
> > > Current 500ms min window size for psi triggers limits polling interval
> > > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > > polling too frequently. However the number of cgroups with triggers is
> > > unlimited, so this protection can be defeated by creating multiple
> > > cgroups with psi triggers (triggers in each cgroup are served by a single
> > > "psimon" kernel thread).
> > > Instead of limiting min polling period, which also limits the latency of
> > > psi events, it's better to limit psi trigger creation to authorized users
> > > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > > makes access rules for cgroup psi files consistent with system-wide ones.
> > > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > > remove the psi window min size limitation.
> > >
> > > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>
> > > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja@quicinc.com/
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >
> > Acked-by: Michal Hocko <mhocko@suse.com>
> >
> > with this to fix
> > [...]
> > > @@ -1278,8 +1277,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
> > >       if (state >= PSI_NONIDLE)
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     if (window_us < WINDOW_MIN_US ||
> > > -             window_us > WINDOW_MAX_US)
> > > +     if (window_us <= 0 || window_us > WINDOW_MAX_US)
> > >               return ERR_PTR(-EINVAL);
> >
> > window_us is u32 sp the check for <= 0 doesn't make any sense.
> 
> Completely missed that. Will change to == 0 and post the new version.

I was about to asky check for 0 actually matters but maybe a future
extension might give it a special meaning so it is likely a good idea to
EINVAL on 0 now.

You can keep my ack on that change
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/1] psi: remove 500ms min window size limitation for triggers

[Michal Hocko]

On Thu 02-03-23 08:08:38, Suren Baghdasaryan wrote:
> On Thu, Mar 2, 2023 at 5:58 AM Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org> wrote:
> >
> > On Wed 01-03-23 11:34:03, Suren Baghdasaryan wrote:
> > > Current 500ms min window size for psi triggers limits polling interval
> > > to 50ms to prevent polling threads from using too much cpu bandwidth by
> > > polling too frequently. However the number of cgroups with triggers is
> > > unlimited, so this protection can be defeated by creating multiple
> > > cgroups with psi triggers (triggers in each cgroup are served by a single
> > > "psimon" kernel thread).
> > > Instead of limiting min polling period, which also limits the latency of
> > > psi events, it's better to limit psi trigger creation to authorized users
> > > only, like we do for system-wide psi triggers (/proc/pressure/* files can
> > > be written only by processes with CAP_SYS_RESOURCE capability). This also
> > > makes access rules for cgroup psi files consistent with system-wide ones.
> > > Add a CAP_SYS_RESOURCE capability check for cgroup psi file writers and
> > > remove the psi window min size limitation.
> > >
> > > Suggested-by: Sudarshan Rajagopalan <quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
> > > Link: https://lore.kernel.org/all/cover.1676067791.git.quic_sudaraja-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org/
> > > Signed-off-by: Suren Baghdasaryan <surenb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >
> > Acked-by: Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>
> >
> > with this to fix
> > [...]
> > > @@ -1278,8 +1277,7 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
> > >       if (state >= PSI_NONIDLE)
> > >               return ERR_PTR(-EINVAL);
> > >
> > > -     if (window_us < WINDOW_MIN_US ||
> > > -             window_us > WINDOW_MAX_US)
> > > +     if (window_us <= 0 || window_us > WINDOW_MAX_US)
> > >               return ERR_PTR(-EINVAL);
> >
> > window_us is u32 sp the check for <= 0 doesn't make any sense.
> 
> Completely missed that. Will change to == 0 and post the new version.

I was about to asky check for 0 actually matters but maybe a future
extension might give it a special meaning so it is likely a good idea to
EINVAL on 0 now.

You can keep my ack on that change
-- 
Michal Hocko
SUSE Labs