[PATCH] xfs_io: prevents the usage in FIFO files

[PATCH] xfs_io: prevents the usage in FIFO files

[Carlos Maiolino]

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(+)

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

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

[Eric Sandeen]

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

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

[Darrick J. Wong]

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

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

[Carlos Maiolino]

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
> 

-- 
Carlos