| Message ID | 20240301015037.22061-1-wegao@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] libswap.c: Check free space with correct mnt path | expand |
Hi Wei, Good catch! Reviewed-by: Li Wang <liwang@redhat.com> On Fri, Mar 1, 2024 at 9:51 AM Wei Gao via ltp <ltp@lists.linux.it> wrote: > The tst_fs_has_free should check fs size of mnt point. > But current code check ".", that means check /tmp/LTP_xxx > instead of /tmp/LTP_xxx/mntpoint. > > Also tst_fs_has_free's "size" parameter's type is unsigned int, > it will overflow if encounter big filesystem block size(such as Btrfs > can use 64k). > > Signed-off-by: Wei Gao <wegao@suse.com> > --- > include/tst_fs.h | 6 +++--- > lib/tst_fs_has_free.c | 2 +- > libs/libltpswap/libswap.c | 4 +++- > 3 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/include/tst_fs.h b/include/tst_fs.h > index 1dd7d32fc..99dbba4d1 100644 > --- a/include/tst_fs.h > +++ b/include/tst_fs.h > @@ -60,7 +60,7 @@ enum { > * @mult: mult should be TST_KB, TST_MB or TST_GB > * the required free space is calculated by @size * @mult > */ > -int tst_fs_has_free_(void (*cleanup)(void), const char *path, unsigned > int size, > +int tst_fs_has_free_(void (*cleanup)(void), const char *path, uint64_t > size, > unsigned int mult); > > /* > @@ -225,7 +225,7 @@ static inline long tst_fs_type(const char *path) > return tst_fs_type_(NULL, path); > } > > -static inline int tst_fs_has_free(const char *path, unsigned int size, > +static inline int tst_fs_has_free(const char *path, uint64_t size, > unsigned int mult) > { > return tst_fs_has_free_(NULL, path, size, mult); > @@ -252,7 +252,7 @@ static inline long tst_fs_type(void (*cleanup)(void), > const char *path) > } > > static inline int tst_fs_has_free(void (*cleanup)(void), const char *path, > - unsigned int size, unsigned int mult) > + uint64_t size, unsigned int mult) > { > return tst_fs_has_free_(cleanup, path, size, mult); > } > diff --git a/lib/tst_fs_has_free.c b/lib/tst_fs_has_free.c > index e82dfa837..080d693ab 100644 > --- a/lib/tst_fs_has_free.c > +++ b/lib/tst_fs_has_free.c > @@ -27,7 +27,7 @@ > #include "tst_fs.h" > > int tst_fs_has_free_(void (*cleanup)(void), const char *path, > - unsigned int size, unsigned int mult) > + uint64_t size, unsigned int mult) > { > struct statfs sf; > > diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c > index c8cbb8ea1..6c75d20fa 100644 > --- a/libs/libltpswap/libswap.c > +++ b/libs/libltpswap/libswap.c > @@ -137,6 +137,7 @@ int make_swapfile(const char *swapfile, int blocks, > int safe) > struct statvfs fs_info; > unsigned long blk_size, bs; > size_t pg_size = sysconf(_SC_PAGESIZE); > + char mnt_path[100]; > > if (statvfs(".", &fs_info) == -1) > return -1; > @@ -149,7 +150,8 @@ int make_swapfile(const char *swapfile, int blocks, > int safe) > else > bs = blk_size; > > - if (!tst_fs_has_free(".", bs * blocks, TST_BYTES)) > + sscanf(swapfile, "%[^/]", mnt_path); > + if (!tst_fs_has_free(mnt_path, bs * blocks, TST_BYTES)) > tst_brk(TCONF, "Insufficient disk space to create swap > file"); > > /* create file */ > -- > 2.35.3 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
Hi Wei, Li, > Hi Wei, > Good catch! Great, thanks, merged! (Tested at least on intel.) > Reviewed-by: Li Wang <liwang@redhat.com> > On Fri, Mar 1, 2024 at 9:51 AM Wei Gao via ltp <ltp@lists.linux.it> wrote: > > The tst_fs_has_free should check fs size of mnt point. > > But current code check ".", that means check /tmp/LTP_xxx > > instead of /tmp/LTP_xxx/mntpoint. > > Also tst_fs_has_free's "size" parameter's type is unsigned int, > > it will overflow if encounter big filesystem block size(such as Btrfs > > can use 64k). I would personally added this as separate previous patch, but kept as is. ... > > +++ b/libs/libltpswap/libswap.c > > @@ -137,6 +137,7 @@ int make_swapfile(const char *swapfile, int blocks, > > int safe) > > struct statvfs fs_info; > > unsigned long blk_size, bs; > > size_t pg_size = sysconf(_SC_PAGESIZE); > > + char mnt_path[100]; > > if (statvfs(".", &fs_info) == -1) > > return -1; > > @@ -149,7 +150,8 @@ int make_swapfile(const char *swapfile, int blocks, > > int safe) > > else > > bs = blk_size; > > - if (!tst_fs_has_free(".", bs * blocks, TST_BYTES)) > > + sscanf(swapfile, "%[^/]", mnt_path); Although this should never fail, I changed it to check return value: - if (!tst_fs_has_free(".", bs * blocks, TST_BYTES)) + if (sscanf(swapfile, "%[^/]", mnt_path) != 1) + tst_brk(TBROK, "sscanf failed"); And I'll ask Andrea Manzini to add SAFE_SSCANF() (easy hack). Kind regards, Petr
diff --git a/include/tst_fs.h b/include/tst_fs.h index 1dd7d32fc..99dbba4d1 100644 --- a/include/tst_fs.h +++ b/include/tst_fs.h @@ -60,7 +60,7 @@ enum { * @mult: mult should be TST_KB, TST_MB or TST_GB * the required free space is calculated by @size * @mult */ -int tst_fs_has_free_(void (*cleanup)(void), const char *path, unsigned int size, +int tst_fs_has_free_(void (*cleanup)(void), const char *path, uint64_t size, unsigned int mult); /* @@ -225,7 +225,7 @@ static inline long tst_fs_type(const char *path) return tst_fs_type_(NULL, path); } -static inline int tst_fs_has_free(const char *path, unsigned int size, +static inline int tst_fs_has_free(const char *path, uint64_t size, unsigned int mult) { return tst_fs_has_free_(NULL, path, size, mult); @@ -252,7 +252,7 @@ static inline long tst_fs_type(void (*cleanup)(void), const char *path) } static inline int tst_fs_has_free(void (*cleanup)(void), const char *path, - unsigned int size, unsigned int mult) + uint64_t size, unsigned int mult) { return tst_fs_has_free_(cleanup, path, size, mult); } diff --git a/lib/tst_fs_has_free.c b/lib/tst_fs_has_free.c index e82dfa837..080d693ab 100644 --- a/lib/tst_fs_has_free.c +++ b/lib/tst_fs_has_free.c @@ -27,7 +27,7 @@ #include "tst_fs.h" int tst_fs_has_free_(void (*cleanup)(void), const char *path, - unsigned int size, unsigned int mult) + uint64_t size, unsigned int mult) { struct statfs sf; diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c index c8cbb8ea1..6c75d20fa 100644 --- a/libs/libltpswap/libswap.c +++ b/libs/libltpswap/libswap.c @@ -137,6 +137,7 @@ int make_swapfile(const char *swapfile, int blocks, int safe) struct statvfs fs_info; unsigned long blk_size, bs; size_t pg_size = sysconf(_SC_PAGESIZE); + char mnt_path[100]; if (statvfs(".", &fs_info) == -1) return -1; @@ -149,7 +150,8 @@ int make_swapfile(const char *swapfile, int blocks, int safe) else bs = blk_size; - if (!tst_fs_has_free(".", bs * blocks, TST_BYTES)) + sscanf(swapfile, "%[^/]", mnt_path); + if (!tst_fs_has_free(mnt_path, bs * blocks, TST_BYTES)) tst_brk(TCONF, "Insufficient disk space to create swap file"); /* create file */
The tst_fs_has_free should check fs size of mnt point. But current code check ".", that means check /tmp/LTP_xxx instead of /tmp/LTP_xxx/mntpoint. Also tst_fs_has_free's "size" parameter's type is unsigned int, it will overflow if encounter big filesystem block size(such as Btrfs can use 64k). Signed-off-by: Wei Gao <wegao@suse.com> --- include/tst_fs.h | 6 +++--- lib/tst_fs_has_free.c | 2 +- libs/libltpswap/libswap.c | 4 +++- 3 files changed, 7 insertions(+), 5 deletions(-)