[PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups

[PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups

[Oleg Nesterov]

Hello.

I tried to audit the users of thread_group_empty() (we need
to change it) and found rcu_my_thread_group_empty() which
looks wrong.

The patches look simple, but I am not sure it is fine to use
rcu_lock_acquire() directly. Perhaps it makes sense to add a
new helper? Note that we have more users which take rcu lock
only to shut up lockdep. Please review.

And I am a bit confused. Perhaps rcu_lock_acquire() should
depend on CONFIG_PROVE_RCU, not on CONFIG_DEBUG_LOCK_ALLOC?
We only need rcu_lock_map/etc for rcu_lockdep_assert().

Oleg.

 fs/file.c                |   24 +++++++++++-------------
 include/linux/fdtable.h  |   19 +++++++++++++------
 include/linux/rcupdate.h |    2 --
 kernel/rcu/update.c      |   11 -----------
 4 files changed, 24 insertions(+), 32 deletions(-)

[PATCH 1/2] introduce __fcheck_files() to fix rcu_dereference_check_fdtable(), kill rcu_my_thread_group_empty()

[Oleg Nesterov]

rcu_dereference_check_fdtable() looks very wrong,

1. rcu_my_thread_group_empty() was added by 844b9a8707f1 "vfs: fix
   RCU-lockdep false positive due to /proc" but it doesn't really
   fix the problem. A CLONE_THREAD (without CLONE_FILES) task can
   hit the same race with get_files_struct().

   And otoh rcu_my_thread_group_empty() can suppress the correct
   warning if the caller is the CLONE_FILES (without CLONE_THREAD)
   task.

2. files->count == 1 check is not really right too. Even if this
   files_struct is not shared it is not safe to access it lockless
   unless the caller is the owner.

   Otoh, this check is sub-optimal. files->count == 0 always means
   it is safe to use it lockless even if files != current->files,
   but put_files_struct() has to take rcu_read_lock(). See the next
   patch.

This patch removes the buggy checks and adds the new helper which
simply does rcu_lock_acquire/release around fcheck_files(). The
files->count == 1 callers, fget_light() and fget_raw_light(), can
use this helper to avoid the warning from RCU-lockdep.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/file.c                |    4 ++--
 include/linux/fdtable.h  |   19 +++++++++++++------
 include/linux/rcupdate.h |    2 --
 kernel/rcu/update.c      |   11 -----------
 4 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 4a78f98..957cbc0 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -707,7 +707,7 @@ struct file *fget_light(unsigned int fd, int *fput_needed)
 
 	*fput_needed = 0;
 	if (atomic_read(&files->count) == 1) {
-		file = fcheck_files(files, fd);
+		file = __fcheck_files(files, fd);
 		if (file && (file->f_mode & FMODE_PATH))
 			file = NULL;
 	} else {
@@ -735,7 +735,7 @@ struct file *fget_raw_light(unsigned int fd, int *fput_needed)
 
 	*fput_needed = 0;
 	if (atomic_read(&files->count) == 1) {
-		file = fcheck_files(files, fd);
+		file = __fcheck_files(files, fd);
 	} else {
 		rcu_read_lock();
 		file = fcheck_files(files, fd);
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 085197b..f39d50a 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -60,10 +60,7 @@ struct files_struct {
 };
 
 #define rcu_dereference_check_fdtable(files, fdtfd) \
-	(rcu_dereference_check((fdtfd), \
-			       lockdep_is_held(&(files)->file_lock) || \
-			       atomic_read(&(files)->count) == 1 || \
-			       rcu_my_thread_group_empty()))
+	rcu_dereference_check((fdtfd), lockdep_is_held(&(files)->file_lock))
 
 #define files_fdtable(files) \
 		(rcu_dereference_check_fdtable((files), (files)->fdt))
@@ -74,9 +71,9 @@ struct dentry;
 
 extern void __init files_defer_init(void);
 
-static inline struct file * fcheck_files(struct files_struct *files, unsigned int fd)
+static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
 {
-	struct file * file = NULL;
+	struct file *file = NULL;
 	struct fdtable *fdt = files_fdtable(files);
 
 	if (fd < fdt->max_fds)
@@ -84,6 +81,16 @@ static inline struct file * fcheck_files(struct files_struct *files, unsigned in
 	return file;
 }
 
+/* The caller must ensure that fd table isn't shared */
+static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
+{
+	struct file *file;
+	/* make rcu_dereference_check_fdtable() happy */
+	rcu_lock_acquire(&rcu_lock_map);
+	file = fcheck_files(files, fd);
+	rcu_lock_release(&rcu_lock_map);
+	return file;
+}
 /*
  * Check whether the specified fd has an open file.
  */
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 39cbb88..a2482cf 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -448,8 +448,6 @@ static inline int rcu_read_lock_sched_held(void)
 
 #ifdef CONFIG_PROVE_RCU
 
-extern int rcu_my_thread_group_empty(void);
-
 /**
  * rcu_lockdep_assert - emit lockdep splat if specified condition not met
  * @c: condition to check
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 6cb3dff..a3596c8 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -195,17 +195,6 @@ void wait_rcu_gp(call_rcu_func_t crf)
 }
 EXPORT_SYMBOL_GPL(wait_rcu_gp);
 
-#ifdef CONFIG_PROVE_RCU
-/*
- * wrapper function to avoid #include problems.
- */
-int rcu_my_thread_group_empty(void)
-{
-	return thread_group_empty(current);
-}
-EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
-#endif /* #ifdef CONFIG_PROVE_RCU */
-
 #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
 static inline void debug_init_rcu_head(struct rcu_head *head)
 {
-- 
1.5.5.1

[PATCH 2/2] change close_files() to use rcu_lock_acquire() to shut up RCU-lockdep

[Oleg Nesterov]

put_files_struct() and close_files() do rcu_read_lock() to make
rcu_dereference_check_fdtable() happy.

We can use rcu_lock_acquire(&rcu_lock_map) instead, this looks
self-explanatory and this is nop without CONFIG_DEBUG_LOCK_ALLOC().

While at it, change close_files() to return fdt, this avoids another
rcu_read_lock() + files_fdtable() in put_files_struct().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/file.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 957cbc0..716d747 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -348,7 +348,7 @@ out:
 	return NULL;
 }
 
-static void close_files(struct files_struct * files)
+static struct fdtable *close_files(struct files_struct * files)
 {
 	int i, j;
 	struct fdtable *fdt;
@@ -358,11 +358,12 @@ static void close_files(struct files_struct * files)
 	/*
 	 * It is safe to dereference the fd table without RCU or
 	 * ->file_lock because this is the last reference to the
-	 * files structure.  But use RCU to shut RCU-lockdep up.
+	 * files structure. But we need to shut RCU-lockdep up.
 	 */
-	rcu_read_lock();
+	rcu_lock_acquire(&rcu_lock_map);
 	fdt = files_fdtable(files);
-	rcu_read_unlock();
+	rcu_lock_release(&rcu_lock_map);
+
 	for (;;) {
 		unsigned long set;
 		i = j * BITS_PER_LONG;
@@ -381,6 +382,8 @@ static void close_files(struct files_struct * files)
 			set >>= 1;
 		}
 	}
+
+	return fdt;
 }
 
 struct files_struct *get_files_struct(struct task_struct *task)
@@ -398,14 +401,9 @@ struct files_struct *get_files_struct(struct task_struct *task)
 
 void put_files_struct(struct files_struct *files)
 {
-	struct fdtable *fdt;
-
 	if (atomic_dec_and_test(&files->count)) {
-		close_files(files);
-		/* not really needed, since nobody can see us */
-		rcu_read_lock();
-		fdt = files_fdtable(files);
-		rcu_read_unlock();
+		struct fdtable *fdt = close_files(files);
+
 		/* free the arrays if they are not embedded */
 		if (fdt != &files->fdtab)
 			__free_fdtable(fdt);
-- 
1.5.5.1

Re: [PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups

[Paul E. McKenney]

On Tue, Jan 07, 2014 at 07:12:58PM +0100, Oleg Nesterov wrote:
> Hello.
> 
> I tried to audit the users of thread_group_empty() (we need
> to change it) and found rcu_my_thread_group_empty() which
> looks wrong.
> 
> The patches look simple, but I am not sure it is fine to use
> rcu_lock_acquire() directly. Perhaps it makes sense to add a
> new helper? Note that we have more users which take rcu lock
> only to shut up lockdep. Please review.
> 
> And I am a bit confused. Perhaps rcu_lock_acquire() should
> depend on CONFIG_PROVE_RCU, not on CONFIG_DEBUG_LOCK_ALLOC?
> We only need rcu_lock_map/etc for rcu_lockdep_assert().

I am not all that excited about invoking rcu_lock_acquire() outside
of RCU...

Another approach would be to add an argument to files_fdtable()
that is zero normally and one for "we know we don't need RCU
protection."  Then rcu_dereference_check() could be something
like the following:

#define files_fdtable(files, c) \
		(rcu_dereference_check_fdtable((files), (files)->fdt) || c)

Would that work?

							Thanx, Paul

> Oleg.
> 
>  fs/file.c                |   24 +++++++++++-------------
>  include/linux/fdtable.h  |   19 +++++++++++++------
>  include/linux/rcupdate.h |    2 --
>  kernel/rcu/update.c      |   11 -----------
>  4 files changed, 24 insertions(+), 32 deletions(-)
> 

Re: [PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups

[Oleg Nesterov]

On 01/08, Paul E. McKenney wrote:
>
> I am not all that excited about invoking rcu_lock_acquire() outside
> of RCU...

Yes, me too. That is why I thought about the helper with a good name,
see below.

> Another approach would be to add an argument to files_fdtable()
> that is zero normally and one for "we know we don't need RCU
> protection."  Then rcu_dereference_check() could be something
> like the following:
>
> #define files_fdtable(files, c) \
> 		(rcu_dereference_check_fdtable((files), (files)->fdt) || c)
>
> Would that work?

Yes, I considered this optiion, but this needs much more uglifications^W
changes.

Either we need to change all users of files_fdtable(), or we need something
like

	#define __rcu_dereference_check_fdtable(files, unshared, fdtfd) \
		rcu_dereference_check((fdtfd), unshared || lockdep_is_held(&(files)->file_lock))

	#define rcu_dereference_check_fdtable(files, fdtfd)
		__rcu_dereference_check_fdtable(files, false, fdtfd)

	#define __files_fdtable(files)
		__rcu_dereference_check_fdtable((files), true, (files)->fdt)

	#define files_fdtable(files)
		__rcu_dereference_check_fdtable((files), false, (files)->fdt)

Plus we need

	static inline struct file *__fcheck_files(struct files_struct *files,
						bool unshared, unsigned int fd)
	{
		struct file *file = NULL;
		struct fdtable *fdt = __rcu_dereference_check_fdtable(files, unshared, files->fdt);

		if (fd < fdt->max_fds)
			file = __rcu_dereference_check_fdtable(files, unshared, fdt->fd[fd]);
		return file;
	}

doesn't look very nice...

As for 2/2, probably close_files() can simply do

	/*
	 * It is safe to dereference the fd table without RCU or
	 * ->file_lock because this is the last reference to the
	 * files structure.
	 */
	fdt = rcu_dereference_raw(files->fdt);

Or we can add

	#define __files_fdtable(files)	\
		 rcu_dereference_raw((files)->fdt)

but it is not clear to me what 1/1 should do. Perhaps

	static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
	{
		struct fdtable *fdt = rcu_dereference_raw(files->fdt);
		struct file *file = NULL;

		if (fd < fdt->max_fds)
			file = rcu_dereference_raw(fdt->fd[fd]);

		return file;
	}

	static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
	{
		rcu_lockdep_assert(rcu_read_lock_held() ||
				   lockdep_is_held(files->file_lock),
				   "message");
		return __fcheck_files(files, fd);
	}

?

Oleg.

Re: [PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups

[Paul E. McKenney]

On Wed, Jan 08, 2014 at 04:19:18PM +0100, Oleg Nesterov wrote:
> On 01/08, Paul E. McKenney wrote:
> >
> > I am not all that excited about invoking rcu_lock_acquire() outside
> > of RCU...
> 
> Yes, me too. That is why I thought about the helper with a good name,
> see below.
> 
> > Another approach would be to add an argument to files_fdtable()
> > that is zero normally and one for "we know we don't need RCU
> > protection."  Then rcu_dereference_check() could be something
> > like the following:
> >
> > #define files_fdtable(files, c) \
> > 		(rcu_dereference_check_fdtable((files), (files)->fdt) || c)
> >
> > Would that work?
> 
> Yes, I considered this optiion, but this needs much more uglifications^W
> changes.
> 
> Either we need to change all users of files_fdtable(), or we need something
> like

There are only about 20 uses of files_fdtable() in 3.12, with almost all
of them in fs/file.c.  So is changing all the users really all that
problematic?

							Thanx, Paul

> 	#define __rcu_dereference_check_fdtable(files, unshared, fdtfd) \
> 		rcu_dereference_check((fdtfd), unshared || lockdep_is_held(&(files)->file_lock))
> 
> 	#define rcu_dereference_check_fdtable(files, fdtfd)
> 		__rcu_dereference_check_fdtable(files, false, fdtfd)
> 
> 	#define __files_fdtable(files)
> 		__rcu_dereference_check_fdtable((files), true, (files)->fdt)
> 
> 	#define files_fdtable(files)
> 		__rcu_dereference_check_fdtable((files), false, (files)->fdt)
> 
> Plus we need
> 
> 	static inline struct file *__fcheck_files(struct files_struct *files,
> 						bool unshared, unsigned int fd)
> 	{
> 		struct file *file = NULL;
> 		struct fdtable *fdt = __rcu_dereference_check_fdtable(files, unshared, files->fdt);
> 
> 		if (fd < fdt->max_fds)
> 			file = __rcu_dereference_check_fdtable(files, unshared, fdt->fd[fd]);
> 		return file;
> 	}
> 
> doesn't look very nice...
> 
> As for 2/2, probably close_files() can simply do
> 
> 	/*
> 	 * It is safe to dereference the fd table without RCU or
> 	 * ->file_lock because this is the last reference to the
> 	 * files structure.
> 	 */
> 	fdt = rcu_dereference_raw(files->fdt);
> 
> Or we can add
> 
> 	#define __files_fdtable(files)	\
> 		 rcu_dereference_raw((files)->fdt)
> 
> but it is not clear to me what 1/1 should do. Perhaps
> 
> 	static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
> 	{
> 		struct fdtable *fdt = rcu_dereference_raw(files->fdt);
> 		struct file *file = NULL;
> 
> 		if (fd < fdt->max_fds)
> 			file = rcu_dereference_raw(fdt->fd[fd]);
> 
> 		return file;
> 	}
> 
> 	static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
> 	{
> 		rcu_lockdep_assert(rcu_read_lock_held() ||
> 				   lockdep_is_held(files->file_lock),
> 				   "message");
> 		return __fcheck_files(files, fd);
> 	}
> 
> ?
> 
> Oleg.
> 

Re: [PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups

[Oleg Nesterov]

On 01/08, Paul E. McKenney wrote:
>
> On Wed, Jan 08, 2014 at 04:19:18PM +0100, Oleg Nesterov wrote:
> > On 01/08, Paul E. McKenney wrote:
> > >
> > > Another approach would be to add an argument to files_fdtable()
> > > that is zero normally and one for "we know we don't need RCU
> > > protection."  Then rcu_dereference_check() could be something
> > > like the following:
> > >
> > > #define files_fdtable(files, c) \
> > > 		(rcu_dereference_check_fdtable((files), (files)->fdt) || c)
> > >
> > > Would that work?
> >
> > Yes, I considered this optiion, but this needs much more uglifications^W
> > changes.
> >
> > Either we need to change all users of files_fdtable(), or we need something
> > like
>
> There are only about 20 uses of files_fdtable() in 3.12, with almost all
> of them in fs/file.c.  So is changing all the users really all that
> problematic?

But only one user, close_files(), needs files_fdtable(files, true). Why
complicate the patch and the code? I think it would be better to simply
change close_files() to use rcu_dereference_raw().

And note that rcu_dereference_check_fdtable() needs the new argument too.

And we should also take care of fcheck_files(),

> > 	static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
> > 	{
> > 		struct fdtable *fdt = rcu_dereference_raw(files->fdt);
> > 		struct file *file = NULL;
> >
> > 		if (fd < fdt->max_fds)
> > 			file = rcu_dereference_raw(fdt->fd[fd]);
> >
> > 		return file;
> > 	}
> >
> > 	static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
> > 	{
> > 		rcu_lockdep_assert(rcu_read_lock_held() ||
> > 				   lockdep_is_held(files->file_lock),
> > 				   "message");
> > 		return __fcheck_files(files, fd);
> > 	}

doesn't this look much simpler than adding the "bool unshared" argument
and changing the callers?

Oleg.

Re: [PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups

[Paul E. McKenney]

On Fri, Jan 10, 2014 at 04:34:59PM +0100, Oleg Nesterov wrote:
> On 01/08, Paul E. McKenney wrote:
> >
> > On Wed, Jan 08, 2014 at 04:19:18PM +0100, Oleg Nesterov wrote:
> > > On 01/08, Paul E. McKenney wrote:
> > > >
> > > > Another approach would be to add an argument to files_fdtable()
> > > > that is zero normally and one for "we know we don't need RCU
> > > > protection."  Then rcu_dereference_check() could be something
> > > > like the following:
> > > >
> > > > #define files_fdtable(files, c) \
> > > > 		(rcu_dereference_check_fdtable((files), (files)->fdt) || c)
> > > >
> > > > Would that work?
> > >
> > > Yes, I considered this optiion, but this needs much more uglifications^W
> > > changes.
> > >
> > > Either we need to change all users of files_fdtable(), or we need something
> > > like
> >
> > There are only about 20 uses of files_fdtable() in 3.12, with almost all
> > of them in fs/file.c.  So is changing all the users really all that
> > problematic?
> 
> But only one user, close_files(), needs files_fdtable(files, true). Why
> complicate the patch and the code? I think it would be better to simply
> change close_files() to use rcu_dereference_raw().
> 
> And note that rcu_dereference_check_fdtable() needs the new argument too.
> 
> And we should also take care of fcheck_files(),
> 
> > > 	static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
> > > 	{
> > > 		struct fdtable *fdt = rcu_dereference_raw(files->fdt);
> > > 		struct file *file = NULL;
> > >
> > > 		if (fd < fdt->max_fds)
> > > 			file = rcu_dereference_raw(fdt->fd[fd]);
> > >
> > > 		return file;
> > > 	}
> > >
> > > 	static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
> > > 	{
> > > 		rcu_lockdep_assert(rcu_read_lock_held() ||
> > > 				   lockdep_is_held(files->file_lock),
> > > 				   "message");
> > > 		return __fcheck_files(files, fd);
> > > 	}
> 
> doesn't this look much simpler than adding the "bool unshared" argument
> and changing the callers?

I might be being too paranoid, but my concern with using rcu_lock_acquire()
and rcu_lock_release() is the possibility of code needing rcu_read_lock()
appearing somewhere in the function-call graph between rcu_lock_acquire()
and rcu_lock_release().  In that case, lockdep would be happy, but the
required RCU protection would not be present.

Sort of like my experience with people using RCU from idle.

							Thanx, Paul

[PATCH v2 0/2] rcu_dereference_check_fdtable fix/cleanups

[Oleg Nesterov]

On 01/10, Paul E. McKenney wrote:
>
> On Fri, Jan 10, 2014 at 04:34:59PM +0100, Oleg Nesterov wrote:
> >
> > doesn't this look much simpler than adding the "bool unshared" argument
> > and changing the callers?
>
> I might be being too paranoid, but my concern with using rcu_lock_acquire()
> and rcu_lock_release()

And I agree, I no longer suggest to use rcu_lock_acquire/release.

It seems that you misunderstood me, let me send v2 for review.

Oleg.

 fs/file.c                |   30 +++++++++++-------------------
 include/linux/fdtable.h  |   35 +++++++++++++++++++++--------------
 include/linux/rcupdate.h |    2 --
 kernel/rcu/update.c      |   11 -----------
 4 files changed, 32 insertions(+), 46 deletions(-)

[PATCH v2 1/2] introduce __fcheck_files() to fix rcu_dereference_check_fdtable(), kill rcu_my_thread_group_empty()

[Oleg Nesterov]

rcu_dereference_check_fdtable() looks very wrong,

1. rcu_my_thread_group_empty() was added by 844b9a8707f1 "vfs: fix
   RCU-lockdep false positive due to /proc" but it doesn't really
   fix the problem. A CLONE_THREAD (without CLONE_FILES) task can
   hit the same race with get_files_struct().

   And otoh rcu_my_thread_group_empty() can suppress the correct
   warning if the caller is the CLONE_FILES (without CLONE_THREAD)
   task.

2. files->count == 1 check is not really right too. Even if this
   files_struct is not shared it is not safe to access it lockless
   unless the caller is the owner.

   Otoh, this check is sub-optimal. files->count == 0 always means
   it is safe to use it lockless even if files != current->files,
   but put_files_struct() has to take rcu_read_lock(). See the next
   patch.

This patch removes the buggy checks and turns fcheck_files() into
__fcheck_files() which uses rcu_dereference_raw(), the "unshared"
callers, fget_light() and fget_raw_light(), can use it to avoid
the warning from RCU-lockdep.

fcheck_files() is trivially reimplemented as rcu_lockdep_assert()
plus __fcheck_files().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/file.c                |    4 ++--
 include/linux/fdtable.h  |   35 +++++++++++++++++++++--------------
 include/linux/rcupdate.h |    2 --
 kernel/rcu/update.c      |   11 -----------
 4 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 4a78f98..957cbc0 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -707,7 +707,7 @@ struct file *fget_light(unsigned int fd, int *fput_needed)
 
 	*fput_needed = 0;
 	if (atomic_read(&files->count) == 1) {
-		file = fcheck_files(files, fd);
+		file = __fcheck_files(files, fd);
 		if (file && (file->f_mode & FMODE_PATH))
 			file = NULL;
 	} else {
@@ -735,7 +735,7 @@ struct file *fget_raw_light(unsigned int fd, int *fput_needed)
 
 	*fput_needed = 0;
 	if (atomic_read(&files->count) == 1) {
-		file = fcheck_files(files, fd);
+		file = __fcheck_files(files, fd);
 	} else {
 		rcu_read_lock();
 		file = fcheck_files(files, fd);
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 085197b..70e8e21 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -59,29 +59,36 @@ struct files_struct {
 	struct file __rcu * fd_array[NR_OPEN_DEFAULT];
 };
 
-#define rcu_dereference_check_fdtable(files, fdtfd) \
-	(rcu_dereference_check((fdtfd), \
-			       lockdep_is_held(&(files)->file_lock) || \
-			       atomic_read(&(files)->count) == 1 || \
-			       rcu_my_thread_group_empty()))
-
-#define files_fdtable(files) \
-		(rcu_dereference_check_fdtable((files), (files)->fdt))
-
 struct file_operations;
 struct vfsmount;
 struct dentry;
 
 extern void __init files_defer_init(void);
 
-static inline struct file * fcheck_files(struct files_struct *files, unsigned int fd)
+#define rcu_dereference_check_fdtable(files, fdtfd) \
+	rcu_dereference_check((fdtfd), lockdep_is_held(&(files)->file_lock))
+
+#define files_fdtable(files) \
+	rcu_dereference_check_fdtable((files), (files)->fdt)
+
+/*
+ * The caller must ensure that fd table isn't shared or hold rcu or file lock
+ */
+static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
 {
-	struct file * file = NULL;
-	struct fdtable *fdt = files_fdtable(files);
+	struct fdtable *fdt = rcu_dereference_raw(files->fdt);
 
 	if (fd < fdt->max_fds)
-		file = rcu_dereference_check_fdtable(files, fdt->fd[fd]);
-	return file;
+		return rcu_dereference_raw(fdt->fd[fd]);
+	return NULL;
+}
+
+static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
+{
+	rcu_lockdep_assert(rcu_read_lock_held() ||
+			   lockdep_is_held(&files->file_lock),
+			   "suspicious rcu_dereference_check() usage");
+	return __fcheck_files(files, fd);
 }
 
 /*
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 39cbb88..a2482cf 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -448,8 +448,6 @@ static inline int rcu_read_lock_sched_held(void)
 
 #ifdef CONFIG_PROVE_RCU
 
-extern int rcu_my_thread_group_empty(void);
-
 /**
  * rcu_lockdep_assert - emit lockdep splat if specified condition not met
  * @c: condition to check
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 6cb3dff..a3596c8 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -195,17 +195,6 @@ void wait_rcu_gp(call_rcu_func_t crf)
 }
 EXPORT_SYMBOL_GPL(wait_rcu_gp);
 
-#ifdef CONFIG_PROVE_RCU
-/*
- * wrapper function to avoid #include problems.
- */
-int rcu_my_thread_group_empty(void)
-{
-	return thread_group_empty(current);
-}
-EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
-#endif /* #ifdef CONFIG_PROVE_RCU */
-
 #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
 static inline void debug_init_rcu_head(struct rcu_head *head)
 {
-- 
1.5.5.1

[PATCH v2 2/2] change close_files() to use rcu_dereference_raw(files->fdt)

[Oleg Nesterov]

put_files_struct() and close_files() do rcu_read_lock() to make
rcu_dereference_check_fdtable() happy.

This looks a bit ugly, files_fdtable() just reads the pointer,
we can simply use rcu_dereference_raw() to avoid the warning.

The patch also changes close_files() to return fdt, this avoids
another rcu_read_lock()/files_fdtable() in put_files_struct().

I think close_files() needs more cleanups:

	- we do not need xchg() exactly because we are the last
	  user of this files_struct

	- "if (file)" should be turned into WARN_ON(!file)

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/file.c |   26 +++++++++-----------------
 1 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 957cbc0..d34e59e 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -348,21 +348,16 @@ out:
 	return NULL;
 }
 
-static void close_files(struct files_struct * files)
+static struct fdtable *close_files(struct files_struct * files)
 {
-	int i, j;
-	struct fdtable *fdt;
-
-	j = 0;
-
 	/*
 	 * It is safe to dereference the fd table without RCU or
 	 * ->file_lock because this is the last reference to the
-	 * files structure.  But use RCU to shut RCU-lockdep up.
+	 * files structure.
 	 */
-	rcu_read_lock();
-	fdt = files_fdtable(files);
-	rcu_read_unlock();
+	struct fdtable *fdt = rcu_dereference_raw(files->fdt);
+	int i, j = 0;
+
 	for (;;) {
 		unsigned long set;
 		i = j * BITS_PER_LONG;
@@ -381,6 +376,8 @@ static void close_files(struct files_struct * files)
 			set >>= 1;
 		}
 	}
+
+	return fdt;
 }
 
 struct files_struct *get_files_struct(struct task_struct *task)
@@ -398,14 +395,9 @@ struct files_struct *get_files_struct(struct task_struct *task)
 
 void put_files_struct(struct files_struct *files)
 {
-	struct fdtable *fdt;
-
 	if (atomic_dec_and_test(&files->count)) {
-		close_files(files);
-		/* not really needed, since nobody can see us */
-		rcu_read_lock();
-		fdt = files_fdtable(files);
-		rcu_read_unlock();
+		struct fdtable *fdt = close_files(files);
+
 		/* free the arrays if they are not embedded */
 		if (fdt != &files->fdtab)
 			__free_fdtable(fdt);
-- 
1.5.5.1

Re: [PATCH v2 0/2] rcu_dereference_check_fdtable fix/cleanups

[Paul E. McKenney]

On Sat, Jan 11, 2014 at 07:19:08PM +0100, Oleg Nesterov wrote:
> On 01/10, Paul E. McKenney wrote:
> >
> > On Fri, Jan 10, 2014 at 04:34:59PM +0100, Oleg Nesterov wrote:
> > >
> > > doesn't this look much simpler than adding the "bool unshared" argument
> > > and changing the callers?
> >
> > I might be being too paranoid, but my concern with using rcu_lock_acquire()
> > and rcu_lock_release()
> 
> And I agree, I no longer suggest to use rcu_lock_acquire/release.
> 
> It seems that you misunderstood me, let me send v2 for review.

Ah, I did misunderstand you.  For both:

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

							Thanx, Paul

[PATCH 0/3] fget*() cleanups

[Oleg Nesterov]

On 01/11, Paul E. McKenney wrote:
>
> On Sat, Jan 11, 2014 at 07:19:08PM +0100, Oleg Nesterov wrote:
> >
> > It seems that you misunderstood me, let me send v2 for review.
>
> Ah, I did misunderstand you.  For both:
>
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Thanks!

While at it, can't we factor out the code in fget*() ?

Probably I dislike the code duplications too much, but imho fget*()
abuse the copy-and-paste.

On top of this series, saves 600 bytes.

Oleg.

 fs/file.c |   70 ++++++++++++++++--------------------------------------------
 1 files changed, 19 insertions(+), 51 deletions(-)

[PATCH 1/3] fs: factor out common code in fget() and fget_raw()

[Oleg Nesterov]

Apart from FMODE_PATH check fget() and fget_raw() are identical,
shift the code into the new simple helper, __fget(fd, mask). Saves
160 bytes.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/file.c |   25 ++++++++-----------------
 1 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index d34e59e..4ed58a3 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -637,16 +637,16 @@ void do_close_on_exec(struct files_struct *files)
 	spin_unlock(&files->file_lock);
 }
 
-struct file *fget(unsigned int fd)
+static struct file *__fget(unsigned int fd, fmode_t mask)
 {
-	struct file *file;
 	struct files_struct *files = current->files;
+	struct file *file;
 
 	rcu_read_lock();
 	file = fcheck_files(files, fd);
 	if (file) {
 		/* File object ref couldn't be taken */
-		if (file->f_mode & FMODE_PATH ||
+		if ((file->f_mode & mask) ||
 		    !atomic_long_inc_not_zero(&file->f_count))
 			file = NULL;
 	}
@@ -655,25 +655,16 @@ struct file *fget(unsigned int fd)
 	return file;
 }
 
+struct file *fget(unsigned int fd)
+{
+	return __fget(fd, FMODE_PATH);
+}
 EXPORT_SYMBOL(fget);
 
 struct file *fget_raw(unsigned int fd)
 {
-	struct file *file;
-	struct files_struct *files = current->files;
-
-	rcu_read_lock();
-	file = fcheck_files(files, fd);
-	if (file) {
-		/* File object ref couldn't be taken */
-		if (!atomic_long_inc_not_zero(&file->f_count))
-			file = NULL;
-	}
-	rcu_read_unlock();
-
-	return file;
+	return __fget(fd, 0);
 }
-
 EXPORT_SYMBOL(fget_raw);
 
 /*
-- 
1.5.5.1

[PATCH 2/3] fs: factor out common code in fget_light() and fget_raw_light()

[Oleg Nesterov]

Apart from FMODE_PATH check fget_light() and fget_raw_light() are
identical, shift the code into the new helper, __fget_light(fd, mask).
Saves 208 bytes.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/file.c |   33 +++++++++------------------------
 1 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 4ed58a3..50c1208 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -683,21 +683,21 @@ EXPORT_SYMBOL(fget_raw);
  * The fput_needed flag returned by fget_light should be passed to the
  * corresponding fput_light.
  */
-struct file *fget_light(unsigned int fd, int *fput_needed)
+struct file *__fget_light(unsigned int fd, fmode_t mask, int *fput_needed)
 {
-	struct file *file;
 	struct files_struct *files = current->files;
+	struct file *file;
 
 	*fput_needed = 0;
 	if (atomic_read(&files->count) == 1) {
 		file = __fcheck_files(files, fd);
-		if (file && (file->f_mode & FMODE_PATH))
+		if (file && (file->f_mode & mask))
 			file = NULL;
 	} else {
 		rcu_read_lock();
 		file = fcheck_files(files, fd);
 		if (file) {
-			if (!(file->f_mode & FMODE_PATH) &&
+			if (!(file->f_mode & mask) &&
 			    atomic_long_inc_not_zero(&file->f_count))
 				*fput_needed = 1;
 			else
@@ -709,30 +709,15 @@ struct file *fget_light(unsigned int fd, int *fput_needed)
 
 	return file;
 }
+struct file *fget_light(unsigned int fd, int *fput_needed)
+{
+	return __fget_light(fd, FMODE_PATH, fput_needed);
+}
 EXPORT_SYMBOL(fget_light);
 
 struct file *fget_raw_light(unsigned int fd, int *fput_needed)
 {
-	struct file *file;
-	struct files_struct *files = current->files;
-
-	*fput_needed = 0;
-	if (atomic_read(&files->count) == 1) {
-		file = __fcheck_files(files, fd);
-	} else {
-		rcu_read_lock();
-		file = fcheck_files(files, fd);
-		if (file) {
-			if (atomic_long_inc_not_zero(&file->f_count))
-				*fput_needed = 1;
-			else
-				/* Didn't get the reference, someone's freed */
-				file = NULL;
-		}
-		rcu_read_unlock();
-	}
-
-	return file;
+	return __fget_light(fd, 0, fput_needed);
 }
 
 void set_close_on_exec(unsigned int fd, int flag)
-- 
1.5.5.1

[PATCH 3/3] fs: __fget_light() can use __fget() in slow path

[Oleg Nesterov]

The slow path in __fget_light() can use __fget() to avoid the
code duplication. Saves 232 bytes.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/file.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 50c1208..771578b 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -694,17 +694,9 @@ struct file *__fget_light(unsigned int fd, fmode_t mask, int *fput_needed)
 		if (file && (file->f_mode & mask))
 			file = NULL;
 	} else {
-		rcu_read_lock();
-		file = fcheck_files(files, fd);
-		if (file) {
-			if (!(file->f_mode & mask) &&
-			    atomic_long_inc_not_zero(&file->f_count))
-				*fput_needed = 1;
-			else
-				/* Didn't get the reference, someone's freed */
-				file = NULL;
-		}
-		rcu_read_unlock();
+		file = __fget(fd, mask);
+		if (file)
+			*fput_needed = 1;
 	}
 
 	return file;
-- 
1.5.5.1

Re: [PATCH 1/3] fs: factor out common code in fget() and fget_raw()

[Paul E. McKenney]

On Mon, Jan 13, 2014 at 04:48:19PM +0100, Oleg Nesterov wrote:
> Apart from FMODE_PATH check fget() and fget_raw() are identical,
> shift the code into the new simple helper, __fget(fd, mask). Saves
> 160 bytes.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Not sure why local variable "file" was moved, but regardless:

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  fs/file.c |   25 ++++++++-----------------
>  1 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index d34e59e..4ed58a3 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -637,16 +637,16 @@ void do_close_on_exec(struct files_struct *files)
>  	spin_unlock(&files->file_lock);
>  }
> 
> -struct file *fget(unsigned int fd)
> +static struct file *__fget(unsigned int fd, fmode_t mask)
>  {
> -	struct file *file;
>  	struct files_struct *files = current->files;
> +	struct file *file;
> 
>  	rcu_read_lock();
>  	file = fcheck_files(files, fd);
>  	if (file) {
>  		/* File object ref couldn't be taken */
> -		if (file->f_mode & FMODE_PATH ||
> +		if ((file->f_mode & mask) ||
>  		    !atomic_long_inc_not_zero(&file->f_count))
>  			file = NULL;
>  	}
> @@ -655,25 +655,16 @@ struct file *fget(unsigned int fd)
>  	return file;
>  }
> 
> +struct file *fget(unsigned int fd)
> +{
> +	return __fget(fd, FMODE_PATH);
> +}
>  EXPORT_SYMBOL(fget);
> 
>  struct file *fget_raw(unsigned int fd)
>  {
> -	struct file *file;
> -	struct files_struct *files = current->files;
> -
> -	rcu_read_lock();
> -	file = fcheck_files(files, fd);
> -	if (file) {
> -		/* File object ref couldn't be taken */
> -		if (!atomic_long_inc_not_zero(&file->f_count))
> -			file = NULL;
> -	}
> -	rcu_read_unlock();
> -
> -	return file;
> +	return __fget(fd, 0);
>  }
> -
>  EXPORT_SYMBOL(fget_raw);
> 
>  /*
> -- 
> 1.5.5.1
> 
> 

Re: [PATCH 2/3] fs: factor out common code in fget_light() and fget_raw_light()

[Paul E. McKenney]

On Mon, Jan 13, 2014 at 04:48:40PM +0100, Oleg Nesterov wrote:
> Apart from FMODE_PATH check fget_light() and fget_raw_light() are
> identical, shift the code into the new helper, __fget_light(fd, mask).
> Saves 208 bytes.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  fs/file.c |   33 +++++++++------------------------
>  1 files changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 4ed58a3..50c1208 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -683,21 +683,21 @@ EXPORT_SYMBOL(fget_raw);
>   * The fput_needed flag returned by fget_light should be passed to the
>   * corresponding fput_light.
>   */
> -struct file *fget_light(unsigned int fd, int *fput_needed)
> +struct file *__fget_light(unsigned int fd, fmode_t mask, int *fput_needed)
>  {
> -	struct file *file;
>  	struct files_struct *files = current->files;
> +	struct file *file;
> 
>  	*fput_needed = 0;
>  	if (atomic_read(&files->count) == 1) {
>  		file = __fcheck_files(files, fd);
> -		if (file && (file->f_mode & FMODE_PATH))
> +		if (file && (file->f_mode & mask))
>  			file = NULL;
>  	} else {
>  		rcu_read_lock();
>  		file = fcheck_files(files, fd);
>  		if (file) {
> -			if (!(file->f_mode & FMODE_PATH) &&
> +			if (!(file->f_mode & mask) &&
>  			    atomic_long_inc_not_zero(&file->f_count))
>  				*fput_needed = 1;
>  			else
> @@ -709,30 +709,15 @@ struct file *fget_light(unsigned int fd, int *fput_needed)
> 
>  	return file;
>  }
> +struct file *fget_light(unsigned int fd, int *fput_needed)
> +{
> +	return __fget_light(fd, FMODE_PATH, fput_needed);
> +}
>  EXPORT_SYMBOL(fget_light);
> 
>  struct file *fget_raw_light(unsigned int fd, int *fput_needed)
>  {
> -	struct file *file;
> -	struct files_struct *files = current->files;
> -
> -	*fput_needed = 0;
> -	if (atomic_read(&files->count) == 1) {
> -		file = __fcheck_files(files, fd);
> -	} else {
> -		rcu_read_lock();
> -		file = fcheck_files(files, fd);
> -		if (file) {
> -			if (atomic_long_inc_not_zero(&file->f_count))
> -				*fput_needed = 1;
> -			else
> -				/* Didn't get the reference, someone's freed */
> -				file = NULL;
> -		}
> -		rcu_read_unlock();
> -	}
> -
> -	return file;
> +	return __fget_light(fd, 0, fput_needed);
>  }
> 
>  void set_close_on_exec(unsigned int fd, int flag)
> -- 
> 1.5.5.1
> 
> 

Re: [PATCH 3/3] fs: __fget_light() can use __fget() in slow path

[Paul E. McKenney]

On Mon, Jan 13, 2014 at 04:49:06PM +0100, Oleg Nesterov wrote:
> The slow path in __fget_light() can use __fget() to avoid the
> code duplication. Saves 232 bytes.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  fs/file.c |   14 +++-----------
>  1 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 50c1208..771578b 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -694,17 +694,9 @@ struct file *__fget_light(unsigned int fd, fmode_t mask, int *fput_needed)
>  		if (file && (file->f_mode & mask))
>  			file = NULL;
>  	} else {
> -		rcu_read_lock();
> -		file = fcheck_files(files, fd);
> -		if (file) {
> -			if (!(file->f_mode & mask) &&
> -			    atomic_long_inc_not_zero(&file->f_count))
> -				*fput_needed = 1;
> -			else
> -				/* Didn't get the reference, someone's freed */
> -				file = NULL;
> -		}
> -		rcu_read_unlock();
> +		file = __fget(fd, mask);
> +		if (file)
> +			*fput_needed = 1;
>  	}
> 
>  	return file;
> -- 
> 1.5.5.1
> 
> 

Re: [PATCH 0/3] fget*() cleanups

[Al Viro]

On Mon, Jan 13, 2014 at 04:47:48PM +0100, Oleg Nesterov wrote:
> On 01/11, Paul E. McKenney wrote:
> >
> > On Sat, Jan 11, 2014 at 07:19:08PM +0100, Oleg Nesterov wrote:
> > >
> > > It seems that you misunderstood me, let me send v2 for review.
> >
> > Ah, I did misunderstand you.  For both:
> >
> > Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Thanks!
> 
> While at it, can't we factor out the code in fget*() ?
> 
> Probably I dislike the code duplications too much, but imho fget*()
> abuse the copy-and-paste.
> 
> On top of this series, saves 600 bytes.

Applied, along with fcheck_files() stuff.  Sorry about late reply, I'm
digging through huge piles of mail and putting a queue for -next right
now...