| Message ID | 20240301071402.159309-1-huangjianan@xiaomi.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,v3] f2fs-tools: fix to check loop device for non-root users | expand |
Hi Huang and Chao. I feel like this special loopback handling alongside Chao's 14197d546b93 on f2fs-tools is just unnecessarily complicating the code flow. We're now doing what, lookup to /sys, parse original backing file, remove trailing newline char, stat()'ing it to make sure it exists? What if the stat()'ed file is a new file after the original backing file has been deleted? Being able to overwrite an active loopback backing file is a semantic that Linux provides willingly. O_EXCL only works on block devices and it's a POSIX guarantee that multiple writers can work on a regular file. IMHO we should honor that, but if we really want to prevent this akin to e2fsprogs, we should be using mntent like e2fsprogs. On Fri, Mar 1, 2024 at 4:15 PM Huang Jianan via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> wrote: > > Currently mkfs/fsck gets the following error when executed by > non-root users: > > Info: open /dev/loop0 failed errno:13 > Error: Not available on mounted device! > > Let's fix it by reading the backing file from sysfs. > > Fixes: 14197d546b93 ("f2fs-tools: fix to check loop device") > Signed-off-by: Huang Jianan <huangjianan@xiaomi.com> > --- > v3: > - Skip deleted backing file. > lib/libf2fs.c | 40 +++++++++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 13 deletions(-) > > diff --git a/lib/libf2fs.c b/lib/libf2fs.c > index d51e485..fad3fd4 100644 > --- a/lib/libf2fs.c > +++ b/lib/libf2fs.c > @@ -832,7 +832,7 @@ int f2fs_dev_is_umounted(char *path) > } > } else if (S_ISREG(st_buf.st_mode)) { > /* check whether regular is backfile of loop device */ > -#if defined(HAVE_LINUX_LOOP_H) && defined(HAVE_LINUX_MAJOR_H) > +#if defined(HAVE_LINUX_MAJOR_H) > struct mntent *mnt; > struct stat st_loop; > FILE *f; > @@ -840,8 +840,9 @@ int f2fs_dev_is_umounted(char *path) > f = setmntent("/proc/mounts", "r"); > > while ((mnt = getmntent(f)) != NULL) { > - struct loop_info64 loopinfo = {0, }; > - int loop_fd, err; > + struct stat st_back; > + int sysfs_fd, rc; > + char buf[PATH_MAX + 1]; > > if (mnt->mnt_fsname[0] != '/') > continue; > @@ -852,23 +853,36 @@ int f2fs_dev_is_umounted(char *path) > if (major(st_loop.st_rdev) != LOOP_MAJOR) > continue; > > - loop_fd = open(mnt->mnt_fsname, O_RDONLY); > - if (loop_fd < 0) { > + snprintf(buf, PATH_MAX, > + "/sys/dev/block/%d:%d/loop/backing_file", > + major(st_loop.st_rdev), minor(st_loop.st_rdev)); > + > + sysfs_fd = open(buf, O_RDONLY); > + if (sysfs_fd < 0) { > MSG(0, "Info: open %s failed errno:%d\n", > - mnt->mnt_fsname, errno); > + buf, errno); > return -1; > } > > - err = ioctl(loop_fd, LOOP_GET_STATUS64, &loopinfo); > - close(loop_fd); > - if (err < 0) { > - MSG(0, "\tError: ioctl LOOP_GET_STATUS64 failed errno:%d!\n", > - errno); > + memset(buf, 0, PATH_MAX + 1); > + rc = read(sysfs_fd, buf, PATH_MAX); > + if (rc < 0) { > + MSG(0, "Info: read %s failed errno:%d\n", > + buf, errno); > return -1; > } > > - if (st_buf.st_dev == loopinfo.lo_device && > - st_buf.st_ino == loopinfo.lo_inode) { > + /* Remove trailing newline */ > + if (rc > 0 && *(buf + rc - 1) == '\n') > + --rc; > + buf[rc] = '\0'; > + > + /* Skip deleted files like "xxx (deleted)" */ > + if (stat(buf, &st_back) != 0) > + continue; > + > + if (st_buf.st_dev == st_back.st_dev && > + st_buf.st_ino == st_back.st_ino) { > MSG(0, "\tError: In use by loop device!\n"); > return -EBUSY; > } > -- > 2.34.1 > > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 2024/3/1 16:39, Juhyung Park wrote: > [外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈 > > Hi Huang and Chao. > > I feel like this special loopback handling alongside Chao's > 14197d546b93 on f2fs-tools is just unnecessarily complicating the code > flow. > We're now doing what, lookup to /sys, parse original backing file, > remove trailing newline char, stat()'ing it to make sure it exists? Indeed this is not a good approach. > What if the stat()'ed file is a new file after the original backing > file has been deleted? > > Being able to overwrite an active loopback backing file is a semantic > that Linux provides willingly. > O_EXCL only works on block devices and it's a POSIX guarantee that > multiple writers can work on a regular file. > > IMHO we should honor that, but if we really want to prevent this akin > to e2fsprogs, we should be using mntent like e2fsprogs. e2fsprogs will continue to check if opening the loop device fails, rather than exiting. This way non-root users can use mkfs/fsck normally, although there may be overwrite issues. Thanks, Jianan > On Fri, Mar 1, 2024 at 4:15 PM Huang Jianan via Linux-f2fs-devel > <linux-f2fs-devel@lists.sourceforge.net> wrote: >> Currently mkfs/fsck gets the following error when executed by >> non-root users: >> >> Info: open /dev/loop0 failed errno:13 >> Error: Not available on mounted device! >> >> Let's fix it by reading the backing file from sysfs. >> >> Fixes: 14197d546b93 ("f2fs-tools: fix to check loop device") >> Signed-off-by: Huang Jianan <huangjianan@xiaomi.com> >> --- >> v3: >> - Skip deleted backing file. >> lib/libf2fs.c | 40 +++++++++++++++++++++++++++------------- >> 1 file changed, 27 insertions(+), 13 deletions(-) >> >> diff --git a/lib/libf2fs.c b/lib/libf2fs.c >> index d51e485..fad3fd4 100644 >> --- a/lib/libf2fs.c >> +++ b/lib/libf2fs.c >> @@ -832,7 +832,7 @@ int f2fs_dev_is_umounted(char *path) >> } >> } else if (S_ISREG(st_buf.st_mode)) { >> /* check whether regular is backfile of loop device */ >> -#if defined(HAVE_LINUX_LOOP_H) && defined(HAVE_LINUX_MAJOR_H) >> +#if defined(HAVE_LINUX_MAJOR_H) >> struct mntent *mnt; >> struct stat st_loop; >> FILE *f; >> @@ -840,8 +840,9 @@ int f2fs_dev_is_umounted(char *path) >> f = setmntent("/proc/mounts", "r"); >> >> while ((mnt = getmntent(f)) != NULL) { >> - struct loop_info64 loopinfo = {0, }; >> - int loop_fd, err; >> + struct stat st_back; >> + int sysfs_fd, rc; >> + char buf[PATH_MAX + 1]; >> >> if (mnt->mnt_fsname[0] != '/') >> continue; >> @@ -852,23 +853,36 @@ int f2fs_dev_is_umounted(char *path) >> if (major(st_loop.st_rdev) != LOOP_MAJOR) >> continue; >> >> - loop_fd = open(mnt->mnt_fsname, O_RDONLY); >> - if (loop_fd < 0) { >> + snprintf(buf, PATH_MAX, >> + "/sys/dev/block/%d:%d/loop/backing_file", >> + major(st_loop.st_rdev), minor(st_loop.st_rdev)); >> + >> + sysfs_fd = open(buf, O_RDONLY); >> + if (sysfs_fd < 0) { >> MSG(0, "Info: open %s failed errno:%d\n", >> - mnt->mnt_fsname, errno); >> + buf, errno); >> return -1; >> } >> >> - err = ioctl(loop_fd, LOOP_GET_STATUS64, &loopinfo); >> - close(loop_fd); >> - if (err < 0) { >> - MSG(0, "\tError: ioctl LOOP_GET_STATUS64 failed errno:%d!\n", >> - errno); >> + memset(buf, 0, PATH_MAX + 1); >> + rc = read(sysfs_fd, buf, PATH_MAX); >> + if (rc < 0) { >> + MSG(0, "Info: read %s failed errno:%d\n", >> + buf, errno); >> return -1; >> } >> >> - if (st_buf.st_dev == loopinfo.lo_device && >> - st_buf.st_ino == loopinfo.lo_inode) { >> + /* Remove trailing newline */ >> + if (rc > 0 && *(buf + rc - 1) == '\n') >> + --rc; >> + buf[rc] = '\0'; >> + >> + /* Skip deleted files like "xxx (deleted)" */ >> + if (stat(buf, &st_back) != 0) >> + continue; >> + >> + if (st_buf.st_dev == st_back.st_dev && >> + st_buf.st_ino == st_back.st_ino) { >> MSG(0, "\tError: In use by loop device!\n"); >> return -EBUSY; >> } >> -- >> 2.34.1 >> >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On 03/01, 黄佳男 wrote: > On 2024/3/1 16:39, Juhyung Park wrote: > > [外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈 > > > > Hi Huang and Chao. > > > > I feel like this special loopback handling alongside Chao's > > 14197d546b93 on f2fs-tools is just unnecessarily complicating the code > > flow. > > We're now doing what, lookup to /sys, parse original backing file, > > remove trailing newline char, stat()'ing it to make sure it exists? > > Indeed this is not a good approach. > > > What if the stat()'ed file is a new file after the original backing > > file has been deleted? > > > > Being able to overwrite an active loopback backing file is a semantic > > that Linux provides willingly. > > O_EXCL only works on block devices and it's a POSIX guarantee that > > multiple writers can work on a regular file. > > > > IMHO we should honor that, but if we really want to prevent this akin > > to e2fsprogs, we should be using mntent like e2fsprogs. > > e2fsprogs will continue to check if opening the loop device fails, > rather than > > exiting. This way non-root users can use mkfs/fsck normally, although there > > may be overwrite issues. I think we need to fix this first and try to find a better way. https://lore.kernel.org/linux-f2fs-devel/20240305204834.101697-1-jaegeuk@kernel.org/T/#u > > > Thanks, > > Jianan > > > On Fri, Mar 1, 2024 at 4:15 PM Huang Jianan via Linux-f2fs-devel > > <linux-f2fs-devel@lists.sourceforge.net> wrote: > >> Currently mkfs/fsck gets the following error when executed by > >> non-root users: > >> > >> Info: open /dev/loop0 failed errno:13 > >> Error: Not available on mounted device! > >> > >> Let's fix it by reading the backing file from sysfs. > >> > >> Fixes: 14197d546b93 ("f2fs-tools: fix to check loop device") > >> Signed-off-by: Huang Jianan <huangjianan@xiaomi.com> > >> --- > >> v3: > >> - Skip deleted backing file. > >> lib/libf2fs.c | 40 +++++++++++++++++++++++++++------------- > >> 1 file changed, 27 insertions(+), 13 deletions(-) > >> > >> diff --git a/lib/libf2fs.c b/lib/libf2fs.c > >> index d51e485..fad3fd4 100644 > >> --- a/lib/libf2fs.c > >> +++ b/lib/libf2fs.c > >> @@ -832,7 +832,7 @@ int f2fs_dev_is_umounted(char *path) > >> } > >> } else if (S_ISREG(st_buf.st_mode)) { > >> /* check whether regular is backfile of loop device */ > >> -#if defined(HAVE_LINUX_LOOP_H) && defined(HAVE_LINUX_MAJOR_H) > >> +#if defined(HAVE_LINUX_MAJOR_H) > >> struct mntent *mnt; > >> struct stat st_loop; > >> FILE *f; > >> @@ -840,8 +840,9 @@ int f2fs_dev_is_umounted(char *path) > >> f = setmntent("/proc/mounts", "r"); > >> > >> while ((mnt = getmntent(f)) != NULL) { > >> - struct loop_info64 loopinfo = {0, }; > >> - int loop_fd, err; > >> + struct stat st_back; > >> + int sysfs_fd, rc; > >> + char buf[PATH_MAX + 1]; > >> > >> if (mnt->mnt_fsname[0] != '/') > >> continue; > >> @@ -852,23 +853,36 @@ int f2fs_dev_is_umounted(char *path) > >> if (major(st_loop.st_rdev) != LOOP_MAJOR) > >> continue; > >> > >> - loop_fd = open(mnt->mnt_fsname, O_RDONLY); > >> - if (loop_fd < 0) { > >> + snprintf(buf, PATH_MAX, > >> + "/sys/dev/block/%d:%d/loop/backing_file", > >> + major(st_loop.st_rdev), minor(st_loop.st_rdev)); > >> + > >> + sysfs_fd = open(buf, O_RDONLY); > >> + if (sysfs_fd < 0) { > >> MSG(0, "Info: open %s failed errno:%d\n", > >> - mnt->mnt_fsname, errno); > >> + buf, errno); > >> return -1; > >> } > >> > >> - err = ioctl(loop_fd, LOOP_GET_STATUS64, &loopinfo); > >> - close(loop_fd); > >> - if (err < 0) { > >> - MSG(0, "\tError: ioctl LOOP_GET_STATUS64 failed errno:%d!\n", > >> - errno); > >> + memset(buf, 0, PATH_MAX + 1); > >> + rc = read(sysfs_fd, buf, PATH_MAX); > >> + if (rc < 0) { > >> + MSG(0, "Info: read %s failed errno:%d\n", > >> + buf, errno); > >> return -1; > >> } > >> > >> - if (st_buf.st_dev == loopinfo.lo_device && > >> - st_buf.st_ino == loopinfo.lo_inode) { > >> + /* Remove trailing newline */ > >> + if (rc > 0 && *(buf + rc - 1) == '\n') > >> + --rc; > >> + buf[rc] = '\0'; > >> + > >> + /* Skip deleted files like "xxx (deleted)" */ > >> + if (stat(buf, &st_back) != 0) > >> + continue; > >> + > >> + if (st_buf.st_dev == st_back.st_dev && > >> + st_buf.st_ino == st_back.st_ino) { > >> MSG(0, "\tError: In use by loop device!\n"); > >> return -EBUSY; > >> } > >> -- > >> 2.34.1 > >> > >> > >> > >> _______________________________________________ > >> Linux-f2fs-devel mailing list > >> Linux-f2fs-devel@lists.sourceforge.net > >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > >
diff --git a/lib/libf2fs.c b/lib/libf2fs.c index d51e485..fad3fd4 100644 --- a/lib/libf2fs.c +++ b/lib/libf2fs.c @@ -832,7 +832,7 @@ int f2fs_dev_is_umounted(char *path) } } else if (S_ISREG(st_buf.st_mode)) { /* check whether regular is backfile of loop device */ -#if defined(HAVE_LINUX_LOOP_H) && defined(HAVE_LINUX_MAJOR_H) +#if defined(HAVE_LINUX_MAJOR_H) struct mntent *mnt; struct stat st_loop; FILE *f; @@ -840,8 +840,9 @@ int f2fs_dev_is_umounted(char *path) f = setmntent("/proc/mounts", "r"); while ((mnt = getmntent(f)) != NULL) { - struct loop_info64 loopinfo = {0, }; - int loop_fd, err; + struct stat st_back; + int sysfs_fd, rc; + char buf[PATH_MAX + 1]; if (mnt->mnt_fsname[0] != '/') continue; @@ -852,23 +853,36 @@ int f2fs_dev_is_umounted(char *path) if (major(st_loop.st_rdev) != LOOP_MAJOR) continue; - loop_fd = open(mnt->mnt_fsname, O_RDONLY); - if (loop_fd < 0) { + snprintf(buf, PATH_MAX, + "/sys/dev/block/%d:%d/loop/backing_file", + major(st_loop.st_rdev), minor(st_loop.st_rdev)); + + sysfs_fd = open(buf, O_RDONLY); + if (sysfs_fd < 0) { MSG(0, "Info: open %s failed errno:%d\n", - mnt->mnt_fsname, errno); + buf, errno); return -1; } - err = ioctl(loop_fd, LOOP_GET_STATUS64, &loopinfo); - close(loop_fd); - if (err < 0) { - MSG(0, "\tError: ioctl LOOP_GET_STATUS64 failed errno:%d!\n", - errno); + memset(buf, 0, PATH_MAX + 1); + rc = read(sysfs_fd, buf, PATH_MAX); + if (rc < 0) { + MSG(0, "Info: read %s failed errno:%d\n", + buf, errno); return -1; } - if (st_buf.st_dev == loopinfo.lo_device && - st_buf.st_ino == loopinfo.lo_inode) { + /* Remove trailing newline */ + if (rc > 0 && *(buf + rc - 1) == '\n') + --rc; + buf[rc] = '\0'; + + /* Skip deleted files like "xxx (deleted)" */ + if (stat(buf, &st_back) != 0) + continue; + + if (st_buf.st_dev == st_back.st_dev && + st_buf.st_ino == st_back.st_ino) { MSG(0, "\tError: In use by loop device!\n"); return -EBUSY; }
Currently mkfs/fsck gets the following error when executed by non-root users: Info: open /dev/loop0 failed errno:13 Error: Not available on mounted device! Let's fix it by reading the backing file from sysfs. Fixes: 14197d546b93 ("f2fs-tools: fix to check loop device") Signed-off-by: Huang Jianan <huangjianan@xiaomi.com> --- v3: - Skip deleted backing file. lib/libf2fs.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)