| Message ID | 5d6e978c803e4e6449cabd89596729bfad996a17.1604408825.git.jstancek@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib/tst_cgroup: fix short reads of mems/cpus | expand |
On Tue, Nov 3, 2020 at 9:10 PM Jan Stancek <jstancek@redhat.com> wrote: > tst_cgroup_cpuset_read_files() is wrongly using sizeof on pointer > type to figure out length of buffer. On some systems there's > more content in cgroup mems/cpus, which leads to partial read > and subsequent EINVAL on write. > > Also the retval buffer should be zero terminated, since buffer > can be passed uninitialized, subsequently leading to writing garbage > to cgroup and again hitting EINVAL. > > Before: > tst_test.c:1250: TINFO: Timeout per run is 0h 05m 00s > tst_device.c:423: TINFO: No device is mounted at cgroup2 > tst_device.c:423: TINFO: No device is mounted at /tmp/cgroup_cst > tst_cgroup.c:100: TINFO: Cgroup(cpuset) v1 mount at /tmp/cgroup_cst > success > safe_macros.c:458: TBROK: tst_cgroup.c:449: > write(6,0x7fffc0425410,18446744073709551615) failed: EINVAL (22) > tst_cgroup.c:173: TWARN: umount /tmp/cgroup_cst: EBUSY (16) > tst_cgroup.c:175: TWARN: rmdir /tmp/cgroup_cst: EBUSY (16) > tst_cgroup.c:178: TINFO: Cgroup v1 unmount success > > After: > tst_test.c:1250: TINFO: Timeout per run is 0h 05m 00s > tst_device.c:423: TINFO: No device is mounted at cgroup2 > tst_device.c:423: TINFO: No device is mounted at /tmp/cgroup_cst > tst_cgroup.c:100: TINFO: Cgroup(cpuset) v1 mount at /tmp/cgroup_cst > success > cpuset01.c:83: TPASS: cpuset test pass > tst_cgroup.c:178: TINFO: Cgroup v1 unmount success > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > --- > include/tst_cgroup.h | 6 ++++-- > lib/tst_cgroup.c | 6 ++++-- > testcases/kernel/mem/cpuset/cpuset01.c | 4 ++-- > 3 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h > index 77780e0d64c9..bfd8482608c4 100644 > --- a/include/tst_cgroup.h > +++ b/include/tst_cgroup.h > @@ -39,7 +39,9 @@ int tst_cgroup_mem_swapacct_enabled(const char > *cgroup_dir); > void tst_cgroup_mem_set_maxswap(const char *cgroup_dir, long memsz); > > /* Set of functions to read/write cpuset controller files content */ > -void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char > *filename, char *retbuf); > -void tst_cgroup_cpuset_write_files(const char *cgroup_dir, const char > *filename, const char *buf); > +void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char > *filename, > + char *retbuf, size_t retbuf_sz); > +void tst_cgroup_cpuset_write_files(const char *cgroup_dir, const char > *filename, > + const char *buf); > > #endif /* TST_CGROUP_H */ > diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c > index ba413d874f69..96c9524d2b3a 100644 > --- a/lib/tst_cgroup.c > +++ b/lib/tst_cgroup.c > @@ -393,7 +393,8 @@ void tst_cgroup_mem_set_maxswap(const char > *cgroup_dir, long memsz) > tst_cgroup_set_knob(cgroup_dir, "memory.swap.max", memsz); > } > > -void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char > *filename, char *retbuf) > +void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char > *filename, > + char *retbuf, size_t retbuf_sz) > { > int fd; > char *cgroup_new_dir; > @@ -417,7 +418,8 @@ void tst_cgroup_cpuset_read_files(const char > *cgroup_dir, const char *filename, > tst_brk(TBROK | TERRNO, "open %s", knob_path); > } > > - if (read(fd, retbuf, sizeof(retbuf)) < 0) > + memset(retbuf, 0, retbuf_sz); > + if (read(fd, retbuf, retbuf_sz) < 0) > tst_brk(TBROK | TERRNO, "read %s", knob_path); > > close(fd); > diff --git a/testcases/kernel/mem/cpuset/cpuset01.c > b/testcases/kernel/mem/cpuset/cpuset01.c > index f70ae0e486c2..fcd22040f7dd 100644 > --- a/testcases/kernel/mem/cpuset/cpuset01.c > +++ b/testcases/kernel/mem/cpuset/cpuset01.c > @@ -51,9 +51,9 @@ static void test_cpuset(void) > unsigned long nmask[MAXNODES / BITS_PER_LONG] = { 0 }; > char mems[BUFSIZ], buf[BUFSIZ]; > > - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf); > + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf, > sizeof(buf)); > tst_cgroup_cpuset_write_files(PATH_TMP_CG_CST, "cpus", buf); > - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems); > + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems, > sizeof(buf)); > sizeof() is generally used to calculate the size (in bytes) of a data type, e.g sizeof(char *). I think here to pass 'BUFSIZ' directly is better than sizeof(buf). Otherwise, looks good to me. > tst_cgroup_cpuset_write_files(PATH_TMP_CG_CST, "mems", mems); > tst_cgroup_move_current(PATH_TMP_CG_CST); > > -- > 2.18.1 > >
On Wed, Nov 4, 2020 at 2:22 PM Li Wang <liwang@redhat.com> wrote: > ... >> - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf); >> + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf, >> sizeof(buf)); >> tst_cgroup_cpuset_write_files(PATH_TMP_CG_CST, "cpus", buf); >> - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems); >> + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems, >> sizeof(buf)); >> > > sizeof() is generally used to calculate the size (in bytes) of a data type, > e.g sizeof(char *). > I think here to pass 'BUFSIZ' directly is better than sizeof(buf). > Jan, do you agree with this point? If yes, I can help to modify and apply this patch.
----- Original Message ----- > On Wed, Nov 4, 2020 at 2:22 PM Li Wang <liwang@redhat.com> wrote: > > > ... > >> - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf); > >> + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf, > >> sizeof(buf)); > >> tst_cgroup_cpuset_write_files(PATH_TMP_CG_CST, "cpus", buf); > >> - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems); > >> + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems, > >> sizeof(buf)); > >> > > > > sizeof() is generally used to calculate the size (in bytes) of a data type, > > e.g sizeof(char *). > > I think here to pass 'BUFSIZ' directly is better than sizeof(buf). > > > > Jan, do you agree with this point? > If yes, I can help to modify and apply this patch. 2nd line should have been sizeof(mems), but BUFSIZ will work too.
On Mon, Nov 9, 2020 at 8:49 PM Jan Stancek <jstancek@redhat.com> wrote: > > > ----- Original Message ----- > > On Wed, Nov 4, 2020 at 2:22 PM Li Wang <liwang@redhat.com> wrote: > > > > > ... > > >> - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf); > > >> + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf, > > >> sizeof(buf)); > > >> tst_cgroup_cpuset_write_files(PATH_TMP_CG_CST, "cpus", buf); > > >> - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems); > > >> + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems, > > >> sizeof(buf)); > > >> > > > > > > sizeof() is generally used to calculate the size (in bytes) of a data > type, > > > e.g sizeof(char *). > > > I think here to pass 'BUFSIZ' directly is better than sizeof(buf). > > > > > > > Jan, do you agree with this point? > > If yes, I can help to modify and apply this patch. > > 2nd line should have been sizeof(mems), but BUFSIZ will work too. > Pushed, thanks!
diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h index 77780e0d64c9..bfd8482608c4 100644 --- a/include/tst_cgroup.h +++ b/include/tst_cgroup.h @@ -39,7 +39,9 @@ int tst_cgroup_mem_swapacct_enabled(const char *cgroup_dir); void tst_cgroup_mem_set_maxswap(const char *cgroup_dir, long memsz); /* Set of functions to read/write cpuset controller files content */ -void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char *filename, char *retbuf); -void tst_cgroup_cpuset_write_files(const char *cgroup_dir, const char *filename, const char *buf); +void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char *filename, + char *retbuf, size_t retbuf_sz); +void tst_cgroup_cpuset_write_files(const char *cgroup_dir, const char *filename, + const char *buf); #endif /* TST_CGROUP_H */ diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c index ba413d874f69..96c9524d2b3a 100644 --- a/lib/tst_cgroup.c +++ b/lib/tst_cgroup.c @@ -393,7 +393,8 @@ void tst_cgroup_mem_set_maxswap(const char *cgroup_dir, long memsz) tst_cgroup_set_knob(cgroup_dir, "memory.swap.max", memsz); } -void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char *filename, char *retbuf) +void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char *filename, + char *retbuf, size_t retbuf_sz) { int fd; char *cgroup_new_dir; @@ -417,7 +418,8 @@ void tst_cgroup_cpuset_read_files(const char *cgroup_dir, const char *filename, tst_brk(TBROK | TERRNO, "open %s", knob_path); } - if (read(fd, retbuf, sizeof(retbuf)) < 0) + memset(retbuf, 0, retbuf_sz); + if (read(fd, retbuf, retbuf_sz) < 0) tst_brk(TBROK | TERRNO, "read %s", knob_path); close(fd); diff --git a/testcases/kernel/mem/cpuset/cpuset01.c b/testcases/kernel/mem/cpuset/cpuset01.c index f70ae0e486c2..fcd22040f7dd 100644 --- a/testcases/kernel/mem/cpuset/cpuset01.c +++ b/testcases/kernel/mem/cpuset/cpuset01.c @@ -51,9 +51,9 @@ static void test_cpuset(void) unsigned long nmask[MAXNODES / BITS_PER_LONG] = { 0 }; char mems[BUFSIZ], buf[BUFSIZ]; - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf); + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "cpus", buf, sizeof(buf)); tst_cgroup_cpuset_write_files(PATH_TMP_CG_CST, "cpus", buf); - tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems); + tst_cgroup_cpuset_read_files(PATH_TMP_CG_CST, "mems", mems, sizeof(buf)); tst_cgroup_cpuset_write_files(PATH_TMP_CG_CST, "mems", mems); tst_cgroup_move_current(PATH_TMP_CG_CST);
tst_cgroup_cpuset_read_files() is wrongly using sizeof on pointer type to figure out length of buffer. On some systems there's more content in cgroup mems/cpus, which leads to partial read and subsequent EINVAL on write. Also the retval buffer should be zero terminated, since buffer can be passed uninitialized, subsequently leading to writing garbage to cgroup and again hitting EINVAL. Before: tst_test.c:1250: TINFO: Timeout per run is 0h 05m 00s tst_device.c:423: TINFO: No device is mounted at cgroup2 tst_device.c:423: TINFO: No device is mounted at /tmp/cgroup_cst tst_cgroup.c:100: TINFO: Cgroup(cpuset) v1 mount at /tmp/cgroup_cst success safe_macros.c:458: TBROK: tst_cgroup.c:449: write(6,0x7fffc0425410,18446744073709551615) failed: EINVAL (22) tst_cgroup.c:173: TWARN: umount /tmp/cgroup_cst: EBUSY (16) tst_cgroup.c:175: TWARN: rmdir /tmp/cgroup_cst: EBUSY (16) tst_cgroup.c:178: TINFO: Cgroup v1 unmount success After: tst_test.c:1250: TINFO: Timeout per run is 0h 05m 00s tst_device.c:423: TINFO: No device is mounted at cgroup2 tst_device.c:423: TINFO: No device is mounted at /tmp/cgroup_cst tst_cgroup.c:100: TINFO: Cgroup(cpuset) v1 mount at /tmp/cgroup_cst success cpuset01.c:83: TPASS: cpuset test pass tst_cgroup.c:178: TINFO: Cgroup v1 unmount success Signed-off-by: Jan Stancek <jstancek@redhat.com> --- include/tst_cgroup.h | 6 ++++-- lib/tst_cgroup.c | 6 ++++-- testcases/kernel/mem/cpuset/cpuset01.c | 4 ++-- 3 files changed, 10 insertions(+), 6 deletions(-)