linux-kernel - Re: [PATCH 0/5] fuse: handle release synchronously (v4)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141017085509.GE5011@tucsk.piliscsaba.szeredi.hu>
Date:	Fri, 17 Oct 2014 10:55:09 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Maxim Patlasov <mpatlasov@...allels.com>,
	Anand Avati <avati@...ster.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Michael j Theall <mtheall@...ibm.com>,
	fuse-devel <fuse-devel@...ts.sourceforge.net>
Subject: Re: [PATCH 0/5] fuse: handle release synchronously (v4)

On Thu, Oct 16, 2014 at 03:54:42PM +0200, Linus Torvalds wrote:
> On Thu, Oct 16, 2014 at 3:43 PM, Miklos Szeredi <miklos@...redi.hu> wrote:
> >
> > One idea is to change ->flush() so it's responsible for fput()-ing the
> > file.  That way we could take control of the actual refcount
> > decrement.  There are only 20 flush instances in the tree, so it
> > wouldn't be a huge change.
> 
> Since that *still* wouldn't fix the problem with the whole "count
> elevated by other things" issue, I really don't want to hear about
> these random broken hacks that are fundamentally broken crap.

The problem with those "count elevated by other things" is that they are
actually bugs.  Take the following test case (this is not made up, I really got
bug reports agains something like this).

mount("/dev/foo", "/mnt");
fd = creat("/mnt/bar");
close(fd);
umount("/mnt")

If that umount fails with EBUSY, that's a bug, since we've released the only
known resource we've opened on that mount.

Now, if some completely unrelated "lsof" instance goes fishing in /proc, then
that will be able to hold that release up, making the test fail.

And it's actually trivially fixable.  See attached patch.

Thanks,
Miklos

diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index e11d7c590bb0..f8a67dafb04f 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -19,6 +19,8 @@ static int seq_show(struct seq_file *m, void *v)
 {
 	struct files_struct *files = NULL;
 	int f_flags = 0, ret = -ENOENT;
+	loff_t f_pos;
+	int mnt_id;
 	struct file *file = NULL;
 	struct task_struct *task;
 
@@ -41,7 +43,13 @@ static int seq_show(struct seq_file *m, void *v)
 			if (close_on_exec(fd, fdt))
 				f_flags |= O_CLOEXEC;
 
-			get_file(file);
+			f_pos = file->f_pos;
+			mnt_id = real_mount(file->f_path.mnt)->mnt_id;
+
+			if (file->f_op->show_fdinfo)
+				get_file(file);
+			else
+				file = NULL;
 			ret = 0;
 		}
 		spin_unlock(&files->file_lock);
@@ -50,11 +58,11 @@ static int seq_show(struct seq_file *m, void *v)
 
 	if (!ret) {
 		seq_printf(m, "pos:\t%lli\nflags:\t0%o\nmnt_id:\t%i\n",
-			   (long long)file->f_pos, f_flags,
-			   real_mount(file->f_path.mnt)->mnt_id);
-		if (file->f_op->show_fdinfo)
+			   (long long) f_pos, f_flags, mnt_id);
+		if (file) {
 			ret = file->f_op->show_fdinfo(m, file);
-		fput(file);
+			fput(file);
+		}
 	}
 
 	return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/