[LTP] [PATCH] lib: switch cgroup bit-fields from signed to unsigned int

[LTP] [PATCH] lib: switch cgroup bit-fields from signed to unsigned int

[Li Wang]

There is a problem in cgroup lib that the declearation int
can lead to -1 during the | operation.

Becasue if the field contains uninitialized garbage data,
a bit-field declared as int could interpret 0b1 as -1 due
to signed arithmetic.

By changing the type to unsigned int, the issue is avoided
since unsigned fields cannot represent negative values.

Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Jin Guojie <guojie.jin@gmail.com>
---

Notes:
    @Cyril, Petr, I vote to merge this patch before the release.
    @Guojie, Could you plz repost your memcontrol04 patch based on this change?

 include/tst_test.h |  2 +-
 lib/tst_cgroup.c   | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/tst_test.h b/include/tst_test.h
index 5b3889e64..eb73cd593 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -610,7 +610,7 @@ struct tst_fs {
 
 	const char *const *needs_cgroup_ctrls;
 
-	int needs_cgroup_nsdelegate:1;
+	unsigned int needs_cgroup_nsdelegate:1;
 };
 
 /**
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 6055015eb..aa13ac8ec 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -44,7 +44,7 @@ struct cgroup_dir {
 	 */
 	int dir_fd;
 
-	int we_created_it:1;
+	unsigned int we_created_it:1;
 };
 
 /* The root of a CGroup hierarchy/tree */
@@ -71,11 +71,11 @@ struct cgroup_root {
 	/* CGroup for current test. Which may have children. */
 	struct cgroup_dir test_dir;
 
-	int nsdelegate:1;
+	unsigned int nsdelegate:1;
 
-	int we_mounted_it:1;
+	unsigned int we_mounted_it:1;
 	/* cpuset is in compatability mode */
-	int no_cpuset_prefix:1;
+	unsigned int no_cpuset_prefix:1;
 };
 
 /* Controller sub-systems */
@@ -138,7 +138,7 @@ struct cgroup_ctrl {
 	/* Runtime; hierarchy the controller is attached to */
 	struct cgroup_root *ctrl_root;
 	/* Runtime; whether we required the controller */
-	int we_require_it:1;
+	unsigned int we_require_it:1;
 };
 
 struct tst_cg_group {
-- 
2.47.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: switch cgroup bit-fields from signed to unsigned int

[Petr Vorel]

Hi Li,

> There is a problem in cgroup lib that the declearation int
> can lead to -1 during the | operation.

> Becasue if the field contains uninitialized garbage data,
> a bit-field declared as int could interpret 0b1 as -1 due
> to signed arithmetic.

> By changing the type to unsigned int, the issue is avoided
> since unsigned fields cannot represent negative values.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Jin Guojie <guojie.jin@gmail.com>
> ---

> Notes:
>     @Cyril, Petr, I vote to merge this patch before the release.
>     @Guojie, Could you plz repost your memcontrol04 patch based on this change?

+1

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: switch cgroup bit-fields from signed to unsigned int

[Cyril Hrubis]

Hi!
> There is a problem in cgroup lib that the declearation int
> can lead to -1 during the | operation.
> 
> Becasue if the field contains uninitialized garbage data,
> a bit-field declared as int could interpret 0b1 as -1 due
> to signed arithmetic.

Uff, so we have a number that can be either -1 or 0. Where did it break?
I suppose that things are okay as long as we do if (s->bar | s->bar).


Anyways:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: switch cgroup bit-fields from signed to unsigned int

[Li Wang]

On Fri, Jan 24, 2025 at 7:20 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > There is a problem in cgroup lib that the declearation int
> > can lead to -1 during the | operation.
> >
> > Becasue if the field contains uninitialized garbage data,
> > a bit-field declared as int could interpret 0b1 as -1 due
> > to signed arithmetic.
>
> Uff, so we have a number that can be either -1 or 0. Where did it break?
>

It doesn't break anything so far, I found that issue while reviewing
Guojie's patch-v2, when that memory_recursiveprot declared as int,
the test result is always wrong.

However, the root->nsdelegate avoids this problem by negating value twice:

static int cgroup_v2_nsdelegate(void)
{
    return !!roots[0].nsdelegate;
}

But I think we should fix this from the root.



> I suppose that things are okay as long as we do if (s->bar | s->bar).
>
>
> Anyways:
>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: switch cgroup bit-fields from signed to unsigned int

[Li Wang]

Patch merged, thanks!

>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp