Found an atomic open related fix in the 9p tree (b6f4bee02f); tured out, it
didn't fully fix 9p and introduced another bug in the process and also several
other filesystems are affected by the same bug. Then I reviewed all the atomic
open implementations and found more bugs. This series tries to address these.
Only tested the fuse fix, the others are compile tested only.
One issue that crops up several times, and which is not strictly related to
atomic open, is that a non-NULL return value of d_splice_alias() (or ->lookup(),
etc..) are not handled properly by filesystems. The reason for this is that a
non-NULL return gets very little testing (only if exported and even then not
easy to trigger). There should be some way to more easily expose such bugs or
improve the API so that these bugs are harder to introduce.
Thanks,
Miklos
---
Miklos Szeredi (11):
vfs: improve i_op->atomic_open() documentation
9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
9p: fix O_EXCL in v9fs_vfs_atomic_open()
fuse: fix O_EXCL in fuse_atomic_open()
cifs: fix filp leak in cifs_atomic_open()
gfs2: d_splice_alias() cant return error
gfs2: pass correct dentry to finish_open() in __gfs2_lookup()
gfs2: fix dentry leaks
gfs2: set FILE_CREATED
nfs: set FILE_CREATED
vfs: don't set FILE_CREATED before calling ->atomic_open()
---
Documentation/filesystems/vfs.txt | 14 +++++++-------
fs/9p/vfs_inode.c | 9 ++++++++-
fs/9p/vfs_inode_dotl.c | 9 +++++----
fs/cifs/dir.c | 1 +
fs/fuse/dir.c | 10 +++++++++-
fs/gfs2/inode.c | 40 ++++++++++++++++++++++-----------------
fs/namei.c | 11 ++++++++---
fs/nfs/dir.c | 3 +++
fs/open.c | 21 +++++++++++++++++---
9 files changed, 82 insertions(+), 36 deletions(-)
From: Miklos Szeredi <[email protected]>
If open flags has O_EXCL and dentry is positive after lookup then return
-EEXIST instead of "1".
This bug resulted in some O_EXCL opens succeeding (on a cache miss) despite
the file already existing.
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: [email protected]
---
fs/fuse/dir.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 62b43b5..694ec6b 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -523,9 +523,17 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
entry = res;
}
- if (!(flags & O_CREAT) || entry->d_inode)
+ if (!(flags & O_CREAT))
goto no_open;
+ if (entry->d_inode) {
+ err = -EEXIST;
+ if (flags & O_EXCL)
+ goto out_dput;
+
+ goto no_open;
+ }
+
/* Only creates */
*opened |= FILE_CREATED;
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
Set FILE_CREATED on O_CREAT|O_EXCL. If the NFS server honored our request
for exclusivity then this must be correct.
Currently this is a no-op, since the VFS sets FILE_CREATED anyway. The
next patch will, however, require this flag to be always set by
filesystems.
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Trond Myklebust <[email protected]>
---
fs/nfs/dir.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index de434f3..854a8f0 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1392,6 +1392,9 @@ static int nfs_finish_open(struct nfs_open_context *ctx,
{
int err;
+ if ((open_flags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
+ *opened |= FILE_CREATED;
+
err = finish_open(file, dentry, do_open, opened);
if (err)
goto out;
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
In gfs2_create_inode() set FILE_CREATED in *opened.
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Steven Whitehouse <[email protected]>
---
fs/gfs2/inode.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 9a1be62..ef411a3 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -694,8 +694,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
mark_inode_dirty(inode);
d_instantiate(dentry, inode);
- if (file)
+ if (file) {
+ *opened |= FILE_CREATED;
error = finish_open(file, dentry, gfs2_open_common, opened);
+ }
gfs2_glock_dq_uninit(ghs);
gfs2_glock_dq_uninit(ghs + 1);
return error;
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
unless it was given an IS_ERR(inode), which isn't the case here. So clean
up the unnecessary error handling in gfs2_create_inode().
This paves the way for real fixes (hence the stable Cc).
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Steven Whitehouse <[email protected]>
Cc: [email protected]
---
fs/gfs2/inode.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 64915ee..6d7f976 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -584,7 +584,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
if (!IS_ERR(inode)) {
d = d_splice_alias(inode, dentry);
error = 0;
- if (file && !IS_ERR(d)) {
+ if (file) {
if (d == NULL)
d = dentry;
if (S_ISREG(inode->i_mode))
@@ -593,8 +593,6 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
error = finish_no_open(file, d);
}
gfs2_glock_dq_uninit(ghs);
- if (IS_ERR(d))
- return PTR_ERR(d);
return error;
} else if (error != -ENOENT) {
goto fail_gunlock;
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
We need to dput() the result of d_splice_alias(), unless it is passed to
finish_no_open().
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Steven Whitehouse <[email protected]>
Cc: [email protected]
---
fs/gfs2/inode.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index abe7dae..9a1be62 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -555,7 +555,6 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
struct gfs2_inode *dip = GFS2_I(dir), *ip;
struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
struct gfs2_glock *io_gl;
- struct dentry *d;
int error;
u32 aflags = 0;
int arq;
@@ -582,15 +581,18 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
error = PTR_ERR(inode);
if (!IS_ERR(inode)) {
- d = d_splice_alias(inode, dentry);
+ struct dentry *d = d_splice_alias(inode, dentry);
error = 0;
if (file) {
- if (d == NULL)
- d = dentry;
- if (S_ISREG(inode->i_mode))
- error = finish_open(file, d, gfs2_open_common, opened);
- else
+ if (S_ISREG(inode->i_mode)) {
+ error = finish_open(file, d ? d : dentry,
+ gfs2_open_common, opened);
+ dput(d);
+ } else {
error = finish_no_open(file, d);
+ }
+ } else {
+ dput(d);
}
gfs2_glock_dq_uninit(ghs);
return error;
@@ -777,8 +779,10 @@ static struct dentry *__gfs2_lookup(struct inode *dir, struct dentry *dentry,
error = finish_open(file, d ? d : dentry, gfs2_open_common, opened);
gfs2_glock_dq_uninit(&gh);
- if (error)
+ if (error) {
+ dput(d);
return ERR_PTR(error);
+ }
return d;
}
@@ -1159,13 +1163,15 @@ static int gfs2_atomic_open(struct inode *dir, struct dentry *dentry,
d = __gfs2_lookup(dir, dentry, file, opened);
if (IS_ERR(d))
return PTR_ERR(d);
- if (d == NULL)
- d = dentry;
- if (d->d_inode) {
+ if (d != NULL)
+ dentry = d;
+ if (dentry->d_inode) {
if (!(*opened & FILE_OPENED))
- return finish_no_open(file, d);
+ return finish_no_open(file, dentry);
+ dput(d);
return 0;
}
+ BUG_ON(d != NULL);
if (!(flags & O_CREAT))
return -ENOENT;
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
AFAICS if d_splice_alias() returned non-NULL, this code would Oops
(finish_open expects an instantiated dentry).
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Steven Whitehouse <[email protected]>
Cc: [email protected]
---
fs/gfs2/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 6d7f976..abe7dae 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -774,7 +774,7 @@ static struct dentry *__gfs2_lookup(struct inode *dir, struct dentry *dentry,
d = d_splice_alias(inode, dentry);
if (file && S_ISREG(inode->i_mode))
- error = finish_open(file, dentry, gfs2_open_common, opened);
+ error = finish_open(file, d ? d : dentry, gfs2_open_common, opened);
gfs2_glock_dq_uninit(&gh);
if (error)
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
If an error occurs after having called finish_open() then fput() needs to
be called on the already opened file.
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Steve French <[email protected]>
Cc: [email protected]
---
fs/cifs/dir.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index d3e2eaa..5384c2a 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -500,6 +500,7 @@ cifs_atomic_open(struct inode *inode, struct dentry *direntry,
if (server->ops->close)
server->ops->close(xid, tcon, &fid);
cifs_del_pending_open(&open);
+ fput(file);
rc = -ENOMEM;
}
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
If open flags has O_EXCL and dentry is positive after lookup then return
-EEXIST instead of "1".
This bug resulted in some O_EXCL opens succeeding (on a cache miss) despite
the file already existing.
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Eric Van Hensbergen <[email protected]>
Cc: [email protected]
---
fs/9p/vfs_inode.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 94de6d1..915cea5 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -863,8 +863,15 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
}
/* Only creates */
- if (!(flags & O_CREAT) || dentry->d_inode)
+ if (!(flags & O_CREAT)) {
return finish_no_open(file, res);
+ } else {
+ err = -EEXIST;
+ if (flags & O_EXCL)
+ goto out;
+
+ return finish_no_open(file, res);
+ }
err = 0;
fid = NULL;
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
If O_CREAT|O_EXCL are passed to open, then we know that either
- the file is successfully created, or
- the operation fails in some way.
So previously we set FILE_CREATED before calling ->atomic_open() so the
filesystem doesn't have to. This, however, led to bugs in the
implementation that went unnoticed when the filesystem didn't check for
existence, yet returned success. To prevent this kind of bug, require
filesystems to always explicitly set FILE_CREATED on O_CREAT|O_EXCL and
verify this in the VFS.
Also added a couple more verifications for the result of atomic_open():
- Warn if filesystem set FILE_CREATED despite the lack of O_CREAT.
- Warn if filesystem set FILE_CREATED but gave a negative dentry.
Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/namei.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 0dc4cbf..22eb548 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2656,6 +2656,7 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,
int acc_mode;
int create_error = 0;
struct dentry *const DENTRY_NOT_SET = (void *) -1UL;
+ bool excl;
BUG_ON(dentry->d_inode);
@@ -2669,10 +2670,9 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,
if ((open_flag & O_CREAT) && !IS_POSIXACL(dir))
mode &= ~current_umask();
- if ((open_flag & (O_EXCL | O_CREAT)) == (O_EXCL | O_CREAT)) {
+ excl = (open_flag & (O_EXCL | O_CREAT)) == (O_EXCL | O_CREAT);
+ if (excl)
open_flag &= ~O_TRUNC;
- *opened |= FILE_CREATED;
- }
/*
* Checking write permission is tricky, bacuse we don't know if we are
@@ -2726,7 +2726,11 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,
}
acc_mode = op->acc_mode;
+ if (WARN_ON(excl && !(*opened & FILE_CREATED)))
+ *opened |= FILE_CREATED;
+
if (*opened & FILE_CREATED) {
+ WARN_ON(!(open_flag & O_CREAT));
fsnotify_create(dir, dentry);
acc_mode = MAY_OPEN;
}
@@ -2740,6 +2744,7 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,
dput(dentry);
dentry = file->f_path.dentry;
}
+ WARN_ON(!dentry->d_inode && (*opened & FILE_CREATED));
if (create_error && dentry->d_inode == NULL) {
error = create_error;
goto out;
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
commit b6f4bee02f "fs/9p: Fix atomic_open" fixed the O_EXCL behavior, but
results in a dentry leak if v9fs_vfs_lookup() returns non-NULL.
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Eric Van Hensbergen <[email protected]>
Cc: M. Mohan Kumar <[email protected]>
Cc: [email protected]
---
fs/9p/vfs_inode_dotl.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index 53687bb..055c159 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -270,10 +270,11 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
if (!(flags & O_CREAT))
return finish_no_open(file, res);
else if (dentry->d_inode) {
- if ((flags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
- return -EEXIST;
- else
- return finish_no_open(file, res);
+ err = -EEXIST;
+ if (flags & O_EXCL)
+ goto out;
+
+ return finish_no_open(file, res);
}
v9ses = v9fs_inode2v9ses(dir);
--
1.8.1.4
From: Miklos Szeredi <[email protected]>
Fix documentation of ->atomic_open() and related functions: finish_open()
and finish_no_open(). Also add details that seem to be unclear and a
source of bugs (some of which are fixed in the following series).
Cc-ing maintainers of all filesystems implementing ->atomic_open().
Signed-off-by: Miklos Szeredi <[email protected]>
Cc: Eric Van Hensbergen <[email protected]>
Cc: Sage Weil <[email protected]>
Cc: Steve French <[email protected]>
Cc: Steven Whitehouse <[email protected]>
Cc: Trond Myklebust <[email protected]>
---
Documentation/filesystems/vfs.txt | 14 +++++++-------
fs/open.c | 21 ++++++++++++++++++---
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index f93a882..deb48b5 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -359,11 +359,9 @@ struct inode_operations {
ssize_t (*listxattr) (struct dentry *, char *, size_t);
int (*removexattr) (struct dentry *, const char *);
void (*update_time)(struct inode *, struct timespec *, int);
- int (*atomic_open)(struct inode *, struct dentry *,
+ int (*atomic_open)(struct inode *, struct dentry *, struct file *,
+ unsigned open_flag, umode_t create_mode, int *opened);
int (*tmpfile) (struct inode *, struct dentry *, umode_t);
-} ____cacheline_aligned;
- struct file *, unsigned open_flag,
- umode_t create_mode, int *opened);
};
Again, all methods are called without any locks being held, unless
@@ -470,9 +468,11 @@ otherwise noted.
method the filesystem can look up, possibly create and open the file in
one atomic operation. If it cannot perform this (e.g. the file type
turned out to be wrong) it may signal this by returning 1 instead of
- usual 0 or -ve . This method is only called if the last
- component is negative or needs lookup. Cached positive dentries are
- still handled by f_op->open().
+ usual 0 or -ve . This method is only called if the last component is
+ negative or needs lookup. Cached positive dentries are still handled by
+ f_op->open(). If the file was created, the FILE_CREATED flag should be
+ set in "opened". In case of O_EXCL the method must only succeed if the
+ file didn't exist and hence FILE_CREATED shall always be set on success.
tmpfile: called in the end of O_TMPFILE open(). Optional, equivalent to
atomically creating, opening and unlinking a file in given directory.
diff --git a/fs/open.c b/fs/open.c
index 2a731b0..d420331 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -744,14 +744,24 @@ cleanup_file:
/**
* finish_open - finish opening a file
- * @od: opaque open data
+ * @file: file pointer
* @dentry: pointer to dentry
* @open: open callback
+ * @opened: state of open
*
* This can be used to finish opening a file passed to i_op->atomic_open().
*
* If the open callback is set to NULL, then the standard f_op->open()
* filesystem callback is substituted.
+ *
+ * NB: the dentry reference is _not_ consumed. If, for example, the dentry is
+ * the return value of d_splice_alias(), then the caller needs to perform dput()
+ * on it after finish_open().
+ *
+ * On successful return @file is a fully instantiated open file. After this, if
+ * an error occurs in ->atomic_open(), it needs to clean up with fput().
+ *
+ * Returns zero on success or -errno if the open failed.
*/
int finish_open(struct file *file, struct dentry *dentry,
int (*open)(struct inode *, struct file *),
@@ -772,11 +782,16 @@ EXPORT_SYMBOL(finish_open);
/**
* finish_no_open - finish ->atomic_open() without opening the file
*
- * @od: opaque open data
+ * @file: file pointer
* @dentry: dentry or NULL (as returned from ->lookup())
*
* This can be used to set the result of a successful lookup in ->atomic_open().
- * The filesystem's atomic_open() method shall return NULL after calling this.
+ *
+ * NB: unlike finish_open() this function does consume the dentry reference and
+ * the caller need not dput() it.
+ *
+ * Returns "1" which must be the return value of ->atomic_open() after having
+ * called this function.
*/
int finish_no_open(struct file *file, struct dentry *dentry)
{
--
1.8.1.4
Hi,
On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <[email protected]>
>
> AFAICS if d_splice_alias() returned non-NULL, this code would Oops
> (finish_open expects an instantiated dentry).
>
> Signed-off-by: Miklos Szeredi <[email protected]>
> Cc: Steven Whitehouse <[email protected]>
> Cc: [email protected]
> ---
> fs/gfs2/inode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 6d7f976..abe7dae 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -774,7 +774,7 @@ static struct dentry *__gfs2_lookup(struct inode *dir, struct dentry *dentry,
>
> d = d_splice_alias(inode, dentry);
> if (file && S_ISREG(inode->i_mode))
> - error = finish_open(file, dentry, gfs2_open_common, opened);
> + error = finish_open(file, d ? d : dentry, gfs2_open_common, opened);
>
> gfs2_glock_dq_uninit(&gh);
> if (error)
Not sure I understand why this is required... when the inode is a
regular file, d can only be an error (if the inode is an error) or it
will be NULL. Since the __gfs2_lookup would terminate further up if the
inode were an error, then d must always be NULL in the regular file
case, so I'm not sure that this is a bug,
Steve.
Hi,
On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <[email protected]>
>
> unless it was given an IS_ERR(inode), which isn't the case here. So clean
> up the unnecessary error handling in gfs2_create_inode().
>
> This paves the way for real fixes (hence the stable Cc).
>
That makes send to me:
Acked-by: Steven Whitehouse <[email protected]>
I can put this in the gfs2 tree if that makes sense to do,
Steve.
> Signed-off-by: Miklos Szeredi <[email protected]>
> Cc: Steven Whitehouse <[email protected]>
> Cc: [email protected]
> ---
> fs/gfs2/inode.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 64915ee..6d7f976 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -584,7 +584,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> if (!IS_ERR(inode)) {
> d = d_splice_alias(inode, dentry);
> error = 0;
> - if (file && !IS_ERR(d)) {
> + if (file) {
> if (d == NULL)
> d = dentry;
> if (S_ISREG(inode->i_mode))
> @@ -593,8 +593,6 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> error = finish_no_open(file, d);
> }
> gfs2_glock_dq_uninit(ghs);
> - if (IS_ERR(d))
> - return PTR_ERR(d);
> return error;
> } else if (error != -ENOENT) {
> goto fail_gunlock;
Hi,
On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <[email protected]>
>
> In gfs2_create_inode() set FILE_CREATED in *opened.
>
Acked-by: Steven Whitehouse <[email protected]>
Thanks for spotting this issue,
Steve.
> Signed-off-by: Miklos Szeredi <[email protected]>
> Cc: Steven Whitehouse <[email protected]>
> ---
> fs/gfs2/inode.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 9a1be62..ef411a3 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -694,8 +694,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>
> mark_inode_dirty(inode);
> d_instantiate(dentry, inode);
> - if (file)
> + if (file) {
> + *opened |= FILE_CREATED;
> error = finish_open(file, dentry, gfs2_open_common, opened);
> + }
> gfs2_glock_dq_uninit(ghs);
> gfs2_glock_dq_uninit(ghs + 1);
> return error;
On Mon, Sep 16, 2013 at 02:13:14PM +0100, Steven Whitehouse wrote:
> Hi,
>
> On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> > From: Miklos Szeredi <[email protected]>
> >
> > AFAICS if d_splice_alias() returned non-NULL, this code would Oops
> > (finish_open expects an instantiated dentry).
> >
> > Signed-off-by: Miklos Szeredi <[email protected]>
> > Cc: Steven Whitehouse <[email protected]>
> > Cc: [email protected]
> > ---
> > fs/gfs2/inode.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > index 6d7f976..abe7dae 100644
> > --- a/fs/gfs2/inode.c
> > +++ b/fs/gfs2/inode.c
> > @@ -774,7 +774,7 @@ static struct dentry *__gfs2_lookup(struct inode *dir, struct dentry *dentry,
> >
> > d = d_splice_alias(inode, dentry);
> > if (file && S_ISREG(inode->i_mode))
> > - error = finish_open(file, dentry, gfs2_open_common, opened);
> > + error = finish_open(file, d ? d : dentry, gfs2_open_common, opened);
> >
> > gfs2_glock_dq_uninit(&gh);
> > if (error)
>
> Not sure I understand why this is required... when the inode is a
> regular file, d can only be an error (if the inode is an error) or it
> will be NULL.
Okay, you're right. Still, something like the following should make this clear
and ensure things don't break in the future.
Thanks,
Miklos
---
fs/gfs2/inode.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -775,8 +775,10 @@ static struct dentry *__gfs2_lookup(stru
}
d = d_splice_alias(inode, dentry);
- if (file && S_ISREG(inode->i_mode))
+ if (file && S_ISREG(inode->i_mode)) {
+ BUG_ON(d);
error = finish_open(file, dentry, gfs2_open_common, opened);
+ }
gfs2_glock_dq_uninit(&gh);
if (error)
On Mon, Sep 16, 2013 at 02:17:49PM +0100, Steven Whitehouse wrote:
> Hi,
>
> On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> > From: Miklos Szeredi <[email protected]>
> >
> > unless it was given an IS_ERR(inode), which isn't the case here. So clean
> > up the unnecessary error handling in gfs2_create_inode().
> >
> > This paves the way for real fixes (hence the stable Cc).
> >
> That makes send to me:
>
> Acked-by: Steven Whitehouse <[email protected]>
>
> I can put this in the gfs2 tree if that makes sense to do,
Sure, please do.
Thanks,
Miklos
> Steve.
>
> > Signed-off-by: Miklos Szeredi <[email protected]>
> > Cc: Steven Whitehouse <[email protected]>
> > Cc: [email protected]
> > ---
> > fs/gfs2/inode.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > index 64915ee..6d7f976 100644
> > --- a/fs/gfs2/inode.c
> > +++ b/fs/gfs2/inode.c
> > @@ -584,7 +584,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> > if (!IS_ERR(inode)) {
> > d = d_splice_alias(inode, dentry);
> > error = 0;
> > - if (file && !IS_ERR(d)) {
> > + if (file) {
> > if (d == NULL)
> > d = dentry;
> > if (S_ISREG(inode->i_mode))
> > @@ -593,8 +593,6 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> > error = finish_no_open(file, d);
> > }
> > gfs2_glock_dq_uninit(ghs);
> > - if (IS_ERR(d))
> > - return PTR_ERR(d);
> > return error;
> > } else if (error != -ENOENT) {
> > goto fail_gunlock;
>
>
Hi,
On Mon, 2013-09-16 at 15:34 +0200, Miklos Szeredi wrote:
> On Mon, Sep 16, 2013 at 02:13:14PM +0100, Steven Whitehouse wrote:
> > Hi,
> >
> > On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> > > From: Miklos Szeredi <[email protected]>
> > >
> > > AFAICS if d_splice_alias() returned non-NULL, this code would Oops
> > > (finish_open expects an instantiated dentry).
> > >
> > > Signed-off-by: Miklos Szeredi <[email protected]>
> > > Cc: Steven Whitehouse <[email protected]>
> > > Cc: [email protected]
> > > ---
> > > fs/gfs2/inode.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > > index 6d7f976..abe7dae 100644
> > > --- a/fs/gfs2/inode.c
> > > +++ b/fs/gfs2/inode.c
> > > @@ -774,7 +774,7 @@ static struct dentry *__gfs2_lookup(struct inode *dir, struct dentry *dentry,
> > >
> > > d = d_splice_alias(inode, dentry);
> > > if (file && S_ISREG(inode->i_mode))
> > > - error = finish_open(file, dentry, gfs2_open_common, opened);
> > > + error = finish_open(file, d ? d : dentry, gfs2_open_common, opened);
> > >
> > > gfs2_glock_dq_uninit(&gh);
> > > if (error)
> >
> > Not sure I understand why this is required... when the inode is a
> > regular file, d can only be an error (if the inode is an error) or it
> > will be NULL.
>
> Okay, you're right. Still, something like the following should make this clear
> and ensure things don't break in the future.
>
or better still, just add a comment to explain the situation as the
reader may still be wondering why that BUG_ON() will never trigger,
Steve.
> Thanks,
> Miklos
>
> ---
> fs/gfs2/inode.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -775,8 +775,10 @@ static struct dentry *__gfs2_lookup(stru
> }
>
> d = d_splice_alias(inode, dentry);
> - if (file && S_ISREG(inode->i_mode))
> + if (file && S_ISREG(inode->i_mode)) {
> + BUG_ON(d);
> error = finish_open(file, dentry, gfs2_open_common, opened);
> + }
>
> gfs2_glock_dq_uninit(&gh);
> if (error)
Hi,
On Mon, 2013-09-16 at 15:35 +0200, Miklos Szeredi wrote:
> On Mon, Sep 16, 2013 at 02:17:49PM +0100, Steven Whitehouse wrote:
> > Hi,
> >
> > On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> > > From: Miklos Szeredi <[email protected]>
> > >
> > > unless it was given an IS_ERR(inode), which isn't the case here. So clean
> > > up the unnecessary error handling in gfs2_create_inode().
> > >
> > > This paves the way for real fixes (hence the stable Cc).
> > >
> > That makes send to me:
> >
> > Acked-by: Steven Whitehouse <[email protected]>
> >
> > I can put this in the gfs2 tree if that makes sense to do,
>
> Sure, please do.
>
> Thanks,
> Miklos
>
Ok. I'll add the patches shortly. I need to try and wrap my brain around
patch 8 too,
Steve.
>
> > Steve.
> >
> > > Signed-off-by: Miklos Szeredi <[email protected]>
> > > Cc: Steven Whitehouse <[email protected]>
> > > Cc: [email protected]
> > > ---
> > > fs/gfs2/inode.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > > index 64915ee..6d7f976 100644
> > > --- a/fs/gfs2/inode.c
> > > +++ b/fs/gfs2/inode.c
> > > @@ -584,7 +584,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> > > if (!IS_ERR(inode)) {
> > > d = d_splice_alias(inode, dentry);
> > > error = 0;
> > > - if (file && !IS_ERR(d)) {
> > > + if (file) {
> > > if (d == NULL)
> > > d = dentry;
> > > if (S_ISREG(inode->i_mode))
> > > @@ -593,8 +593,6 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> > > error = finish_no_open(file, d);
> > > }
> > > gfs2_glock_dq_uninit(ghs);
> > > - if (IS_ERR(d))
> > > - return PTR_ERR(d);
> > > return error;
> > > } else if (error != -ENOENT) {
> > > goto fail_gunlock;
> >
> >
On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <[email protected]>
>
> commit b6f4bee02f "fs/9p: Fix atomic_open" fixed the O_EXCL behavior, but
> results in a dentry leak if v9fs_vfs_lookup() returns non-NULL.
Frankly, I would prefer to deal with that in fs/namei.c:atomic_open()
instead. I.e. let it call finish_no_open() as it used to do and
turn
if (create_error && dentry->d_inode == NULL) {
error = create_error;
goto out;
}
in fs/namei.c:atomic_open() into
if (!dentry->d_inode) {
if (create_error) {
error = create_error;
goto out;
}
} else if ((open_flag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL)) {
error = -EEXIST;
goto out;
}
rather than try to deal with that crap in each instance of ->atomic_open()...
Objections?
On Mon, Sep 16, 2013 at 8:19 PM, Al Viro <[email protected]> wrote:
> On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
>> From: Miklos Szeredi <[email protected]>
>>
>> commit b6f4bee02f "fs/9p: Fix atomic_open" fixed the O_EXCL behavior, but
>> results in a dentry leak if v9fs_vfs_lookup() returns non-NULL.
>
> Frankly, I would prefer to deal with that in fs/namei.c:atomic_open()
> instead. I.e. let it call finish_no_open() as it used to do and
> turn
> if (create_error && dentry->d_inode == NULL) {
> error = create_error;
> goto out;
> }
> in fs/namei.c:atomic_open() into
> if (!dentry->d_inode) {
> if (create_error) {
> error = create_error;
> goto out;
> }
> } else if ((open_flag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL)) {
> error = -EEXIST;
> goto out;
> }
>
> rather than try to deal with that crap in each instance of ->atomic_open()...
> Objections?
->atomic_open() could be any one of
lookup
lookup+create
lookup+create+open
If it's the second one then the above is wrong. Sure, we could check
FILE_CREATED as well, and if file wasn't created yet dentry is
positive then we return EEXIST. But for that to be correct we need
the last patch in the series, preventing FILE_CREATED from being set
unconditionally.
Thanks,
Miklos
On Mon, Sep 16, 2013 at 09:03:25PM +0200, Miklos Szeredi wrote:
> On Mon, Sep 16, 2013 at 8:19 PM, Al Viro <[email protected]> wrote:
> > On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
> >> From: Miklos Szeredi <[email protected]>
> >>
> >> commit b6f4bee02f "fs/9p: Fix atomic_open" fixed the O_EXCL behavior, but
> >> results in a dentry leak if v9fs_vfs_lookup() returns non-NULL.
> >
> > Frankly, I would prefer to deal with that in fs/namei.c:atomic_open()
> > instead. I.e. let it call finish_no_open() as it used to do and
> > turn
> > if (create_error && dentry->d_inode == NULL) {
> > error = create_error;
> > goto out;
> > }
> > in fs/namei.c:atomic_open() into
> > if (!dentry->d_inode) {
> > if (create_error) {
> > error = create_error;
> > goto out;
> > }
> > } else if ((open_flag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL)) {
> > error = -EEXIST;
> > goto out;
> > }
> >
> > rather than try to deal with that crap in each instance of ->atomic_open()...
> > Objections?
>
> ->atomic_open() could be any one of
>
> lookup
> lookup+create
> lookup+create+open
>
> If it's the second one then the above is wrong. Sure, we could check
> FILE_CREATED as well, and if file wasn't created yet dentry is
> positive then we return EEXIST. But for that to be correct we need
> the last patch in the series, preventing FILE_CREATED from being set
> unconditionally.
You mean, lookup + create + return finish_no_open()? Does anything actually
do that? I agree that we want your "deal with setting FILE_CREATED in
filesystems", BTW, and I'm fine with putting it in front of the rest of
the queue.
I would definitely prefer EEXIST logics dealt with in fs/namei.c - if nothing
else, it had been done wrong in too many instances...
On Mon, Sep 16, 2013 at 9:50 PM, Al Viro <[email protected]> wrote:
> On Mon, Sep 16, 2013 at 09:03:25PM +0200, Miklos Szeredi wrote:
>> On Mon, Sep 16, 2013 at 8:19 PM, Al Viro <[email protected]> wrote:
>> > On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
>> >> From: Miklos Szeredi <[email protected]>
>> >>
>> >> commit b6f4bee02f "fs/9p: Fix atomic_open" fixed the O_EXCL behavior, but
>> >> results in a dentry leak if v9fs_vfs_lookup() returns non-NULL.
>> >
>> > Frankly, I would prefer to deal with that in fs/namei.c:atomic_open()
>> > instead. I.e. let it call finish_no_open() as it used to do and
>> > turn
>> > if (create_error && dentry->d_inode == NULL) {
>> > error = create_error;
>> > goto out;
>> > }
>> > in fs/namei.c:atomic_open() into
>> > if (!dentry->d_inode) {
>> > if (create_error) {
>> > error = create_error;
>> > goto out;
>> > }
>> > } else if ((open_flag & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL)) {
>> > error = -EEXIST;
>> > goto out;
>> > }
>> >
>> > rather than try to deal with that crap in each instance of ->atomic_open()...
>> > Objections?
>>
>> ->atomic_open() could be any one of
>>
>> lookup
>> lookup+create
>> lookup+create+open
>>
>> If it's the second one then the above is wrong. Sure, we could check
>> FILE_CREATED as well, and if file wasn't created yet dentry is
>> positive then we return EEXIST. But for that to be correct we need
>> the last patch in the series, preventing FILE_CREATED from being set
>> unconditionally.
>
> You mean, lookup + create + return finish_no_open()? Does anything actually
> do that?
Fuse does.
> I agree that we want your "deal with setting FILE_CREATED in
> filesystems", BTW, and I'm fine with putting it in front of the rest of
> the queue.
>
> I would definitely prefer EEXIST logics dealt with in fs/namei.c - if nothing
> else, it had been done wrong in too many instances...
Okay.
Thanks,
Miklos
On Mon, Sep 16, 2013 at 10:09:46PM +0200, Miklos Szeredi wrote:
> > I would definitely prefer EEXIST logics dealt with in fs/namei.c - if nothing
> > else, it had been done wrong in too many instances...
>
> Okay.
OK, so I'm taking 1, 5, 9, 10, 11, then add check to fs/namei.c:atomic_open(),
leaving the rest of gfs2 ones for gfs2 tree... Give me a few and I'll push
that.
On Mon, Sep 16, 2013 at 11:02:41PM +0100, Al Viro wrote:
> On Mon, Sep 16, 2013 at 10:09:46PM +0200, Miklos Szeredi wrote:
>
> > > I would definitely prefer EEXIST logics dealt with in fs/namei.c - if nothing
> > > else, it had been done wrong in too many instances...
> >
> > Okay.
>
> OK, so I'm taking 1, 5, 9, 10, 11, then add check to fs/namei.c:atomic_open(),
> leaving the rest of gfs2 ones for gfs2 tree... Give me a few and I'll push
> that.
Pushed (head at d332c7). That should cover 9p and fuse as well, AFAICS.
Do you have any problems with that variant?
On Tue, Sep 17, 2013 at 1:28 AM, Al Viro <[email protected]> wrote:
> On Mon, Sep 16, 2013 at 11:02:41PM +0100, Al Viro wrote:
>> On Mon, Sep 16, 2013 at 10:09:46PM +0200, Miklos Szeredi wrote:
>>
>> > > I would definitely prefer EEXIST logics dealt with in fs/namei.c - if nothing
>> > > else, it had been done wrong in too many instances...
>> >
>> > Okay.
>>
>> OK, so I'm taking 1, 5, 9, 10, 11, then add check to fs/namei.c:atomic_open(),
>> leaving the rest of gfs2 ones for gfs2 tree... Give me a few and I'll push
>> that.
>
> Pushed (head at d332c7). That should cover 9p and fuse as well, AFAICS.
> Do you have any problems with that variant?
Just one. This needs to be removed, since this condition is now
explicitly allowed and later checked for:
if (WARN_ON(excl && !(*opened & FILE_CREATED)))
*opened |= FILE_CREATED;
Thanks,
Miklos
On Tue, Sep 17, 2013 at 12:16:56PM +0200, Miklos Szeredi wrote:
> Just one. This needs to be removed, since this condition is now
> explicitly allowed and later checked for:
>
> if (WARN_ON(excl && !(*opened & FILE_CREATED)))
> *opened |= FILE_CREATED;
D'oh... Fixed and pushed.
On Tue, Sep 17, 2013 at 1:44 PM, Al Viro <[email protected]> wrote:
> On Tue, Sep 17, 2013 at 12:16:56PM +0200, Miklos Szeredi wrote:
>
>> Just one. This needs to be removed, since this condition is now
>> explicitly allowed and later checked for:
>>
>> if (WARN_ON(excl && !(*opened & FILE_CREATED)))
>> *opened |= FILE_CREATED;
>
> D'oh... Fixed and pushed.
Okay, but moving the fsnotify_create() to after the no-open section
is wrong, I think, It's needed for the case of ->atomic_open() doing
lookup/create/no_open too.
Thanks,
Miklos
On Tue, Sep 17, 2013 at 05:36:49PM +0200, Miklos Szeredi wrote:
> On Tue, Sep 17, 2013 at 1:44 PM, Al Viro <[email protected]> wrote:
> > On Tue, Sep 17, 2013 at 12:16:56PM +0200, Miklos Szeredi wrote:
> >
> >> Just one. This needs to be removed, since this condition is now
> >> explicitly allowed and later checked for:
> >>
> >> if (WARN_ON(excl && !(*opened & FILE_CREATED)))
> >> *opened |= FILE_CREATED;
> >
> > D'oh... Fixed and pushed.
>
> Okay, but moving the fsnotify_create() to after the no-open section
> is wrong, I think, It's needed for the case of ->atomic_open() doing
> lookup/create/no_open too.
What a mess... It's actually even uglier than that - which dentry should
we pass to fsnotify_create() in case where finish_no_open() has been given
a non-NULL dentry other than one we had passed to ->atomic_open()? I think
that version in mainline is actually broken in that respect as far as fuse
is concerned, not that anybody sane could expect ...notify to work on fuse.
Anyway, I've pushed what I think is a sane fix. Please, review and test -
I don't have a setup for testing fsnotify on fuse.
On Tue, Sep 17, 2013 at 11:23 PM, Al Viro <[email protected]> wrote:
> On Tue, Sep 17, 2013 at 05:36:49PM +0200, Miklos Szeredi wrote:
>> On Tue, Sep 17, 2013 at 1:44 PM, Al Viro <[email protected]> wrote:
>> > On Tue, Sep 17, 2013 at 12:16:56PM +0200, Miklos Szeredi wrote:
>> >
>> >> Just one. This needs to be removed, since this condition is now
>> >> explicitly allowed and later checked for:
>> >>
>> >> if (WARN_ON(excl && !(*opened & FILE_CREATED)))
>> >> *opened |= FILE_CREATED;
>> >
>> > D'oh... Fixed and pushed.
>>
>> Okay, but moving the fsnotify_create() to after the no-open section
>> is wrong, I think, It's needed for the case of ->atomic_open() doing
>> lookup/create/no_open too.
>
> What a mess... It's actually even uglier than that - which dentry should
> we pass to fsnotify_create() in case where finish_no_open() has been given
> a non-NULL dentry other than one we had passed to ->atomic_open()? I think
> that version in mainline is actually broken in that respect as far as fuse
> is concerned, not that anybody sane could expect ...notify to work on fuse.
Yeah, your version is definitely nicer. The correctness of the old
version could be argued thus: if FILE_CREATED was set, then the file
didn't exist before, so there's no sense in reusing or allocating
another dentry. But yes, the API allows it.
Thanks,
Miklos
Do you want me to merge this via my tree (cifs-2.6.git) or another?
On Mon, Sep 16, 2013 at 7:51 AM, Miklos Szeredi <[email protected]> wrote:
> From: Miklos Szeredi <[email protected]>
>
> If an error occurs after having called finish_open() then fput() needs to
> be called on the already opened file.
>
> Signed-off-by: Miklos Szeredi <[email protected]>
> Cc: Steve French <[email protected]>
> Cc: [email protected]
> ---
> fs/cifs/dir.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index d3e2eaa..5384c2a 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -500,6 +500,7 @@ cifs_atomic_open(struct inode *inode, struct dentry *direntry,
> if (server->ops->close)
> server->ops->close(xid, tcon, &fid);
> cifs_del_pending_open(&open);
> + fput(file);
> rc = -ENOMEM;
> }
>
> --
> 1.8.1.4
>
--
Thanks,
Steve
On Wed, Sep 18, 2013 at 10:19:20AM -0500, Steve French wrote:
> Do you want me to merge this via my tree (cifs-2.6.git) or another?
It's in vfs.git#for-linus. I'll send a pull request later today...