| Message ID | 20231026094957.15371-1-rpalethorpe@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] cgroup: Handle trailing new line in cgroup.controllers | expand |
LGTM (although it is a bit unfortunate that we have to hardcode
MAX_CGROUP_TYPE_NAMELEN)
Reviewed-by: Marius Kittler <mkittler@suse.de>
Hi Richie, Reviewed-by: Petr Vorel <pvorel@suse.cz> > V2: > * Add underscore > * Add length check > * Expand commit message > * Use shorter syntax +1 > lib/tst_cgroup.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c > index 5240aadaa..c5cb20505 100644 > --- a/lib/tst_cgroup.c > +++ b/lib/tst_cgroup.c > @@ -433,9 +433,20 @@ __attribute__ ((nonnull, warn_unused_result)) > static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name) > { > struct cgroup_ctrl *ctrl; > + int l = 0; > + char c = ctrl_name[l]; > + > + while (c == '_' || (c >= 'a' && c <= 'z')) > + c = ctrl_name[++l]; > + > + if (l > 32) +1 for this check. I would slightly prefer to add MAX_CGROUP_TYPE_NAMELEN definition into our include/tst_cgroup.h, but it can stay as is. BTW include/linux/cgroup-defs.h contains also MAX_CGROUP_ROOT_NAMELEN, there are also other checks in C files (CGROUP_FILE_NAME_MAX) in case it makes sense to have some checks for these (I have no idea). Kind regards, Petr > + tst_res(TWARN, "Subsys name len greater than max known value of MAX_CGROUP_TYPE_NAMELEN: %d > 32", l); > + > + if (!(c == '\n' || c == '\0')) > + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, c); > for_each_ctrl(ctrl) { > - if (!strcmp(ctrl_name, ctrl->ctrl_name)) > + if (!strncmp(ctrl_name, ctrl->ctrl_name, l)) > return ctrl; > }
Hello, Petr Vorel <pvorel@suse.cz> writes: > Hi Richie, > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > >> V2: >> * Add underscore >> * Add length check >> * Expand commit message >> * Use shorter syntax > +1 > >> lib/tst_cgroup.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c >> index 5240aadaa..c5cb20505 100644 >> --- a/lib/tst_cgroup.c >> +++ b/lib/tst_cgroup.c >> @@ -433,9 +433,20 @@ __attribute__ ((nonnull, warn_unused_result)) >> static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name) >> { >> struct cgroup_ctrl *ctrl; >> + int l = 0; >> + char c = ctrl_name[l]; >> + >> + while (c == '_' || (c >= 'a' && c <= 'z')) >> + c = ctrl_name[++l]; >> + >> + if (l > 32) > +1 for this check. > I would slightly prefer to add MAX_CGROUP_TYPE_NAMELEN definition into our > include/tst_cgroup.h, but it can stay as is. > > BTW include/linux/cgroup-defs.h contains also MAX_CGROUP_ROOT_NAMELEN, there are > also other checks in C files (CGROUP_FILE_NAME_MAX) in case it makes sense to > have some checks for these (I have no idea). I'm not sure about this. On the one hand it may detect a bug in the library or kernel. On the other it's not exposed to userland or defined anywhere. For me the main thing is to avoid janitorial work like creating and merging patches just to increase some internal kernel value that was increased or is ignored in some corner case. I'll post another patch if there is some obvious way to check the root name length that would help the library. Thanks for the review, I'll merge this now. > > Kind regards, > Petr > >> + tst_res(TWARN, "Subsys name len greater than max known value of MAX_CGROUP_TYPE_NAMELEN: %d > 32", l); > >> + >> + if (!(c == '\n' || c == '\0')) >> + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, c); > >> for_each_ctrl(ctrl) { >> - if (!strcmp(ctrl_name, ctrl->ctrl_name)) >> + if (!strncmp(ctrl_name, ctrl->ctrl_name, l)) >> return ctrl; >> }
> Hello, > Petr Vorel <pvorel@suse.cz> writes: > > Hi Richie, > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > >> V2: > >> * Add underscore > >> * Add length check > >> * Expand commit message > >> * Use shorter syntax > > +1 > >> lib/tst_cgroup.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c > >> index 5240aadaa..c5cb20505 100644 > >> --- a/lib/tst_cgroup.c > >> +++ b/lib/tst_cgroup.c > >> @@ -433,9 +433,20 @@ __attribute__ ((nonnull, warn_unused_result)) > >> static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name) > >> { > >> struct cgroup_ctrl *ctrl; > >> + int l = 0; > >> + char c = ctrl_name[l]; > >> + > >> + while (c == '_' || (c >= 'a' && c <= 'z')) > >> + c = ctrl_name[++l]; > >> + > >> + if (l > 32) > > +1 for this check. > > I would slightly prefer to add MAX_CGROUP_TYPE_NAMELEN definition into our > > include/tst_cgroup.h, but it can stay as is. > > BTW include/linux/cgroup-defs.h contains also MAX_CGROUP_ROOT_NAMELEN, there are > > also other checks in C files (CGROUP_FILE_NAME_MAX) in case it makes sense to > > have some checks for these (I have no idea). > I'm not sure about this. On the one hand it may detect a bug in the > library or kernel. On the other it's not exposed to userland or defined > anywhere. Yep, it's just an idea, I'm not sure myself. > For me the main thing is to avoid janitorial work like creating and > merging patches just to increase some internal kernel value that was > increased or is ignored in some corner case. +1 > I'll post another patch if there is some obvious way to check the root > name length that would help the library. +1 > Thanks for the review, I'll merge this now. Great. Kind regards, Petr > > Kind regards, > > Petr > >> + tst_res(TWARN, "Subsys name len greater than max known value of MAX_CGROUP_TYPE_NAMELEN: %d > 32", l); > >> + > >> + if (!(c == '\n' || c == '\0')) > >> + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, c); > >> for_each_ctrl(ctrl) { > >> - if (!strcmp(ctrl_name, ctrl->ctrl_name)) > >> + if (!strncmp(ctrl_name, ctrl->ctrl_name, l)) > >> return ctrl; > >> }
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c index 5240aadaa..c5cb20505 100644 --- a/lib/tst_cgroup.c +++ b/lib/tst_cgroup.c @@ -433,9 +433,20 @@ __attribute__ ((nonnull, warn_unused_result)) static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name) { struct cgroup_ctrl *ctrl; + int l = 0; + char c = ctrl_name[l]; + + while (c == '_' || (c >= 'a' && c <= 'z')) + c = ctrl_name[++l]; + + if (l > 32) + tst_res(TWARN, "Subsys name len greater than max known value of MAX_CGROUP_TYPE_NAMELEN: %d > 32", l); + + if (!(c == '\n' || c == '\0')) + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, c); for_each_ctrl(ctrl) { - if (!strcmp(ctrl_name, ctrl->ctrl_name)) + if (!strncmp(ctrl_name, ctrl->ctrl_name, l)) return ctrl; }
The last item in cgroup.controllers (misc or rdma in my case) contained a new line character which caused the controller search to fail. This commit avoids including the newline character inside the name comparison. The search failure caused the "cgroup_regression_test.sh" test to fail with a confusing error when it tries to mount a V1 subsys thus removing the V2 and causing the available set of V2s to change between scans. According to the V2 docs subsys names can only include lowercase characters and '_'. So we strictly look for those characters. The newline (and delimiting space) is just what the kernel currently prints. IDK if it is specified anywhere, but if it changes then the error should be obvious. Fixes: 310da3784 ("Add new CGroups APIs") Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> Cc: Petr Vorel <pvorel@suse.cz> Cc: Marius Kittler <mkittler@suse.de> --- V2: * Add underscore * Add length check * Expand commit message * Use shorter syntax lib/tst_cgroup.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)