Message ID | 20210326154032.86410-1-jlayton@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ceph: fix inode leak on getattr error in __fh_to_dentry | expand |
On 2021/3/26 23:40, Jeff Layton wrote: > Cc: Luis Henriques <lhenriques@suse.de> > Fixes: 878dabb64117 (ceph: don't return -ESTALE if there's still an open file) > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/export.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c > index f22156ee7306..17d8c8f4ec89 100644 > --- a/fs/ceph/export.c > +++ b/fs/ceph/export.c > @@ -178,8 +178,10 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino) > return ERR_CAST(inode); > /* We need LINK caps to reliably check i_nlink */ > err = ceph_do_getattr(inode, CEPH_CAP_LINK_SHARED, false); > - if (err) > + if (err) { > + iput(inode); > return ERR_PTR(err); > + } > /* -ESTALE if inode as been unlinked and no file is open */ > if ((inode->i_nlink == 0) && (atomic_read(&inode->i_count) == 1)) { > iput(inode); Reviewed-by: Xiubo Li <xiubli@redhat.com>
On Fri, Mar 26, 2021 at 11:40:32AM -0400, Jeff Layton wrote: > Cc: Luis Henriques <lhenriques@suse.de> > Fixes: 878dabb64117 (ceph: don't return -ESTALE if there's still an open file) > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/export.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c > index f22156ee7306..17d8c8f4ec89 100644 > --- a/fs/ceph/export.c > +++ b/fs/ceph/export.c > @@ -178,8 +178,10 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino) > return ERR_CAST(inode); > /* We need LINK caps to reliably check i_nlink */ > err = ceph_do_getattr(inode, CEPH_CAP_LINK_SHARED, false); > - if (err) > + if (err) { > + iput(inode); To be honest, I'm failing to see where we could be leaking the inode here. We're trying to get LINK caps to do the check bellow; if ceph_do_getattr() fails, the inode reference it (may) grabs will be released by calling ceph_mdsc_put_request(). Do you see any other possibility? Cheers, -- Luís > return ERR_PTR(err); > + } > /* -ESTALE if inode as been unlinked and no file is open */ > if ((inode->i_nlink == 0) && (atomic_read(&inode->i_count) == 1)) { > iput(inode); > -- > 2.30.2 >
On Tue, 2021-03-30 at 14:46 +0100, Luis Henriques wrote: > On Fri, Mar 26, 2021 at 11:40:32AM -0400, Jeff Layton wrote: > > Cc: Luis Henriques <lhenriques@suse.de> > > Fixes: 878dabb64117 (ceph: don't return -ESTALE if there's still an open file) > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ceph/export.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c > > index f22156ee7306..17d8c8f4ec89 100644 > > --- a/fs/ceph/export.c > > +++ b/fs/ceph/export.c > > @@ -178,8 +178,10 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino) > > return ERR_CAST(inode); > > /* We need LINK caps to reliably check i_nlink */ > > err = ceph_do_getattr(inode, CEPH_CAP_LINK_SHARED, false); > > - if (err) > > + if (err) { > > + iput(inode); > > To be honest, I'm failing to see where we could be leaking the inode here. > We're trying to get LINK caps to do the check bellow; if ceph_do_getattr() > fails, the inode reference it (may) grabs will be released by calling > ceph_mdsc_put_request(). > > Do you see any other possibility? > We already hold a reference to the inode at this point by virtue of the successful return from __lookup_inode. ceph_do_getattr does not consume that reference on success or failure, AFAICT.
On Tue, Mar 30, 2021 at 12:53:51PM -0400, Jeff Layton wrote: > On Tue, 2021-03-30 at 14:46 +0100, Luis Henriques wrote: > > On Fri, Mar 26, 2021 at 11:40:32AM -0400, Jeff Layton wrote: > > > Cc: Luis Henriques <lhenriques@suse.de> > > > Fixes: 878dabb64117 (ceph: don't return -ESTALE if there's still an open file) > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/ceph/export.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/ceph/export.c b/fs/ceph/export.c > > > index f22156ee7306..17d8c8f4ec89 100644 > > > --- a/fs/ceph/export.c > > > +++ b/fs/ceph/export.c > > > @@ -178,8 +178,10 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino) > > > return ERR_CAST(inode); > > > /* We need LINK caps to reliably check i_nlink */ > > > err = ceph_do_getattr(inode, CEPH_CAP_LINK_SHARED, false); > > > - if (err) > > > + if (err) { > > > + iput(inode); > > > > To be honest, I'm failing to see where we could be leaking the inode here. > > We're trying to get LINK caps to do the check bellow; if ceph_do_getattr() > > fails, the inode reference it (may) grabs will be released by calling > > ceph_mdsc_put_request(). > > > > Do you see any other possibility? > > > > We already hold a reference to the inode at this point by virtue of the > successful return from __lookup_inode. ceph_do_getattr does not consume > that reference on success or failure, AFAICT. Doh! Of course. I was looking at it the wrong way. Cheers, -- Luís
diff --git a/fs/ceph/export.c b/fs/ceph/export.c index f22156ee7306..17d8c8f4ec89 100644 --- a/fs/ceph/export.c +++ b/fs/ceph/export.c @@ -178,8 +178,10 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino) return ERR_CAST(inode); /* We need LINK caps to reliably check i_nlink */ err = ceph_do_getattr(inode, CEPH_CAP_LINK_SHARED, false); - if (err) + if (err) { + iput(inode); return ERR_PTR(err); + } /* -ESTALE if inode as been unlinked and no file is open */ if ((inode->i_nlink == 0) && (atomic_read(&inode->i_count) == 1)) { iput(inode);
Cc: Luis Henriques <lhenriques@suse.de> Fixes: 878dabb64117 (ceph: don't return -ESTALE if there's still an open file) Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/export.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)