Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1641751 > unrolled thread

[PATCH] ubifs: Fix inode leak in xattr code

Back to article view | Back to linux.kernel

Contents

  [PATCH] ubifs: Fix inode leak in xattr code Richard Weinberger <richard@nod.at> - 2017-05-15 16:30 +0200
    Re: [PATCH] ubifs: Fix inode leak in xattr code Artem Bityutskiy <dedekind1@gmail.com> - 2017-05-15 17:00 +0200
      Re: [PATCH] ubifs: Fix inode leak in xattr code Richard Weinberger <richard@nod.at> - 2017-05-15 17:30 +0200
        Re: [PATCH] ubifs: Fix inode leak in xattr code Artem Bityutskiy <dedekind1@gmail.com> - 2017-05-15 18:10 +0200
          Re: [PATCH] ubifs: Fix inode leak in xattr code Richard Weinberger <richard@nod.at> - 2017-05-15 18:30 +0200
          Re: [PATCH] ubifs: Fix inode leak in xattr code Richard Weinberger <richard@nod.at> - 2017-05-15 20:00 +0200
          Re: [PATCH] ubifs: Fix inode leak in xattr code Richard Weinberger <richard@nod.at> - 2017-05-17 00:30 +0200

#1641751 — [PATCH] ubifs: Fix inode leak in xattr code

UBIFS handles extended attributes just like files, as consequence of
that, they also have inodes.
Therefore UBIFS does all the inode machinery also for xattrs. Since new
inodes have i_nlink of 1, a file or xattr inode will be evicted
if i_nlink goes down to 0 after an unlink. UBIFS assumes this model also
for xattrs, which is not correct.
One can create a file "foo" with xattr "user.test". By reading
"user.test" an inode will be created, and by deleting "user.test" it
will get evicted later. The assumption breaks if the file "foo", which
hosts the xattrs, will be removed. VFS nor UBIFS does not remove each
xattr via ubifs_xattr_remove(), it just removes the host inode from
the TNC and all underlying xattr nodes too.
The inode will stay in the system with i_count=0, i_nlink=1 and
i_state=I_REFERENCED until UBIFS is umounted.

To solve this problem, set i_nlink for all xattr inodes to 0, such that
the iput() in the UBIFS xattr code makes the temporary inode vanish
immediately.

Fixes: 1e51764a3c2ac05a ("UBIFS: add new flash file system")
Cc: <stable@vger.kernel.org>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/xattr.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index efe00fcb8b75..7d23404d73dc 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -173,7 +173,7 @@ static int create_xattr(struct ubifs_info *c, struct inode *host,
 	mutex_unlock(&host_ui->ui_mutex);
 
 	ubifs_release_budget(c, &req);
-	insert_inode_hash(inode);
+	clear_nlink(inode);
 	iput(inode);
 	return 0;
 
@@ -272,8 +272,10 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
 			  (int)PTR_ERR(inode));
 		return inode;
 	}
-	if (ubifs_inode(inode)->xattr)
+	if (ubifs_inode(inode)->xattr) {
+		clear_nlink(inode);
 		return inode;
+	}
 	ubifs_err(c, "corrupt extended attribute entry");
 	iput(inode);
 	return ERR_PTR(-EINVAL);
@@ -546,7 +548,6 @@ static int ubifs_xattr_remove(struct inode *host, const char *name)
 	}
 
 	ubifs_assert(inode->i_nlink == 1);
-	clear_nlink(inode);
 	err = remove_xattr(c, host, inode, &nm);
 	if (err)
 		set_nlink(inode, 1);
-- 
2.7.3

[toc] | [next] | [standalone]

#1641775

On Mon, 2017-05-15 at 16:20 +0200, Richard Weinberger wrote:
> To solve this problem, set i_nlink for all xattr inodes to 0, such
> that
> the iput() in the UBIFS xattr code makes the temporary inode vanish
> immediately.

What if there is iget right after iput? With this patch, will we need
to go all the way to the slow media instead of just getting having the
inode from the inode cache?

[toc] | [prev] | [next] | [standalone]

#1641811

Artem,

