Re: [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings

2021-03-30 Thread Michal Koutný
On Tue, Mar 30, 2021 at 11:00:36AM +0200, Arnd Bergmann  wrote:
> Would it be possible to enclose most or all of kernel/cgroup/cgroup.c
> in an #ifdef CGROUP_SUBSYS_COUNT block?
Even without any controllers, there can still be named hierarchies (v1)
or the default hierarchy (v2) (for instance) for process tracking
purposes. So only parts of kernel/cgroup/cgroup.c could be ifdef'd.

Beware that CGROUP_SUBSYS_COUNT is not known at preprocessing stage (you
could have a macro alternative though).

> I didn't try that myself, but this might be a way to guarantee that
> there cannot be any callers (it would cause a link error).
Such a guarantee would be nicer, I agree. I tried a bit but anandoned it
when I saw macros proliferate (which I found less readable than your
current variant). But YMMV.

Michal


signature.asc
Description: Digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings

2021-03-30 Thread Michal Koutný
On Mon, Mar 22, 2021 at 05:02:44PM +0100, Arnd Bergmann  wrote:
> I'm not sure what is expected to happen for such a configuration,
> presumably these functions are never calls in that case.
Yes, the functions you patched would only be called from subsystems or
there should be no way to obtain a struct cgroup_subsys reference
anyway (hence it's ok to always branch as if ss==NULL).

I'd prefer a variant that wouldn't compile the affected codepaths when
there are no subsystems registered, however, I couldn't come up with a
way how to do it without some preprocessor ugliness.

Reviewed-by: Michal Koutný 


signature.asc
Description: Digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings

2021-03-30 Thread Arnd Bergmann
On Tue, Mar 30, 2021 at 10:41 AM Michal Koutný  wrote:
>
> On Mon, Mar 22, 2021 at 05:02:44PM +0100, Arnd Bergmann  
> wrote:
> > I'm not sure what is expected to happen for such a configuration,
> > presumably these functions are never calls in that case.
> Yes, the functions you patched would only be called from subsystems or
> there should be no way to obtain a struct cgroup_subsys reference
> anyway (hence it's ok to always branch as if ss==NULL).
>
> I'd prefer a variant that wouldn't compile the affected codepaths when
> there are no subsystems registered, however, I couldn't come up with a
> way how to do it without some preprocessor ugliness.

Would it be possible to enclose most or all of kernel/cgroup/cgroup.c
in an #ifdef CGROUP_SUBSYS_COUNT block? I didn't try that
myself, but this might be a way to guarantee that there cannot
be any callers (it would cause a link error).

> Reviewed-by: Michal Koutný 

Thanks

Arnd
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH 06/11] cgroup: fix -Wzero-length-bounds warnings

2021-03-22 Thread Arnd Bergmann
From: Arnd Bergmann 

When cgroups are enabled, but every single subsystem is turned off,
CGROUP_SUBSYS_COUNT is zero, and the cgrp->subsys[] array has no
members.

gcc-11 points out that this leads to an invalid access in any function
that might access this array:

kernel/cgroup/cgroup.c: In function 'cgroup_addrm_files':
kernel/cgroup/cgroup.c:460:58: warning: array subscript '' is outside 
the bounds of an interior zero-length array 'struct cgroup_subsys_state *[0]' 
[-Wzero-length-bounds]
kernel/cgroup/cgroup.c:460:24: note: in expansion of macro 
'rcu_dereference_check'
  460 | return rcu_dereference_check(cgrp->subsys[ss->id],
  |^
In file included from include/linux/cgroup.h:28,
 from kernel/cgroup/cgroup-internal.h:5,
 from kernel/cgroup/cgroup.c:31:
include/linux/cgroup-defs.h:422:43: note: while referencing 'subsys'
  422 | struct cgroup_subsys_state __rcu *subsys[CGROUP_SUBSYS_COUNT];

I'm not sure what is expected to happen for such a configuration,
presumably these functions are never calls in that case. Adding a
sanity check in each function we get the warning for manages to shut
up the warnings and do nothing instead.

Signed-off-by: Arnd Bergmann 
---
I'm grouping this together with the -Wstringop-overread warnings,
since the underlying logic in gcc seems to be the same.
---
 kernel/cgroup/cgroup.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 9153b20e5cc6..3477f1dc7872 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -456,7 +456,7 @@ static u16 cgroup_ss_mask(struct cgroup *cgrp)
 static struct cgroup_subsys_state *cgroup_css(struct cgroup *cgrp,
  struct cgroup_subsys *ss)
 {
-   if (ss)
+   if (ss && (CGROUP_SUBSYS_COUNT > 0))
return rcu_dereference_check(cgrp->subsys[ss->id],
lockdep_is_held(_mutex));
else
@@ -534,6 +534,9 @@ struct cgroup_subsys_state *cgroup_e_css(struct cgroup 
*cgrp,
 {
struct cgroup_subsys_state *css;
 
+   if (CGROUP_SUBSYS_COUNT == 0)
+   return NULL;
+
do {
css = cgroup_css(cgrp, ss);
 
@@ -561,6 +564,9 @@ struct cgroup_subsys_state *cgroup_get_e_css(struct cgroup 
*cgrp,
 {
struct cgroup_subsys_state *css;
 
+   if (CGROUP_SUBSYS_COUNT == 0)
+   return NULL;
+
rcu_read_lock();
 
do {
@@ -630,7 +636,7 @@ struct cgroup_subsys_state *of_css(struct kernfs_open_file 
*of)
 * the matching css from the cgroup's subsys table is guaranteed to
 * be and stay valid until the enclosing operation is complete.
 */
-   if (cft->ss)
+   if (cft->ss && CGROUP_SUBSYS_COUNT > 0)
return rcu_dereference_raw(cgrp->subsys[cft->ss->id]);
else
return >self;
@@ -2343,6 +2349,9 @@ struct task_struct *cgroup_taskset_next(struct 
cgroup_taskset *tset,
struct css_set *cset = tset->cur_cset;
struct task_struct *task = tset->cur_task;
 
+   if (CGROUP_SUBSYS_COUNT == 0)
+   return NULL;
+
while (>mg_node != tset->csets) {
if (!task)
task = list_first_entry(>mg_tasks,
@@ -4523,7 +4532,7 @@ void css_task_iter_start(struct cgroup_subsys_state *css, 
unsigned int flags,
it->ss = css->ss;
it->flags = flags;
 
-   if (it->ss)
+   if (it->ss && CGROUP_SUBSYS_COUNT > 0)
it->cset_pos = >cgroup->e_csets[css->ss->id];
else
it->cset_pos = >cgroup->cset_links;
-- 
2.29.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel