| Message ID | 20230604061658.49004-1-faithilikerun@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] block/file-posix: fix g_file_get_contents return path | expand |
On 6/4/23 2:16 AM, Sam Li wrote: > The g_file_get_contents() function returns a g_boolean. If it fails, the > returned value will be 0 instead of -1. Solve the issue by skipping > assigning ret value. > > This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed > by an NVMe partition e.g. /dev/nvme0n1p1 on s390x. > > Signed-off-by: Sam Li <faithilikerun@gmail.com> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com> Also tested to verify that this fix resolves the reported issue. Thanks!
On Sun, Jun 04, 2023 at 02:16:57PM +0800, Sam Li wrote: > The g_file_get_contents() function returns a g_boolean. If it fails, the > returned value will be 0 instead of -1. Solve the issue by skipping > assigning ret value. > > This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed > by an NVMe partition e.g. /dev/nvme0n1p1 on s390x. > > Signed-off-by: Sam Li <faithilikerun@gmail.com> > --- > block/file-posix.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) The number of bytes returned was never used, so changing the return value to 0 or -errno is fine: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 6/4/23 2:16 AM, Sam Li wrote: > The g_file_get_contents() function returns a g_boolean. If it fails, the > returned value will be 0 instead of -1. Solve the issue by skipping > assigning ret value. > > This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed > by an NVMe partition e.g. /dev/nvme0n1p1 on s390x. > > Signed-off-by: Sam Li <faithilikerun@gmail.com> Polite ping on this patch -- this issue still exists in master as of today and this patch resolves it for me. Just want to make sure it gets into 8.1 Thanks, Matt > --- > block/file-posix.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index ac1ed54811..0d9d179a35 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st) > static int get_sysfs_str_val(struct stat *st, const char *attribute, > char **val) { > g_autofree char *sysfspath = NULL; > - int ret; > size_t len; > > if (!S_ISBLK(st->st_mode)) { > @@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute, > sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s", > major(st->st_rdev), minor(st->st_rdev), > attribute); > - ret = g_file_get_contents(sysfspath, val, &len, NULL); > - if (ret == -1) { > + if (!g_file_get_contents(sysfspath, val, &len, NULL)) { > return -ENOENT; > } > > @@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute, > if (*(p + len - 1) == '\n') { > *(p + len - 1) = '\0'; > } > - return ret; > + return 0; > } > #endif >
On 7/5/23 10:54 AM, Matthew Rosato wrote: > On 6/4/23 2:16 AM, Sam Li wrote: >> The g_file_get_contents() function returns a g_boolean. If it fails, the >> returned value will be 0 instead of -1. Solve the issue by skipping >> assigning ret value. >> >> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed >> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x. >> >> Signed-off-by: Sam Li <faithilikerun@gmail.com> > > Polite ping on this patch -- this issue still exists in master as of today and this patch resolves it for me. Just want to make sure it gets into 8.1 > Ping -- I can still reproduce this crash on -rc1. Any chance this patch can get picked up for the 8.1 release? @Sam I see you sent a v2 of only patch #2 in this series ('block/file-posix: fix wps checking in raw_co_prw').. I wonder if this one just got forgotten since it wasn't sent as part of v2. Maybe try a resend of this patch by itself (plus the review tags added)? Thanks, Matt > >> --- >> block/file-posix.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index ac1ed54811..0d9d179a35 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st) >> static int get_sysfs_str_val(struct stat *st, const char *attribute, >> char **val) { >> g_autofree char *sysfspath = NULL; >> - int ret; >> size_t len; >> >> if (!S_ISBLK(st->st_mode)) { >> @@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute, >> sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s", >> major(st->st_rdev), minor(st->st_rdev), >> attribute); >> - ret = g_file_get_contents(sysfspath, val, &len, NULL); >> - if (ret == -1) { >> + if (!g_file_get_contents(sysfspath, val, &len, NULL)) { >> return -ENOENT; >> } >> >> @@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute, >> if (*(p + len - 1) == '\n') { >> *(p + len - 1) = '\0'; >> } >> - return ret; >> + return 0; >> } >> #endif >> > >
Matthew Rosato <mjrosato@linux.ibm.com> 于2023年7月27日周四 19:46写道： > > On 7/5/23 10:54 AM, Matthew Rosato wrote: > > On 6/4/23 2:16 AM, Sam Li wrote: > >> The g_file_get_contents() function returns a g_boolean. If it fails, the > >> returned value will be 0 instead of -1. Solve the issue by skipping > >> assigning ret value. > >> > >> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed > >> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x. > >> > >> Signed-off-by: Sam Li <faithilikerun@gmail.com> > > > > Polite ping on this patch -- this issue still exists in master as of today and this patch resolves it for me. Just want to make sure it gets into 8.1 > > > > Ping -- I can still reproduce this crash on -rc1. Any chance this patch can get picked up for the 8.1 release? > > @Sam I see you sent a v2 of only patch #2 in this series ('block/file-posix: fix wps checking in raw_co_prw').. I wonder if this one just got forgotten since it wasn't sent as part of v2. Maybe try a resend of this patch by itself (plus the review tags added)? Ok, I will resend it as a separate patch. > > Thanks, > Matt > > > > >> --- > >> block/file-posix.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/block/file-posix.c b/block/file-posix.c > >> index ac1ed54811..0d9d179a35 100644 > >> --- a/block/file-posix.c > >> +++ b/block/file-posix.c > >> @@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st) > >> static int get_sysfs_str_val(struct stat *st, const char *attribute, > >> char **val) { > >> g_autofree char *sysfspath = NULL; > >> - int ret; > >> size_t len; > >> > >> if (!S_ISBLK(st->st_mode)) { > >> @@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute, > >> sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s", > >> major(st->st_rdev), minor(st->st_rdev), > >> attribute); > >> - ret = g_file_get_contents(sysfspath, val, &len, NULL); > >> - if (ret == -1) { > >> + if (!g_file_get_contents(sysfspath, val, &len, NULL)) { > >> return -ENOENT; > >> } > >> > >> @@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute, > >> if (*(p + len - 1) == '\n') { > >> *(p + len - 1) = '\0'; > >> } > >> - return ret; > >> + return 0; > >> } > >> #endif > >> > > > > >
diff --git a/block/file-posix.c b/block/file-posix.c index ac1ed54811..0d9d179a35 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st) static int get_sysfs_str_val(struct stat *st, const char *attribute, char **val) { g_autofree char *sysfspath = NULL; - int ret; size_t len; if (!S_ISBLK(st->st_mode)) { @@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute, sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s", major(st->st_rdev), minor(st->st_rdev), attribute); - ret = g_file_get_contents(sysfspath, val, &len, NULL); - if (ret == -1) { + if (!g_file_get_contents(sysfspath, val, &len, NULL)) { return -ENOENT; } @@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute, if (*(p + len - 1) == '\n') { *(p + len - 1) = '\0'; } - return ret; + return 0; } #endif
The g_file_get_contents() function returns a g_boolean. If it fails, the returned value will be 0 instead of -1. Solve the issue by skipping assigning ret value. This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed by an NVMe partition e.g. /dev/nvme0n1p1 on s390x. Signed-off-by: Sam Li <faithilikerun@gmail.com> --- block/file-posix.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)