| Message ID | 20190413165449.11168-1-kirr@nexedi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] vfs: pass ppos=NULL to .read()/.write() of FMODE_STREAM files | expand |
On Sat, Apr 13, 2019 at 9:55 AM Kirill Smelkov <kirr@nexedi.com> wrote: > > --- 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; This part looks silly. We should just avoid all the position overflow games when we don't have a position at all (ie streaming). You can't overflow what you don't use. Similarly, you can't use ranged mandatory locking on a stream, so the mandatory locking thing seems dependent on pos too. So I think that with a NULL ppos being possible, we should just change the code to just do all of that conditionally on having a position, rather than saying that the position of a stream is always 0. That said, this whole "let's make it possible to not have a position at all" is a big change, and there's no way I'll apply these before the 5.2 merge window. And I'd really like to have people (Al?) look at this and go "yeah, makes sense". I do think that moving to a model where we wither have a (properly locked) file position or no pos pointer at all is the right model (ie I'd really like to get rid of the mixed case), but there might be some practical problem that makes it impractical. Because the *real* problem with the mixed case is not "insane people who do bad things might get odd results". No, the real problem with the mixed case is that it could be a security issue (ie: one process intentionally changes the file position just as another process is going a 'read' and then avoids some limit test because the limit test was done using the old 'pos' value but the actual IO was done using the new one). So I suspect that we will have to either - get rid of the mixed case entirely (and do only properly locked f_pos accesses or pass is a NULL f_pos) - continue to support the mixed case, but also continue to support the nasty temporary 'pos' value with that file_pos_{read,write}() thing. IOW, I would not be ok with passing in a shared - and unlocked - &file->f_pos value to random drivers that *might* do odd things when a race happens. Linus
On Sat, Apr 13, 2019 at 10:27:00AM -0700, Linus Torvalds wrote: > And I'd really like to have people (Al?) look at this and go "yeah, > makes sense". I do think that moving to a model where we wither have a > (properly locked) file position or no pos pointer at all is the right > model (ie I'd really like to get rid of the mixed case), but there > might be some practical problem that makes it impractical. > > Because the *real* problem with the mixed case is not "insane people > who do bad things might get odd results". No, the real problem with > the mixed case is that it could be a security issue (ie: one process > intentionally changes the file position just as another process is > going a 'read' and then avoids some limit test because the limit test > was done using the old 'pos' value but the actual IO was done using > the new one). > > So I suspect that we will have to either > > - get rid of the mixed case entirely (and do only properly locked > f_pos accesses or pass is a NULL f_pos) Good luck. Character devices get no exclusion often do use position are many might be buried behind several layers of indirection ... and often left unmaintained for a decade or two IOW, I don't see how you'd go about eliminating the mixed case...
On Sat, Apr 13, 2019 at 10:27:00AM -0700, Linus Torvalds wrote: > On Sat, Apr 13, 2019 at 9:55 AM Kirill Smelkov <kirr@nexedi.com> wrote: > > > > --- 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; > > This part looks silly. We should just avoid all the position overflow > games when we don't have a position at all (ie streaming). You can't > overflow what you don't use. > > Similarly, you can't use ranged mandatory locking on a stream, so the > mandatory locking thing seems dependent on pos too. > > So I think that with a NULL ppos being possible, we should just change > the code to just do all of that conditionally on having a position, > rather than saying that the position of a stream is always 0. Linus, thanks for feedback. After checking a bit of history of rw_verify_area and seeing what it does I agree with you. I was initially confused because rw_verify_area calls security_file_permission() at its tail and so I thought we cannot skip rw_verify_area for !ppos case. After studying a bit I see now that there are several things going on and indeed moving ranged locks checking into under `if ppos!=NULL` makes sense. This code is new to me; apologize for not getting it right initially. > That said, this whole "let's make it possible to not have a position > at all" is a big change, and there's no way I'll apply these before > the 5.2 merge window. Ok. > And I'd really like to have people (Al?) look at this and go "yeah, > makes sense". I do think that moving to a model where we wither have a > (properly locked) file position or no pos pointer at all is the right > model (ie I'd really like to get rid of the mixed case), but there > might be some practical problem that makes it impractical. > > Because the *real* problem with the mixed case is not "insane people > who do bad things might get odd results". No, the real problem with > the mixed case is that it could be a security issue (ie: one process > intentionally changes the file position just as another process is > going a 'read' and then avoids some limit test because the limit test > was done using the old 'pos' value but the actual IO was done using > the new one). > > So I suspect that we will have to either > > - get rid of the mixed case entirely (and do only properly locked > f_pos accesses or pass is a NULL f_pos) > > - continue to support the mixed case, but also continue to support > the nasty temporary 'pos' value with that file_pos_{read,write}() > thing. > > IOW, I would not be ok with passing in a shared - and unlocked - > &file->f_pos value to random drivers that *might* do odd things when a > race happens. I see. That's why I splitted the whole change into 2: the first patch only adds ppos=NULL semantic for FMODE_STREAM and does not change to passing &file->f_pos directly. There are signs that people can be starting to use stream_open even now - before we do mass conversion. That's why it is better to fix ppos semantic for stream case early. You said it won't happen before next merge window - ok, but ppos=NULL change is separate and does not depend on passing &file->f_pos directly. So let's please apply ppos=NULL patch early when merge window begins and independently of what happens with whether we'll switch to direct usage of &file->f_pos. Looking forward to get review from others too. And I appreciate if people could help at least somehow with "getting rid of mixed case entirely" (i.e. always lock f_pos_lock on !FMODE_STREAM), because this transition starts to diverge from my particular use-case too far. To me it makes sense to do that transition as follows: - convert nonseekable_open -> stream_open via stream_open.cocci; - audit other nonseekable_open calls and convert left users that truly don't depend on position to stream_open; - extend stream_open.cocci to analyze alloc_file_pseudo as well (this will cover pipes and sockets), or maybe convert pipes and sockets to FMODE_STREAM manually; - extend stream_open.cocci to analyze file_operations that use no_llseek or noop_llseek, but do not use nonseekable_open or alloc_file_pseudo. This might find files that have stream semantic but are opened differently; - extend stream_open.cocci to analyze file_operations whose .read/.write do not use ppos at all (independently of how file was opened); - ... - after that remove FMODE_ATOMIC_POS and always take f_pos_lock if !FMODE_STREAM; - gather bug reports for deadlocked read/write and convert missed cases to FMODE_STREAM, probably extending stream_open.cocci along the road to catch similar cases. i.e. always take f_pos_lock unless a file is explicitly marked as being stream, and try to find and cover all files that are streams. Updated patch for ppos=NULL for stream case with rw_verify_area change fixed is below. Kirill ---- 8<---- From 3df9bc6c8ec58baec37bf413e2c0cef07308988f Mon Sep 17 00:00:00 2001 From: Kirill Smelkov <kirr@nexedi.com> Date: Fri, 12 Apr 2019 12:31:57 +0300 Subject: [PATCH v2 1/2] vfs: pass ppos=NULL to .read()/.write() of FMODE_STREAM files This amends commit 10dce8af3422 ("fs: stream_open - opener for stream-like files so that read and write can run simultaneously without deadlock") in how position is passed into .read()/.write() handler for stream-like files: Rasmus noticed that we currently pass 0 as position and ignore any position change if that is done by a file implementation. This papers over bugs if ppos is used in files that declare themselves as being stream-like as such bugs will go unnoticed. Even if a file implementation is correctly converted into using stream_open, its read/write later could be changed to use ppos and even though that won't be working correctly, that bug might go unnoticed without someone doing wrong behaviour analysis. It is thus better to pass ppos=NULL into read/write for stream-like files as that don't give any chance for ppos usage bugs because it will oops if ppos is ever used inside .read() or .write(). Note 1: rw_verify_area, new_sync_{read,write} needs to be updated because they are called by vfs_read/vfs_write & friends before file_operations .read/.write . Note 2: if file backend uses new-style .read_iter/.write_iter, position is still passed into there as non-pointer kiocb.ki_pos . Currently stream_open.cocci (semantic patch added by 10dce8af3422) ignores files whose file_operations has *_iter methods. Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Kirill Smelkov <kirr@nexedi.com> --- v2: fixed ppos handling in rw_verify_area per Linus comment. fs/open.c | 5 ++- fs/read_write.c | 113 ++++++++++++++++++++++++++++-------------------- 2 files changed, 70 insertions(+), 48 deletions(-) diff --git a/fs/open.c b/fs/open.c index a00350018a47..9c7d724a6f67 100644 --- a/fs/open.c +++ b/fs/open.c @@ -1219,8 +1219,9 @@ EXPORT_SYMBOL(nonseekable_open); /* * stream_open is used by subsystems that want stream-like file descriptors. * Such file descriptors are not seekable and don't have notion of position - * (file.f_pos is always 0). Contrary to file descriptors of other regular - * files, .read() and .write() can run simultaneously. + * (file.f_pos is always 0 and ppos passed to .read()/.write() is always NULL). + * Contrary to file descriptors of other regular files, .read() and .write() + * can run simultaneously. * * stream_open never fails and is marked to return int so that it could be * directly used as file_operations.open . diff --git a/fs/read_write.c b/fs/read_write.c index 61b43ad7608e..c543d965e288 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -365,29 +365,37 @@ SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high, int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count) { struct inode *inode; - loff_t pos; int retval = -EINVAL; inode = file_inode(file); if (unlikely((ssize_t) count < 0)) return retval; - pos = *ppos; - if (unlikely(pos < 0)) { - if (!unsigned_offsets(file)) - return retval; - if (count >= -pos) /* both values are in 0..LLONG_MAX */ - return -EOVERFLOW; - } else if (unlikely((loff_t) (pos + count) < 0)) { - if (!unsigned_offsets(file)) - return retval; - } - if (unlikely(inode->i_flctx && mandatory_lock(inode))) { - retval = locks_mandatory_area(inode, file, pos, pos + count - 1, - read_write == READ ? F_RDLCK : F_WRLCK); - if (retval < 0) - return retval; + /* + * ranged mandatory locking does not apply to streams - it makes sense + * only for files where position has a meaning. + */ + if (ppos) { + loff_t pos = *ppos; + + if (unlikely(pos < 0)) { + if (!unsigned_offsets(file)) + return retval; + if (count >= -pos) /* both values are in 0..LLONG_MAX */ + return -EOVERFLOW; + } else if (unlikely((loff_t) (pos + count) < 0)) { + if (!unsigned_offsets(file)) + return retval; + } + + if (unlikely(inode->i_flctx && mandatory_lock(inode))) { + retval = locks_mandatory_area(inode, file, pos, pos + count - 1, + read_write == READ ? F_RDLCK : F_WRLCK); + if (retval < 0) + return retval; + } } + return security_file_permission(file, read_write == READ ? MAY_READ : MAY_WRITE); } @@ -400,12 +408,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 +477,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; } @@ -558,15 +567,10 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_ return ret; } -static inline loff_t file_pos_read(struct file *file) -{ - return file->f_mode & FMODE_STREAM ? 0 : file->f_pos; -} - -static inline void file_pos_write(struct file *file, loff_t pos) +/* file_ppos returns &file->f_pos or NULL if file is stream */ +static inline loff_t *file_ppos(struct file *file) { - if ((file->f_mode & FMODE_STREAM) == 0) - file->f_pos = pos; + return file->f_mode & FMODE_STREAM ? NULL : &file->f_pos; } ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count) @@ -575,10 +579,14 @@ ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count) ssize_t ret = -EBADF; if (f.file) { - loff_t pos = file_pos_read(f.file); - ret = vfs_read(f.file, buf, count, &pos); - if (ret >= 0) - file_pos_write(f.file, pos); + loff_t pos, *ppos = file_ppos(f.file); + if (ppos) { + pos = *ppos; + ppos = &pos; + } + ret = vfs_read(f.file, buf, count, ppos); + if (ret >= 0 && ppos) + f.file->f_pos = pos; fdput_pos(f); } return ret; @@ -595,10 +603,14 @@ ssize_t ksys_write(unsigned int fd, const char __user *buf, size_t count) ssize_t ret = -EBADF; if (f.file) { - loff_t pos = file_pos_read(f.file); - ret = vfs_write(f.file, buf, count, &pos); - if (ret >= 0) - file_pos_write(f.file, pos); + loff_t pos, *ppos = file_ppos(f.file); + if (ppos) { + pos = *ppos; + ppos = &pos; + } + ret = vfs_write(f.file, buf, count, ppos); + if (ret >= 0 && ppos) + f.file->f_pos = pos; fdput_pos(f); } @@ -673,14 +685,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; } @@ -1013,10 +1026,14 @@ static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec, ssize_t ret = -EBADF; if (f.file) { - loff_t pos = file_pos_read(f.file); - ret = vfs_readv(f.file, vec, vlen, &pos, flags); - if (ret >= 0) - file_pos_write(f.file, pos); + loff_t pos, *ppos = file_ppos(f.file); + if (ppos) { + pos = *ppos; + ppos = &pos; + } + ret = vfs_readv(f.file, vec, vlen, ppos, flags); + if (ret >= 0 && ppos) + f.file->f_pos = pos; fdput_pos(f); } @@ -1033,10 +1050,14 @@ static ssize_t do_writev(unsigned long fd, const struct iovec __user *vec, ssize_t ret = -EBADF; if (f.file) { - loff_t pos = file_pos_read(f.file); - ret = vfs_writev(f.file, vec, vlen, &pos, flags); - if (ret >= 0) - file_pos_write(f.file, pos); + loff_t pos, *ppos = file_ppos(f.file); + if (ppos) { + pos = *ppos; + ppos = &pos; + } + ret = vfs_writev(f.file, vec, vlen, ppos, flags); + if (ret >= 0 && ppos) + f.file->f_pos = pos; fdput_pos(f); }
diff --git a/fs/open.c b/fs/open.c index a00350018a47..9c7d724a6f67 100644 --- a/fs/open.c +++ b/fs/open.c @@ -1219,8 +1219,9 @@ EXPORT_SYMBOL(nonseekable_open); /* * stream_open is used by subsystems that want stream-like file descriptors. * Such file descriptors are not seekable and don't have notion of position - * (file.f_pos is always 0). Contrary to file descriptors of other regular - * files, .read() and .write() can run simultaneously. + * (file.f_pos is always 0 and ppos passed to .read()/.write() is always NULL). + * Contrary to file descriptors of other regular files, .read() and .write() + * can run simultaneously. * * stream_open never fails and is marked to return int so that it could be * directly used as file_operations.open . diff --git a/fs/read_write.c b/fs/read_write.c index 61b43ad7608e..d62556be6848 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; } @@ -558,15 +559,10 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_ return ret; } -static inline loff_t file_pos_read(struct file *file) -{ - return file->f_mode & FMODE_STREAM ? 0 : file->f_pos; -} - -static inline void file_pos_write(struct file *file, loff_t pos) +/* file_ppos returns &file->f_pos or NULL if file is stream */ +static inline loff_t *file_ppos(struct file *file) { - if ((file->f_mode & FMODE_STREAM) == 0) - file->f_pos = pos; + return file->f_mode & FMODE_STREAM ? NULL : &file->f_pos; } ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count) @@ -575,10 +571,14 @@ ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count) ssize_t ret = -EBADF; if (f.file) { - loff_t pos = file_pos_read(f.file); - ret = vfs_read(f.file, buf, count, &pos); - if (ret >= 0) - file_pos_write(f.file, pos); + loff_t pos, *ppos = file_ppos(f.file); + if (ppos) { + pos = *ppos; + ppos = &pos; + } + ret = vfs_read(f.file, buf, count, ppos); + if (ret >= 0 && ppos) + f.file->f_pos = pos; fdput_pos(f); } return ret; @@ -595,10 +595,14 @@ ssize_t ksys_write(unsigned int fd, const char __user *buf, size_t count) ssize_t ret = -EBADF; if (f.file) { - loff_t pos = file_pos_read(f.file); - ret = vfs_write(f.file, buf, count, &pos); - if (ret >= 0) - file_pos_write(f.file, pos); + loff_t pos, *ppos = file_ppos(f.file); + if (ppos) { + pos = *ppos; + ppos = &pos; + } + ret = vfs_write(f.file, buf, count, ppos); + if (ret >= 0 && ppos) + f.file->f_pos = pos; fdput_pos(f); } @@ -673,14 +677,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; } @@ -1013,10 +1018,14 @@ static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec, ssize_t ret = -EBADF; if (f.file) { - loff_t pos = file_pos_read(f.file); - ret = vfs_readv(f.file, vec, vlen, &pos, flags); - if (ret >= 0) - file_pos_write(f.file, pos); + loff_t pos, *ppos = file_ppos(f.file); + if (ppos) { + pos = *ppos; + ppos = &pos; + } + ret = vfs_readv(f.file, vec, vlen, ppos, flags); + if (ret >= 0 && ppos) + f.file->f_pos = pos; fdput_pos(f); } @@ -1033,10 +1042,14 @@ static ssize_t do_writev(unsigned long fd, const struct iovec __user *vec, ssize_t ret = -EBADF; if (f.file) { - loff_t pos = file_pos_read(f.file); - ret = vfs_writev(f.file, vec, vlen, &pos, flags); - if (ret >= 0) - file_pos_write(f.file, pos); + loff_t pos, *ppos = file_ppos(f.file); + if (ppos) { + pos = *ppos; + ppos = &pos; + } + ret = vfs_writev(f.file, vec, vlen, ppos, flags); + if (ret >= 0 && ppos) + f.file->f_pos = pos; fdput_pos(f); }
This amends commit 10dce8af3422 ("fs: stream_open - opener for stream-like files so that read and write can run simultaneously without deadlock") in how position is passed into .read()/.write() handler for stream-like files: Rasmus noticed that we currently pass 0 as position and ignore any position change if that is done by a file implementation. This papers over bugs if ppos is used in files that declare themselves as being stream-like as such bugs will go unnoticed. Even if a file implementation is correctly converted into using stream_open, its read/write later could be changed to use ppos and even though that won't be working correctly, that bug might go unnoticed without someone doing wrong behaviour analysis. It is thus better to pass ppos=NULL into read/write for stream-like files as that don't give any chance for ppos usage bugs because it will oops if ppos is ever used inside .read() or .write(). Note 1: rw_verify_area, new_sync_{read,write} needs to be updated because they are called by vfs_read/vfs_write & friends before file_operations .read/.write . Note 2: if file backend uses new-style .read_iter/.write_iter, position is still passed into there as non-pointer kiocb.ki_pos . Currently stream_open.cocci (semantic patch added by 10dce8af3422) ignores files whose file_operations has *_iter methods. Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Kirill Smelkov <kirr@nexedi.com> --- fs/open.c | 5 ++-- fs/read_write.c | 75 +++++++++++++++++++++++++++++-------------------- 2 files changed, 47 insertions(+), 33 deletions(-)