rcu_dereference_check_fdtable fix/cleanups

PATCH Superseded Oleg Nesterov 3 participants 20 messages Jan 7, 2014 – Jan 13, 2014

Subsystems (2)

VFS RCU

lore

Share & tools

lore

Oleg Nesterov

author

2014-01-07 18:13 UTC #0 lore raw

cover rcu_dereference_check_fdtable fix/cleanups

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(-)

Oleg Nesterov

author

2014-01-07 18:13 UTC #2 lore raw

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

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 <[email protected]>

---
 fs/file.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

fs/file.c +9 ~~-11~~

index 957cbc0..716d747 100644
@@ -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);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-01-07 18:14 UTC #1 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

fs/file.c +2 -2

index 4a78f98..957cbc0 100644
@@ -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);

include/linux/fdtable.h +13 -6

index 085197b..f39d50a 100644
@@ -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.
  */

include/linux/rcupdate.h +0 -2

index 39cbb88..a2482cf 100644
@@ -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

kernel/rcu/update.c +0 ~~-11~~

index 6cb3dff..a3596c8 100644
@@ -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)
 {

Signature · 1 line

1.5.5.1

Paul E. McKenney

2014-01-08 13:29 UTC #3 lore raw

In reply to Oleg Nesterov

Quoted 6 lines · 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.

Quoted 8 lines

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

Quoted 8 lines

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(-)

Oleg Nesterov

author

2014-01-08 15:25 UTC #4 lore raw

Quoted 3 lines · Paul E. McKenney

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.

Quoted 9 lines

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.

Paul E. McKenney

2014-01-09 22:45 UTC #5 lore raw

Quoted 16 lines · Oleg Nesterov

Quoted 3 lines · Paul E. McKenney
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.
Quoted 8 lines
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)

Quoted 7 lines

Quoted 1 line
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

Quoted 66 lines

	#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.

Oleg Nesterov

author

2014-01-10 15:34 UTC #6 lore raw

Quoted 17 lines · Paul E. McKenney

Quoted 15 lines · Oleg Nesterov
Quoted 10 lines · Paul E. McKenney
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.

Quoted 6 lines

Quoted 2 lines
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(),

Quoted 11 lines

	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;
	}

Quoted 7 lines

	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.

Paul E. McKenney

2014-01-11 06:34 UTC #7 lore raw

Quoted 52 lines · Oleg Nesterov

Quoted 17 lines · Paul E. McKenney
Quoted 15 lines · Oleg Nesterov
Quoted 10 lines · Paul E. McKenney
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.
Quoted 6 lines
Quoted 2 lines
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(),
Quoted 11 lines
Quoted 11 lines
	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;
	}
Quoted 7 lines
Quoted 7 lines
	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);
	}

Quoted 2 lines

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

Oleg Nesterov

author

2014-01-11 18:18 UTC #8 lore raw

v2 cover rcu_dereference_check_fdtable fix/cleanups

Quoted 8 lines · Paul E. McKenney

Quoted 3 lines · Oleg Nesterov
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(-)

Oleg Nesterov

author

2014-01-11 18:19 UTC #9 lore raw

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

rcu_dereference_check_fdtable() looks very wrong,

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.

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 <[email protected]>

---
 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(-)

fs/file.c +2 -2

index 4a78f98..957cbc0 100644
@@ -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);

include/linux/fdtable.h +21 ~~-14~~

index 085197b..70e8e21 100644
@@ -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);
 }
 
 /*

include/linux/rcupdate.h +0 -2

index 39cbb88..a2482cf 100644
@@ -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

kernel/rcu/update.c +0 ~~-11~~

index 6cb3dff..a3596c8 100644
@@ -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)
 {

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-01-11 18:19 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
 fs/file.c |   26 +++++++++-----------------
 1 files changed, 9 insertions(+), 17 deletions(-)

fs/file.c +9 ~~-17~~

index 957cbc0..d34e59e 100644
@@ -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);

Signature · 1 line

1.5.5.1

Paul E. McKenney

Acked-by on the v2 series

2014-01-11 22:27 UTC #11 lore raw

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

Quoted 6 lines · Paul E. McKenney
Quoted 3 lines · Oleg Nesterov
doesn't this look much simpler than adding the "bool unshared" argument and changing the callers?

Quoted 6 lines

Quoted 2 lines
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 <[email protected]>

Thanx, Paul

Oleg Nesterov

author

2014-01-13 15:47 UTC #12 lore raw

cover fget*() cleanups

Quoted 8 lines · Paul E. McKenney

Quoted 2 lines · Oleg Nesterov
It seems that you misunderstood me, let me send v2 for review.
Ah, I did misunderstand you. For both:
Acked-by Paul E. McKenney <[email protected]>

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(-)

Oleg Nesterov

author

2014-01-13 15:47 UTC #13 lore raw

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

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 <[email protected]>

---
 fs/file.c |   25 ++++++++-----------------
 1 files changed, 8 insertions(+), 17 deletions(-)

fs/file.c +8 ~~-17~~

index d34e59e..4ed58a3 100644
@@ -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);
 
 /*

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-01-13 15:48 UTC #17 lore raw

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

In reply to Oleg Nesterov

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

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/file.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

fs/file.c +3 ~~-11~~

index 50c1208..771578b 100644
@@ -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;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-01-13 15:53 UTC #15 lore raw

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

In reply to 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 <[email protected]>

---
 fs/file.c |   33 +++++++++------------------------
 1 files changed, 9 insertions(+), 24 deletions(-)

fs/file.c +9 ~~-24~~

index 4ed58a3..50c1208 100644
@@ -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)

Signature · 1 line

1.5.5.1

Paul E. McKenney

Reviewed-by on v1 1/3

2014-01-13 23:29 UTC #14 lore raw

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

In reply to Oleg Nesterov

Quoted 5 lines · 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 <[email protected]>

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

Reviewed-by Paul E. McKenney <[email protected]>

Quoted 63 lines

---
 fs/file.c |   25 ++++++++-----------------
 1 files changed, 8 insertions(+), 17 deletions(-)

fs/file.c +8 ~~-17~~

index d34e59e..4ed58a3 100644
@@ -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);
 
 /*

Signature · 1 line

1.5.5.1

Paul E. McKenney

Reviewed-by on v1 2/3

2014-01-13 23:32 UTC #16 lore raw

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

In reply to Oleg Nesterov

Quoted 5 lines · 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 <[email protected]>

Reviewed-by Paul E. McKenney <[email protected]>

Quoted 74 lines

---
 fs/file.c |   33 +++++++++------------------------
 1 files changed, 9 insertions(+), 24 deletions(-)

fs/file.c +9 ~~-24~~

index 4ed58a3..50c1208 100644
@@ -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)

Signature · 1 line

1.5.5.1

Paul E. McKenney

Reviewed-by on v1 3/3

2014-01-13 23:37 UTC #18 lore raw

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

In reply to Oleg Nesterov

Quoted 4 lines · Oleg Nesterov

The slow path in __fget_light() can use __fget() to avoid the code duplication. Saves 232 bytes.
Signed-off-by Oleg Nesterov <[email protected]>

Reviewed-by Paul E. McKenney <[email protected]>

Quoted 33 lines

---
 fs/file.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

fs/file.c +3 ~~-11~~

index 50c1208..771578b 100644
@@ -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;

Signature · 1 line

1.5.5.1

Al Viro

maintainer Applied

2014-01-13 23:45 UTC #19 lore raw

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

In reply to Oleg Nesterov

Quoted 10 lines · Oleg Nesterov

Quoted 8 lines · Paul E. McKenney
Quoted 2 lines · Oleg Nesterov
It seems that you misunderstood me, let me send v2 for review.
Ah, I did misunderstand you. For both:
Acked-by Paul E. McKenney <[email protected]>

Quoted 8 lines

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...