| Message ID | 20200817220425.9389-11-ebiederm@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/17] exec: Move unshare_files to fix posix file locking during exec | expand |
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
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
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; } }
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(-)