[PATCH 2/7] revoke: add f_light flag for struct file

[PATCH 2/7] revoke: add f_light flag for struct file

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

This adds a f_light flag to struct file to indicate that the file was
looked up with fget_light().  Needed by revoke to ensure we don't
close a file pointer while someone is using it without actually
holding a reference.

These bits were taken from the forced unmount patches by Tigran
Aivazian.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 fs/file_table.c      |    1 +
 include/linux/file.h |   14 ++++++++++++++
 include/linux/fs.h   |    2 ++
 3 files changed, 17 insertions(+)

Index: uml-2.6/fs/file_table.c
===================================================================
--- uml-2.6.orig/fs/file_table.c	2007-03-08 10:24:24.000000000 +0200
+++ uml-2.6/fs/file_table.c	2007-03-08 10:24:31.000000000 +0200
@@ -219,6 +219,7 @@
 	*fput_needed = 0;
 	if (likely((atomic_read(&files->count) == 1))) {
 		file = fcheck_files(files, fd);
+		set_f_light(file);
 	} else {
 		rcu_read_lock();
 		file = fcheck_files(files, fd);
Index: uml-2.6/include/linux/file.h
===================================================================
--- uml-2.6.orig/include/linux/file.h	2007-03-08 10:24:24.000000000 +0200
+++ uml-2.6/include/linux/file.h	2007-03-08 10:24:31.000000000 +0200
@@ -6,6 +6,7 @@
 #define __LINUX_FILE_H
 
 #include <asm/atomic.h>
+#include <linux/fs.h>
 #include <linux/posix_types.h>
 #include <linux/compiler.h>
 #include <linux/spinlock.h>
@@ -62,10 +63,23 @@
 extern void FASTCALL(__fput(struct file *));
 extern void FASTCALL(fput(struct file *));
 
+static inline void clear_f_light(struct file *file)
+{
+	file->f_light = 0;
+}
+
+static inline void set_f_light(struct file *file)
+{
+	if (file)
+		file->f_light = 1;
+}
+
 static inline void fput_light(struct file *file, int fput_needed)
 {
 	if (unlikely(fput_needed))
 		fput(file);
+	else
+		clear_f_light(file);
 }
 
 extern struct file * FASTCALL(fget(unsigned int fd));
Index: uml-2.6/include/linux/fs.h
===================================================================
--- uml-2.6.orig/include/linux/fs.h	2007-03-08 10:24:24.000000000 +0200
+++ uml-2.6/include/linux/fs.h	2007-03-08 10:24:31.000000000 +0200
@@ -739,6 +739,8 @@
 	struct list_head	f_ep_links;
 	spinlock_t		f_ep_lock;
 #endif /* #ifdef CONFIG_EPOLL */
+	/* This instance is being used without holding a reference. */
+	int			f_light;
 	struct address_space	*f_mapping;
 };
 extern spinlock_t files_lock;

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Eric Dumazet]

On Friday 09 March 2007 09:14, Pekka J Enberg wrote:
> From: Pekka Enberg <penberg@cs.helsinki.fi>
>
> This adds a f_light flag to struct file to indicate that the file was
> looked up with fget_light().  Needed by revoke to ensure we don't
> close a file pointer while someone is using it without actually
> holding a reference.
>
> These bits were taken from the forced unmount patches by Tigran
> Aivazian.

Well, I disagree very much with this patch.

One of the interest of fget_light() is not dirtying file structure (avoiding 
atomic changes to f_count).

You add a 'flag' (4 bytes !) at the end of the file structure (so in a 
different cache line than the parts that are usually accessed in a fd_related 
syscall) and dirty this part at syscall entry and exit. Thats really a heavy 
price for supporting an unlikely revoke() syscall.

Also, the thing is racy.

( BTW, the whole revoke() concept is evil, especially if we want to avoid 
using inodes/dentries for some kind of pseudo files like sockets / pipes)

Cannot we use a flag in 'struct files_struct', set to one when the task is 
mono-thread (at task creation in fact), and set to 0 when it creates a new 
thread  (or when someone remotely access to this "struct files_struct" 
in /proc/pid/fd/... )

No need to set back this flag to 1 when task revert to mono-threaded, since 
this case is probably unlikely. This way we can be non racy.

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Pekka Enberg]

On 3/9/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> Cannot we use a flag in 'struct files_struct', set to one when the task is
> mono-thread (at task creation in fact), and set to 0 when it creates a new
> thread  (or when someone remotely access to this "struct files_struct"
> in /proc/pid/fd/... )

How does that work? fget_light() has a built-in assumption that as
long as you don't share files_struct, it's okay not to take an extra
reference as current is only one doing close(2) and revoke(2) changes
that. So it's not really about being single-threaded or not.

On 3/9/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> Also, the thing is racy.

Aah, fget_light() indeed has a race window between fcheck_files() and
set_f_light().

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Eric Dumazet]

On Friday 09 March 2007 11:43, Pekka Enberg wrote:
> On 3/9/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> > Cannot we use a flag in 'struct files_struct', set to one when the task
> > is mono-thread (at task creation in fact), and set to 0 when it creates a
> > new thread  (or when someone remotely access to this "struct
> > files_struct" in /proc/pid/fd/... )
>
> How does that work? fget_light() has a built-in assumption that as
> long as you don't share files_struct, it's okay not to take an extra
> reference as current is only one doing close(2) and revoke(2) changes
> that. So it's not really about being single-threaded or not.

I just dropped one (silly ?) idea and expected you made the hard work :)