Am 15.05.2017 um 16:53 schrieb Artem Bityutskiy:
> On Mon, 2017-05-15 at 16:20 +0200, Richard Weinberger wrote:
>> To solve this problem, set i_nlink for all xattr inodes to 0, such
>> that
>> the iput() in the UBIFS xattr code makes the temporary inode vanish
>> immediately.
> 
> What if there is iget right after iput? With this patch, will we need
> to go all the way to the slow media instead of just getting having the
> inode from the inode cache?

Hmm, why would we go down to the slow media?
Since the xattr was just looked up there is a high chance that the TNC
will still contain the node.
A new inode needs to be allocated, though.

Alternatively we could add a iget_locked/drop_nlink/iput sequence to
ubifs_tnc_remove_ino(). But that will make unlink() much slower for files
that contain xattrs.

Thanks,
//richard

[toc] | [prev] | [next] | [standalone]

#1641862

On Mon, 2017-05-15 at 17:22 +0200, Richard Weinberger wrote:
> Alternatively we could add a iget_locked/drop_nlink/iput sequence to
> ubifs_tnc_remove_ino(). But that will make unlink() much slower for
> files
> that contain xattrs.

At that level we'd need to do it for every xattr, even those that were
never be accessed, which would be slow indeed.

But we really only need to check the inode cache: hey, icache, I am
dying, and if you have any of my guys (xattrs), I want them to die with
me. 

So the question is how to find our guys in the inode cache. I am not
sure. Probably be we'd have to have our own list of cached inodes in
the host inode, and maintain it.

Artem.

[toc] | [prev] | [next] | [standalone]

#1641874

Artem,

Am 15.05.2017 um 18:05 schrieb Artem Bityutskiy:
> On Mon, 2017-05-15 at 17:22 +0200, Richard Weinberger wrote:
>> Alternatively we could add a iget_locked/drop_nlink/iput sequence to
>> ubifs_tnc_remove_ino(). But that will make unlink() much slower for
>> files
>> that contain xattrs.
> 
> At that level we'd need to do it for every xattr, even those that were
> never be accessed, which would be slow indeed.
> 
> But we really only need to check the inode cache: hey, icache, I am
> dying, and if you have any of my guys (xattrs), I want them to die with
> me. 
> 
> So the question is how to find our guys in the inode cache. I am not
> sure. Probably be we'd have to have our own list of cached inodes in
> the host inode, and maintain it.

Before we try to be clever and implement that, let me benchmark the current
approach.
Debugging leaking inodes is no fun and I'd like to avoid that for a second
time. ;-)

Thanks,
//richard

[toc] | [prev] | [next] | [standalone]

#1641948

Artem,

Am 15.05.2017 um 18:05 schrieb Artem Bityutskiy:
> On Mon, 2017-05-15 at 17:22 +0200, Richard Weinberger wrote:
>> Alternatively we could add a iget_locked/drop_nlink/iput sequence to
>> ubifs_tnc_remove_ino(). But that will make unlink() much slower for
>> files
>> that contain xattrs.
> 
> At that level we'd need to do it for every xattr, even those that were
> never be accessed, which would be slow indeed.
> 
> But we really only need to check the inode cache: hey, icache, I am
> dying, and if you have any of my guys (xattrs), I want them to die with
> me. 
> 
> So the question is how to find our guys in the inode cache. I am not
> sure. Probably be we'd have to have our own list of cached inodes in
> the host inode, and maintain it.

BTW: Do you happen to know how other filesystems cache xattrs?
If they actually do.

Thanks,
//richard

[toc] | [prev] | [next] | [standalone]

#1642880

Artem,

Am 15.05.2017 um 18:05 schrieb Artem Bityutskiy:
> On Mon, 2017-05-15 at 17:22 +0200, Richard Weinberger wrote:
>> Alternatively we could add a iget_locked/drop_nlink/iput sequence to
>> ubifs_tnc_remove_ino(). But that will make unlink() much slower for
>> files
>> that contain xattrs.
> 
> At that level we'd need to do it for every xattr, even those that were
> never be accessed, which would be slow indeed.
> 
> But we really only need to check the inode cache: hey, icache, I am
> dying, and if you have any of my guys (xattrs), I want them to die with
> me. 

That turned out to be much easier than expected.
Patch ahead!

Thanks,
//richard

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web