Commit d22e022b authored by Kirill Smelkov's avatar Kirill Smelkov

vfs: pass ppos=NULL to .read()/.write() of FMODE_STREAM files

This amends commit 10dce8af ("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 10dce8af) ignores files
whose file_operations has *_iter methods.
Suggested-by: default avatarRasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Kirill Smelkov's avatarKirill Smelkov <kirr@nexedi.com>
(cherry picked from commit db234833)
parent 6407f44a
...@@ -1219,8 +1219,9 @@ EXPORT_SYMBOL(nonseekable_open); ...@@ -1219,8 +1219,9 @@ EXPORT_SYMBOL(nonseekable_open);
/* /*
* stream_open is used by subsystems that want stream-like file descriptors. * 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 * 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 * (file.f_pos is always 0 and ppos passed to .read()/.write() is always NULL).
* files, .read() and .write() can run simultaneously. * 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 * stream_open never fails and is marked to return int so that it could be
* directly used as file_operations.open . * directly used as file_operations.open .
......
...@@ -365,29 +365,37 @@ SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high, ...@@ -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) int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count)
{ {
struct inode *inode; struct inode *inode;
loff_t pos;
int retval = -EINVAL; int retval = -EINVAL;
inode = file_inode(file); inode = file_inode(file);
if (unlikely((ssize_t) count < 0)) if (unlikely((ssize_t) count < 0))
return retval; 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, * ranged mandatory locking does not apply to streams - it makes sense
read_write == READ ? F_RDLCK : F_WRLCK); * only for files where position has a meaning.
if (retval < 0) */
return retval; 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, return security_file_permission(file,
read_write == READ ? MAY_READ : MAY_WRITE); 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 ...@@ -400,12 +408,13 @@ static ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, lo
ssize_t ret; ssize_t ret;
init_sync_kiocb(&kiocb, filp); init_sync_kiocb(&kiocb, filp);
kiocb.ki_pos = *ppos; kiocb.ki_pos = (ppos ? *ppos : 0);
iov_iter_init(&iter, READ, &iov, 1, len); iov_iter_init(&iter, READ, &iov, 1, len);
ret = call_read_iter(filp, &kiocb, &iter); ret = call_read_iter(filp, &kiocb, &iter);
BUG_ON(ret == -EIOCBQUEUED); BUG_ON(ret == -EIOCBQUEUED);
*ppos = kiocb.ki_pos; if (ppos)
*ppos = kiocb.ki_pos;
return ret; return ret;
} }
...@@ -468,12 +477,12 @@ static ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t ...@@ -468,12 +477,12 @@ static ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t
ssize_t ret; ssize_t ret;
init_sync_kiocb(&kiocb, filp); init_sync_kiocb(&kiocb, filp);
kiocb.ki_pos = *ppos; kiocb.ki_pos = (ppos ? *ppos : 0);
iov_iter_init(&iter, WRITE, &iov, 1, len); iov_iter_init(&iter, WRITE, &iov, 1, len);
ret = call_write_iter(filp, &kiocb, &iter); ret = call_write_iter(filp, &kiocb, &iter);
BUG_ON(ret == -EIOCBQUEUED); BUG_ON(ret == -EIOCBQUEUED);
if (ret > 0) if (ret > 0 && ppos)
*ppos = kiocb.ki_pos; *ppos = kiocb.ki_pos;
return ret; return ret;
} }
...@@ -558,15 +567,10 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_ ...@@ -558,15 +567,10 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_
return ret; return ret;
} }
static inline loff_t file_pos_read(struct file *file) /* file_ppos returns &file->f_pos or NULL if file is stream */
{ static inline loff_t *file_ppos(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)
{ {
if ((file->f_mode & FMODE_STREAM) == 0) return file->f_mode & FMODE_STREAM ? NULL : &file->f_pos;
file->f_pos = pos;
} }
ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count) 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) ...@@ -575,10 +579,14 @@ ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count)
ssize_t ret = -EBADF; ssize_t ret = -EBADF;
if (f.file) { if (f.file) {
loff_t pos = file_pos_read(f.file); loff_t pos, *ppos = file_ppos(f.file);
ret = vfs_read(f.file, buf, count, &pos); if (ppos) {
if (ret >= 0) pos = *ppos;
file_pos_write(f.file, pos); ppos = &pos;
}
ret = vfs_read(f.file, buf, count, ppos);
if (ret >= 0 && ppos)
f.file->f_pos = pos;
fdput_pos(f); fdput_pos(f);
} }
return ret; return ret;
...@@ -595,10 +603,14 @@ ssize_t ksys_write(unsigned int fd, const char __user *buf, size_t count) ...@@ -595,10 +603,14 @@ ssize_t ksys_write(unsigned int fd, const char __user *buf, size_t count)
ssize_t ret = -EBADF; ssize_t ret = -EBADF;
if (f.file) { if (f.file) {
loff_t pos = file_pos_read(f.file); loff_t pos, *ppos = file_ppos(f.file);
ret = vfs_write(f.file, buf, count, &pos); if (ppos) {
if (ret >= 0) pos = *ppos;
file_pos_write(f.file, pos); ppos = &pos;
}
ret = vfs_write(f.file, buf, count, ppos);
if (ret >= 0 && ppos)
f.file->f_pos = pos;
fdput_pos(f); fdput_pos(f);
} }
...@@ -673,14 +685,15 @@ static ssize_t do_iter_readv_writev(struct file *filp, struct iov_iter *iter, ...@@ -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); ret = kiocb_set_rw_flags(&kiocb, flags);
if (ret) if (ret)
return ret; return ret;
kiocb.ki_pos = *ppos; kiocb.ki_pos = (ppos ? *ppos : 0);
if (type == READ) if (type == READ)
ret = call_read_iter(filp, &kiocb, iter); ret = call_read_iter(filp, &kiocb, iter);
else else
ret = call_write_iter(filp, &kiocb, iter); ret = call_write_iter(filp, &kiocb, iter);
BUG_ON(ret == -EIOCBQUEUED); BUG_ON(ret == -EIOCBQUEUED);
*ppos = kiocb.ki_pos; if (ppos)
*ppos = kiocb.ki_pos;
return ret; return ret;
} }
...@@ -1013,10 +1026,14 @@ static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec, ...@@ -1013,10 +1026,14 @@ static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec,
ssize_t ret = -EBADF; ssize_t ret = -EBADF;
if (f.file) { if (f.file) {
loff_t pos = file_pos_read(f.file); loff_t pos, *ppos = file_ppos(f.file);
ret = vfs_readv(f.file, vec, vlen, &pos, flags); if (ppos) {
if (ret >= 0) pos = *ppos;
file_pos_write(f.file, pos); ppos = &pos;
}
ret = vfs_readv(f.file, vec, vlen, ppos, flags);
if (ret >= 0 && ppos)
f.file->f_pos = pos;
fdput_pos(f); fdput_pos(f);
} }
...@@ -1033,10 +1050,14 @@ static ssize_t do_writev(unsigned long fd, const struct iovec __user *vec, ...@@ -1033,10 +1050,14 @@ static ssize_t do_writev(unsigned long fd, const struct iovec __user *vec,
ssize_t ret = -EBADF; ssize_t ret = -EBADF;
if (f.file) { if (f.file) {
loff_t pos = file_pos_read(f.file); loff_t pos, *ppos = file_ppos(f.file);
ret = vfs_writev(f.file, vec, vlen, &pos, flags); if (ppos) {
if (ret >= 0) pos = *ppos;
file_pos_write(f.file, pos); ppos = &pos;
}
ret = vfs_writev(f.file, vec, vlen, ppos, flags);
if (ret >= 0 && ppos)
f.file->f_pos = pos;
fdput_pos(f); fdput_pos(f);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment