| Message ID | 20250118102615.127485-1-liwang@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib: switch cgroup bit-fields from signed to unsigned int | expand |
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
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>
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 > >
Patch merged, thanks!
>
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 {
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(-)