Re: [PATCH 11/17] bpf/task_iter: In task_file_seq_get_next use fnext_task
kernel test robot 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
>
> 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
>129static struct file *
>130task_file_seq_get_next(struct bpf_iter_seq_task_file_info *info,
>131 struct task_struct **task)
>132{
>133struct pid_namespace *ns = info->common.ns;
>134u32 curr_tid = info->tid;
>135struct task_struct *curr_task;
>136unsigned 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 */
>142again:
>143if (*task) {
>144curr_task = *task;
>145curr_fd = info->fd;
>146} else {
>147curr_task = task_seq_get_next(ns, &curr_tid);
>148if (!curr_task)
>149return NULL;
>150
>151/* set *task and info->tid */
>152*task = curr_task;
>153if (curr_tid == info->tid) {
>154curr_fd = info->fd;
>155} else {
>156info->tid = curr_tid;
>157curr_fd = 0;
>158}
>159}
>160
>161rcu_read_lock();
> > 162for (;; curr_fd++) {
>163struct file *f;
>164
>165f = fnext_task(curr_task, &curr_fd);
>166if (!f)
>167break;
>168
>169/* set info->fd */
>170info->fd = curr_fd;
>171get_file(f);
>172rcu_read_unlock();
>173return f;
>174}
>175
>176/* the current task is done, go to the next task */
>177rcu_read_unlock();
>178put_task_struct(curr_task);
>179*task = NULL;
>180info->fd = 0;
>181curr_tid = ++(info->tid);
>182goto again;
>183}
>184
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [PATCH 11/17] bpf/task_iter: In task_file_seq_get_next use fnext_task
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
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-...@lists.01.org
.config.gz
Description: application/gzip
[PATCH 11/17] bpf/task_iter: In task_file_seq_get_next use fnext_task
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.ga31...@redhat.com
Suggested-by: Oleg Nesterov
Signed-off-by: "Eric W. Biederman"
---
kernel/bpf/task_iter.c | 43 ++
1 file changed, 10 insertions(+), 33 deletions(-)
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;
