[11/17] bpf/task_iter: In task_file_seq_get_next use fnext_task

Submitted by Eric W. Biederman on Aug. 17, 2020, 10:04 p.m.

Details

Message ID 20200817220425.9389-11-ebiederm@xmission.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Eric W. Biederman Aug. 17, 2020, 10:04 p.m.
When discussing[1] exec and posix file locks it was realized that none
of the callers of get_files_struct fundamentally needed to call
get_files_struct, and that by switching them to helper functions
instead it will both simplify their code and remove unnecessary
increments of files_struct.count.  Those unnecessary increments can
result in exec unnecessarily unsharing files_struct which breaking
posix locks, and it can result in fget_light having to fallback to
fget reducing system performance.

Using fnext_task simplifies task_file_seq_get_next, by moving the
checking for the maximum file descritor into the generic code, and by
remvoing the need for capturing and releasing a reference on
files_struct.  As the reference count of files_struct no longer needs
to be maintained bpf_iter_seq_task_file_info can have it's files member
removed and task_file_seq_get_next no longer it's fstruct argument.

The curr_fd local variable does need to become unsigned to be used
with fnext_task.  As curr_fd is assigned from and assigned a u32
making curr_fd an unsigned int won't cause problems and might prevent
them.

[1] https://lkml.kernel.org/r/20180915160423.GA31461@redhat.com
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/bpf/task_iter.c | 43 ++++++++++--------------------------------
 1 file changed, 10 insertions(+), 33 deletions(-)

Patch hide | download patch | download mbox

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 232df29793e9..831d42d7543a 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -122,45 +122,33 @@  struct bpf_iter_seq_task_file_info {
 	 */
 	struct bpf_iter_seq_task_common common;
 	struct task_struct *task;
-	struct files_struct *files;
 	u32 tid;
 	u32 fd;
 };
 
 static struct file *
 task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info,
-		       struct task_struct **task, struct files_struct **fstruct)
+		       struct task_struct **task)
 {
 	struct pid_namespace *ns = info->common.ns;
-	u32 curr_tid = info->tid, max_fds;
-	struct files_struct *curr_files;
+	u32 curr_tid = info->tid;
 	struct task_struct *curr_task;
-	int curr_fd = info->fd;
+	unsigned int curr_fd = info->fd;
 
 	/* If this function returns a non-NULL file object,
-	 * it held a reference to the task/files_struct/file.
+	 * it held a reference to the task/file.
 	 * Otherwise, it does not hold any reference.
 	 */
 again:
 	if (*task) {
 		curr_task = *task;
-		curr_files = *fstruct;
 		curr_fd = info->fd;
 	} else {
 		curr_task = task_seq_get_next(ns, &curr_tid);
 		if (!curr_task)
 			return NULL;
 
-		curr_files = get_files_struct(curr_task);
-		if (!curr_files) {
-			put_task_struct(curr_task);
-			curr_tid = ++(info->tid);
-			info->fd = 0;
-			goto again;
-		}
-
-		/* set *fstruct, *task and info->tid */
-		*fstruct = curr_files;
+		/* set *task and info->tid */
 		*task = curr_task;
 		if (curr_tid == info->tid) {
 			curr_fd = info->fd;
@@ -171,13 +159,12 @@  task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info,
 	}
 
 	rcu_read_lock();
-	max_fds = files_fdtable(curr_files)->max_fds;
-	for (; curr_fd < max_fds; curr_fd++) {
+	for (;; curr_fd++) {
 		struct file *f;
 
-		f = fcheck_files(curr_files, curr_fd);
+		f = fnext_task(curr_task, &curr_fd);
 		if (!f)
-			continue;
+			break;
 
 		/* set info->fd */
 		info->fd = curr_fd;
@@ -188,10 +175,8 @@  task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info,
 
 	/* the current task is done, go to the next task */
 	rcu_read_unlock();
-	put_files_struct(curr_files);
 	put_task_struct(curr_task);
 	*task = NULL;
-	*fstruct = NULL;
 	info->fd = 0;
 	curr_tid = ++(info->tid);
 	goto again;
@@ -200,13 +185,11 @@  task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info,
 static void *task_file_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	struct bpf_iter_seq_task_file_info *info = seq->private;
-	struct files_struct *files = NULL;
 	struct task_struct *task = NULL;
 	struct file *file;
 
-	file = task_file_seq_get_next(info, &task, &files);
+	file = task_file_seq_get_next(info, &task);
 	if (!file) {
-		info->files = NULL;
 		info->task = NULL;
 		return NULL;
 	}
@@ -214,7 +197,6 @@  static void *task_file_seq_start(struct seq_file *seq, loff_t *pos)
 	if (*pos == 0)
 		++*pos;
 	info->task = task;
-	info->files = files;
 
 	return file;
 }
@@ -222,22 +204,19 @@  static void *task_file_seq_start(struct seq_file *seq, loff_t *pos)
 static void *task_file_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
 	struct bpf_iter_seq_task_file_info *info = seq->private;
-	struct files_struct *files = info->files;
 	struct task_struct *task = info->task;
 	struct file *file;
 
 	++*pos;
 	++info->fd;
 	fput((struct file *)v);
-	file = task_file_seq_get_next(info, &task, &files);
+	file = task_file_seq_get_next(info, &task);
 	if (!file) {
-		info->files = NULL;
 		info->task = NULL;
 		return NULL;
 	}
 
 	info->task = task;
-	info->files = files;
 
 	return file;
 }
@@ -286,9 +265,7 @@  static void task_file_seq_stop(struct seq_file *seq, void *v)
 		(void)__task_file_seq_show(seq, v, true);
 	} else {
 		fput((struct file *)v);
-		put_files_struct(info->files);
 		put_task_struct(info->task);
-		info->files = NULL;
 		info->task = NULL;
 	}
 }

