* [PATCH 01/11] vfs: improve i_op->atomic_open() documentation
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
@ 2013-09-16 12:51 ` Miklos Szeredi
2013-09-16 12:51 ` [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl() Miklos Szeredi
` (9 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:51 UTC (permalink / raw)
To: viro
Cc: linux-fsdevel, linux-kernel, mszeredi, Eric Van Hensbergen,
Sage Weil, Steve French, Steven Whitehouse, Trond Myklebust
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
Cc: Eric Van Hensbergen <ericvh@gmail.com>
Cc: Sage Weil <sage@inktank.com>
Cc: Steve French <sfrench@samba.org>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
2013-09-16 12:51 ` [PATCH 01/11] vfs: improve i_op->atomic_open() documentation Miklos Szeredi
@ 2013-09-16 12:51 ` Miklos Szeredi
2013-09-16 18:19 ` Al Viro
2013-09-16 12:51 ` [PATCH 03/11] 9p: fix O_EXCL in v9fs_vfs_atomic_open() Miklos Szeredi
` (8 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:51 UTC (permalink / raw)
To: viro
Cc: linux-fsdevel, linux-kernel, mszeredi, Eric Van Hensbergen,
M. Mohan Kumar, stable
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
Cc: Eric Van Hensbergen <ericvh@gmail.com>
Cc: M. Mohan Kumar <mohan@in.ibm.com>
Cc: stable@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-16 12:51 ` [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl() Miklos Szeredi
@ 2013-09-16 18:19 ` Al Viro
2013-09-16 19:03 ` Miklos Szeredi
0 siblings, 1 reply; 32+ messages in thread
From: Al Viro @ 2013-09-16 18:19 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-fsdevel, linux-kernel, mszeredi, Eric Van Hensbergen,
M. Mohan Kumar, stable
On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> 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?
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-16 18:19 ` Al Viro
@ 2013-09-16 19:03 ` Miklos Szeredi
2013-09-16 19:50 ` Al Viro
0 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 19:03 UTC (permalink / raw)
To: Al Viro
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
On Mon, Sep 16, 2013 at 8:19 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
>> From: Miklos Szeredi <mszeredi@suse.cz>
>>
>> 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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-16 19:03 ` Miklos Szeredi
@ 2013-09-16 19:50 ` Al Viro
2013-09-16 20:09 ` Miklos Szeredi
0 siblings, 1 reply; 32+ messages in thread
From: Al Viro @ 2013-09-16 19:50 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
On Mon, Sep 16, 2013 at 09:03:25PM +0200, Miklos Szeredi wrote:
> On Mon, Sep 16, 2013 at 8:19 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
> >> From: Miklos Szeredi <mszeredi@suse.cz>
> >>
> >> 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...
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-16 19:50 ` Al Viro
@ 2013-09-16 20:09 ` Miklos Szeredi
2013-09-16 22:02 ` Al Viro
0 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 20:09 UTC (permalink / raw)
To: Al Viro
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
On Mon, Sep 16, 2013 at 9:50 PM, Al Viro <viro@zeniv.linux.org.uk> 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 <viro@zeniv.linux.org.uk> wrote:
>> > On Mon, Sep 16, 2013 at 02:51:56PM +0200, Miklos Szeredi wrote:
>> >> From: Miklos Szeredi <mszeredi@suse.cz>
>> >>
>> >> 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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-16 20:09 ` Miklos Szeredi
@ 2013-09-16 22:02 ` Al Viro
2013-09-16 23:28 ` Al Viro
0 siblings, 1 reply; 32+ messages in thread
From: Al Viro @ 2013-09-16 22:02 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-16 22:02 ` Al Viro
@ 2013-09-16 23:28 ` Al Viro
2013-09-17 10:16 ` Miklos Szeredi
0 siblings, 1 reply; 32+ messages in thread
From: Al Viro @ 2013-09-16 23:28 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
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?
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-16 23:28 ` Al Viro
@ 2013-09-17 10:16 ` Miklos Szeredi
2013-09-17 11:44 ` Al Viro
0 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-17 10:16 UTC (permalink / raw)
To: Al Viro
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
On Tue, Sep 17, 2013 at 1:28 AM, Al Viro <viro@zeniv.linux.org.uk> 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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-17 10:16 ` Miklos Szeredi
@ 2013-09-17 11:44 ` Al Viro
2013-09-17 15:36 ` Miklos Szeredi
0 siblings, 1 reply; 32+ messages in thread
From: Al Viro @ 2013-09-17 11:44 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-17 11:44 ` Al Viro
@ 2013-09-17 15:36 ` Miklos Szeredi
2013-09-17 21:23 ` Al Viro
0 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-17 15:36 UTC (permalink / raw)
To: Al Viro
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
On Tue, Sep 17, 2013 at 1:44 PM, Al Viro <viro@zeniv.linux.org.uk> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-17 15:36 ` Miklos Szeredi
@ 2013-09-17 21:23 ` Al Viro
2013-09-18 8:55 ` Miklos Szeredi
0 siblings, 1 reply; 32+ messages in thread
From: Al Viro @ 2013-09-17 21:23 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
On Tue, Sep 17, 2013 at 05:36:49PM +0200, Miklos Szeredi wrote:
> On Tue, Sep 17, 2013 at 1:44 PM, Al Viro <viro@zeniv.linux.org.uk> 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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
2013-09-17 21:23 ` Al Viro
@ 2013-09-18 8:55 ` Miklos Szeredi
0 siblings, 0 replies; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-18 8:55 UTC (permalink / raw)
To: Al Viro
Cc: Linux-Fsdevel, Kernel Mailing List, mszeredi@suse.cz,
Eric Van Hensbergen, M. Mohan Kumar, stable
On Tue, Sep 17, 2013 at 11:23 PM, Al Viro <viro@zeniv.linux.org.uk> 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 <viro@zeniv.linux.org.uk> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 03/11] 9p: fix O_EXCL in v9fs_vfs_atomic_open()
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
2013-09-16 12:51 ` [PATCH 01/11] vfs: improve i_op->atomic_open() documentation Miklos Szeredi
2013-09-16 12:51 ` [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl() Miklos Szeredi
@ 2013-09-16 12:51 ` Miklos Szeredi
2013-09-16 12:51 ` [PATCH 04/11] fuse: fix O_EXCL in fuse_atomic_open() Miklos Szeredi
` (7 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:51 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi, Eric Van Hensbergen,
stable
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
Cc: Eric Van Hensbergen <ericvh@gmail.com>
Cc: stable@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 04/11] fuse: fix O_EXCL in fuse_atomic_open()
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
` (2 preceding siblings ...)
2013-09-16 12:51 ` [PATCH 03/11] 9p: fix O_EXCL in v9fs_vfs_atomic_open() Miklos Szeredi
@ 2013-09-16 12:51 ` Miklos Szeredi
2013-09-16 12:51 ` [PATCH 05/11] cifs: fix filp leak in cifs_atomic_open() Miklos Szeredi
` (6 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:51 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi, stable
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
Cc: stable@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 05/11] cifs: fix filp leak in cifs_atomic_open()
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
` (3 preceding siblings ...)
2013-09-16 12:51 ` [PATCH 04/11] fuse: fix O_EXCL in fuse_atomic_open() Miklos Szeredi
@ 2013-09-16 12:51 ` Miklos Szeredi
2013-09-18 15:19 ` Steve French
2013-09-16 12:52 ` [PATCH 06/11] gfs2: d_splice_alias() cant return error Miklos Szeredi
` (5 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:51 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi, Steve French, stable
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
Cc: Steve French <sfrench@samba.org>
Cc: stable@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 05/11] cifs: fix filp leak in cifs_atomic_open()
2013-09-16 12:51 ` [PATCH 05/11] cifs: fix filp leak in cifs_atomic_open() Miklos Szeredi
@ 2013-09-18 15:19 ` Steve French
2013-09-18 15:22 ` Al Viro
0 siblings, 1 reply; 32+ messages in thread
From: Steve French @ 2013-09-18 15:19 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Al Viro, linux-fsdevel, LKML, mszeredi, Steve French, stable
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 <miklos@szeredi.hu> wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> 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 <mszeredi@suse.cz>
> Cc: Steve French <sfrench@samba.org>
> Cc: stable@vger.kernel.org
> ---
> 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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 06/11] gfs2: d_splice_alias() cant return error
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
` (4 preceding siblings ...)
2013-09-16 12:51 ` [PATCH 05/11] cifs: fix filp leak in cifs_atomic_open() Miklos Szeredi
@ 2013-09-16 12:52 ` Miklos Szeredi
2013-09-16 13:17 ` Steven Whitehouse
2013-09-16 12:52 ` [PATCH 07/11] gfs2: pass correct dentry to finish_open() in __gfs2_lookup() Miklos Szeredi
` (4 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:52 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi, Steven Whitehouse, stable
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: stable@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 06/11] gfs2: d_splice_alias() cant return error
2013-09-16 12:52 ` [PATCH 06/11] gfs2: d_splice_alias() cant return error Miklos Szeredi
@ 2013-09-16 13:17 ` Steven Whitehouse
2013-09-16 13:35 ` Miklos Szeredi
0 siblings, 1 reply; 32+ messages in thread
From: Steven Whitehouse @ 2013-09-16 13:17 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: viro, linux-fsdevel, linux-kernel, mszeredi, stable
Hi,
On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> 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 <swhiteho@redhat.com>
I can put this in the gfs2 tree if that makes sense to do,
Steve.
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> Cc: Steven Whitehouse <swhiteho@redhat.com>
> Cc: stable@vger.kernel.org
> ---
> 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;
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 06/11] gfs2: d_splice_alias() cant return error
2013-09-16 13:17 ` Steven Whitehouse
@ 2013-09-16 13:35 ` Miklos Szeredi
2013-09-16 13:56 ` Steven Whitehouse
0 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 13:35 UTC (permalink / raw)
To: Steven Whitehouse; +Cc: viro, linux-fsdevel, linux-kernel, mszeredi, stable
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 <mszeredi@suse.cz>
> >
> > 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 <swhiteho@redhat.com>
>
> 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 <mszeredi@suse.cz>
> > Cc: Steven Whitehouse <swhiteho@redhat.com>
> > Cc: stable@vger.kernel.org
> > ---
> > 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;
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 06/11] gfs2: d_splice_alias() cant return error
2013-09-16 13:35 ` Miklos Szeredi
@ 2013-09-16 13:56 ` Steven Whitehouse
0 siblings, 0 replies; 32+ messages in thread
From: Steven Whitehouse @ 2013-09-16 13:56 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: viro, linux-fsdevel, linux-kernel, mszeredi, stable
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 <mszeredi@suse.cz>
> > >
> > > 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 <swhiteho@redhat.com>
> >
> > 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 <mszeredi@suse.cz>
> > > Cc: Steven Whitehouse <swhiteho@redhat.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > 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;
> >
> >
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 07/11] gfs2: pass correct dentry to finish_open() in __gfs2_lookup()
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
` (5 preceding siblings ...)
2013-09-16 12:52 ` [PATCH 06/11] gfs2: d_splice_alias() cant return error Miklos Szeredi
@ 2013-09-16 12:52 ` Miklos Szeredi
2013-09-16 13:13 ` Steven Whitehouse
2013-09-16 12:52 ` [PATCH 08/11] gfs2: fix dentry leaks Miklos Szeredi
` (3 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:52 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi, Steven Whitehouse, stable
From: Miklos Szeredi <mszeredi@suse.cz>
AFAICS if d_splice_alias() returned non-NULL, this code would Oops
(finish_open expects an instantiated dentry).
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: stable@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 07/11] gfs2: pass correct dentry to finish_open() in __gfs2_lookup()
2013-09-16 12:52 ` [PATCH 07/11] gfs2: pass correct dentry to finish_open() in __gfs2_lookup() Miklos Szeredi
@ 2013-09-16 13:13 ` Steven Whitehouse
2013-09-16 13:34 ` Miklos Szeredi
0 siblings, 1 reply; 32+ messages in thread
From: Steven Whitehouse @ 2013-09-16 13:13 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: viro, linux-fsdevel, linux-kernel, mszeredi, stable
Hi,
On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> AFAICS if d_splice_alias() returned non-NULL, this code would Oops
> (finish_open expects an instantiated dentry).
>
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> Cc: Steven Whitehouse <swhiteho@redhat.com>
> Cc: stable@vger.kernel.org
> ---
> 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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 07/11] gfs2: pass correct dentry to finish_open() in __gfs2_lookup()
2013-09-16 13:13 ` Steven Whitehouse
@ 2013-09-16 13:34 ` Miklos Szeredi
2013-09-16 13:54 ` Steven Whitehouse
0 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 13:34 UTC (permalink / raw)
To: Steven Whitehouse; +Cc: viro, linux-fsdevel, linux-kernel, mszeredi, stable
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 <mszeredi@suse.cz>
> >
> > AFAICS if d_splice_alias() returned non-NULL, this code would Oops
> > (finish_open expects an instantiated dentry).
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> > Cc: Steven Whitehouse <swhiteho@redhat.com>
> > Cc: stable@vger.kernel.org
> > ---
> > 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)
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 07/11] gfs2: pass correct dentry to finish_open() in __gfs2_lookup()
2013-09-16 13:34 ` Miklos Szeredi
@ 2013-09-16 13:54 ` Steven Whitehouse
0 siblings, 0 replies; 32+ messages in thread
From: Steven Whitehouse @ 2013-09-16 13:54 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: viro, linux-fsdevel, linux-kernel, mszeredi, stable
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 <mszeredi@suse.cz>
> > >
> > > AFAICS if d_splice_alias() returned non-NULL, this code would Oops
> > > (finish_open expects an instantiated dentry).
> > >
> > > Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> > > Cc: Steven Whitehouse <swhiteho@redhat.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > 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)
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 08/11] gfs2: fix dentry leaks
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
` (6 preceding siblings ...)
2013-09-16 12:52 ` [PATCH 07/11] gfs2: pass correct dentry to finish_open() in __gfs2_lookup() Miklos Szeredi
@ 2013-09-16 12:52 ` Miklos Szeredi
2013-09-16 12:52 ` [PATCH 09/11] gfs2: set FILE_CREATED Miklos Szeredi
` (2 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:52 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi, Steven Whitehouse, stable
From: Miklos Szeredi <mszeredi@suse.cz>
We need to dput() the result of d_splice_alias(), unless it is passed to
finish_no_open().
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: stable@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 09/11] gfs2: set FILE_CREATED
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
` (7 preceding siblings ...)
2013-09-16 12:52 ` [PATCH 08/11] gfs2: fix dentry leaks Miklos Szeredi
@ 2013-09-16 12:52 ` Miklos Szeredi
2013-09-16 13:27 ` Steven Whitehouse
2013-09-16 12:52 ` [PATCH 10/11] nfs: " Miklos Szeredi
2013-09-16 12:52 ` [PATCH 11/11] vfs: don't set FILE_CREATED before calling ->atomic_open() Miklos Szeredi
10 siblings, 1 reply; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:52 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi, Steven Whitehouse
From: Miklos Szeredi <mszeredi@suse.cz>
In gfs2_create_inode() set FILE_CREATED in *opened.
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Cc: Steven Whitehouse <swhiteho@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 09/11] gfs2: set FILE_CREATED
2013-09-16 12:52 ` [PATCH 09/11] gfs2: set FILE_CREATED Miklos Szeredi
@ 2013-09-16 13:27 ` Steven Whitehouse
0 siblings, 0 replies; 32+ messages in thread
From: Steven Whitehouse @ 2013-09-16 13:27 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: viro, linux-fsdevel, linux-kernel, mszeredi
Hi,
On Mon, 2013-09-16 at 14:52 +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@suse.cz>
>
> In gfs2_create_inode() set FILE_CREATED in *opened.
>
Acked-by: Steven Whitehouse <swhiteho@redhat.com>
Thanks for spotting this issue,
Steve.
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> Cc: Steven Whitehouse <swhiteho@redhat.com>
> ---
> 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;
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 10/11] nfs: set FILE_CREATED
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
` (8 preceding siblings ...)
2013-09-16 12:52 ` [PATCH 09/11] gfs2: set FILE_CREATED Miklos Szeredi
@ 2013-09-16 12:52 ` Miklos Szeredi
2013-09-16 12:52 ` [PATCH 11/11] vfs: don't set FILE_CREATED before calling ->atomic_open() Miklos Szeredi
10 siblings, 0 replies; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:52 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi, Trond Myklebust
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 11/11] vfs: don't set FILE_CREATED before calling ->atomic_open()
2013-09-16 12:51 [PATCH 00/11] atomic open related fixes Miklos Szeredi
` (9 preceding siblings ...)
2013-09-16 12:52 ` [PATCH 10/11] nfs: " Miklos Szeredi
@ 2013-09-16 12:52 ` Miklos Szeredi
10 siblings, 0 replies; 32+ messages in thread
From: Miklos Szeredi @ 2013-09-16 12:52 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel, linux-kernel, mszeredi
From: Miklos Szeredi <mszeredi@suse.cz>
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 <mszeredi@suse.cz>
---
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
^ permalink raw reply related [flat|nested] 32+ messages in thread