Re: [PATCH 1/3] fs: stream_open - opener for stream-like files so that read and write can run simultaneously without deadlock

[Kirill Smelkov]

+Linus, +Al, +linux-fsdevel, +linux-kernel

On Tue, Apr 09, 2019 at 11:50:23PM +0200, Rasmus Villemoes wrote:
> On 09/04/2019 22.38, Kirill Smelkov wrote:
> > On Tue, Apr 09, 2019 at 09:43:37AM +0200, Rasmus Villemoes wrote:
> >> On 26/03/2019 23.20, Kirill Smelkov wrote:
> >>
> >>> 2. Add stream_open() to kernel to open stream-like non-seekable file descriptors.
> >>>    Read and write on such file descriptors would never use nor change ppos. And
> >>>    with that property on stream-like files read and write will be running without
> >>>    taking f_pos lock - i.e. read and write could be running simultaneously.
> >>>
> >>
> >>> diff --git a/fs/read_write.c b/fs/read_write.c
> >>> index d83003d856a0..91cb375f9840 100644
> >>> --- a/fs/read_write.c
> >>> +++ b/fs/read_write.c
> >>> @@ -560,12 +560,13 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_
> >>>  
> >>>  static inline loff_t file_pos_read(struct file *file)
> >>>  {
> >>> -	return file->f_pos;
> >>> +	return file->f_mode & FMODE_STREAM ? 0 : file->f_pos;
> >>>  }
> >>>  
> >>>  static inline void file_pos_write(struct file *file, loff_t pos)
> >>>  {
> >>> -	file->f_pos = pos;
> >>> +	if ((file->f_mode & FMODE_STREAM) == 0)
> >>> +		file->f_pos = pos;
> >>>  }
> >>
> >> Perhaps leave file_pos_read alone (to avoid the conditional load), and
> >> do WARN_ON_ONCE((file->f_mode & FMODE_STREAM) && pos != 0) or maybe !=
> >> file->f_pos in _write to catch wrongly converted drivers - once the dust
> >> has settled and patches backported to -stable, that WARN_ON_ONCE can
> >> also be removed.
> >>
> >> Or, a more brutal way to detect .read or .write callbacks that do use
> >> ppos despite FMODE_STREAM is to lift the FMODE_STREAM handling to the
> >> callers of file_pos_{read,write} and pass on (file->f_mode &
> >> FMODE_STREAM) ? NULL : &pos instead of &pos. That would also just add
> >> one instead of two conditionals to the read/write paths.
> > 
> > Good idea to pass NULL as ppos into FMODE_STREAM users as this will show
> > as oops if a driver is wrongly converted and uses the position. Do you mean
> > something like this:
> > 
> 
> Not that invasive. I was more thinking of reverting the
> file_pos_read/write to their original state, then do this one-liner once
> per caller of those
> 
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -575,7 +575,7 @@ ssize_t ksys_read(unsigned int fd, char __user *buf,
> size_t count)
> 
>         if (f.file) {
>                 loff_t pos = file_pos_read(f.file);
> -               ret = vfs_read(f.file, buf, count, &pos);
> +               ret = vfs_read(f.file, buf, count, (file->f_mode & FMODE_STREAM) ? NULL : &pos);
>                 if (ret >= 0)
>                         file_pos_write(f.file, pos);
>                 fdput_pos(f);
> 
> The idea being to minimize the number of extra branches introduced in
> the read and write syscalls due to the FMODE_STREAM. So the above
> catches wrongly done conversions (or backports - perhaps an earlier
> version of some ->read callback did use ppos...) in a rather blunt way.

I checked generated x86_64 assembly and from the point of view of adding
minimum extra branches this is indeed the best version - no branches are
added at all as `(file->f_mode & FMODE_STREAM) ? NULL : &pos` is
translated to cmov instruction.

However file->f_pos writing is still there and it will bug under race
detector, e.g. under KTSAN because read and write can be running
simultaneously. Maybe it is not only race bug, but also a bit of
slowdown as read and write code paths write to the same memory thus
needing inter-cpu synchronization if read and write handlers are on
different cpus. However for this I'm not sure.

> The alternative was to revert file_pos_read() to just return file->f_pos
> as it used to, and just do
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 177ccc3d405a..97180d61a918 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -565,6 +565,7 @@ static inline loff_t file_pos_read(struct file *file)
> 
>  static inline void file_pos_write(struct file *file, loff_t pos)
>  {
> +       WARN_ON_ONCE((file->f_mode & FMODE_STREAM) && pos != 0);
>         file->f_pos = pos;
>  }
> 
> which is, essentially, also just one more branch (at least for all the
> ordinary, non-STREAM, files); this has the advantage of not oopsing, and
> just means that ->f_pos gets updated racily until somebody reacts to the
> WARN.

I prefer we pass NULL for ppos to FOPEN_STREAM files, because above
WARN, even if it triggers, will not tell which file implementation is
guilty - it will show the stack trace only till generic VFS code, if I
understand correctly.

> In either case, the current code papers over bugs introduced by
> conversion/backporting and has more branches.

Yes, thanks for brining this up. It is indeed better that we pass NULL
into converted drivers to catch correctness. Please see below for
updated patch and branches analysis.

> > but I wonder why we are discussing in private?
> 
> Well, the cc list looked too big for some mostly micro-optimzation
> comments, and I couldn't figure out a proper subset to trim it to. But
> if you think the sanity-checking (either variant) is worth it before the
> mass-conversion starts, I'm happy to take the discussion back to lkml.

Added relevant (I think) people and lists to Cc.

I was going to propose attached patch, but it turned out that if we
start to pass ppos=NULL, we also have to update at least new_sync_read,
new_sync_write, do_iter_writev_writev, rw_verify_area etc - functions
that can be called from under vfs_read/vfs_write/..., at least this way:

diff --git a/fs/read_write.c b/fs/read_write.c
index 4bdd1731859c..80724e9791bd 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -371,7 +371,7 @@ int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t
        inode = file_inode(file);
        if (unlikely((ssize_t) count < 0))
                return retval;
-       pos = *ppos;
+       pos = (ppos ? *ppos : 0);
        if (unlikely(pos < 0)) {
                if (!unsigned_offsets(file))
                        return retval;
@@ -400,12 +400,13 @@ static ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, lo
        ssize_t ret;

        init_sync_kiocb(&kiocb, filp);
-       kiocb.ki_pos = *ppos;
+       kiocb.ki_pos = (ppos ? *ppos : 0);
        iov_iter_init(&iter, READ, &iov, 1, len);

        ret = call_read_iter(filp, &kiocb, &iter);
        BUG_ON(ret == -EIOCBQUEUED);
-       *ppos = kiocb.ki_pos;
+       if (ppos)
+               *ppos = kiocb.ki_pos;
        return ret;
 }

@@ -468,12 +469,12 @@ static ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t
        ssize_t ret;

        init_sync_kiocb(&kiocb, filp);
-       kiocb.ki_pos = *ppos;
+       kiocb.ki_pos = (ppos ? *ppos : 0);
        iov_iter_init(&iter, WRITE, &iov, 1, len);

        ret = call_write_iter(filp, &kiocb, &iter);
        BUG_ON(ret == -EIOCBQUEUED);
-       if (ret > 0)
+       if (ret > 0 && ppos)
                *ppos = kiocb.ki_pos;
        return ret;
 }
@@ -666,14 +667,15 @@ static ssize_t do_iter_readv_writev(struct file *filp, struct iov_iter *iter,
        ret = kiocb_set_rw_flags(&kiocb, flags);
        if (ret)
                return ret;
-       kiocb.ki_pos = *ppos;
+       kiocb.ki_pos = (ppos ? *ppos : 0);

        if (type == READ)
                ret = call_read_iter(filp, &kiocb, iter);
        else
                ret = call_write_iter(filp, &kiocb, iter);
        BUG_ON(ret == -EIOCBQUEUED);
-       *ppos = kiocb.ki_pos;
+       if (ppos)
+               *ppos = kiocb.ki_pos;
        return ret;
 }

There is no notion of NULL ppos in kiocb, and I'm not familiar with that
subsystem, so it would be good to first get feedback before deciding on
how/where to move on.

Linus, anyone, comments?

Thanks beforehand,
Kirill

---- 8< ----