Signed-off-by: OGAWA Hirofumi <[email protected]>
---
fs/namei.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff -puN fs/namei.c~namei-path_to_nameidata-cleanup fs/namei.c
--- linux-2.6/fs/namei.c~namei-path_to_nameidata-cleanup 2010-01-10 21:15:42.000000000 +0900
+++ linux-2.6-hirofumi/fs/namei.c 2010-01-12 00:15:04.000000000 +0900
@@ -541,9 +541,10 @@ static void path_put_conditional(struct
static inline void path_to_nameidata(struct path *path, struct nameidata *nd)
{
dput(nd->path.dentry);
- if (nd->path.mnt != path->mnt)
+ if (nd->path.mnt != path->mnt) {
mntput(nd->path.mnt);
- nd->path.mnt = path->mnt;
+ nd->path.mnt = path->mnt;
+ }
nd->path.dentry = path->dentry;
}
_
--
OGAWA Hirofumi <[email protected]>
Hi,
I noticed refcnt leak in do_filp_open() by recent change. Could you
review this one?
__do_follow_link() handles "nd->path and path" refcnt by special way.
If path->mnt == nd->path.mnt (i.e. those is sharing the refcnt), it
gets refcnt of path->mnt, to make simple error path of it.
So, we can't call __do_follow_link() twice without special care,
because it will get refcnt of path->mnt twice. (i.e. current
do_filp_open() is leaking path->mnt if first __do_follow_link()
returned -ESTALE and path->mnt != nd->path.mnt)
This moves the special refcnt handling from __do_follow_link() as
path_unshare_refcnt(). Then call it once for that do_filp_open() path.
Signed-off-by: OGAWA Hirofumi <[email protected]>
---
fs/namei.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff -puN fs/namei.c~namei-do_filp_open-fix fs/namei.c
--- linux-2.6/fs/namei.c~namei-do_filp_open-fix 2010-01-12 00:15:08.000000000 +0900
+++ linux-2.6-hirofumi/fs/namei.c 2010-01-12 00:15:08.000000000 +0900
@@ -548,6 +548,18 @@ static inline void path_to_nameidata(str
nd->path.dentry = path->dentry;
}
+/* NOTE: this is intended as special operation for __do_follow_link() */
+static inline void path_unshare_refcnt(struct path *path, struct nameidata *nd)
+{
+ if (path->mnt == nd->path.mnt)
+ mntget(path->mnt);
+}
+
+/*
+ * The caller should take special care about path and nd->path
+ * reference count. (e.g. to make simple error path, call
+ * path_unshare_refcnt() before this, etc.)
+ */
static __always_inline int __do_follow_link(struct path *path, struct nameidata *nd)
{
int error;
@@ -559,9 +571,8 @@ static __always_inline int __do_follow_l
if (path->mnt != nd->path.mnt) {
path_to_nameidata(path, nd);
- dget(dentry);
+ path_get(path);
}
- mntget(path->mnt);
cookie = dentry->d_inode->i_op->follow_link(dentry, nd);
error = PTR_ERR(cookie);
if (!IS_ERR(cookie)) {
@@ -602,6 +613,7 @@ static inline int do_follow_link(struct
current->link_count++;
current->total_link_count++;
nd->depth++;
+ path_unshare_refcnt(path, nd);
err = __do_follow_link(path, nd);
path_put(path);
current->link_count--;
@@ -1856,6 +1868,7 @@ do_link:
goto exit_dput;
save = nd.path;
path_get(&save);
+ path_unshare_refcnt(&path, &nd);
error = __do_follow_link(&path, &nd);
if (error == -ESTALE) {
/* nd.path had been dropped */
_
--
OGAWA Hirofumi <[email protected]>
On Tue, Jan 12, 2010 at 03:35:13AM +0900, OGAWA Hirofumi wrote:
> Hi,
>
> I noticed refcnt leak in do_filp_open() by recent change. Could you
> review this one?
>
>
> __do_follow_link() handles "nd->path and path" refcnt by special way.
> If path->mnt == nd->path.mnt (i.e. those is sharing the refcnt), it
> gets refcnt of path->mnt, to make simple error path of it.
>
> So, we can't call __do_follow_link() twice without special care,
> because it will get refcnt of path->mnt twice. (i.e. current
> do_filp_open() is leaking path->mnt if first __do_follow_link()
> returned -ESTALE and path->mnt != nd->path.mnt)
>
> This moves the special refcnt handling from __do_follow_link() as
> path_unshare_refcnt(). Then call it once for that do_filp_open() path.
Point, but... that's not the way I'd do it (again, see #untested for the
direction it's heading). What we ought to do is simply "put ourselves
in trust-no-one mode (LOOKUP_REVAL) and restart the entire thing; if
we'd already been through that, fail immediately".
And yes, it needs to be pulled in front of queue and go in before .34.
Will do shortly.
On Wed, Jan 13, 2010 at 07:51:00PM +0000, Al Viro wrote:
> Point, but... that's not the way I'd do it (again, see #untested for the
> direction it's heading). What we ought to do is simply "put ourselves
> in trust-no-one mode (LOOKUP_REVAL) and restart the entire thing; if
> we'd already been through that, fail immediately".
>
> And yes, it needs to be pulled in front of queue and go in before .34.
> Will do shortly.
See commit 1fe1244faf0e862342bd2ae29e341cc957469aee in vfs-2.6.git#for-linus
(that's going to be rc5 fixes push tonight, after several more things get
shifted there).
Al Viro <[email protected]> writes:
> On Wed, Jan 13, 2010 at 07:51:00PM +0000, Al Viro wrote:
>
>> Point, but... that's not the way I'd do it (again, see #untested for the
>> direction it's heading). What we ought to do is simply "put ourselves
>> in trust-no-one mode (LOOKUP_REVAL) and restart the entire thing; if
>> we'd already been through that, fail immediately".
>>
>> And yes, it needs to be pulled in front of queue and go in before .34.
>> Will do shortly.
>
> See commit 1fe1244faf0e862342bd2ae29e341cc957469aee in vfs-2.6.git#for-linus
> (that's going to be rc5 fixes push tonight, after several more things get
> shifted there).
Thanks. I'll see later.
--
OGAWA Hirofumi <[email protected]>