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  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]
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/

Powered by blists - more mailing lists