Comments

kernel test robot Aug. 18, 2020, 5:39 a.m.
Hi "Eric,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf/master]
[also build test WARNING on linus/master v5.9-rc1 next-20200817]
[cannot apply to bpf-next/master linux/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Eric-W-Biederman/exec-Move-unshare_files-to-fix-posix-file-locking-during-exec/20200818-061552
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: i386-randconfig-m021-20200818 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

smatch warnings:
kernel/bpf/task_iter.c:162 task_file_seq_get_next() warn: ignoring unreachable code.

# https://github.com/0day-ci/linux/commit/66f80aa453b17f8932b42e18265dba5fdb32490e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Eric-W-Biederman/exec-Move-unshare_files-to-fix-posix-file-locking-during-exec/20200818-061552
git checkout 66f80aa453b17f8932b42e18265dba5fdb32490e
vim +162 kernel/bpf/task_iter.c

   128	
   129	static struct file *
   130	task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info,
   131			       struct task_struct **task)
   132	{
   133		struct pid_namespace *ns = info->common.ns;
   134		u32 curr_tid = info->tid;
   135		struct task_struct *curr_task;
   136		unsigned int curr_fd = info->fd;
   137	
   138		/* If this function returns a non-NULL file object,
   139		 * it held a reference to the task/file.
   140		 * Otherwise, it does not hold any reference.
   141		 */
   142	again:
   143		if (*task) {
   144			curr_task = *task;
   145			curr_fd = info->fd;
   146		} else {
   147			curr_task = task_seq_get_next(ns, &curr_tid);
   148			if (!curr_task)
   149				return NULL;
   150	
   151			/* set *task and info->tid */
   152			*task = curr_task;
   153			if (curr_tid == info->tid) {
   154				curr_fd = info->fd;
   155			} else {
   156				info->tid = curr_tid;
   157				curr_fd = 0;
   158			}
   159		}
   160	
   161		rcu_read_lock();
 > 162		for (;; curr_fd++) {
   163			struct file *f;
   164	
   165			f = fnext_task(curr_task, &curr_fd);
   166			if (!f)
   167				break;
   168	
   169			/* set info->fd */
   170			info->fd = curr_fd;
   171			get_file(f);
   172			rcu_read_unlock();
   173			return f;
   174		}
   175	
   176		/* the current task is done, go to the next task */
   177		rcu_read_unlock();
   178		put_task_struct(curr_task);
   179		*task = NULL;
   180		info->fd = 0;
   181		curr_tid = ++(info->tid);
   182		goto again;
   183	}
   184	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Eric W. Biederman Aug. 18, 2020, 12:54 p.m.
kernel test robot <lkp@intel.com> writes:

> Hi "Eric,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on bpf/master]
> [also build test WARNING on linus/master v5.9-rc1 next-20200817]
> [cannot apply to bpf-next/master linux/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/0day-ci/linux/commits/Eric-W-Biederman/exec-Move-unshare_files-to-fix-posix-file-locking-during-exec/20200818-061552
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
> config: i386-randconfig-m021-20200818 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> smatch warnings:
> kernel/bpf/task_iter.c:162 task_file_seq_get_next() warn: ignoring unreachable code.

What is smatch warning about?

Perhaps I am blind but I don't see any unreachable code there.

Doh!  I see it.  That loop will never loop so curr_fd++ is unreachable.
Yes that should get fixed just so the code is readable.

I will change that.

Eric


>    128	
>    129	static struct file *
>    130	task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info,
>    131			       struct task_struct **task)
>    132	{
>    133		struct pid_namespace *ns = info->common.ns;
>    134		u32 curr_tid = info->tid;
>    135		struct task_struct *curr_task;
>    136		unsigned int curr_fd = info->fd;
>    137	
>    138		/* If this function returns a non-NULL file object,
>    139		 * it held a reference to the task/file.
>    140		 * Otherwise, it does not hold any reference.
>    141		 */
>    142	again:
>    143		if (*task) {
>    144			curr_task = *task;
>    145			curr_fd = info->fd;
>    146		} else {
>    147			curr_task = task_seq_get_next(ns, &curr_tid);
>    148			if (!curr_task)
>    149				return NULL;
>    150	
>    151			/* set *task and info->tid */
>    152			*task = curr_task;
>    153			if (curr_tid == info->tid) {
>    154				curr_fd = info->fd;
>    155			} else {
>    156				info->tid = curr_tid;
>    157				curr_fd = 0;
>    158			}
>    159		}
>    160	
>    161		rcu_read_lock();
>  > 162		for (;; curr_fd++) {
>    163			struct file *f;
>    164	
>    165			f = fnext_task(curr_task, &curr_fd);
>    166			if (!f)
>    167				break;
>    168	
>    169			/* set info->fd */
>    170			info->fd = curr_fd;
>    171			get_file(f);
>    172			rcu_read_unlock();
>    173			return f;
>    174		}
>    175	
>    176		/* the current task is done, go to the next task */
>    177		rcu_read_unlock();
>    178		put_task_struct(curr_task);
>    179		*task = NULL;
>    180		info->fd = 0;
>    181		curr_tid = ++(info->tid);
>    182		goto again;
>    183	}
>    184	
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org