Re: [PATCH] xfs_io: prevents the usage in FIFO files

["Darrick J. Wong" <darrick.wong@oracle.com>]

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);
> > +}
> >