cgroup: Avoid -Wstringop-overflow warnings

PATCH Superseded Gustavo A. R. Silva 4 participants 5 messages Aug 16, 2023 – Aug 17, 2023 cgroup

Share & tools

Gustavo A. R. Silva

author

2023-08-16 20:50 UTC #0 lore raw

PATCH v2 1/1 cgroup: Avoid -Wstringop-overflow warnings

Change the notation from pointer-to-array to pointer-to-pointer. With this, we avoid the compiler complaining about trying to access a region of size zero as an argument during function calls.

Address the following -Wstringop-overflow warnings seen when built with ARM architecture and aspeed_g4_defconfig configuration (notice that under this configuration CGROUP_SUBSYS_COUNT == 0):

kernel/cgroup/cgroup.c:1208:16: warning: 'find_existing_css_set' accessing 4 bytes in a region of size 0 [-Wstringop-overflow=] kernel/cgroup/cgroup.c:1258:15: warning: 'css_set_hash' accessing 4 bytes in a region of size 0 [-Wstringop-overflow=] kernel/cgroup/cgroup.c:6089:18: warning: 'css_set_hash' accessing 4 bytes in a region of size 0 [-Wstringop-overflow=] kernel/cgroup/cgroup.c:6153:18: warning: 'css_set_hash' accessing 4 bytes in a region of size 0 [-Wstringop-overflow=]

This results in no differences in binary output.

Link https://github.com/KSPP/linux/issues/316

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
- Use pointer-to-pointer instead of pointer-to-array.
- Update changelog text.

v1:
- Link: https://lore.kernel.org/linux-hardening/ZIpm3pcs3iCP9UaR@work/

 kernel/cgroup/cgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/cgroup/cgroup.c +2 -2

index ccbbba06da5b..68e2d9812e3f 100644
@@ -929,7 +929,7 @@ static void css_set_move_task(struct task_struct *task,
 #define CSS_SET_HASH_BITS	7
 static DEFINE_HASHTABLE(css_set_table, CSS_SET_HASH_BITS);
 
-static unsigned long css_set_hash(struct cgroup_subsys_state *css[])
+static unsigned long css_set_hash(struct cgroup_subsys_state **css)
 {
 	unsigned long key = 0UL;
 	struct cgroup_subsys *ss;

@@ -1070,7 +1070,7 @@ static bool compare_css_sets(struct css_set *cset,
  */
 static struct css_set *find_existing_css_set(struct css_set *old_cset,
 					struct cgroup *cgrp,
-					struct cgroup_subsys_state *template[])
+					struct cgroup_subsys_state **template)
 {
 	struct cgroup_root *root = cgrp->root;
 	struct cgroup_subsys *ss;

Signature · 1 line

2.34.1

Tejun Heo

maintainer

2023-08-16 20:51 UTC #1 lore raw

Hello,

Quoted 4 lines · Gustavo A. R. Silva

Change the notation from pointer-to-array to pointer-to-pointer. With this, we avoid the compiler complaining about trying to access a region of size zero as an argument during function calls.

Haha, I thought the functions were actually accessing the memory. This can't be an intended behavior on the compiler's side, right?

Thanks.

Signature · 1 line

tejun

Kees Cook

2023-08-16 20:57 UTC #2 lore raw

Quoted 10 lines · Tejun Heo

Hello,
Quoted 4 lines · Gustavo A. R. Silva
Change the notation from pointer-to-array to pointer-to-pointer. With this, we avoid the compiler complaining about trying to access a region of size zero as an argument during function calls.
Haha, I thought the functions were actually accessing the memory. This can't be an intended behavior on the compiler's side, right?

I think it's a result of inlining -- the compiler ends up with a case where it looks like it might be possible to index a zero-sized array, but it is "accidentally safe".

Signature · 1 line

Kees Cook

Tejun Heo

maintainer

2023-08-16 21:01 UTC #3 lore raw

Quoted 9 lines · Kees Cook

Quoted 8 lines · Tejun Heo
Hello,
Quoted 4 lines · Gustavo A. R. Silva
Change the notation from pointer-to-array to pointer-to-pointer. With this, we avoid the compiler complaining about trying to access a region of size zero as an argument during function calls.

Quoted 6 lines

Quoted 2 lines
Haha, I thought the functions were actually accessing the memory. This can't be an intended behavior on the compiler's side, right?
I think it's a result of inlining -- the compiler ends up with a case where it looks like it might be possible to index a zero-sized array, but it is "accidentally safe".

Ah I see. It's not that the compiler knows that ** access is safe. It's more that it only applies the check on arrays. Is that right? Gustavo, I don't mind the patch but can you update the patch description a bit explaining a bit more on what's going on with the complier? It doesn't have to be the full explanation but it'd be useful to explicitly point out that we're just working around the compiler being a bit silly.

Thanks.

Signature · 1 line

tejun

Gustavo A. R. Silva

2023-08-17 17:23 UTC #4 lore raw

Quoted 13 lines · Tejun Heo

Quoted 12 lines · Kees Cook
Quoted 10 lines · Tejun Heo
Hello,
Quoted 4 lines · Gustavo A. R. Silva
Change the notation from pointer-to-array to pointer-to-pointer. With this, we avoid the compiler complaining about trying to access a region of size zero as an argument during function calls.
Haha, I thought the functions were actually accessing the memory. This can't be an intended behavior on the compiler's side, right?

Quoted 6 lines

Quoted 3 lines
I think it's a result of inlining -- the compiler ends up with a case where it looks like it might be possible to index a zero-sized array, but it is "accidentally safe".
Ah I see. It's not that the compiler knows that ** access is safe. It's more that it only applies the check on arrays. Is that right? Gustavo, I don't

That's correct.

Quoted 4 lines

mind the patch but can you update the patch description a bit explaining a bit more on what's going on with the complier? It doesn't have to be the full explanation but it'd be useful to explicitly point out that we're just working around the compiler being a bit silly.

I just sent v3:

https://lore.kernel.org/linux-hardening/ZN5WkbPelHUSTXOA@work/

Thanks
--
Gustavo