There are many places inside vfs/fs where code flow depends on file->f_flags,
but this check is racy because one can change it via fcntl(,F_SETFL,)
For example O_DIRECT usually flag checked twice:
xxx_file_write_iter -> check O_DIRECT, and perform some optimizations
->__generic_file_write_iter -> check O_DIRECT,
which may break things: for example http://www.spinics.net/lists/linux-ext4/msg45683.html
For that reason some filesystems simply do not use __generic_file_write_iter()
wihch result in code duplication. Right way to fix this is to save volatile flags
inside kiocb->ki_flags similar to ->ki_pos
Other private discussion: message-id:[email protected]
TOC:
##First two patches introduce helpers and update generic code
kiocb_flags-v1/0001-fs-save-file-f_flags-to-kiocb-ki_flags.patch
kiocb_flags-v1/0002-vfs-check-kiocb-ki_flags-instead-filp-fl_flags.patch
## Switch filesystems to kiocb->ki_flags
kiocb_flags-v1/0003-ext4-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0004-9p-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0005-btrfs-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0006-ceph-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0007-cifs-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0008-gfs2-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0009-nfs-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0010-ntfs-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0011-ocfs2-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0012-udf-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0013-xfs-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0014-fuse-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
## Fix pipe and splice issues caused by race with fcntl(,F_SETFL,)
kiocb_flags-v1/0015-pipe-use-is_xxx_kiocb-instead-of-filp-fl_flags.patch
kiocb_flags-v1/0016-splice-fix-race-beween-splice_write-vs-fcntl-F_SETFL.patch
Patch set survived basic run of xfstests
There are many places inside vfs/fs where code flow depends on file->f_flags,
but this check is racy because one can change it via fcntl(,F_SETFL,)
For example O_DIRECT usually flag checked twice:
xxx_file_write_iter -> check O_DIRECT, and perform some optimization
->__generic_file_write_iter -> check O_DIRECT,
which may break things: for example http://www.spinics.net/lists/linux-ext4/msg45683.html
For that reason some filesystems simply do not use __generic_file_write_iter()
which result in code duplication. Right way to fix this is to save volatile flags
inside kiocb->ki_flags similar to ->ki_pos
Other private discussion: message-id:[email protected]
This patch store O_DIRECT|O_APPEND|O_NONBLOCK|O_NDELAY
to kiocb->ki_flags on kiocb initialization.
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/aio.c | 7 ++++---
fs/read_write.c | 20 ++++++++++++++++++++
include/linux/fs.h | 30 +++++++++++++++++++++++++++---
3 files changed, 51 insertions(+), 6 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index 3b8467a..f58c4d6 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1482,6 +1482,7 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
struct iocb *iocb, bool compat)
{
struct aio_kiocb *req;
+ struct file* filp;
ssize_t ret;
/* enforce forwards compatibility on users */
@@ -1504,14 +1505,14 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
if (unlikely(!req))
return -EAGAIN;
- req->common.ki_filp = fget(iocb->aio_fildes);
- if (unlikely(!req->common.ki_filp)) {
+ filp = fget(iocb->aio_fildes);
+ if (unlikely(!filp)) {
ret = -EBADF;
goto out_put_req;
}
+ kiocb_init_file(&req->common, filp);
req->common.ki_pos = iocb->aio_offset;
req->common.ki_complete = aio_complete;
- req->common.ki_flags = 0;
if (iocb->aio_flags & IOCB_FLAG_RESFD) {
/*
diff --git a/fs/read_write.c b/fs/read_write.c
index 69128b3..00e1ca4 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -41,6 +41,26 @@ static inline int unsigned_offsets(struct file *file)
return file->f_mode & FMODE_UNSIGNED_OFFSET;
}
+void kiocb_init_file(struct kiocb *kiocb, struct file *filp)
+{
+ kiocb->ki_flags = 0;
+ kiocb->ki_filp = filp;
+
+ /* Socket aio */
+ if (kiocb->ki_filp == NULL)
+ return;
+
+ if (filp->f_flags & O_APPEND)
+ kiocb->ki_flags |= IOCB_APPEND;
+ if (filp->f_flags & O_NONBLOCK)
+ kiocb->ki_flags |= IOCB_NONBLOCK;
+ if (filp->f_flags & O_NDELAY)
+ kiocb->ki_flags |= IOCB_NDELAY;
+ if (filp->f_flags & O_DIRECT)
+ kiocb->ki_flags |= IOCB_DIRECT;
+}
+EXPORT_SYMBOL(kiocb_init_file);
+
/**
* vfs_setpos - update the file offset for lseek
* @file: file structure in question
diff --git a/include/linux/fs.h b/include/linux/fs.h
index dfbd88a..4c20030 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -315,6 +315,10 @@ struct address_space;
struct writeback_control;
#define IOCB_EVENTFD (1 << 0)
+#define IOCB_APPEND (1 << 1)
+#define IOCB_NONBLOCK (1 << 2)
+#define IOCB_NDELAY (1 << 3)
+#define IOCB_DIRECT (1 << 4)
struct kiocb {
struct file *ki_filp;
@@ -329,11 +333,11 @@ static inline bool is_sync_kiocb(struct kiocb *kiocb)
return kiocb->ki_complete == NULL;
}
+extern void kiocb_init_file(struct kiocb *kiocb, struct file *filp);
static inline void init_sync_kiocb(struct kiocb *kiocb, struct file *filp)
{
- *kiocb = (struct kiocb) {
- .ki_filp = filp,
- };
+ memset(kiocb, 0 , sizeof(*kiocb));
+ kiocb_init_file(kiocb, filp);
}
/*
@@ -2776,6 +2780,26 @@ extern int generic_show_options(struct seq_file *m, struct dentry *root);
extern void save_mount_options(struct super_block *sb, char *options);
extern void replace_mount_options(struct super_block *sb, char *options);
+static inline bool is_append_kiocb(struct kiocb *kiocb)
+{
+ return kiocb->ki_flags & IOCB_APPEND;
+}
+
+static inline bool is_direct_kiocb(struct kiocb *kiocb)
+{
+ return (kiocb->ki_flags & IOCB_DIRECT) |
+ IS_DAX(file_inode(kiocb->ki_filp));
+
+}
+
+
+static inline bool is_nonblock_kiocb(struct kiocb *kiocb)
+{
+ return kiocb->ki_flags & IOCB_NONBLOCK;
+}
+
+/* XXX: this is obsolete helper, and will be removed soon.
+ * One should use io_direct_kiocb() instead */
static inline bool io_is_direct(struct file *filp)
{
return (filp->f_flags & O_DIRECT) || IS_DAX(file_inode(filp));
--
1.7.1
generic_write_checks now accept kiocb as an argument
Unfortunetly it is impossible to get rid of old interface because some crappy
do not support write_iter interface so leave __generic_write_checks as backward
compatibility helper.
Signed-off-by: Dmitry Monakhov <[email protected]>
---
include/linux/fs.h | 8 +++++++-
mm/filemap.c | 13 +++++++------
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4c20030..992685e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2570,7 +2570,7 @@ extern int sb_min_blocksize(struct super_block *, int);
extern int generic_file_mmap(struct file *, struct vm_area_struct *);
extern int generic_file_readonly_mmap(struct file *, struct vm_area_struct *);
-int generic_write_checks(struct file *file, loff_t *pos, size_t *count, int isblk);
+int __generic_write_checks(struct file * file, loff_t *pos, size_t *count, int isblk, int append);
extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);
@@ -2798,6 +2798,12 @@ static inline bool is_nonblock_kiocb(struct kiocb *kiocb)
return kiocb->ki_flags & IOCB_NONBLOCK;
}
+static inline int generic_write_checks(struct kiocb *iocb, loff_t *pos, size_t *count, int isblk)
+{
+ return __generic_write_checks(iocb->ki_filp, pos, count, isblk,
+ is_append_kiocb(iocb));
+}
+
/* XXX: this is obsolete helper, and will be removed soon.
* One should use io_direct_kiocb() instead */
static inline bool io_is_direct(struct file *filp)
diff --git a/mm/filemap.c b/mm/filemap.c
index 876f4e6..b519824 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1694,7 +1694,7 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
loff_t *ppos = &iocb->ki_pos;
loff_t pos = *ppos;
- if (io_is_direct(file)) {
+ if (is_direct_kiocb(iocb)) {
struct address_space *mapping = file->f_mapping;
struct inode *inode = mapping->host;
size_t count = iov_iter_count(iter);
@@ -2260,7 +2260,8 @@ EXPORT_SYMBOL(read_cache_page_gfp);
* Returns appropriate error code that caller should return or
* zero in case that write should be allowed.
*/
-inline int generic_write_checks(struct file *file, loff_t *pos, size_t *count, int isblk)
+inline int __generic_write_checks(struct file *file, loff_t *pos, size_t *count,
+ int isblk, int is_append)
{
struct inode *inode = file->f_mapping->host;
unsigned long limit = rlimit(RLIMIT_FSIZE);
@@ -2270,7 +2271,7 @@ inline int generic_write_checks(struct file *file, loff_t *pos, size_t *count, i
if (!isblk) {
/* FIXME: this is for backwards compatibility with 2.4 */
- if (file->f_flags & O_APPEND)
+ if (is_append)
*pos = i_size_read(inode);
if (limit != RLIM_INFINITY) {
@@ -2333,7 +2334,7 @@ inline int generic_write_checks(struct file *file, loff_t *pos, size_t *count, i
}
return 0;
}
-EXPORT_SYMBOL(generic_write_checks);
+EXPORT_SYMBOL(__generic_write_checks);
int pagecache_write_begin(struct file *file, struct address_space *mapping,
loff_t pos, unsigned len, unsigned flags,
@@ -2565,7 +2566,7 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
/* We can write back this queue in page reclaim */
current->backing_dev_info = inode_to_bdi(inode);
- err = generic_write_checks(file, &pos, &count, S_ISBLK(inode->i_mode));
+ err = generic_write_checks(iocb, &pos, &count, S_ISBLK(inode->i_mode));
if (err)
goto out;
@@ -2582,7 +2583,7 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (err)
goto out;
- if (io_is_direct(file)) {
+ if (is_direct_kiocb(iocb)) {
loff_t endbyte;
written = generic_file_direct_write(iocb, from, pos);
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/ext4/file.c | 12 +++++-------
1 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 598abbb..27cf1cc 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -95,7 +95,6 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
struct inode *inode = file_inode(iocb->ki_filp);
struct mutex *aio_mutex = NULL;
struct blk_plug plug;
- int o_direct = io_is_direct(file);
int overwrite = 0;
size_t length = iov_iter_count(from);
ssize_t ret;
@@ -105,18 +104,17 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
* Unaligned direct AIO must be serialized; see comment above
* In the case of O_APPEND, assume that we must always serialize
*/
- if (o_direct &&
+ if (is_direct_kiocb(iocb) &&
ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
!is_sync_kiocb(iocb) &&
- (file->f_flags & O_APPEND ||
- ext4_unaligned_aio(inode, from, pos))) {
+ (is_append_kiocb(iocb) || ext4_unaligned_aio(inode, from, pos))) {
aio_mutex = ext4_aio_mutex(inode);
mutex_lock(aio_mutex);
ext4_unwritten_wait(inode);
}
mutex_lock(&inode->i_mutex);
- if (file->f_flags & O_APPEND)
+ if (is_append_kiocb(iocb))
iocb->ki_pos = pos = i_size_read(inode);
/*
@@ -138,7 +136,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
}
iocb->private = &overwrite;
- if (o_direct) {
+ if (is_direct_kiocb(iocb)) {
blk_start_plug(&plug);
@@ -182,7 +180,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (err < 0)
ret = err;
}
- if (o_direct)
+ if (is_direct_kiocb(iocb))
blk_finish_plug(&plug);
errout:
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/9p/vfs_file.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index b401337..c2a120a 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -509,7 +509,8 @@ v9fs_file_write(struct file *filp, const char __user * data,
loff_t origin = *offset;
- retval = generic_write_checks(filp, &origin, &count, 0);
+ retval = __generic_write_checks(filp, &origin, &count, 0,
+ filp->f_flags & O_APPEND);
if (retval)
goto out;
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/btrfs/file.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index aee18f8..4dc3856 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1747,7 +1747,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
mutex_lock(&inode->i_mutex);
current->backing_dev_info = inode_to_bdi(inode);
- err = generic_write_checks(file, &pos, &count, S_ISBLK(inode->i_mode));
+ err = generic_write_checks(iocb, &pos, &count, S_ISBLK(inode->i_mode));
if (err) {
mutex_unlock(&inode->i_mutex);
goto out;
@@ -1800,7 +1800,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
if (sync)
atomic_inc(&BTRFS_I(inode)->sync_writers);
- if (file->f_flags & O_DIRECT) {
+ if (is_direct_kiocb(iocb)) {
num_written = __btrfs_direct_write(iocb, from, pos);
} else {
num_written = __btrfs_buffered_write(file, from, pos);
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/ceph/file.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 139f2fe..3ac67bf 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -442,7 +442,7 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *i,
dout("sync_read on file %p %llu~%u %s\n", file, off,
(unsigned)len,
- (file->f_flags & O_DIRECT) ? "O_DIRECT" : "");
+ (is_direct_kiocb(iocb)) ? "O_DIRECT" : "");
if (!len)
return 0;
@@ -457,7 +457,7 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *i,
if (ret < 0)
return ret;
- if (file->f_flags & O_DIRECT) {
+ if (is_direct_kiocb(iocb)) {
while (iov_iter_count(i)) {
size_t start;
ssize_t n;
@@ -828,8 +828,7 @@ again:
return ret;
if ((got & (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) == 0 ||
- (iocb->ki_filp->f_flags & O_DIRECT) ||
- (fi->flags & CEPH_F_SYNC)) {
+ is_direct_kiocb(iocb) || (fi->flags & CEPH_F_SYNC)) {
dout("aio_sync_read %p %llx.%llx %llu~%u got cap refs on %s\n",
inode, ceph_vinop(inode), iocb->ki_pos, (unsigned)len,
@@ -953,7 +952,7 @@ static ssize_t ceph_write_iter(struct kiocb *iocb, struct iov_iter *from)
/* We can write back this queue in page reclaim */
current->backing_dev_info = inode_to_bdi(inode);
- err = generic_write_checks(file, &pos, &count, S_ISBLK(inode->i_mode));
+ err = generic_write_checks(iocb, &pos, &count, S_ISBLK(inode->i_mode));
if (err)
goto out;
@@ -997,12 +996,12 @@ retry_snap:
inode, ceph_vinop(inode), pos, count, ceph_cap_string(got));
if ((got & (CEPH_CAP_FILE_BUFFER|CEPH_CAP_FILE_LAZYIO)) == 0 ||
- (file->f_flags & O_DIRECT) || (fi->flags & CEPH_F_SYNC)) {
+ is_direct_kiocb(iocb) || (fi->flags & CEPH_F_SYNC)) {
struct iov_iter data;
mutex_unlock(&inode->i_mutex);
/* we might need to revert back to that point */
data = *from;
- if (file->f_flags & O_DIRECT)
+ if (is_direct_kiocb(iocb))
written = ceph_sync_direct_write(iocb, &data, pos);
else
written = ceph_sync_write(iocb, &data, pos);
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/cifs/file.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index a94b3e6..25527e9 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2560,7 +2560,7 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from,
}
static ssize_t
-cifs_iovec_write(struct file *file, struct iov_iter *from, loff_t *poffset)
+cifs_iovec_write(struct kiocb *iocb, struct iov_iter *from, loff_t *poffset)
{
size_t len;
ssize_t total_written = 0;
@@ -2573,7 +2573,7 @@ cifs_iovec_write(struct file *file, struct iov_iter *from, loff_t *poffset)
int rc;
len = iov_iter_count(from);
- rc = generic_write_checks(file, poffset, &len, 0);
+ rc = generic_write_checks(iocb, poffset, &len, 0);
if (rc)
return rc;
@@ -2583,8 +2583,8 @@ cifs_iovec_write(struct file *file, struct iov_iter *from, loff_t *poffset)
iov_iter_truncate(from, len);
INIT_LIST_HEAD(&wdata_list);
- cifs_sb = CIFS_FILE_SB(file);
- open_file = file->private_data;
+ cifs_sb = CIFS_FILE_SB(iocb->ki_filp);
+ open_file = iocb->ki_filp->private_data;
tcon = tlink_tcon(open_file->tlink);
if (!tcon->ses->server->ops->async_writev)
@@ -2670,7 +2670,7 @@ ssize_t cifs_user_writev(struct kiocb *iocb, struct iov_iter *from)
* write request.
*/
- written = cifs_iovec_write(iocb->ki_filp, from, &pos);
+ written = cifs_iovec_write(iocb, from, &pos);
if (written > 0) {
set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags);
iocb->ki_pos = pos;
@@ -2696,7 +2696,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
*/
down_read(&cinode->lock_sem);
mutex_lock(&inode->i_mutex);
- if (file->f_flags & O_APPEND)
+ if (is_append_kiocb(iocb))
lock_pos = i_size_read(inode);
if (!cifs_find_lock_conflict(cfile, lock_pos, iov_iter_count(from),
server->vals->exclusive_lock_type, NULL,
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/gfs2/file.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index f6fc412..25da110 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -709,7 +709,7 @@ static ssize_t gfs2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
gfs2_size_hint(file, iocb->ki_pos, iov_iter_count(from));
- if (file->f_flags & O_APPEND) {
+ if (is_append_kiocb(iocb)) {
struct gfs2_holder gh;
ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/nfs/direct.c | 7 +++----
fs/nfs/file.c | 6 +++---
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index c3929fb..76950c6 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -964,8 +964,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter,
loff_t pos)
{
ssize_t result = -EINVAL;
- struct file *file = iocb->ki_filp;
- struct address_space *mapping = file->f_mapping;
+ struct address_space *mapping = iocb->ki_filp->f_mapping;
struct inode *inode = mapping->host;
struct nfs_direct_req *dreq;
struct nfs_lock_context *l_ctx;
@@ -976,9 +975,9 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb, struct iov_iter *iter,
nfs_add_stats(mapping->host, NFSIOS_DIRECTWRITTENBYTES, count);
dfprintk(FILE, "NFS: direct write(%pD2, %zd@%Ld)\n",
- file, count, (long long) pos);
+ iocb->ki_filp, count, (long long) pos);
- result = generic_write_checks(file, &pos, &count, 0);
+ result = generic_write_checks(iocb, &pos, &count, 0);
if (result)
goto out;
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 37b1558..709cb7f 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -170,7 +170,7 @@ nfs_file_read(struct kiocb *iocb, struct iov_iter *to)
struct inode *inode = file_inode(iocb->ki_filp);
ssize_t result;
- if (iocb->ki_filp->f_flags & O_DIRECT)
+ if (is_direct_kiocb(iocb))
return nfs_file_direct_read(iocb, to, iocb->ki_pos);
dprintk("NFS: read(%pD2, %zu@%lu)\n",
@@ -680,7 +680,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
if (result)
return result;
- if (file->f_flags & O_DIRECT)
+ if (is_direct_kiocb(iocb))
return nfs_file_direct_write(iocb, from, pos);
dprintk("NFS: write(%pD2, %zu@%Ld)\n",
@@ -692,7 +692,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
/*
* O_APPEND implies that we must revalidate the file length.
*/
- if (file->f_flags & O_APPEND) {
+ if (is_append_kiocb(iocb)) {
result = nfs_revalidate_file_size(inode, file);
if (result)
goto out;
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/ntfs/file.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
index c1da78d..648b3a4 100644
--- a/fs/ntfs/file.c
+++ b/fs/ntfs/file.c
@@ -345,7 +345,8 @@ static ssize_t ntfs_prepare_file_for_write(struct file *file, loff_t *ppos,
(unsigned long long)*ppos, (unsigned long)*count);
/* We can write back this queue in page reclaim. */
current->backing_dev_info = inode_to_bdi(vi);
- err = generic_write_checks(file, ppos, count, S_ISBLK(vi->i_mode));
+ err = __generic_write_checks(file, ppos, count, S_ISBLK(vi->i_mode),
+ file->f_flags & O_APPEND);
if (unlikely(err))
goto out;
/*
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/ocfs2/file.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index 266845d..c22b240 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2283,8 +2283,8 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
if (count == 0)
return 0;
- appending = file->f_flags & O_APPEND ? 1 : 0;
- direct_io = file->f_flags & O_DIRECT ? 1 : 0;
+ appending = is_append_kiocb(iocb);
+ direct_io = is_direct_kiocb(iocb);
mutex_lock(&inode->i_mutex);
@@ -2374,7 +2374,7 @@ relock:
/* communicate with ocfs2_dio_end_io */
ocfs2_iocb_set_rw_locked(iocb, rw_level);
- ret = generic_write_checks(file, ppos, &count,
+ ret = generic_write_checks(iocb, ppos, &count,
S_ISBLK(inode->i_mode));
if (ret)
goto out_dio;
@@ -2436,7 +2436,7 @@ relock:
out_dio:
/* buffered aio wouldn't have proper lock coverage today */
- BUG_ON(ret == -EIOCBQUEUED && !(file->f_flags & O_DIRECT));
+ BUG_ON(ret == -EIOCBQUEUED && !is_direct_kiocb(iocb));
if (((file->f_flags & O_DSYNC) && !direct_io) || IS_SYNC(inode) ||
((file->f_flags & O_DIRECT) && !direct_io)) {
@@ -2547,7 +2547,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
* buffered reads protect themselves in ->readpage(). O_DIRECT reads
* need locks to protect pending reads from racing with truncate.
*/
- if (filp->f_flags & O_DIRECT) {
+ if (is_direct_kiocb(iocb)) {
have_alloc_sem = 1;
ocfs2_iocb_set_sem_locked(iocb);
@@ -2581,7 +2581,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
trace_generic_file_aio_read_ret(ret);
/* buffered aio wouldn't have proper lock coverage today */
- BUG_ON(ret == -EIOCBQUEUED && !(filp->f_flags & O_DIRECT));
+ BUG_ON(ret == -EIOCBQUEUED && !is_direct_kiocb(iocb));
/* see ocfs2_file_write_iter */
if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/udf/file.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/udf/file.c b/fs/udf/file.c
index 7f885cc..78ccc0f 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -128,7 +128,7 @@ static ssize_t udf_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
mutex_lock(&inode->i_mutex);
down_write(&iinfo->i_data_sem);
if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
- if (file->f_flags & O_APPEND)
+ if (is_append_kiocb(iocb))
pos = inode->i_size;
else
pos = iocb->ki_pos;
--
1.7.1
Also function interface cleanup in order to vfs:write_iter interface agreeament
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/xfs/xfs_file.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index f44212f..148039b 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -279,7 +279,7 @@ xfs_file_read_iter(
XFS_STATS_INC(xs_read_calls);
- if (unlikely(file->f_flags & O_DIRECT))
+ if (unlikely(is_direct_kiocb(iocb)))
ioflags |= XFS_IO_ISDIRECT;
if (file->f_mode & FMODE_NOCMTIME)
ioflags |= XFS_IO_INVIS;
@@ -544,17 +544,18 @@ xfs_zero_eof(
*/
STATIC ssize_t
xfs_file_aio_write_checks(
- struct file *file,
+ struct kiocb *iocb,
loff_t *pos,
size_t *count,
int *iolock)
{
+ struct file *file = iocb->ki_filp;
struct inode *inode = file->f_mapping->host;
struct xfs_inode *ip = XFS_I(inode);
int error = 0;
restart:
- error = generic_write_checks(file, pos, count, S_ISBLK(inode->i_mode));
+ error = generic_write_checks(iocb, pos, count, S_ISBLK(inode->i_mode));
if (error)
return error;
@@ -678,7 +679,7 @@ xfs_file_dio_aio_write(
xfs_rw_ilock(ip, iolock);
}
- ret = xfs_file_aio_write_checks(file, &pos, &count, &iolock);
+ ret = xfs_file_aio_write_checks(iocb, &pos, &count, &iolock);
if (ret)
goto out;
iov_iter_truncate(from, count);
@@ -739,7 +740,7 @@ xfs_file_buffered_aio_write(
xfs_rw_ilock(ip, iolock);
- ret = xfs_file_aio_write_checks(file, &pos, &count, &iolock);
+ ret = xfs_file_aio_write_checks(iocb, &pos, &count, &iolock);
if (ret)
goto out;
@@ -803,7 +804,7 @@ xfs_file_write_iter(
if (XFS_FORCED_SHUTDOWN(ip->i_mount))
return -EIO;
- if (unlikely(file->f_flags & O_DIRECT))
+ if (unlikely(is_direct_kiocb(iocb)))
ret = xfs_file_dio_aio_write(iocb, from);
else
ret = xfs_file_buffered_aio_write(iocb, from);
--
1.7.1
Cc: [email protected]
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/fuse/file.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index ff102cb..ba8ad87 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1167,7 +1167,7 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
/* We can write back this queue in page reclaim */
current->backing_dev_info = inode_to_bdi(inode);
- err = generic_write_checks(file, &pos, &count, S_ISBLK(inode->i_mode));
+ err = generic_write_checks(iocb, &pos, &count, S_ISBLK(inode->i_mode));
if (err)
goto out;
@@ -1183,7 +1183,7 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (err)
goto out;
- if (file->f_flags & O_DIRECT) {
+ if (is_direct_kiocb(iocb)) {
written = generic_file_direct_write(iocb, from, pos);
if (written < 0 || !iov_iter_count(from))
goto out;
@@ -1421,7 +1421,8 @@ static ssize_t __fuse_direct_write(struct fuse_io_priv *io,
ssize_t res;
- res = generic_write_checks(file, ppos, &count, 0);
+ res = __generic_write_checks(file, ppos, &count, 0,
+ file->f_flags & O_APPEND);
if (!res) {
iov_iter_truncate(iter, count);
res = fuse_direct_io(io, iter, ppos, FUSE_DIO_WRITE);
--
1.7.1
Also fix other long standing issues caused by fcntl(,F_SETFL,):
- One can disable O_DIRECT for pipe[1] (paketized IO), but can not enable it again.
- Currently we do not set O_APPEND on pipe[1] (IMHO it is wrong, but let it be)
so it is reasonable to completely prohibit change O_APPEND flag on both
end's of pipe. Add ->check_flags method in order to diallow O_APPEND toggling.
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/fcntl.c | 6 ++++--
fs/pipe.c | 20 +++++++++++++++-----
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index ee85cd4..0bdc9c7 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -51,9 +51,11 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
if (arg & O_NDELAY)
arg |= O_NONBLOCK;
+ /* allowed only for inodes with ->direct_io method or write pipe */
if (arg & O_DIRECT) {
- if (!filp->f_mapping || !filp->f_mapping->a_ops ||
- !filp->f_mapping->a_ops->direct_IO)
+ if ((!filp->f_mapping || !filp->f_mapping->a_ops ||
+ !filp->f_mapping->a_ops->direct_IO) &&
+ !(get_pipe_info(filp) && (filp->f_flags | O_WRONLY)))
return -EINVAL;
}
diff --git a/fs/pipe.c b/fs/pipe.c
index 2d084f2..95b5fe4 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -301,7 +301,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
*/
if (ret)
break;
- if (filp->f_flags & O_NONBLOCK) {
+ if (is_nonblock_kiocb(iocb)) {
ret = -EAGAIN;
break;
}
@@ -329,9 +329,9 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
return ret;
}
-static inline int is_packetized(struct file *file)
+static inline int is_packetized(struct kiocb *kiocb)
{
- return (file->f_flags & O_DIRECT) != 0;
+ return is_direct_kiocb(kiocb);
}
static ssize_t
@@ -427,7 +427,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
buf->offset = 0;
buf->len = copied;
buf->flags = 0;
- if (is_packetized(filp)) {
+ if (is_packetized(iocb)) {
buf->ops = &packet_pipe_buf_ops;
buf->flags = PIPE_BUF_FLAG_PACKET;
}
@@ -439,7 +439,7 @@ pipe_write(struct kiocb *iocb, struct iov_iter *from)
}
if (bufs < pipe->buffers)
continue;
- if (filp->f_flags & O_NONBLOCK) {
+ if (is_nonblock_kiocb(iocb)) {
if (!ret)
ret = -EAGAIN;
break;
@@ -943,6 +943,15 @@ err:
return ret;
}
+/* XXX: Currently it is not possible distinguish read side from write one */
+static int pipe_check_flags(int flags)
+{
+ if (flags & O_APPEND)
+ return -EINVAL;
+
+ return 0;
+}
+
const struct file_operations pipefifo_fops = {
.open = fifo_open,
.llseek = no_llseek,
@@ -954,6 +963,7 @@ const struct file_operations pipefifo_fops = {
.unlocked_ioctl = pipe_ioctl,
.release = pipe_release,
.fasync = pipe_fasync,
+ .check_flags = pipe_check_flags,
};
/*
--
1.7.1
file->f_flags & O_APPEND is checked twice
-> do_splice_direct or do_splice: return EINVAL if O_APPEND enabled
-> generic_write_checks: seek to end in case of O_APPEND
This is obviously whong and result in unpredictable behaviour if raced with
fcntl. It is reasonable to recheck append flag after kiocb was constructed (
f_flags becomes stable), for that reason we should use special analog of
vfs_write_iter()
Signed-off-by: Dmitry Monakhov <[email protected]>
---
fs/splice.c | 23 ++++++++++++++++++++++-
1 files changed, 22 insertions(+), 1 deletions(-)
diff --git a/fs/splice.c b/fs/splice.c
index 41cbb16..7ac43db 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -922,6 +922,27 @@ ssize_t splice_from_pipe(struct pipe_inode_info *pipe, struct file *out,
return ret;
}
+ssize_t splice_iter_write(struct file *file, struct iov_iter *iter, loff_t *ppos)
+{
+ struct kiocb kiocb;
+ ssize_t ret;
+
+ if (!file->f_op->write_iter)
+ return -EINVAL;
+
+ init_sync_kiocb(&kiocb, file);
+ if (is_append_kiocb(&kiocb))
+ return -EINVAL;
+
+ kiocb.ki_pos = *ppos;
+ iter->type |= WRITE;
+ ret = file->f_op->write_iter(&kiocb, iter);
+ BUG_ON(ret == -EIOCBQUEUED);
+ if (ret > 0)
+ *ppos = kiocb.ki_pos;
+ return ret;
+}
+
/**
* iter_file_splice_write - splice data from a pipe to a file
@@ -1005,7 +1026,7 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out,
iov_iter_bvec(&from, ITER_BVEC | WRITE, array, n,
sd.total_len - left);
- ret = vfs_iter_write(out, &from, &sd.pos);
+ ret = splice_iter_write(out, &from, &sd.pos);
if (ret <= 0)
break;
--
1.7.1
On Sat, Apr 04, 2015 at 11:13:11PM +0400, Dmitry Monakhov wrote:
> generic_write_checks now accept kiocb as an argument
> Unfortunetly it is impossible to get rid of old interface because some crappy
> do not support write_iter interface so leave __generic_write_checks as backward
> compatibility helper.
Check the current vfs.git#for-next (there's even some generic_write_checks()
work in it). The same goes for the rest of the series. Please, rebase it.
What's more, generic_write_checks() should take iov_iter *, not the address
of something its ->count had been copied into. Note that all callers of that
thing end up doing iov_iter_truncate() pretty soon afterwards. I hadn't
pushed that one out yet (there is some weirdness in ocfs2 which might be
a bug; I want to sort that out first), but that's where it's going.
Al Viro <[email protected]> writes:
> On Sat, Apr 04, 2015 at 11:13:11PM +0400, Dmitry Monakhov wrote:
>> generic_write_checks now accept kiocb as an argument
>> Unfortunetly it is impossible to get rid of old interface because some crappy
>> do not support write_iter interface so leave __generic_write_checks as backward
>> compatibility helper.
>
> Check the current vfs.git#for-next (there's even some generic_write_checks()
> work in it). The same goes for the rest of the series. Please, rebase it.
Ok. Yes you right. I've prepared patches against vfs.git#for-next(48a0c62) but
it is too old. Will rebase. Also it is reasonable to fold my fs-xxx
conversion to one combo patch similar to (d04cfe7840)
>
> What's more, generic_write_checks() should take iov_iter *, not the address
> of something its ->count had been copied into. Note that all callers of that
> thing end up doing iov_iter_truncate() pretty soon afterwards. I hadn't
> pushed that one out yet (there is some weirdness in ocfs2 which might be
> a bug; I want to sort that out first), but that's where it's going.
I'm not sure I have get your point about ocfs2 because it does
iov_iter_truncate() right after generic_write_checks()
But nfs definitely has a bug because iter was not truncated after generic_write_checks
->nfs_file_direct_write
->generic_write_checks(file, &pos, &count);
dreq->bytes_left = count;
task_io_account_write(count);
->nfs_direct_write_schedule_iovec(dreq, iter, pos);
###Ignore dreq->bytes_left(ala count) and submit non truncated iter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Sun, Apr 05, 2015 at 02:03:22PM +0300, Dmitry Monakhov wrote:
> I'm not sure I have get your point about ocfs2 because it does
> iov_iter_truncate() right after generic_write_checks()
This
ret = ocfs2_prepare_inode_for_write(file, ppos, count, appending,
&can_do_direct, &has_refcount);
being done before generic_write_checks(). It actually duplicates some
parts of generic_write_checks() inside (O_APPEND-related, and AFAICS
they _are_ triggered twice that way).
On Sun, Apr 05, 2015 at 07:11:45PM +0100, Al Viro wrote:
> On Sun, Apr 05, 2015 at 02:03:22PM +0300, Dmitry Monakhov wrote:
>
> > I'm not sure I have get your point about ocfs2 because it does
> > iov_iter_truncate() right after generic_write_checks()
>
> This
> ret = ocfs2_prepare_inode_for_write(file, ppos, count, appending,
> &can_do_direct, &has_refcount);
> being done before generic_write_checks(). It actually duplicates some
> parts of generic_write_checks() inside (O_APPEND-related, and AFAICS
> they _are_ triggered twice that way).
XFS seems to be buggered as well:
/* DIO must be aligned to device logical sector size */
if ((pos | count) & target->bt_logical_sectormask)
return -EINVAL;
/* "unaligned" here means not aligned to a filesystem block */
if ((pos & mp->m_blockmask) || ((pos + count) & mp->m_blockmask))
unaligned_io = 1;
...
ret = xfs_file_aio_write_checks(file, &pos, &count, &iolock);
now, play with rlimit() and suddenly the alignment checks above have nothing
to do with what'll actually happen after that sucker - it's calling
generic_write_checks(), so...
Incidentally, we want the result of alignment check to decide how to take
the lock that protects the file size, so simply lifting O_APPEND treatment
above those won't do. I suspect that in case of lock taken shared we
need to redo alignment checks and treat "it became unaligned" as "unlock
and redo it with lock taken exclusive".
BTW, xfs_break_layouts() having dropped and regained lock would invalidate
the O_APPEND treatment in generic_write_checks() just prior (both in
xfs_file_aio_write_checks())...
Al "really not fond of xfs_rw_ilock()" Viro...
Hi,
On 04/04/15 20:13, Dmitry Monakhov wrote:
> Cc: [email protected]
> Signed-off-by: Dmitry Monakhov <[email protected]>
Acked-by: Steven Whitehouse <[email protected]>
Steve.
> ---
> fs/gfs2/file.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index f6fc412..25da110 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -709,7 +709,7 @@ static ssize_t gfs2_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>
> gfs2_size_hint(file, iocb->ki_pos, iov_iter_count(from));
>
> - if (file->f_flags & O_APPEND) {
> + if (is_append_kiocb(iocb)) {
> struct gfs2_holder gh;
>
> ret = gfs2_glock_nq_init(ip->i_gl, LM_ST_SHARED, 0, &gh);