| Message ID | 20181016094343.21102-1-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs_io: prevents the usage in FIFO files | expand |
On 10/16/18 4:43 AM, Carlos Maiolino wrote: > Recently we had a bug report of xfs_io frozen on a file, which ended up > being a pipe, and xfs_io was waiting for data on the other side of the > pipe. > > Although xfs_io was not stuck due a bug itself, we can do better and > check the file type before opening the file. xfs_io has very limited > usage on pipes, so, just check and deny opening of FIFO files. This seems a little too heavy-handed. Operating on pipes is probably rare, but xfs_io is supposed to be a generic I/O tool. Today I can do for example: # mkfifo pipe # xfs_io -c stat pipe fd.path = "pipe" fd.flags = non-sync,non-direct,read-write stat.ino = 102077957 stat.type = fifo stat.size = 0 stat.blocks = 0 What xfs_io command was stuck in particular? These all seem to be handled uh, at least without a hang, if not correctly: # xfs_io -c "pread 0 4096" pipe pread: Illegal seek # xfs_io -c "pwrite 0 4096" pipe pwrite: Illegal seek # xfs_io -c "bmap" pipe foreign file active, bmap command is for XFS filesystems only (?!) Oh, this one is interesting ;) # xfs_io -c "fiemap" pipe pipe: xfs_io: ioctl(FS_IOC_FIEMAP) ["pipe"]: Structure needs cleaning ... [149881.306316] XFS (dm-0): Internal error xfs_bmapi_read at line 3817 of file fs/xfs/libxfs/xfs_bmap.c. Caller xfs_file_iomap_begin+0x16c/0x9f0 [xfs] (eek?) Anyway, I wonder if we can be more targeted in denying pipes if necessary, maybe CMD_PIPE_OK or CMD_NO_PIPE if that makes sense? -Eric > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > > I belive having a generic helper to check the file type may have other uses too, > so I opted to make it a generic helper. > > include/libfrog.h | 2 ++ > io/open.c | 6 ++++++ > libfrog/util.c | 12 ++++++++++++ > 3 files changed, 20 insertions(+) > > diff --git a/include/libfrog.h b/include/libfrog.h > index d33f0146..693d026d 100644 > --- a/include/libfrog.h > +++ b/include/libfrog.h > @@ -5,7 +5,9 @@ > */ > #ifndef __LIBFROG_UTIL_H_ > #define __LIBFROG_UTIL_H_ > +#include <sys/types.h> > > unsigned int log2_roundup(unsigned int i); > +unsigned int check_file_type(char *name, mode_t mode); > > #endif /* __LIBFROG_UTIL_H_ */ > diff --git a/io/open.c b/io/open.c > index 6ea3e9a2..25f44b64 100644 > --- a/io/open.c > +++ b/io/open.c > @@ -9,6 +9,7 @@ > #include "init.h" > #include "io.h" > #include "libxfs.h" > +#include "libfrog.h" > > #ifndef __O_TMPFILE > #if defined __alpha__ > @@ -59,6 +60,11 @@ openfile( > int fd; > int oflags; > > + if (check_file_type(path, S_IFIFO)) { > + fprintf(stderr, _("xfs_io does not work on FIFO files\n")); > + return -1; > + } > + > oflags = flags & IO_READONLY ? O_RDONLY : O_RDWR; > if (flags & IO_APPEND) > oflags |= O_APPEND; > diff --git a/libfrog/util.c b/libfrog/util.c > index ff935184..de0b8542 100644 > --- a/libfrog/util.c > +++ b/libfrog/util.c > @@ -5,6 +5,9 @@ > */ > #include "platform_defs.h" > #include "libfrog.h" > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <unistd.h> > > /* > * libfrog is a collection of miscellaneous userspace utilities. > @@ -22,3 +25,12 @@ log2_roundup(unsigned int i) > } > return rval; > } > + > +unsigned int > +check_file_type(char *name, mode_t mode) > +{ > + struct stat sb; > + > + lstat(name, &sb); > + return (sb.st_mode & mode); > +} >
On Thu, Nov 01, 2018 at 10:41:42AM -0500, Eric Sandeen wrote: > On 10/16/18 4:43 AM, Carlos Maiolino wrote: > > Recently we had a bug report of xfs_io frozen on a file, which ended up > > being a pipe, and xfs_io was waiting for data on the other side of the > > pipe. > > > > Although xfs_io was not stuck due a bug itself, we can do better and > > check the file type before opening the file. xfs_io has very limited > > usage on pipes, so, just check and deny opening of FIFO files. > > This seems a little too heavy-handed. Operating on pipes is probably > rare, but xfs_io is supposed to be a generic I/O tool. Agreed. Suppose I want to try reflinking a pipe in an xfstest? ;) > Today I can do for example: > > # mkfifo pipe > > # xfs_io -c stat pipe > fd.path = "pipe" > fd.flags = non-sync,non-direct,read-write > stat.ino = 102077957 > stat.type = fifo > stat.size = 0 > stat.blocks = 0 > > What xfs_io command was stuck in particular? These all seem to be handled uh, at least without a hang, if not correctly: > > # xfs_io -c "pread 0 4096" pipe > pread: Illegal seek > # xfs_io -c "pwrite 0 4096" pipe > pwrite: Illegal seek > # xfs_io -c "bmap" pipe > foreign file active, bmap command is for XFS filesystems only > > (?!) > > Oh, this one is interesting ;) > > # xfs_io -c "fiemap" pipe > pipe: > xfs_io: ioctl(FS_IOC_FIEMAP) ["pipe"]: Structure needs cleaning > > ... > > [149881.306316] XFS (dm-0): Internal error xfs_bmapi_read at line 3817 of file fs/xfs/libxfs/xfs_bmap.c. Caller xfs_file_iomap_begin+0x16c/0x9f0 [xfs] > > > (eek?) /me suspects we should change the vfs ioctl_fiemap() to return zero records (or EOPNOTSUPP?) for non-file, non-dir fds... --D > Anyway, I wonder if we can be more targeted in denying pipes if necessary, maybe CMD_PIPE_OK or CMD_NO_PIPE if that makes sense? > > -Eric > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > > > I belive having a generic helper to check the file type may have other uses too, > > so I opted to make it a generic helper. > > > > include/libfrog.h | 2 ++ > > io/open.c | 6 ++++++ > > libfrog/util.c | 12 ++++++++++++ > > 3 files changed, 20 insertions(+) > > > > diff --git a/include/libfrog.h b/include/libfrog.h > > index d33f0146..693d026d 100644 > > --- a/include/libfrog.h > > +++ b/include/libfrog.h > > @@ -5,7 +5,9 @@ > > */ > > #ifndef __LIBFROG_UTIL_H_ > > #define __LIBFROG_UTIL_H_ > > +#include <sys/types.h> > > > > unsigned int log2_roundup(unsigned int i); > > +unsigned int check_file_type(char *name, mode_t mode); > > > > #endif /* __LIBFROG_UTIL_H_ */ > > diff --git a/io/open.c b/io/open.c > > index 6ea3e9a2..25f44b64 100644 > > --- a/io/open.c > > +++ b/io/open.c > > @@ -9,6 +9,7 @@ > > #include "init.h" > > #include "io.h" > > #include "libxfs.h" > > +#include "libfrog.h" > > > > #ifndef __O_TMPFILE > > #if defined __alpha__ > > @@ -59,6 +60,11 @@ openfile( > > int fd; > > int oflags; > > > > + if (check_file_type(path, S_IFIFO)) { > > + fprintf(stderr, _("xfs_io does not work on FIFO files\n")); > > + return -1; > > + } > > + > > oflags = flags & IO_READONLY ? O_RDONLY : O_RDWR; > > if (flags & IO_APPEND) > > oflags |= O_APPEND; > > diff --git a/libfrog/util.c b/libfrog/util.c > > index ff935184..de0b8542 100644 > > --- a/libfrog/util.c > > +++ b/libfrog/util.c > > @@ -5,6 +5,9 @@ > > */ > > #include "platform_defs.h" > > #include "libfrog.h" > > +#include <sys/types.h> > > +#include <sys/stat.h> > > +#include <unistd.h> > > > > /* > > * libfrog is a collection of miscellaneous userspace utilities. > > @@ -22,3 +25,12 @@ log2_roundup(unsigned int i) > > } > > return rval; > > } > > + > > +unsigned int > > +check_file_type(char *name, mode_t mode) > > +{ > > + struct stat sb; > > + > > + lstat(name, &sb); > > + return (sb.st_mode & mode); > > +} > >
Hi guys. On Thu, Nov 01, 2018 at 09:32:09AM -0700, Darrick J. Wong wrote: > On Thu, Nov 01, 2018 at 10:41:42AM -0500, Eric Sandeen wrote: > > On 10/16/18 4:43 AM, Carlos Maiolino wrote: > > > > This seems a little too heavy-handed. Operating on pipes is probably > > rare, but xfs_io is supposed to be a generic I/O tool. > > Agreed. Suppose I want to try reflinking a pipe in an xfstest? ;) > > > Today I can do for example: > > > > # mkfifo pipe > > > > # xfs_io -c stat pipe > > fd.path = "pipe" > > fd.flags = non-sync,non-direct,read-write > > stat.ino = 102077957 > > stat.type = fifo > > stat.size = 0 > > stat.blocks = 0 > > > > What xfs_io command was stuck in particular? These all seem to be handled uh, at least without a hang, if not correctly: > > > > # xfs_io -c "pread 0 4096" pipe > > pread: Illegal seek > > # xfs_io -c "pwrite 0 4096" pipe > > pwrite: Illegal seek > > # xfs_io -c "bmap" pipe > > foreign file active, bmap command is for XFS filesystems only > > I just did a quick look at the FIFO code in kernel to better understand what's going on and what can we do about it. What happens is if the pipe is opened for Write only or read only, it will block waiting for the other side of the pipe (if NONBLOCK isn't used). While, if the file is opened as O_RDWR, the same is not true, as the following snippet taken from fifo_open(): =============================== switch (filp->f_mode) { case FMODE_READ: ... if (!is_pipe && !pipe->writers) { if (wait_for_partner(pipe, &pipe->w_counter)) ... case FMODE_WRITE: ... if (!is_pipe && !pipe->readers) { if (wait_for_partner(pipe, &pipe->r_counter)) ... . . case FMODE_READ | FMODE_WRITE: /* * O_RDWR * POSIX.1 leaves this case "undefined" when O_NONBLOCK is set. * This implementation will NEVER block on a O_RDWR open, since * the process can at least talk to itself. */ pipe->readers++; pipe->writers++; pipe->r_counter++; pipe->w_counter++; if (pipe->readers == 1 || pipe->writers == 1) wake_up_partner(pipe); break; ================================ So, I was wondering if maybe, we should only deny the usage of pipes when `xfs_io -r` is used? Or should we keep the granularity of each command having its own decision if it's allowed over pipes or not. Or even a new argument allowing the usage of pipes or not (I'm not a fan of new arguments, but this is still a feasible option). Cheers > > (?!) > > > > Oh, this one is interesting ;) > > > > # xfs_io -c "fiemap" pipe > > pipe: > > xfs_io: ioctl(FS_IOC_FIEMAP) ["pipe"]: Structure needs cleaning > > > > ... > > > > [149881.306316] XFS (dm-0): Internal error xfs_bmapi_read at line 3817 of file fs/xfs/libxfs/xfs_bmap.c. Caller xfs_file_iomap_begin+0x16c/0x9f0 [xfs] > > > > > > (eek?) > > /me suspects we should change the vfs ioctl_fiemap() to return zero > records (or EOPNOTSUPP?) for non-file, non-dir fds... > > --D >
diff --git a/include/libfrog.h b/include/libfrog.h index d33f0146..693d026d 100644 --- a/include/libfrog.h +++ b/include/libfrog.h @@ -5,7 +5,9 @@ */ #ifndef __LIBFROG_UTIL_H_ #define __LIBFROG_UTIL_H_ +#include <sys/types.h> unsigned int log2_roundup(unsigned int i); +unsigned int check_file_type(char *name, mode_t mode); #endif /* __LIBFROG_UTIL_H_ */ diff --git a/io/open.c b/io/open.c index 6ea3e9a2..25f44b64 100644 --- a/io/open.c +++ b/io/open.c @@ -9,6 +9,7 @@ #include "init.h" #include "io.h" #include "libxfs.h" +#include "libfrog.h" #ifndef __O_TMPFILE #if defined __alpha__ @@ -59,6 +60,11 @@ openfile( int fd; int oflags; + if (check_file_type(path, S_IFIFO)) { + fprintf(stderr, _("xfs_io does not work on FIFO files\n")); + return -1; + } + oflags = flags & IO_READONLY ? O_RDONLY : O_RDWR; if (flags & IO_APPEND) oflags |= O_APPEND; diff --git a/libfrog/util.c b/libfrog/util.c index ff935184..de0b8542 100644 --- a/libfrog/util.c +++ b/libfrog/util.c @@ -5,6 +5,9 @@ */ #include "platform_defs.h" #include "libfrog.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> /* * libfrog is a collection of miscellaneous userspace utilities. @@ -22,3 +25,12 @@ log2_roundup(unsigned int i) } return rval; } + +unsigned int +check_file_type(char *name, mode_t mode) +{ + struct stat sb; + + lstat(name, &sb); + return (sb.st_mode & mode); +}
Recently we had a bug report of xfs_io frozen on a file, which ended up being a pipe, and xfs_io was waiting for data on the other side of the pipe. Although xfs_io was not stuck due a bug itself, we can do better and check the file type before opening the file. xfs_io has very limited usage on pipes, so, just check and deny opening of FIFO files. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- I belive having a generic helper to check the file type may have other uses too, so I opted to make it a generic helper. include/libfrog.h | 2 ++ io/open.c | 6 ++++++ libfrog/util.c | 12 ++++++++++++ 3 files changed, 20 insertions(+)