[PATCH 0/2] xfs_io: additions for testing copy_range

[PATCH 0/2] xfs_io: additions for testing copy_range

[Dave Chinner]

Hi folks,

In trying to exercise copy_file_range() error conditions, I needed
to be able to pass pipes to copy_range and to have writeable file
descriptors open over read only files. These two patches allow these
things to be done by preventing pipes blocking on opening and
allowing xfs_io to change read/write permissions on open files.

Cheers,

Dave.

[PATCH 1/2] io: open pipes in non-blocking mode

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

So that O_RDONLY open commands (such as from copy_range) do not
block forever waiting on a non-existent writer.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 io/open.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/io/open.c b/io/open.c
index 6ea3e9a2019f..b1d9a0fa317c 100644
--- a/io/open.c
+++ b/io/open.c
@@ -56,6 +56,7 @@ openfile(
 	struct fs_path	*fs_path)
 {
 	struct fs_path	*fsp;
+	struct stat	st;
 	int		fd;
 	int		oflags;
 
@@ -79,6 +80,18 @@ openfile(
 	if (flags & IO_NOFOLLOW)
 		oflags |= O_NOFOLLOW;
 
+	/*
+	 * if we've been passed a pipe to open, don't block waiting for a
+	 * reader or writer to appear. We want to either succeed or error out
+	 * immediately.
+	 */
+	if (stat(path, &st) < 0 && errno != ENOENT) {
+		perror("stat");
+		return -1;
+	}
+	if (S_ISFIFO(st.st_mode))
+		oflags |= O_NONBLOCK;
+
 	fd = open(path, oflags, mode);
 	if (fd < 0) {
 		if (errno == EISDIR &&
-- 
2.19.1

[PATCH 2/2] xfs_io: allow open file permissions to be changed

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

I need to be able to open a file read-write, then change the
permissions on the file to read-only to check that copy_file_range
returns EPERM correctly in that case. This can't be done as root,
because root ignores file permissions, but as a normal user we can't
open a 0444 file for writing and so can't actually test writing to
a read-only file without some method of "open read-write, change
permissions to read-only, try to write to file through open
read-write file".

So, allow adding or removing write permissions on an open file.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 io/open.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/io/open.c b/io/open.c
index b1d9a0fa317c..153d4a836d4c 100644
--- a/io/open.c
+++ b/io/open.c
@@ -44,6 +44,7 @@ static cmdinfo_t chproj_cmd;
 static cmdinfo_t lsproj_cmd;
 static cmdinfo_t extsize_cmd;
 static cmdinfo_t inode_cmd;
+static cmdinfo_t chmod_cmd;
 static prid_t prid;
 static long extsize;
 
@@ -809,6 +810,48 @@ inode_f(
 	return 0;
 }
 
+static void
+chmod_help(void)
+{
+	printf(_(
+"\n"
+" Change the read/write permissions on the current file\n"
+"\n"
+" Options:\n"
+" -r -- make the file read only (0444 permissions)\n"
+" -w -- make the file read/write (0664 permissions)\n"
+"\n"));
+}
+
+static int
+chmod_f(
+	int		argc,
+	char		**argv)
+{
+	mode_t		mode = S_IRUSR | S_IRGRP | S_IROTH;
+	int		c;
+
+	while ((c = getopt(argc, argv, "rw")) != EOF) {
+		switch (c) {
+		case 'r':
+			break;
+		case 'w':
+			mode |= S_IWUSR | S_IWGRP;
+			break;
+		default:
+			return command_usage(&chmod_cmd);
+		}
+	}
+
+	if (argc != optind)
+		return command_usage(&chmod_cmd);
+
+	if (fchmod(file->fd, mode) < 0) {
+		exitcode = 1;
+		perror("fchmod");
+	}
+	return 0;
+}
 void
 open_init(void)
 {
@@ -871,10 +914,21 @@ open_init(void)
 		_("Query inode number usage in the filesystem");
 	inode_cmd.help = inode_help;
 
+	chmod_cmd.name = "chmod";
+	chmod_cmd.cfunc = chmod_f;
+	chmod_cmd.args = _("-r | -w");
+	chmod_cmd.argmin = 1;
+	chmod_cmd.argmax = 1;
+	chmod_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
+	chmod_cmd.oneline =
+		_("change the read/write permissios on the currently open file");
+	chmod_cmd.help = chmod_help;
+
 	add_command(&open_cmd);
 	add_command(&close_cmd);
 	add_command(&chproj_cmd);
 	add_command(&lsproj_cmd);
 	add_command(&extsize_cmd);
 	add_command(&inode_cmd);
+	add_command(&chmod_cmd);
 }
-- 
2.19.1

[PATCH 3/2] xfs_io: copy_file_range length is a size_t

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

copy_file_range() takes a size_t as it's length, not a "long long".
Therefore we need to be able to pass sizes larger than 8EB to it
to be able to test the interface fully and that requires copy_range
to accept all values except an explicit error value of "-1LL".

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 io/copy_file_range.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index f118a3bfd506..4e2969c9ca47 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -34,7 +34,7 @@ copy_range_help(void)
  * glibc buffered copy fallback.
  */
 static loff_t
-copy_file_range_cmd(int fd, long long *src, long long *dst, long long len)
+copy_file_range_cmd(int fd, long long *src, long long *dst, size_t len)
 {
 	loff_t ret;
 
@@ -78,7 +78,7 @@ copy_range_f(int argc, char **argv)
 {
 	long long src = 0;
 	long long dst = 0;
-	long long len = 0;
+	size_t len = 0;
 	int opt;
 	int ret;
 	int fd;
@@ -104,7 +104,7 @@ copy_range_f(int argc, char **argv)
 			break;
 		case 'l':
 			len = cvtnum(fsblocksize, fssectsize, optarg);
-			if (len < 0) {
+			if (len == -1LL) {
 				printf(_("invalid length -- %s\n"), optarg);
 				return 0;
 			}

Re: [PATCH 1/2] io: open pipes in non-blocking mode

[Jan Tulak]

On Sun, Dec 2, 2018 at 9:54 PM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> So that O_RDONLY open commands (such as from copy_range) do not
> block forever waiting on a non-existent writer.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good.

Reviewed-by: Jan Tulak <jtulak@redhat.com>

> ---
>  io/open.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/io/open.c b/io/open.c
> index 6ea3e9a2019f..b1d9a0fa317c 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -56,6 +56,7 @@ openfile(
>         struct fs_path  *fs_path)
>  {
>         struct fs_path  *fsp;
> +       struct stat     st;
>         int             fd;
>         int             oflags;
>
> @@ -79,6 +80,18 @@ openfile(
>         if (flags & IO_NOFOLLOW)
>                 oflags |= O_NOFOLLOW;
>
> +       /*
> +        * if we've been passed a pipe to open, don't block waiting for a
> +        * reader or writer to appear. We want to either succeed or error out
> +        * immediately.
> +        */
> +       if (stat(path, &st) < 0 && errno != ENOENT) {
> +               perror("stat");
> +               return -1;
> +       }
> +       if (S_ISFIFO(st.st_mode))
> +               oflags |= O_NONBLOCK;
> +
>         fd = open(path, oflags, mode);
>         if (fd < 0) {
>                 if (errno == EISDIR &&
> --
> 2.19.1
>


-- 
Jan Tulak

Re: [PATCH 3/2] xfs_io: copy_file_range length is a size_t

[Jan Tulak]

On Mon, Dec 3, 2018 at 1:20 AM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> copy_file_range() takes a size_t as it's length, not a "long long".
> Therefore we need to be able to pass sizes larger than 8EB to it
> to be able to test the interface fully and that requires copy_range
> to accept all values except an explicit error value of "-1LL".
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Reviewed-by: Jan Tulak <jtulak@redhat.com>

> ---
>  io/copy_file_range.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index f118a3bfd506..4e2969c9ca47 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -34,7 +34,7 @@ copy_range_help(void)
>   * glibc buffered copy fallback.
>   */
>  static loff_t
> -copy_file_range_cmd(int fd, long long *src, long long *dst, long long len)
> +copy_file_range_cmd(int fd, long long *src, long long *dst, size_t len)
>  {
>         loff_t ret;
>
> @@ -78,7 +78,7 @@ copy_range_f(int argc, char **argv)
>  {
>         long long src = 0;
>         long long dst = 0;
> -       long long len = 0;
> +       size_t len = 0;
>         int opt;
>         int ret;
>         int fd;
> @@ -104,7 +104,7 @@ copy_range_f(int argc, char **argv)
>                         break;
>                 case 'l':
>                         len = cvtnum(fsblocksize, fssectsize, optarg);
> -                       if (len < 0) {
> +                       if (len == -1LL) {
>                                 printf(_("invalid length -- %s\n"), optarg);
>                                 return 0;
>                         }
>


-- 
Jan Tulak

Re: [PATCH 2/2] xfs_io: allow open file permissions to be changed

[Jan Tulak]

On Sun, Dec 2, 2018 at 9:54 PM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> I need to be able to open a file read-write, then change the
> permissions on the file to read-only to check that copy_file_range
> returns EPERM correctly in that case. This can't be done as root,
> because root ignores file permissions, but as a normal user we can't
> open a 0444 file for writing and so can't actually test writing to
> a read-only file without some method of "open read-write, change
> permissions to read-only, try to write to file through open
> read-write file".
>
> So, allow adding or removing write permissions on an open file.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Should there be a man page update as well? Other commands from open.c
have a corresponding man section, but:
$ grep -c chmod man/man8/xfs_io.8
0

And I wonder if the two permissions (0444 and 0664) are enough, or we
might want to add other modes as well. But maybe that can be added
when the need comes... Otherwise it looks good.

> ---
>  io/open.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> diff --git a/io/open.c b/io/open.c
> index b1d9a0fa317c..153d4a836d4c 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -44,6 +44,7 @@ static cmdinfo_t chproj_cmd;
>  static cmdinfo_t lsproj_cmd;
>  static cmdinfo_t extsize_cmd;
>  static cmdinfo_t inode_cmd;
> +static cmdinfo_t chmod_cmd;
>  static prid_t prid;
>  static long extsize;
>
> @@ -809,6 +810,48 @@ inode_f(
>         return 0;
>  }
>
> +static void
> +chmod_help(void)
> +{
> +       printf(_(
> +"\n"
> +" Change the read/write permissions on the current file\n"
> +"\n"
> +" Options:\n"
> +" -r -- make the file read only (0444 permissions)\n"
> +" -w -- make the file read/write (0664 permissions)\n"
> +"\n"));
> +}
> +
> +static int
> +chmod_f(
> +       int             argc,
> +       char            **argv)
> +{
> +       mode_t          mode = S_IRUSR | S_IRGRP | S_IROTH;
> +       int             c;
> +
> +       while ((c = getopt(argc, argv, "rw")) != EOF) {
> +               switch (c) {
> +               case 'r':
> +                       break;
> +               case 'w':
> +                       mode |= S_IWUSR | S_IWGRP;
> +                       break;
> +               default:
> +                       return command_usage(&chmod_cmd);
> +               }
> +       }
> +
> +       if (argc != optind)
> +               return command_usage(&chmod_cmd);
> +
> +       if (fchmod(file->fd, mode) < 0) {
> +               exitcode = 1;
> +               perror("fchmod");
> +       }
> +       return 0;
> +}
>  void
>  open_init(void)
>  {
> @@ -871,10 +914,21 @@ open_init(void)
>                 _("Query inode number usage in the filesystem");
>         inode_cmd.help = inode_help;
>
> +       chmod_cmd.name = "chmod";
> +       chmod_cmd.cfunc = chmod_f;
> +       chmod_cmd.args = _("-r | -w");
> +       chmod_cmd.argmin = 1;
> +       chmod_cmd.argmax = 1;
> +       chmod_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
> +       chmod_cmd.oneline =
> +               _("change the read/write permissios on the currently open file");
> +       chmod_cmd.help = chmod_help;
> +
>         add_command(&open_cmd);
>         add_command(&close_cmd);
>         add_command(&chproj_cmd);
>         add_command(&lsproj_cmd);
>         add_command(&extsize_cmd);
>         add_command(&inode_cmd);
> +       add_command(&chmod_cmd);
>  }
> --
> 2.19.1
>


-- 
Jan Tulak

Re: [PATCH 1/2] io: open pipes in non-blocking mode

[Darrick J. Wong]

On Mon, Dec 03, 2018 at 07:53:42AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> So that O_RDONLY open commands (such as from copy_range) do not
> block forever waiting on a non-existent writer.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  io/open.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/io/open.c b/io/open.c
> index 6ea3e9a2019f..b1d9a0fa317c 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -56,6 +56,7 @@ openfile(
>  	struct fs_path	*fs_path)
>  {
>  	struct fs_path	*fsp;
> +	struct stat	st;
>  	int		fd;
>  	int		oflags;
>  
> @@ -79,6 +80,18 @@ openfile(
>  	if (flags & IO_NOFOLLOW)
>  		oflags |= O_NOFOLLOW;
>  
> +	/*
> +	 * if we've been passed a pipe to open, don't block waiting for a
> +	 * reader or writer to appear. We want to either succeed or error out
> +	 * immediately.
> +	 */
> +	if (stat(path, &st) < 0 && errno != ENOENT) {
> +		perror("stat");
> +		return -1;
> +	}
> +	if (S_ISFIFO(st.st_mode))

Hmm... does stat() set st.st_mode even if it returns ENOENT?  Or are we
possibly branching on an uninitialized st here?

struct stat	st = { 0 }; ?

The rest seems fine to me.

--D

> +		oflags |= O_NONBLOCK;
> +
>  	fd = open(path, oflags, mode);
>  	if (fd < 0) {
>  		if (errno == EISDIR &&
> -- 
> 2.19.1
> 

Re: [PATCH 2/2] xfs_io: allow open file permissions to be changed

[Darrick J. Wong]

On Mon, Dec 03, 2018 at 07:53:43AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> I need to be able to open a file read-write, then change the
> permissions on the file to read-only to check that copy_file_range
> returns EPERM correctly in that case. This can't be done as root,
> because root ignores file permissions, but as a normal user we can't
> open a 0444 file for writing and so can't actually test writing to
> a read-only file without some method of "open read-write, change
> permissions to read-only, try to write to file through open
> read-write file".
> 
> So, allow adding or removing write permissions on an open file.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  io/open.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/io/open.c b/io/open.c
> index b1d9a0fa317c..153d4a836d4c 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -44,6 +44,7 @@ static cmdinfo_t chproj_cmd;
>  static cmdinfo_t lsproj_cmd;
>  static cmdinfo_t extsize_cmd;
>  static cmdinfo_t inode_cmd;
> +static cmdinfo_t chmod_cmd;
>  static prid_t prid;
>  static long extsize;
>  
> @@ -809,6 +810,48 @@ inode_f(
>  	return 0;
>  }
>  
> +static void
> +chmod_help(void)
> +{
> +	printf(_(
> +"\n"
> +" Change the read/write permissions on the current file\n"
> +"\n"
> +" Options:\n"
> +" -r -- make the file read only (0444 permissions)\n"
> +" -w -- make the file read/write (0664 permissions)\n"
> +"\n"));
> +}
> +
> +static int
> +chmod_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	mode_t		mode = S_IRUSR | S_IRGRP | S_IROTH;
> +	int		c;
> +
> +	while ((c = getopt(argc, argv, "rw")) != EOF) {
> +		switch (c) {
> +		case 'r':
> +			break;
> +		case 'w':
> +			mode |= S_IWUSR | S_IWGRP;
> +			break;
> +		default:
> +			return command_usage(&chmod_cmd);
> +		}
> +	}
> +
> +	if (argc != optind)
> +		return command_usage(&chmod_cmd);
> +
> +	if (fchmod(file->fd, mode) < 0) {
> +		exitcode = 1;
> +		perror("fchmod");
> +	}
> +	return 0;
> +}
>  void
>  open_init(void)
>  {
> @@ -871,10 +914,21 @@ open_init(void)
>  		_("Query inode number usage in the filesystem");
>  	inode_cmd.help = inode_help;
>  
> +	chmod_cmd.name = "chmod";
> +	chmod_cmd.cfunc = chmod_f;
> +	chmod_cmd.args = _("-r | -w");
> +	chmod_cmd.argmin = 1;
> +	chmod_cmd.argmax = 1;
> +	chmod_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
> +	chmod_cmd.oneline =
> +		_("change the read/write permissios on the currently open file");

"permissions"...

Also, there needs to be a manpage update for this, or else xfs/293 fails.

--D

> +	chmod_cmd.help = chmod_help;
> +
>  	add_command(&open_cmd);
>  	add_command(&close_cmd);
>  	add_command(&chproj_cmd);
>  	add_command(&lsproj_cmd);
>  	add_command(&extsize_cmd);
>  	add_command(&inode_cmd);
> +	add_command(&chmod_cmd);
>  }
> -- 
> 2.19.1
> 

Re: [PATCH 3/2] xfs_io: copy_file_range length is a size_t

[Darrick J. Wong]

On Mon, Dec 03, 2018 at 11:20:36AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> copy_file_range() takes a size_t as it's length, not a "long long".
> Therefore we need to be able to pass sizes larger than 8EB to it
> to be able to test the interface fully and that requires copy_range
> to accept all values except an explicit error value of "-1LL".
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  io/copy_file_range.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index f118a3bfd506..4e2969c9ca47 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -34,7 +34,7 @@ copy_range_help(void)
>   * glibc buffered copy fallback.
>   */
>  static loff_t
> -copy_file_range_cmd(int fd, long long *src, long long *dst, long long len)
> +copy_file_range_cmd(int fd, long long *src, long long *dst, size_t len)
>  {
>  	loff_t ret;
>  
> @@ -78,7 +78,7 @@ copy_range_f(int argc, char **argv)
>  {
>  	long long src = 0;
>  	long long dst = 0;
> -	long long len = 0;
> +	size_t len = 0;
>  	int opt;
>  	int ret;
>  	int fd;
> @@ -104,7 +104,7 @@ copy_range_f(int argc, char **argv)
>  			break;
>  		case 'l':
>  			len = cvtnum(fsblocksize, fssectsize, optarg);
> -			if (len < 0) {
> +			if (len == -1LL) {
>  				printf(_("invalid length -- %s\n"), optarg);
>  				return 0;
>  			}
> 

Re: [PATCH 2/2] xfs_io: allow open file permissions to be changed

[Dave Chinner]

On Mon, Dec 03, 2018 at 11:17:05AM +0100, Jan Tulak wrote:
> On Sun, Dec 2, 2018 at 9:54 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > I need to be able to open a file read-write, then change the
> > permissions on the file to read-only to check that copy_file_range
> > returns EPERM correctly in that case. This can't be done as root,
> > because root ignores file permissions, but as a normal user we can't
> > open a 0444 file for writing and so can't actually test writing to
> > a read-only file without some method of "open read-write, change
> > permissions to read-only, try to write to file through open
> > read-write file".
> >
> > So, allow adding or removing write permissions on an open file.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> Should there be a man page update as well? Other commands from open.c
> have a corresponding man section, but:
> $ grep -c chmod man/man8/xfs_io.8
> 0

I forgot that, thanks. So many other things to deal with.

> And I wonder if the two permissions (0444 and 0664) are enough, or we
> might want to add other modes as well. But maybe that can be added
> when the need comes... Otherwise it looks good.

If we need more than "make read-only" or "make read-write" then we
can do something more. But for pretty much all the cases where fine
grained permissions are needed, the chmod command itself should
suffice....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] io: open pipes in non-blocking mode

[Dave Chinner]

On Mon, Dec 03, 2018 at 08:20:30AM -0800, Darrick J. Wong wrote:
> On Mon, Dec 03, 2018 at 07:53:42AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > So that O_RDONLY open commands (such as from copy_range) do not
> > block forever waiting on a non-existent writer.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  io/open.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/io/open.c b/io/open.c
> > index 6ea3e9a2019f..b1d9a0fa317c 100644
> > --- a/io/open.c
> > +++ b/io/open.c
> > @@ -56,6 +56,7 @@ openfile(
> >  	struct fs_path	*fs_path)
> >  {
> >  	struct fs_path	*fsp;
> > +	struct stat	st;
> >  	int		fd;
> >  	int		oflags;
> >  
> > @@ -79,6 +80,18 @@ openfile(
> >  	if (flags & IO_NOFOLLOW)
> >  		oflags |= O_NOFOLLOW;
> >  
> > +	/*
> > +	 * if we've been passed a pipe to open, don't block waiting for a
> > +	 * reader or writer to appear. We want to either succeed or error out
> > +	 * immediately.
> > +	 */
> > +	if (stat(path, &st) < 0 && errno != ENOENT) {
> > +		perror("stat");
> > +		return -1;
> > +	}
> > +	if (S_ISFIFO(st.st_mode))
> 
> Hmm... does stat() set st.st_mode even if it returns ENOENT?  Or are we
> possibly branching on an uninitialized st here?
> 
> struct stat	st = { 0 }; ?

Yeah, I'll fix that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] io: open pipes in non-blocking mode

[Eric Sandeen]

On 12/3/18 2:16 PM, Dave Chinner wrote:
> On Mon, Dec 03, 2018 at 08:20:30AM -0800, Darrick J. Wong wrote:
>> On Mon, Dec 03, 2018 at 07:53:42AM +1100, Dave Chinner wrote:
>>> From: Dave Chinner <dchinner@redhat.com>
>>>
>>> So that O_RDONLY open commands (such as from copy_range) do not
>>> block forever waiting on a non-existent writer.
>>>
>>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>>> ---
>>>  io/open.c | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/io/open.c b/io/open.c
>>> index 6ea3e9a2019f..b1d9a0fa317c 100644
>>> --- a/io/open.c
>>> +++ b/io/open.c
>>> @@ -56,6 +56,7 @@ openfile(
>>>  	struct fs_path	*fs_path)
>>>  {
>>>  	struct fs_path	*fsp;
>>> +	struct stat	st;
>>>  	int		fd;
>>>  	int		oflags;
>>>  
>>> @@ -79,6 +80,18 @@ openfile(
>>>  	if (flags & IO_NOFOLLOW)
>>>  		oflags |= O_NOFOLLOW;
>>>  
>>> +	/*
>>> +	 * if we've been passed a pipe to open, don't block waiting for a
>>> +	 * reader or writer to appear. We want to either succeed or error out
>>> +	 * immediately.
>>> +	 */
>>> +	if (stat(path, &st) < 0 && errno != ENOENT) {
>>> +		perror("stat");
>>> +		return -1;
>>> +	}
>>> +	if (S_ISFIFO(st.st_mode))
>>
>> Hmm... does stat() set st.st_mode even if it returns ENOENT?  Or are we
>> possibly branching on an uninitialized st here?
>>
>> struct stat	st = { 0 }; ?
> 
> Yeah, I'll fix that.

I'll just do that on commit.

-Eric

[PATCH 2/2 V2] xfs_io: allow open file permissions to be changed

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

I need to be able to open a file read-write, then change the
permissions on the file to read-only to check that copy_file_range
returns EPERM correctly in that case. This can't be done as root,
because root ignores file permissions, but as a normal user we can't
open a 0444 file for writing and so can't actually test writing to
a read-only file without some method of "open read-write, change
permissions to read-only, try to write to file through open
read-write file".

So, allow adding or removing write permissions on an open file.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
Version 2:
- include a man page update from the new function

 io/open.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 man/man8/xfs_io.8 |  9 +++++++++
 2 files changed, 63 insertions(+)

diff --git a/io/open.c b/io/open.c
index 21c0e054f8d2..2663c38e9681 100644
--- a/io/open.c
+++ b/io/open.c
@@ -44,6 +44,7 @@ static cmdinfo_t chproj_cmd;
 static cmdinfo_t lsproj_cmd;
 static cmdinfo_t extsize_cmd;
 static cmdinfo_t inode_cmd;
+static cmdinfo_t chmod_cmd;
 static prid_t prid;
 static long extsize;
 
@@ -809,6 +810,48 @@ inode_f(
 	return 0;
 }
 
+static void
+chmod_help(void)
+{
+	printf(_(
+"\n"
+" Change the read/write permissions on the current file\n"
+"\n"
+" Options:\n"
+" -r -- make the file read only (0444 permissions)\n"
+" -w -- make the file read/write (0664 permissions)\n"
+"\n"));
+}
+
+static int
+chmod_f(
+	int		argc,
+	char		**argv)
+{
+	mode_t		mode = S_IRUSR | S_IRGRP | S_IROTH;
+	int		c;
+
+	while ((c = getopt(argc, argv, "rw")) != EOF) {
+		switch (c) {
+		case 'r':
+			break;
+		case 'w':
+			mode |= S_IWUSR | S_IWGRP;
+			break;
+		default:
+			return command_usage(&chmod_cmd);
+		}
+	}
+
+	if (argc != optind)
+		return command_usage(&chmod_cmd);
+
+	if (fchmod(file->fd, mode) < 0) {
+		exitcode = 1;
+		perror("fchmod");
+	}
+	return 0;
+}
 void
 open_init(void)
 {
@@ -871,10 +914,21 @@ open_init(void)
 		_("Query inode number usage in the filesystem");
 	inode_cmd.help = inode_help;
 
+	chmod_cmd.name = "chmod";
+	chmod_cmd.cfunc = chmod_f;
+	chmod_cmd.args = _("-r | -w");
+	chmod_cmd.argmin = 1;
+	chmod_cmd.argmax = 1;
+	chmod_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
+	chmod_cmd.oneline =
+		_("change the read/write permissios on the currently open file");
+	chmod_cmd.help = chmod_help;
+
 	add_command(&open_cmd);
 	add_command(&close_cmd);
 	add_command(&chproj_cmd);
 	add_command(&lsproj_cmd);
 	add_command(&extsize_cmd);
 	add_command(&inode_cmd);
+	add_command(&chmod_cmd);
 }
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index f1099c32de66..68e03b4558b9 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -1226,6 +1226,15 @@ characters long.
 .B crc32cselftest
 Test the internal crc32c implementation to make sure that it computes results
 correctly.
+.TP
+.B chmod \-r | \-w
+Change the mode of the currently open file. The
+.B \-r
+option will set the file permissions to read-only (0444), whilst the
+.B \-w
+option will set the file permissions to read-write (0644). This allows xfs_io to
+set up mismatches between the file permissions and the open file descriptor
+read/write mode to exercise permission checks inside various syscalls.
 .SH SEE ALSO
 .BR mkfs.xfs (8),
 .BR xfsctl (3),

Re: [PATCH 2/2] xfs_io: allow open file permissions to be changed

[Eric Sandeen]

On 12/2/18 2:53 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> I need to be able to open a file read-write, then change the
> permissions on the file to read-only to check that copy_file_range
> returns EPERM correctly in that case. This can't be done as root,
> because root ignores file permissions, but as a normal user we can't
> open a 0444 file for writing and so can't actually test writing to
> a read-only file without some method of "open read-write, change
> permissions to read-only, try to write to file through open
> read-write file".
> 
> So, allow adding or removing write permissions on an open file.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

I'll add:

       chmod -r | -w
              Changes the permissions of the open file to read-only (0444) or read/write (0664).

just below open/close in the xfs_io manpage and add my:

[sandeen: Add man page entry]
Reviewed-by: Eric Sandeen <sandeen@redhat.com>

if that's cool with you.

> ---
>  io/open.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/io/open.c b/io/open.c
> index b1d9a0fa317c..153d4a836d4c 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -44,6 +44,7 @@ static cmdinfo_t chproj_cmd;
>  static cmdinfo_t lsproj_cmd;
>  static cmdinfo_t extsize_cmd;
>  static cmdinfo_t inode_cmd;
> +static cmdinfo_t chmod_cmd;
>  static prid_t prid;
>  static long extsize;
>  
> @@ -809,6 +810,48 @@ inode_f(
>  	return 0;
>  }
>  
> +static void
> +chmod_help(void)
> +{
> +	printf(_(
> +"\n"
> +" Change the read/write permissions on the current file\n"
> +"\n"
> +" Options:\n"
> +" -r -- make the file read only (0444 permissions)\n"
> +" -w -- make the file read/write (0664 permissions)\n"
> +"\n"));
> +}
> +
> +static int
> +chmod_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	mode_t		mode = S_IRUSR | S_IRGRP | S_IROTH;
> +	int		c;
> +
> +	while ((c = getopt(argc, argv, "rw")) != EOF) {
> +		switch (c) {
> +		case 'r':
> +			break;
> +		case 'w':
> +			mode |= S_IWUSR | S_IWGRP;
> +			break;
> +		default:
> +			return command_usage(&chmod_cmd);
> +		}
> +	}
> +
> +	if (argc != optind)
> +		return command_usage(&chmod_cmd);
> +
> +	if (fchmod(file->fd, mode) < 0) {
> +		exitcode = 1;
> +		perror("fchmod");
> +	}
> +	return 0;
> +}
>  void
>  open_init(void)
>  {
> @@ -871,10 +914,21 @@ open_init(void)
>  		_("Query inode number usage in the filesystem");
>  	inode_cmd.help = inode_help;
>  
> +	chmod_cmd.name = "chmod";
> +	chmod_cmd.cfunc = chmod_f;
> +	chmod_cmd.args = _("-r | -w");
> +	chmod_cmd.argmin = 1;
> +	chmod_cmd.argmax = 1;
> +	chmod_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
> +	chmod_cmd.oneline =
> +		_("change the read/write permissios on the currently open file");
> +	chmod_cmd.help = chmod_help;
> +
>  	add_command(&open_cmd);
>  	add_command(&close_cmd);
>  	add_command(&chproj_cmd);
>  	add_command(&lsproj_cmd);
>  	add_command(&extsize_cmd);
>  	add_command(&inode_cmd);
> +	add_command(&chmod_cmd);
>  }
> 

Re: [PATCH 2/2 V2] xfs_io: allow open file permissions to be changed

[Eric Sandeen]

On 12/4/18 10:02 PM, Dave Chinner wrote:
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> I need to be able to open a file read-write, then change the
> permissions on the file to read-only to check that copy_file_range
> returns EPERM correctly in that case. This can't be done as root,
> because root ignores file permissions, but as a normal user we can't
> open a 0444 file for writing and so can't actually test writing to
> a read-only file without some method of "open read-write, change
> permissions to read-only, try to write to file through open
> read-write file".
> 
> So, allow adding or removing write permissions on an open file.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

whooops we crossed the streams :D

I'll move the man update to the file command section, but:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
> Version 2:
> - include a man page update from the new function
> 
>  io/open.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  man/man8/xfs_io.8 |  9 +++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/io/open.c b/io/open.c
> index 21c0e054f8d2..2663c38e9681 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -44,6 +44,7 @@ static cmdinfo_t chproj_cmd;
>  static cmdinfo_t lsproj_cmd;
>  static cmdinfo_t extsize_cmd;
>  static cmdinfo_t inode_cmd;
> +static cmdinfo_t chmod_cmd;
>  static prid_t prid;
>  static long extsize;
>  
> @@ -809,6 +810,48 @@ inode_f(
>  	return 0;
>  }
>  
> +static void
> +chmod_help(void)
> +{
> +	printf(_(
> +"\n"
> +" Change the read/write permissions on the current file\n"
> +"\n"
> +" Options:\n"
> +" -r -- make the file read only (0444 permissions)\n"
> +" -w -- make the file read/write (0664 permissions)\n"
> +"\n"));
> +}
> +
> +static int
> +chmod_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	mode_t		mode = S_IRUSR | S_IRGRP | S_IROTH;
> +	int		c;
> +
> +	while ((c = getopt(argc, argv, "rw")) != EOF) {
> +		switch (c) {
> +		case 'r':
> +			break;
> +		case 'w':
> +			mode |= S_IWUSR | S_IWGRP;
> +			break;
> +		default:
> +			return command_usage(&chmod_cmd);
> +		}
> +	}
> +
> +	if (argc != optind)
> +		return command_usage(&chmod_cmd);
> +
> +	if (fchmod(file->fd, mode) < 0) {
> +		exitcode = 1;
> +		perror("fchmod");
> +	}
> +	return 0;
> +}
>  void
>  open_init(void)
>  {
> @@ -871,10 +914,21 @@ open_init(void)
>  		_("Query inode number usage in the filesystem");
>  	inode_cmd.help = inode_help;
>  
> +	chmod_cmd.name = "chmod";
> +	chmod_cmd.cfunc = chmod_f;
> +	chmod_cmd.args = _("-r | -w");
> +	chmod_cmd.argmin = 1;
> +	chmod_cmd.argmax = 1;
> +	chmod_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
> +	chmod_cmd.oneline =
> +		_("change the read/write permissios on the currently open file");
> +	chmod_cmd.help = chmod_help;
> +
>  	add_command(&open_cmd);
>  	add_command(&close_cmd);
>  	add_command(&chproj_cmd);
>  	add_command(&lsproj_cmd);
>  	add_command(&extsize_cmd);
>  	add_command(&inode_cmd);
> +	add_command(&chmod_cmd);
>  }
> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
> index f1099c32de66..68e03b4558b9 100644
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -1226,6 +1226,15 @@ characters long.
>  .B crc32cselftest
>  Test the internal crc32c implementation to make sure that it computes results
>  correctly.
> +.TP
> +.B chmod \-r | \-w
> +Change the mode of the currently open file. The
> +.B \-r
> +option will set the file permissions to read-only (0444), whilst the
> +.B \-w
> +option will set the file permissions to read-write (0644). This allows xfs_io to
> +set up mismatches between the file permissions and the open file descriptor
> +read/write mode to exercise permission checks inside various syscalls.
>  .SH SEE ALSO
>  .BR mkfs.xfs (8),
>  .BR xfsctl (3),
>