Re: [PATCH v2 1/7] fsstress: allow fsync on directories too

[Dave Chinner <david@fromorbit.com>]

On Wed, Apr 03, 2019 at 05:35:20PM +0000, Filipe Manana wrote:
> On Wed, Apr 3, 2019 at 3:18 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Mon, Apr 01, 2019 at 01:50:18PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > Currently the fsync function can only be performed against regular files.
> > > Allow it to operate on directories too, to increase test coverage and
> > > allow for chances of finding bugs in a filesystem's implementation of
> > > fsync against directories.
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > >
> > > V2: Added helper functions to open and close files or directories.
> >
> > not exactly what I meant, more below....
> > >
> > >  ltp/fsstress.c | 42 +++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 39 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> > > index 2223fd7d..1169b840 100644
> > > --- a/ltp/fsstress.c
> > > +++ b/ltp/fsstress.c
> > > @@ -303,6 +303,7 @@ int       attr_remove_path(pathname_t *, const char *, int);
> > >  int  attr_set_path(pathname_t *, const char *, const char *, const int, int);
> > >  void check_cwd(void);
> > >  void cleanup_flist(void);
> > > +void close_file_or_dir(int, DIR *);
> > >  int  creat_path(pathname_t *, mode_t);
> > >  void dcache_enter(int, int);
> > >  void dcache_init(void);
> > > @@ -324,6 +325,7 @@ void      make_freq_table(void);
> > >  int  mkdir_path(pathname_t *, mode_t);
> > >  int  mknod_path(pathname_t *, mode_t, dev_t);
> > >  void namerandpad(int, char *, int);
> > > +int  open_file_or_dir(pathname_t *, int, DIR **);
> > >  int  open_path(pathname_t *, int);
> > >  DIR  *opendir_path(pathname_t *);
> > >  void process_freq(char *);
> > > @@ -852,6 +854,15 @@ cleanup_flist(void)
> > >       }
> > >  }
> > >
> > > +void
> > > +close_file_or_dir(int fd, DIR *dir)
> > > +{
> > > +     if (dir)
> > > +             closedir(dir);
> > > +     else
> > > +             close(fd);
> > > +}
> > > +
> > >  int
> > >  creat_path(pathname_t *name, mode_t mode)
> > >  {
> > > @@ -1385,6 +1396,30 @@ namerandpad(int id, char *buf, int i)
> > >  }
> > >
> > >  int
> > > +open_file_or_dir(pathname_t *name, int flags, DIR **dir)
> > > +{
> > > +     int fd;
> > > +
> > > +     fd = open_path(name, flags);
> > > +     if (fd < 0 && errno == EISDIR) {
> > > +             *dir = opendir_path(name);
> >
> > Why this function, and not just open(O_DIRECTORY)?
> 
> The reason I did it is because if flags are O_WRONLY (or O_RDWR),
> opening a directory always fails (even with O_DIRECTORY) with errno
> EISDIR.
> If the access flags are O_RDONLY, opening a directory succeeds
> regardless of O_DIRECTORY being passed to open(2).
> 
> Since I supposed fsetxattr(2) and fsync(2) could be considered write
> operations, I went for the use of opendir_path(), using the dir stream
> and all that hassle.
> 
> But now I just tested if fsync and fsetxattr work if the file is open
> O_RDONLY, and to my surprise they do.
> 
> Test program:
> 
> https://friendpaste.com/7gUN4kB3ZfHwVdlDggodUY
> 
> Output with O_WRONLY |  O_DIRECTORY passed to open(2):
> 
> $ mkfs.xfs -f /dev/sdc
> $ mount /dev/sdc /mnt
> 
> $ touch /mnt/file
> $ mkdir /mnt/dir
> 
> $ ./test_open /mnt/file
> open error 20: Not a directory
> $ ./test_open /mnt/dir
> open error 21: Is a directory
> 
> Output with only O_RDONLY passed to open(2):
> 
> $ mkfs.xfs -f /dev/sdc
> $ mount /dev/sdc /mnt
> 
> $ touch /mnt/file
> $ mkdir /mnt/dir
> 
> $ ./test_open /mnt/file
> file open fd = 3, setxattr(fd) = 0, fsync(fd) = 0
> $ ./test_open /mnt/dir
> file open fd = 3, setxattr(fd) = 0, fsync(fd) = 0
> 
> Perhaps I did something wrong, as it's late here right now.

Pretty sure this is because setxattr() checks inode permissions, not
file descriptor read/write modes to determine whether an xattr can
be set/modified/removed. i.e. you own the directory, have write
permissions to the directory, and so you can minipulate xattrs on
it.

FWIW, why even bother with opening fd's at all here? you could just
use set/get/removexattr() directly on the path name and not have to
worry about opening the file...

> So should we go for ensuring O_RDONLY is used when the first open
> fails with EISDIR (and add some comment explaining this in case I'm
> not the only for which this was a surprise)

If we actually need to open the fd, then yes - using
opendir()/dirfd() is essentially doing that for you behind the
scenes.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com