* [PATCH 02/11] 9p: fix dentry leak in v9fs_vfs_atomic_open_dotl()
[not found] <1379335925-30858-1-git-send-email-miklos@szeredi.hu>
@ 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
` (5 subsequent siblings)
6 siblings, 1 reply; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ messages in thread
* [PATCH 03/11] 9p: fix O_EXCL in v9fs_vfs_atomic_open()
[not found] <1379335925-30858-1-git-send-email-miklos@szeredi.hu>
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
` (4 subsequent siblings)
6 siblings, 0 replies; 26+ 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] 26+ messages in thread* [PATCH 04/11] fuse: fix O_EXCL in fuse_atomic_open()
[not found] <1379335925-30858-1-git-send-email-miklos@szeredi.hu>
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 ` [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
` (3 subsequent siblings)
6 siblings, 0 replies; 26+ 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] 26+ messages in thread* [PATCH 05/11] cifs: fix filp leak in cifs_atomic_open()
[not found] <1379335925-30858-1-git-send-email-miklos@szeredi.hu>
` (2 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
` (2 subsequent siblings)
6 siblings, 1 reply; 26+ 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] 26+ 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; 26+ 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] 26+ messages in thread
* [PATCH 06/11] gfs2: d_splice_alias() cant return error
[not found] <1379335925-30858-1-git-send-email-miklos@szeredi.hu>
` (3 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
2013-09-16 12:52 ` [PATCH 08/11] gfs2: fix dentry leaks Miklos Szeredi
6 siblings, 1 reply; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ messages in thread
* [PATCH 07/11] gfs2: pass correct dentry to finish_open() in __gfs2_lookup()
[not found] <1379335925-30858-1-git-send-email-miklos@szeredi.hu>
` (4 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
6 siblings, 1 reply; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ messages in thread
* [PATCH 08/11] gfs2: fix dentry leaks
[not found] <1379335925-30858-1-git-send-email-miklos@szeredi.hu>
` (5 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
6 siblings, 0 replies; 26+ 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] 26+ messages in thread