Then just drop the fget_light() 'optimisation' and always take a reference 
(atomic on f_count) regardless of single-thread or not. Instead of dirtying 
f_light, just do the straightforward thing and be with it.

(that is : fget_light() = fget() = no more keeping fput_needed everywhere, and 
convoluted things in some dark sides of the kernel.

It will save some conditional branches and complexity, and you dont need this 
f_light thing.

>
> On 3/9/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> > Also, the thing is racy.
>
> Aah, fget_light() indeed has a race window between fcheck_files() and
> set_f_light().

Yes, you see how hard it is to get this right.

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Pekka Enberg]

On 3/9/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> Then just drop the fget_light() 'optimisation' and always take a reference
> (atomic on f_count) regardless of single-thread or not. Instead of dirtying
> f_light, just do the straightforward thing and be with it.

That's what I did first but akpm thought it was "unfortunate." Hmm.. ;-)

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Pekka Enberg]

On 3/9/07, Eric Dumazet <dada1@cosmosbay.com> wrote:
> > Then just drop the fget_light() 'optimisation' and always take a reference
> > (atomic on f_count) regardless of single-thread or not. Instead of dirtying
> > f_light, just do the straightforward thing and be with it.

On 3/9/07, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> That's what I did first but akpm thought it was "unfortunate." Hmm.. ;-)

Btw, what we can do is delay closing the actual revoked file until the
task terminates. This has the unfortunate side-effect that a task has
no way of freeing the resources now. But, I am beginning to think it's
not a big problem because the inode mapping will be zapped immediately
upon revoke anyway...

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Pekka J Enberg]

At some point in time, I wrote:
> > Btw, what we can do is delay closing the actual revoked file until the
> > task terminates. This has the unfortunate side-effect that a task has
> > no way of freeing the resources now. But, I am beginning to think it's
> > not a big problem because the inode mapping will be zapped immediately
> > upon revoke anyway...
 
On Fri, 9 Mar 2007, Alan Cox wrote:
> Actually you can't entirely do this. The revoke() definition states
> explicitly that the driver close occurs at the point of revoke() not
> later.
> 
> That can however be pushed into the device revoke method for the cases
> where it might matter (eg tty).

Yeah, you just make f_ops->revoke close the driver and f_ops->flush a 
no-op if the driver has already been closed.

			Pekka

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Alan Cox]

> Btw, what we can do is delay closing the actual revoked file until the
> task terminates. This has the unfortunate side-effect that a task has
> no way of freeing the resources now. But, I am beginning to think it's
> not a big problem because the inode mapping will be zapped immediately
> upon revoke anyway...

Actually you can't entirely do this. The revoke() definition states
explicitly that the driver close occurs at the point of revoke() not
later.

That can however be pushed into the device revoke method for the cases
where it might matter (eg tty).

Alan

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Benjamin LaHaise]

On Fri, Mar 09, 2007 at 12:13:35PM +0100, Eric Dumazet wrote:
> Then just drop the fget_light() 'optimisation' and always take a reference 
> (atomic on f_count) regardless of single-thread or not. Instead of dirtying 
> f_light, just do the straightforward thing and be with it.
> 
> (that is : fget_light() = fget() = no more keeping fput_needed everywhere, and 
> convoluted things in some dark sides of the kernel.

And it makes things rather slower for a lot of single threaded applications 
on modern systems.  Yes, fget_light can be done much more cleanly, but please 
don't go around ripping out optimizations just because.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <zyntrop@kvack.org>.

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Pekka Enberg]

On Fri, Mar 09, 2007 at 12:13:35PM +0100, Eric Dumazet wrote:
> > Then just drop the fget_light() 'optimisation' and always take a reference
> > (atomic on f_count) regardless of single-thread or not. Instead of dirtying
> > f_light, just do the straightforward thing and be with it.
> >
> > (that is : fget_light() = fget() = no more keeping fput_needed everywhere, and
> > convoluted things in some dark sides of the kernel.

On 3/9/07, Benjamin LaHaise <bcrl@kvack.org> wrote:
> And it makes things rather slower for a lot of single threaded applications
> on modern systems.  Yes, fget_light can be done much more cleanly, but please
> don't go around ripping out optimizations just because.

Don't worry, the fget_light() bits are no longer needed:
http://lkml.org/lkml/2007/3/9/151

Re: [PATCH 2/7] revoke: add f_light flag for struct file

[Eric Dumazet]

On Friday 09 March 2007 17:11, Benjamin LaHaise wrote:
> On Fri, Mar 09, 2007 at 12:13:35PM +0100, Eric Dumazet wrote:
> > Then just drop the fget_light() 'optimisation' and always take a
> > reference (atomic on f_count) regardless of single-thread or not. Instead
> > of dirtying f_light, just do the straightforward thing and be with it.
> >
> > (that is : fget_light() = fget() = no more keeping fput_needed
> > everywhere, and convoluted things in some dark sides of the kernel.
>
> And it makes things rather slower for a lot of single threaded applications
> on modern systems.  Yes, fget_light can be done much more cleanly, but
> please don't go around ripping out optimizations just because.

Sure. But I apparently was the only guy to react to the f_light horror story.

And it seems a solution was found, after some mail exchanges.

In French we have this expression : "Precher le faux pour savoir le vrai"

You could translate to "make false statements in order to discover the truth" 
or "to tell a lie in order to get at the truth" or maybe "playing the devil's 
advocate", but really the French one is better :)