A while ago it was reported that posix file locking goes wrong when a
multi-threaded process calls exec. I looked into the history and this
is definitely a regression, that should be fixed if we can.
This set of changes cleanups of the code in exec so hopefully this code
will not regress again. Then it adds helpers and fixes the users of
files_struct so the reference count is only incremented if COPY_FILES is
passed to clone. Then it removes helpers (get_files_struct,
__install_fd, __alloc_fd, __close_fd) that are no longer needed and
if used would encourage code that increments the count of files_struct
somewhere besides in clone when COPY_FILES is passed.
In addition to fixing the bug in exec and simplifing the code this set
of changes by virtue of getting files_struct.count correct it optimizes
fdget. With proc and other places not temporarily increasing the count
on files_struct __fget_light should succeed more often in being able to
return a struct file without touching it's reference count.
Fixing the count in files_struct was suggested by Oleg[1].
For those that are interested in the history of this issue I have
included as much of it as I could find in the first change.
fs/coredump.c | 5 +--
fs/exec.c | 26 ++++++-----
fs/file.c | 110 +++++++++++++++++++++--------------------------
fs/open.c | 2 +-
fs/proc/fd.c | 47 +++++++-------------
include/linux/fdtable.h | 13 ++----
include/linux/syscalls.h | 6 +--
kernel/bpf/syscall.c | 20 ++-------
kernel/bpf/task_iter.c | 43 +++++-------------
kernel/fork.c | 12 +++---
kernel/kcmp.c | 20 ++-------
11 files changed, 109 insertions(+), 195 deletions(-)
Eric W. Biederman (17):
exec: Move unshare_files to fix posix file locking during exec
exec: Simplify unshare_files
exec: Remove reset_files_struct
kcmp: In kcmp_epoll_target use fget_task
bpf: In bpf_task_fd_query use fget_task
file: Implement fcheck_task
proc/fd: In tid_fd_mode use fcheck_task
proc/fd: In proc_fd_link use fcheck_task
file: Implement fnext_task
proc/fd: In proc_readfd_common use fnext_task
bpf/task_iter: In task_file_seq_get_next use fnext_task
proc/fd: In fdinfo seq_show don't use get_files_struct
file: Remove get_files_struct
file: Merge __fd_install into fd_install
file: In f_dupfd read RLIMIT_NOFILE once.
file: Merge __alloc_fd into alloc_fd
file: Rename __close_fd to close_fd and remove the files parameter
[1] https://lkml.kernel.org/r/[email protected]
Reported-by: Jeff Layton <[email protected]>
Reported-by: Daniel P. Berrangé <[email protected]>
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
Eric
Many moons ago the binfmts were doing some very questionable things
with file descriptors and an unsharing of the file descriptor table
was added to make things better[1][2]. The helper steal_files was
added to avoid breaking the userspace programs[3][4][6].
Unfortunately it turned out that steal_locks did not work for network
file systems[5], so it was removed to see if anyone would
complain[7][8]. It was thought at the time that NPTL would not be
affected as the unshare_files happened after the other threads were
killed[8]. Unfortunately because there was an unshare_files in
binfmt_elf.c before the threads were killed this analysis was
incorrect.
This unshare_files in binfmt_elf.c resulted in the unshares_files
happening whenever threads were present. Which led to unshare_files
being moved to the start of do_execve[9].
Later the problems were rediscovered and suggested approach was to
readd steal_locks under a different name[10]. I happened to be
reviewing patches and I noticed that this approach was a step
backwards[11].
I proposed simply moving unshare_files[12] and it was pointed
out that moving unshare_files without auditing the code was
also unsafe[13].
There were then several attempts to solve this[14][15][16] and I even
posted this set of changes[17]. Unfortunately because auditing all of
execve is time consuming this change did not make it in at the time.
Well now that I am cleaning up exec I have made the time to read
through all of the binfmts and the only playing with file descriptors
is either the security modules closing them in
security_bprm_committing_creds or is in the generic code in fs/exec.c.
None of it happens before begin_new_exec is called.
So move unshare_files into begin_new_exec, after the point of no
return. If memory is very very very low and the application calling
exec is sharing file descriptor tables between processes we might fail
past the point of no return. Which is unfortunate but no different
than any of the other places where we allocate memory after the point
of no return.
This movement allows another process that shares the file table, or
another thread of the same process and that closes files or changes
their close on exec behavior and races with execve to cause some
unexpected things to happen. There is only one time of check to time
of use race and it is just there so that execve fails instead of
an interpreter failing when it tries to open the file it is supposed
to be interpreting. Failing later if userspace is being silly is
not a problem.
With this change it the following discription from the removal
of steal_locks[8] finally becomes true.
Apps using NPTL are not affected, since all other threads are killed before
execve.
Apps using LinuxThreads are only affected if they
- have multiple threads during exec (LinuxThreads doesn't kill other
threads, the app may do it with pthread_kill_other_threads_np())
- rely on POSIX locks being inherited across exec
Both conditions are documented, but not their interaction.
Apps using clone() natively are affected if they
- use clone(CLONE_FILES)
- rely on POSIX locks being inherited across exec
I have investigated some paths to make it possible to solve this
without moving unshare_files but they all look more complicated[18].
Reported-by: Daniel P. Berrangé <[email protected]>
Reported-by: Jeff Layton <[email protected]>
History-tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
[1] 02cda956de0b ("[PATCH] unshare_files"
[2] 04e9bcb4d106 ("[PATCH] use new unshare_files helper")
[3] 088f5d7244de ("[PATCH] add steal_locks helper")
[4] 02c541ec8ffa ("[PATCH] use new steal_locks helper")
[5] https://lkml.kernel.org/r/[email protected]
[6] https://lkml.kernel.org/r/[email protected]
[7] https://lkml.kernel.org/r/[email protected]
[8] c89681ed7d0e ("[PATCH] remove steal_locks()")
[9] fd8328be874f ("[PATCH] sanitize handling of shared descriptor tables in failing execve()")
[10] https://lkml.kernel.org/r/[email protected]
[11] https://lkml.kernel.org/r/[email protected]
[12] https://lkml.kernel.org/r/[email protected]
[13] https://lkml.kernel.org/r/[email protected]
[14] https://lkml.kernel.org/r/[email protected]
[15] https://lkml.kernel.org/r/[email protected]
[16] https://lkml.kernel.org/r/[email protected]
[17] https://lkml.kernel.org/r/[email protected]
[18] https://lkml.kernel.org/r/[email protected]
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/exec.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index a91003e28eaa..17c007bba712 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1354,6 +1354,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
int begin_new_exec(struct linux_binprm * bprm)
{
struct task_struct *me = current;
+ struct files_struct *displaced;
int retval;
/* Once we are committed compute the creds */
@@ -1373,6 +1374,13 @@ int begin_new_exec(struct linux_binprm * bprm)
if (retval)
goto out;
+ /* Ensure the files table is not shared. */
+ retval = unshare_files(&displaced);
+ if (retval)
+ goto out;
+ if (displaced)
+ put_files_struct(displaced);
+
/*
* Must be called _before_ exec_mmap() as bprm->mm is
* not visibile until then. This also enables the update
@@ -1892,16 +1900,11 @@ static int bprm_execve(struct linux_binprm *bprm,
int fd, struct filename *filename, int flags)
{
struct file *file;
- struct files_struct *displaced;
int retval;
- retval = unshare_files(&displaced);
- if (retval)
- return retval;
-
retval = prepare_bprm_creds(bprm);
if (retval)
- goto out_files;
+ return retval;
check_unsafe_exec(bprm);
current->in_execve = 1;
@@ -1916,8 +1919,12 @@ static int bprm_execve(struct linux_binprm *bprm,
bprm->file = file;
/*
* Record that a name derived from an O_CLOEXEC fd will be
- * inaccessible after exec. Relies on having exclusive access to
- * current->files (due to unshare_files above).
+ * inaccessible after exec. This allows the code in exec to
+ * choose to fail when the executable is not mmaped into the
+ * interpreter and an open file descriptor is not passed to
+ * the interpreter. This makes for a better user experience
+ * than having the interpreter start and then immediately fail
+ * when it finds the executable is inaccessible.
*/
if (bprm->fdpath &&
close_on_exec(fd, rcu_dereference_raw(current->files->fdt)))
@@ -1938,8 +1945,6 @@ static int bprm_execve(struct linux_binprm *bprm,
rseq_execve(current);
acct_update_integrals(current);
task_numa_free(current, false);
- if (displaced)
- put_files_struct(displaced);
return retval;
out:
@@ -1956,10 +1961,6 @@ static int bprm_execve(struct linux_binprm *bprm,
current->fs->in_exec = 0;
current->in_execve = 0;
-out_files:
- if (displaced)
- reset_files_struct(displaced);
-
return retval;
}
--
2.25.0
Now that exec no longer needs to restore the previous value of current->files
on error there are no more callers of reset_files_struct so remove it.
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/file.c | 12 ------------
include/linux/fdtable.h | 1 -
2 files changed, 13 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 21c0893f2f1d..c585dbaf31a3 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -435,18 +435,6 @@ void put_files_struct(struct files_struct *files)
}
}
-void reset_files_struct(struct files_struct *files)
-{
- struct task_struct *tsk = current;
- struct files_struct *old;
-
- old = tsk->files;
- task_lock(tsk);
- tsk->files = files;
- task_unlock(tsk);
- put_files_struct(old);
-}
-
void exit_files(struct task_struct *tsk)
{
struct files_struct * files = tsk->files;
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index f46a084b60b2..7cc9885044d9 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -108,7 +108,6 @@ struct task_struct;
struct files_struct *get_files_struct(struct task_struct *);
void put_files_struct(struct files_struct *fs);
-void reset_files_struct(struct files_struct *);
int unshare_files(void);
struct files_struct *dup_fd(struct files_struct *, unsigned, int *) __latent_entropy;
void do_close_on_exec(struct files_struct *);
--
2.25.0
Use the helper fget_task and simplify the code.
As well as simplifying the code this removes one unnecessary increment of
struct files_struct. This unnecessary increment of files_struct.count can
result in exec unnecessarily unsharing files_struct and breaking posix
locks, and it can result in fget_light having to fallback to fget reducing
performance.
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
kernel/kcmp.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/kernel/kcmp.c b/kernel/kcmp.c
index b3ff9288c6cc..87c48c0104ad 100644
--- a/kernel/kcmp.c
+++ b/kernel/kcmp.c
@@ -107,7 +107,6 @@ static int kcmp_epoll_target(struct task_struct *task1,
{
struct file *filp, *filp_epoll, *filp_tgt;
struct kcmp_epoll_slot slot;
- struct files_struct *files;
if (copy_from_user(&slot, uslot, sizeof(slot)))
return -EFAULT;
@@ -116,23 +115,12 @@ static int kcmp_epoll_target(struct task_struct *task1,
if (!filp)
return -EBADF;
- files = get_files_struct(task2);
- if (!files)
+ filp_epoll = fget_task(task2, slot.efd);
+ if (!filp_epoll)
return -EBADF;
- spin_lock(&files->file_lock);
- filp_epoll = fcheck_files(files, slot.efd);
- if (filp_epoll)
- get_file(filp_epoll);
- else
- filp_tgt = ERR_PTR(-EBADF);
- spin_unlock(&files->file_lock);
- put_files_struct(files);
-
- if (filp_epoll) {
- filp_tgt = get_epoll_tfile_raw_ptr(filp_epoll, slot.tfd, slot.toff);
- fput(filp_epoll);
- }
+ filp_tgt = get_epoll_tfile_raw_ptr(filp_epoll, slot.tfd, slot.toff);
+ fput(filp_epoll);
if (IS_ERR(filp_tgt))
return PTR_ERR(filp_tgt);
--
2.25.0
As a companion to fget_task implement fcheck_task for use for querying
a process about a specific file.
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/file.c | 13 +++++++++++++
include/linux/fdtable.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/fs/file.c b/fs/file.c
index c585dbaf31a3..8d4b385055e9 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -863,6 +863,19 @@ struct file *fget_task(struct task_struct *task, unsigned int fd)
return file;
}
+struct file *fcheck_task(struct task_struct *task, unsigned int fd)
+{
+ /* Must be called with rcu_read_lock held */
+ struct file *file = NULL;
+
+ task_lock(task);
+ if (task->files)
+ file = fcheck_files(task->files, fd);
+ task_unlock(task);
+
+ return file;
+}
+
/*
* Lightweight file lookup - no refcnt increment if fd table isn't shared.
*
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 7cc9885044d9..def9debd2ce2 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -103,6 +103,7 @@ static inline struct file *fcheck_files(struct files_struct *files, unsigned int
* Check whether the specified fd has an open file.
*/
#define fcheck(fd) fcheck_files(current->files, fd)
+struct file *fcheck_task(struct task_struct *task, unsigned int fd);
struct task_struct;
--
2.25.0
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 fcheck_task instead of get_files_struct clarifies tid_fd_mode by
removing a step.
[1] https://lkml.kernel.org/r/[email protected]
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/proc/fd.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 81882a13212d..4048a87c51ee 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -83,18 +83,13 @@ static const struct file_operations proc_fdinfo_file_operations = {
static bool tid_fd_mode(struct task_struct *task, unsigned fd, fmode_t *mode)
{
- struct files_struct *files = get_files_struct(task);
struct file *file;
- if (!files)
- return false;
-
rcu_read_lock();
- file = fcheck_files(files, fd);
+ file = fcheck_task(task, fd);
if (file)
*mode = file->f_mode;
rcu_read_unlock();
- put_files_struct(files);
return !!file;
}
--
2.25.0
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 proc_readfd_common, 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.
[1] https://lkml.kernel.org/r/[email protected]
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: Eric W. Biederman <[email protected]>
---
fs/proc/fd.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index abfdcb21cc79..d9fee5390fd7 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -218,7 +218,6 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
instantiate_t instantiate)
{
struct task_struct *p = get_proc_task(file_inode(file));
- struct files_struct *files;
unsigned int fd;
if (!p)
@@ -226,22 +225,18 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
if (!dir_emit_dots(file, ctx))
goto out;
- files = get_files_struct(p);
- if (!files)
- goto out;
rcu_read_lock();
- for (fd = ctx->pos - 2;
- fd < files_fdtable(files)->max_fds;
- fd++, ctx->pos++) {
+ for (fd = ctx->pos - 2;; fd++, ctx->pos++) {
struct file *f;
struct fd_data data;
char name[10 + 1];
unsigned int len;
- f = fcheck_files(files, fd);
+ f = fnext_task(p, &fd);
+ ctx->pos = fd;
if (!f)
- continue;
+ break;
data.mode = f->f_mode;
rcu_read_unlock();
data.fd = fd;
@@ -250,13 +245,11 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
if (!proc_fill_cache(file, ctx,
name, len, instantiate, p,
&data))
- goto out_fd_loop;
+ goto out;
cond_resched();
rcu_read_lock();
}
rcu_read_unlock();
-out_fd_loop:
- put_files_struct(files);
out:
put_task_struct(p);
return 0;
--
2.25.0
As a companion to fget_task and fcheck_task implement fnext_task that
will return the struct file for the first file descriptor show number
is equal or greater than the fd argument value, or NULL if there is
no such struct file.
This allows file descriptors of foreign processes to be iterated through
safely, without needed to increment the count on files_struct.
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/file.c | 21 +++++++++++++++++++++
include/linux/fdtable.h | 1 +
2 files changed, 22 insertions(+)
diff --git a/fs/file.c b/fs/file.c
index 8d4b385055e9..88f9f78869f8 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -876,6 +876,27 @@ struct file *fcheck_task(struct task_struct *task, unsigned int fd)
return file;
}
+struct file *fnext_task(struct task_struct *task, unsigned int *ret_fd)
+{
+ /* Must be called with rcu_read_lock held */
+ struct files_struct *files;
+ unsigned int fd = *ret_fd;
+ struct file *file = NULL;
+
+ task_lock(task);
+ files = task->files;
+ if (files) {
+ for (; fd < files_fdtable(files)->max_fds; fd++) {
+ file = fcheck_files(files, fd);
+ if (file)
+ break;
+ }
+ }
+ task_unlock(task);
+ *ret_fd = fd;
+ return file;
+}
+
/*
* Lightweight file lookup - no refcnt increment if fd table isn't shared.
*
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index def9debd2ce2..a3a054084f49 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -104,6 +104,7 @@ static inline struct file *fcheck_files(struct files_struct *files, unsigned int
*/
#define fcheck(fd) fcheck_files(current->files, fd)
struct file *fcheck_task(struct task_struct *task, unsigned int fd);
+struct file *fnext_task(struct task_struct *task, unsigned int *fd);
struct task_struct;
--
2.25.0
Now that exec no longer needs to return the unshared files to their
previous value there is no reason to return displaced.
Instead when unshare_fd creates a copy of the file table, call
put_files_struct before returning from unshare_files.
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/coredump.c | 5 +----
fs/exec.c | 5 +----
include/linux/fdtable.h | 2 +-
kernel/fork.c | 12 ++++++------
4 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index 76e7c10edfc0..568d6e391082 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -585,7 +585,6 @@ void do_coredump(const kernel_siginfo_t *siginfo)
int ispipe;
size_t *argv = NULL;
int argc = 0;
- struct files_struct *displaced;
/* require nonrelative corefile path and be extra careful */
bool need_suid_safe = false;
bool core_dumped = false;
@@ -791,11 +790,9 @@ void do_coredump(const kernel_siginfo_t *siginfo)
}
/* get us an unshared descriptor table; almost always a no-op */
- retval = unshare_files(&displaced);
+ retval = unshare_files();
if (retval)
goto close_fail;
- if (displaced)
- put_files_struct(displaced);
if (!dump_interrupted()) {
/*
* umh disabled with CONFIG_STATIC_USERMODEHELPER_PATH="" would
diff --git a/fs/exec.c b/fs/exec.c
index 17c007bba712..9b723d2560d1 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1354,7 +1354,6 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
int begin_new_exec(struct linux_binprm * bprm)
{
struct task_struct *me = current;
- struct files_struct *displaced;
int retval;
/* Once we are committed compute the creds */
@@ -1375,11 +1374,9 @@ int begin_new_exec(struct linux_binprm * bprm)
goto out;
/* Ensure the files table is not shared. */
- retval = unshare_files(&displaced);
+ retval = unshare_files();
if (retval)
goto out;
- if (displaced)
- put_files_struct(displaced);
/*
* Must be called _before_ exec_mmap() as bprm->mm is
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index a32bf47c593e..f46a084b60b2 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -109,7 +109,7 @@ struct task_struct;
struct files_struct *get_files_struct(struct task_struct *);
void put_files_struct(struct files_struct *fs);
void reset_files_struct(struct files_struct *);
-int unshare_files(struct files_struct **);
+int unshare_files(void);
struct files_struct *dup_fd(struct files_struct *, unsigned, int *) __latent_entropy;
void do_close_on_exec(struct files_struct *);
int iterate_fd(struct files_struct *, unsigned,
diff --git a/kernel/fork.c b/kernel/fork.c
index 4d32190861bd..3049a41076f3 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2995,21 +2995,21 @@ SYSCALL_DEFINE1(unshare, unsigned long, unshare_flags)
* the exec layer of the kernel.
*/
-int unshare_files(struct files_struct **displaced)
+int unshare_files(void)
{
struct task_struct *task = current;
- struct files_struct *copy = NULL;
+ struct files_struct *old, *copy = NULL;
int error;
error = unshare_fd(CLONE_FILES, NR_OPEN_MAX, ©);
- if (error || !copy) {
- *displaced = NULL;
+ if (error || !copy)
return error;
- }
- *displaced = task->files;
+
+ old = task->files;
task_lock(task);
task->files = copy;
task_unlock(task);
+ put_files_struct(old);
return 0;
}
--
2.25.0
Simplify the code, and remove the chance of races by reading
RLIMIT_NOFILE only once in f_dupfd.
Pass the read value of RLIMIT_NOFILE into alloc_fd which is the other
location the rlimit was read in f_dupfd. As f_dupfd is the only
caller of alloc_fd this changing alloc_fd is trivially safe.
Further this causes alloc_fd to take all of the same arguments as
__alloc_fd except for the files_struct argument.
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/file.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 1a755811669d..505b2e81ad3e 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -523,9 +523,9 @@ int __alloc_fd(struct files_struct *files,
return error;
}
-static int alloc_fd(unsigned start, unsigned flags)
+static int alloc_fd(unsigned start, unsigned end, unsigned flags)
{
- return __alloc_fd(current->files, start, rlimit(RLIMIT_NOFILE), flags);
+ return __alloc_fd(current->files, start, end, flags);
}
int __get_unused_fd_flags(unsigned flags, unsigned long nofile)
@@ -1158,10 +1158,11 @@ SYSCALL_DEFINE1(dup, unsigned int, fildes)
int f_dupfd(unsigned int from, struct file *file, unsigned flags)
{
+ unsigned long nofile = rlimit(RLIMIT_NOFILE);
int err;
- if (from >= rlimit(RLIMIT_NOFILE))
+ if (from >= nofile)
return -EINVAL;
- err = alloc_fd(from, flags);
+ err = alloc_fd(from, nofile, flags);
if (err >= 0) {
get_file(file);
fd_install(err, file);
--
2.25.0
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/[email protected]
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
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;
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;
}
}
--
2.25.0
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.
Instead hold task_lock for the duration that task->files needs to be
stable in seq_show. The task_lock was already taken in
get_files_struct, and so skipping get_files_struct performs less work
overall, and avoids the problems with the files_struct reference
count.
[1] https://lkml.kernel.org/r/[email protected]
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/proc/fd.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index d9fee5390fd7..0b46eea154b7 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -28,9 +28,8 @@ static int seq_show(struct seq_file *m, void *v)
if (!task)
return -ENOENT;
- files = get_files_struct(task);
- put_task_struct(task);
-
+ task_lock(task);
+ files = task->files;
if (files) {
unsigned int fd = proc_fd(m->private);
@@ -47,8 +46,9 @@ static int seq_show(struct seq_file *m, void *v)
ret = 0;
}
spin_unlock(&files->file_lock);
- put_files_struct(files);
}
+ task_unlock(task);
+ put_task_struct(task);
if (ret)
return ret;
@@ -57,6 +57,7 @@ static int seq_show(struct seq_file *m, void *v)
(long long)file->f_pos, f_flags,
real_mount(file->f_path.mnt)->mnt_id);
+ /* show_fd_locks() never deferences files so a stale value is safe */
show_fd_locks(m, file, files);
if (seq_has_overflowed(m))
goto out;
--
2.25.0
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 fcheck_task instead of get_files_struct simplifies proc_fd_link by
removing unnecessary locking, and reference counting.
[1] https://lkml.kernel.org/r/[email protected]
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/proc/fd.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 4048a87c51ee..abfdcb21cc79 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -141,29 +141,23 @@ static const struct dentry_operations tid_fd_dentry_operations = {
static int proc_fd_link(struct dentry *dentry, struct path *path)
{
- struct files_struct *files = NULL;
struct task_struct *task;
int ret = -ENOENT;
task = get_proc_task(d_inode(dentry));
if (task) {
- files = get_files_struct(task);
- put_task_struct(task);
- }
-
- if (files) {
unsigned int fd = proc_fd(d_inode(dentry));
struct file *fd_file;
- spin_lock(&files->file_lock);
- fd_file = fcheck_files(files, fd);
+ rcu_read_lock();
+ fd_file = fcheck_task(task, fd);
if (fd_file) {
*path = fd_file->f_path;
path_get(&fd_file->f_path);
ret = 0;
}
- spin_unlock(&files->file_lock);
- put_files_struct(files);
+ rcu_read_unlock();
+ put_task_struct(task);
}
return ret;
--
2.25.0
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.
Use fget_task to simplify bpf_task_fd_query.
[1] https://lkml.kernel.org/r/[email protected]
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
kernel/bpf/syscall.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 86299a292214..93657d5f6538 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3741,7 +3741,6 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
pid_t pid = attr->task_fd_query.pid;
u32 fd = attr->task_fd_query.fd;
const struct perf_event *event;
- struct files_struct *files;
struct task_struct *task;
struct file *file;
int err;
@@ -3759,23 +3758,11 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
if (!task)
return -ENOENT;
- files = get_files_struct(task);
- put_task_struct(task);
- if (!files)
- return -ENOENT;
-
err = 0;
- spin_lock(&files->file_lock);
- file = fcheck_files(files, fd);
+ file = fget_task(task, fd);
+ put_task_struct(task);
if (!file)
- err = -EBADF;
- else
- get_file(file);
- spin_unlock(&files->file_lock);
- put_files_struct(files);
-
- if (err)
- goto out;
+ return -EBADF;
if (file->f_op == &bpf_link_fops) {
struct bpf_link *link = file->private_data;
@@ -3815,7 +3802,6 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
err = -ENOTSUPP;
put_file:
fput(file);
-out:
return err;
}
--
2.25.0
The function __fd_install was added to support binder[1]. With binder
fixed[2] there are no more users. Further with get_files_struct
removed there can be no more users of __fd_install that pass anything
except current->files.
As fd_install just calls __fd_install with "files=current->files",
merge them together by transforming the files parameter into a
local variable initialized to current->files.
[1] f869e8a7f753 ("expose a low-level variant of fd_install() for binder")
[2] 44d8047f1d87 ("binder: use standard functions to allocate fds")
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/file.c | 25 ++++++-------------------
include/linux/fdtable.h | 2 --
2 files changed, 6 insertions(+), 21 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 605e756f3c63..1a755811669d 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -157,7 +157,7 @@ static int expand_fdtable(struct files_struct *files, unsigned int nr)
spin_unlock(&files->file_lock);
new_fdt = alloc_fdtable(nr);
- /* make sure all __fd_install() have seen resize_in_progress
+ /* make sure all fd_install() have seen resize_in_progress
* or have finished their rcu_read_lock_sched() section.
*/
if (atomic_read(&files->count) > 1)
@@ -180,7 +180,7 @@ static int expand_fdtable(struct files_struct *files, unsigned int nr)
rcu_assign_pointer(files->fdt, new_fdt);
if (cur_fdt != &files->fdtab)
call_rcu(&cur_fdt->rcu, free_fdtable_rcu);
- /* coupled with smp_rmb() in __fd_install() */
+ /* coupled with smp_rmb() in fd_install() */
smp_wmb();
return 1;
}
@@ -569,17 +569,13 @@ EXPORT_SYMBOL(put_unused_fd);
* It should never happen - if we allow dup2() do it, _really_ bad things
* will follow.
*
- * NOTE: __fd_install() variant is really, really low-level; don't
- * use it unless you are forced to by truly lousy API shoved down
- * your throat. 'files' *MUST* be either current->files or obtained
- * by get_files_struct(current) done by whoever had given it to you,
- * or really bad things will happen. Normally you want to use
- * fd_install() instead.
+ * This consumes the "file" refcount, so callers should treat it
+ * as if they had called fput(file).
*/
-void __fd_install(struct files_struct *files, unsigned int fd,
- struct file *file)
+void fd_install(unsigned int fd, struct file *file)
{
+ struct files_struct *files = current->files;
struct fdtable *fdt;
rcu_read_lock_sched();
@@ -601,15 +597,6 @@ void __fd_install(struct files_struct *files, unsigned int fd,
rcu_read_unlock_sched();
}
-/*
- * This consumes the "file" refcount, so callers should treat it
- * as if they had called fput(file).
- */
-void fd_install(unsigned int fd, struct file *file)
-{
- __fd_install(current->files, fd, file);
-}
-
EXPORT_SYMBOL(fd_install);
static struct file *pick_file(struct files_struct *files, unsigned fd)
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 8c4bc6aa19c9..4da8aacc461a 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -118,8 +118,6 @@ int iterate_fd(struct files_struct *, unsigned,
extern int __alloc_fd(struct files_struct *files,
unsigned start, unsigned end, unsigned flags);
-extern void __fd_install(struct files_struct *files,
- unsigned int fd, struct file *file);
extern int __close_fd(struct files_struct *files,
unsigned int fd);
extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags);
--
2.25.0
The function __alloc_fd was added to support binder[1]. With binder
fixed[2] there are no more users. Further with get_files_struct
removed there can be no more users of __alloc_fd that pass anything
except current->files.
As alloc_fd just calls __alloc_fd with "files=current->files",
merge them together by transforming the files parameter into a
ocal variable initialized to current->files.
[1] dcfadfa4ec5a ("new helper: __alloc_fd()")
[2] 44d8047f1d87 ("binder: use standard functions to allocate fds")
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/file.c | 11 +++--------
include/linux/fdtable.h | 2 --
2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 505b2e81ad3e..221fc4f97f61 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -465,9 +465,9 @@ static unsigned int find_next_fd(struct fdtable *fdt, unsigned int start)
/*
* allocate a file descriptor, mark it busy.
*/
-int __alloc_fd(struct files_struct *files,
- unsigned start, unsigned end, unsigned flags)
+static int alloc_fd(unsigned start, unsigned end, unsigned flags)
{
+ struct files_struct *files = current->files;
unsigned int fd;
int error;
struct fdtable *fdt;
@@ -523,14 +523,9 @@ int __alloc_fd(struct files_struct *files,
return error;
}
-static int alloc_fd(unsigned start, unsigned end, unsigned flags)
-{
- return __alloc_fd(current->files, start, end, flags);
-}
-
int __get_unused_fd_flags(unsigned flags, unsigned long nofile)
{
- return __alloc_fd(current->files, 0, nofile, flags);
+ return alloc_fd(0, nofile, flags);
}
int get_unused_fd_flags(unsigned flags)
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 4da8aacc461a..d8f6c4921d85 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -116,8 +116,6 @@ int iterate_fd(struct files_struct *, unsigned,
int (*)(const void *, struct file *, unsigned),
const void *);
-extern int __alloc_fd(struct files_struct *files,
- unsigned start, unsigned end, unsigned flags);
extern int __close_fd(struct files_struct *files,
unsigned int fd);
extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags);
--
2.25.0
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.
Now that get_files_struct has no more users and can not cause the
problems for posix file locking and fget_light remove get_files_struct
so that it does not gain any new users.
[1] https://lkml.kernel.org/r/[email protected]
Suggested-by: Oleg Nesterov <[email protected]>
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/file.c | 13 -------------
include/linux/fdtable.h | 1 -
2 files changed, 14 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 88f9f78869f8..605e756f3c63 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -410,19 +410,6 @@ static struct fdtable *close_files(struct files_struct * files)
return fdt;
}
-struct files_struct *get_files_struct(struct task_struct *task)
-{
- struct files_struct *files;
-
- task_lock(task);
- files = task->files;
- if (files)
- atomic_inc(&files->count);
- task_unlock(task);
-
- return files;
-}
-
void put_files_struct(struct files_struct *files)
{
if (atomic_dec_and_test(&files->count)) {
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index a3a054084f49..8c4bc6aa19c9 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -108,7 +108,6 @@ struct file *fnext_task(struct task_struct *task, unsigned int *fd);
struct task_struct;
-struct files_struct *get_files_struct(struct task_struct *);
void put_files_struct(struct files_struct *fs);
int unshare_files(void);
struct files_struct *dup_fd(struct files_struct *, unsigned, int *) __latent_entropy;
--
2.25.0
The function __close_fd was added to support binder[1]. Now that
binder has been fixed to no longer need __close_fd[2] and
get_files_struct has been removed it is no longer possible to
correctly call __close_fd with anything current->files pass as it's
files parameter.
Therefore transform the files parameter into a local variable
initialized to current->files, and rename __close_fd to close_fd to
reflect this change, and keep it in sync with the similar changes to
__alloc_fd, and __fd_install.
This removes the need for callers to care about the extra care that
needs to be take if anything except current->files is passed, by
limiting the callers to only operation on current->files.
[1] 483ce1d4b8c3 ("take descriptor-related part of close() to file.c")
[2] 44d8047f1d87 ("binder: use standard functions to allocate fds")
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
fs/file.c | 10 ++++------
fs/open.c | 2 +-
include/linux/fdtable.h | 3 +--
include/linux/syscalls.h | 6 +++---
4 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 221fc4f97f61..b06b04d65070 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -614,11 +614,9 @@ static struct file *pick_file(struct files_struct *files, unsigned fd)
return file;
}
-/*
- * The same warnings as for __alloc_fd()/__fd_install() apply here...
- */
-int __close_fd(struct files_struct *files, unsigned fd)
+int close_fd(unsigned fd)
{
+ struct files_struct *files = current->files;
struct file *file;
file = pick_file(files, fd);
@@ -627,7 +625,7 @@ int __close_fd(struct files_struct *files, unsigned fd)
return filp_close(file, files);
}
-EXPORT_SYMBOL(__close_fd); /* for ksys_close() */
+EXPORT_SYMBOL(close_fd); /* for ksys_close() */
/**
* __close_range() - Close all file descriptors in a given range.
@@ -1010,7 +1008,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
struct files_struct *files = current->files;
if (!file)
- return __close_fd(files, fd);
+ return close_fd(fd);
if (fd >= rlimit(RLIMIT_NOFILE))
return -EBADF;
diff --git a/fs/open.c b/fs/open.c
index 9af548fb841b..581a674d7eee 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1292,7 +1292,7 @@ EXPORT_SYMBOL(filp_close);
*/
SYSCALL_DEFINE1(close, unsigned int, fd)
{
- int retval = __close_fd(current->files, fd);
+ int retval = close_fd(fd);
/* can't restart close syscall because file table entry was cleared */
if (unlikely(retval == -ERESTARTSYS ||
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index d8f6c4921d85..d1b8f3d85493 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -116,8 +116,7 @@ int iterate_fd(struct files_struct *, unsigned,
int (*)(const void *, struct file *, unsigned),
const void *);
-extern int __close_fd(struct files_struct *files,
- unsigned int fd);
+extern int close_fd(unsigned int fd);
extern int __close_range(unsigned int fd, unsigned int max_fd, unsigned int flags);
extern int __close_fd_get_file(unsigned int fd, struct file **res);
extern int unshare_fd(unsigned long unshare_flags, unsigned int max_fds,
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 75ac7f8ae93c..a3f48cf49206 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1293,16 +1293,16 @@ static inline long ksys_ftruncate(unsigned int fd, loff_t length)
return do_sys_ftruncate(fd, length, 1);
}
-extern int __close_fd(struct files_struct *files, unsigned int fd);
+extern int close_fd(unsigned int fd);
/*
* In contrast to sys_close(), this stub does not check whether the syscall
* should or should not be restarted, but returns the raw error codes from
- * __close_fd().
+ * close_fd().
*/
static inline int ksys_close(unsigned int fd)
{
- return __close_fd(current->files, fd);
+ return close_fd(fd);
}
extern long do_sys_truncate(const char __user *pathname, loff_t length);
--
2.25.0
I like the series, but I have to say that the extension of the
horrible "fcheck*()" interfaces raises my hackles..
That interface is very very questionable to begin with, and it's easy
to get wrong.
I don't see you getting it wrong - you do seem to hold the RCU read
lock in the places I checked, but it worries me.
I think my worry could be at least partially mitigated with more
comments and docs:
On Mon, Aug 17, 2020 at 3:10 PM Eric W. Biederman <[email protected]> wrote:
>
> +struct file *fnext_task(struct task_struct *task, unsigned int *ret_fd)
Please please *please* make it clear that because this does *not*
increment any reference counts, you have to be very very careful using
the "struct file" pointer this returns.
I really dislike the naming. The old "fcheck()" naming came from the
fact that at least one original user just mainly checked if the result
was NULL or not. And then others had to be careful to either hold the
file_lock spinlock, or at least the RCU read lock so that the result
didn't go away.
Here, you have "fnext_task()", and it doesn't even have that "check"
warning mark, or any comment about how dangerous it can be to use..
So the rule is that the "struct file" pointer this returns can only be
read under RCU, but the 'fd' it returns has a longer lifetime.
I think your uses are ok, and I think this is an improvement in
general, I just want a bigger "WARNING! Here be dragons!" sign (and
naming could be nicer).
Linus
On Mon, Aug 17, 2020 at 3:11 PM Eric W. Biederman <[email protected]> wrote:
>
> Instead hold task_lock for the duration that task->files needs to be
> stable in seq_show. The task_lock was already taken in
> get_files_struct, and so skipping get_files_struct performs less work
> overall, and avoids the problems with the files_struct reference
> count.
Hmm. Do we even need that task_lock() at all? Couldn't we do this all
with just the RCU lock held for reading?
As far as I can tell, we don't really need the task lock. We don't
even need the get/pid_task_struct().
Can't we just do
rcu_read_lock();
task = pid_task(proc_pid(inode), PIDTYPE_PID);
if (task) {
unsigned int fd = proc_fd(m->private);
struct file *file = fcheck_task(task, fd);
if (file)
.. do the seq_printf ..
and do it all with no refcount games or task locking at all?
Anyway, I don't dislike your patch per se, I just get the feeling that
you could go a bit further in that cleanup..
And it's quite possible that I'm wrong, and you can't just use the RCU
lock, but as far as I can tell, both the task lookup and the file
lookup *already* really both depend on the RCU lock anyway, so the
other locking and reference counting games really do seem superfluous.
Please just send me a belittling email telling me what a nincompoop I
am if I have missed something.
Linus
On Mon, Aug 17, 2020 at 6:06 PM Eric W. Biederman <[email protected]> wrote:
>
> I struggle with the fcheck name as I have not seen or at least not
> registed on the the user that just checks to see if the result is NULL.
> So the name fcheck never made a bit of sense to me.
Yeah, that name is not great. I just don't want to make things even worse.
> I will see if I can come up with some good descriptive comments around
> these functions. Along with describing what these things are doing I am
> thinking maybe I should put "_rcu" in their names and have a debug check
> that verifies "_rcu" is held.
Yeah, something along the lines of "rcu_lookup_fd_task(tsk,fd)" would
be a *lot* more descriptive than fcheck_task().
And I think "fnext_task()" could be "rcu_lookup_next_fd_task(tsk,fd)".
Yes, those are much longer names, but it's not like you end up typing
them all that often, and I think being descriptive would be worth it.
And "fcheck()" and "fcheck_files()" would be good to rename too along
the same lines.
Something like "rcu_lookup_fd()" and "rcu_lookup_fd_files()" respectively?
I'm obviously trying to go for a "rcu_lookup_fd*()" kind of pattern,
and I'm not married to _that_ particular pattern but I think it would
be better than what we have now.
Linus
On Mon, Aug 17, 2020 at 6:13 PM Eric W. Biederman <[email protected]> wrote:
>
> task_lock is needed to protect task->files.
Hah. Right you are. I found a few cases where we didn't do that, but I
hadn't noticed that they were all of the pattern
struct task_struct *tsk = current;
so "tsk->files" was safe for that reason..
So never mind.
Linus
On Mon, Aug 17, 2020 at 05:04:18PM -0500, Eric W. Biederman wrote:
> 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 proc_readfd_common, 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.
Yecchhh... So you keep locking/unlocking the damn thing for each
descriptor?
On Mon, Aug 17, 2020 at 8:46 PM Eric W. Biederman <[email protected]> wrote:
>
> I am definitely willing to look at it. Do we think there would be enough
> traffic on task_lock from /proc/<pid>/fd access to make it work doing?
not from /proc, but bpf iterator in kernel/bpf/task_iter.c that is
being modified
in the other patch is used to process 100+k tasks. The faster it can go
through them the better. We don't see task spin_lock being a bottleneck though.
To test it just do 'bpftool prog show'. It will use task iterator undercover.
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 <[email protected]>
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/[email protected]
On Mon, Aug 17, 2020 at 05:04:09PM -0500, Eric W. Biederman wrote:
> Many moons ago the binfmts were doing some very questionable things
> with file descriptors and an unsharing of the file descriptor table
> was added to make things better[1][2]. The helper steal_files was
> added to avoid breaking the userspace programs[3][4][6].
>
> Unfortunately it turned out that steal_locks did not work for network
> file systems[5], so it was removed to see if anyone would
> complain[7][8]. It was thought at the time that NPTL would not be
> affected as the unshare_files happened after the other threads were
> killed[8]. Unfortunately because there was an unshare_files in
> binfmt_elf.c before the threads were killed this analysis was
> incorrect.
>
> This unshare_files in binfmt_elf.c resulted in the unshares_files
> happening whenever threads were present. Which led to unshare_files
> being moved to the start of do_execve[9].
>
> Later the problems were rediscovered and suggested approach was to
> readd steal_locks under a different name[10]. I happened to be
> reviewing patches and I noticed that this approach was a step
> backwards[11].
>
> I proposed simply moving unshare_files[12] and it was pointed
> out that moving unshare_files without auditing the code was
> also unsafe[13].
>
> There were then several attempts to solve this[14][15][16] and I even
> posted this set of changes[17]. Unfortunately because auditing all of
> execve is time consuming this change did not make it in at the time.
>
> Well now that I am cleaning up exec I have made the time to read
> through all of the binfmts and the only playing with file descriptors
> is either the security modules closing them in
> security_bprm_committing_creds or is in the generic code in fs/exec.c.
> None of it happens before begin_new_exec is called.
>
> So move unshare_files into begin_new_exec, after the point of no
> return. If memory is very very very low and the application calling
> exec is sharing file descriptor tables between processes we might fail
> past the point of no return. Which is unfortunate but no different
> than any of the other places where we allocate memory after the point
> of no return.
>
> This movement allows another process that shares the file table, or
> another thread of the same process and that closes files or changes
> their close on exec behavior and races with execve to cause some
> unexpected things to happen. There is only one time of check to time
It seems to only make the already existing race window wider by moving
it from bprm_execve() to begin_new_exec() which isn't great but probably
ok since done for a good reason.
> of use race and it is just there so that execve fails instead of
> an interpreter failing when it tries to open the file it is supposed
> to be interpreting. Failing later if userspace is being silly is
> not a problem.
>
> With this change it the following discription from the removal
> of steal_locks[8] finally becomes true.
>
> Apps using NPTL are not affected, since all other threads are killed before
> execve.
>
> Apps using LinuxThreads are only affected if they
>
> - have multiple threads during exec (LinuxThreads doesn't kill other
> threads, the app may do it with pthread_kill_other_threads_np())
> - rely on POSIX locks being inherited across exec
>
> Both conditions are documented, but not their interaction.
>
> Apps using clone() natively are affected if they
>
> - use clone(CLONE_FILES)
> - rely on POSIX locks being inherited across exec
>
> I have investigated some paths to make it possible to solve this
> without moving unshare_files but they all look more complicated[18].
>
> Reported-by: Daniel P. Berrangé <[email protected]>
> Reported-by: Jeff Layton <[email protected]>
> History-tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> [1] 02cda956de0b ("[PATCH] unshare_files"
> [2] 04e9bcb4d106 ("[PATCH] use new unshare_files helper")
> [3] 088f5d7244de ("[PATCH] add steal_locks helper")
> [4] 02c541ec8ffa ("[PATCH] use new steal_locks helper")
> [5] https://lkml.kernel.org/r/[email protected]
> [6] https://lkml.kernel.org/r/[email protected]
> [7] https://lkml.kernel.org/r/[email protected]
> [8] c89681ed7d0e ("[PATCH] remove steal_locks()")
> [9] fd8328be874f ("[PATCH] sanitize handling of shared descriptor tables in failing execve()")
> [10] https://lkml.kernel.org/r/[email protected]
> [11] https://lkml.kernel.org/r/[email protected]
> [12] https://lkml.kernel.org/r/[email protected]
> [13] https://lkml.kernel.org/r/[email protected]
> [14] https://lkml.kernel.org/r/[email protected]
> [15] https://lkml.kernel.org/r/[email protected]
> [16] https://lkml.kernel.org/r/[email protected]
> [17] https://lkml.kernel.org/r/[email protected]
> [18] https://lkml.kernel.org/r/[email protected]
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Slightly scary change but it solves a problem.
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:11PM -0500, Eric W. Biederman wrote:
> Now that exec no longer needs to restore the previous value of current->files
> on error there are no more callers of reset_files_struct so remove it.
>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:10PM -0500, Eric W. Biederman wrote:
> Now that exec no longer needs to return the unshared files to their
> previous value there is no reason to return displaced.
>
> Instead when unshare_fd creates a copy of the file table, call
> put_files_struct before returning from unshare_files.
>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Looks good.
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:23PM -0500, Eric W. Biederman wrote:
> Simplify the code, and remove the chance of races by reading
> RLIMIT_NOFILE only once in f_dupfd.
>
> Pass the read value of RLIMIT_NOFILE into alloc_fd which is the other
> location the rlimit was read in f_dupfd. As f_dupfd is the only
> caller of alloc_fd this changing alloc_fd is trivially safe.
>
> Further this causes alloc_fd to take all of the same arguments as
> __alloc_fd except for the files_struct argument.
>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:22PM -0500, Eric W. Biederman wrote:
> The function __fd_install was added to support binder[1]. With binder
> fixed[2] there are no more users. Further with get_files_struct
> removed there can be no more users of __fd_install that pass anything
> except current->files.
>
> As fd_install just calls __fd_install with "files=current->files",
> merge them together by transforming the files parameter into a
> local variable initialized to current->files.
>
> [1] f869e8a7f753 ("expose a low-level variant of fd_install() for binder")
> [2] 44d8047f1d87 ("binder: use standard functions to allocate fds")
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
+1 on __fd_install() going away.
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:24PM -0500, Eric W. Biederman wrote:
> The function __alloc_fd was added to support binder[1]. With binder
> fixed[2] there are no more users. Further with get_files_struct
> removed there can be no more users of __alloc_fd that pass anything
> except current->files.
>
> As alloc_fd just calls __alloc_fd with "files=current->files",
> merge them together by transforming the files parameter into a
> ocal variable initialized to current->files.
>
> [1] dcfadfa4ec5a ("new helper: __alloc_fd()")
> [2] 44d8047f1d87 ("binder: use standard functions to allocate fds")
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:25PM -0500, Eric W. Biederman wrote:
> The function __close_fd was added to support binder[1]. Now that
> binder has been fixed to no longer need __close_fd[2] and
> get_files_struct has been removed it is no longer possible to
> correctly call __close_fd with anything current->files pass as it's
> files parameter.
>
> Therefore transform the files parameter into a local variable
> initialized to current->files, and rename __close_fd to close_fd to
> reflect this change, and keep it in sync with the similar changes to
> __alloc_fd, and __fd_install.
>
> This removes the need for callers to care about the extra care that
> needs to be take if anything except current->files is passed, by
> limiting the callers to only operation on current->files.
>
> [1] 483ce1d4b8c3 ("take descriptor-related part of close() to file.c")
> [2] 44d8047f1d87 ("binder: use standard functions to allocate fds")
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:16PM -0500, Eric W. Biederman wrote:
> 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 fcheck_task instead of get_files_struct simplifies proc_fd_link by
> removing unnecessary locking, and reference counting.
>
> [1] https://lkml.kernel.org/r/[email protected]
> Suggested-by: Oleg Nesterov <[email protected]>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
> fs/proc/fd.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index 4048a87c51ee..abfdcb21cc79 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -141,29 +141,23 @@ static const struct dentry_operations tid_fd_dentry_operations = {
>
> static int proc_fd_link(struct dentry *dentry, struct path *path)
> {
> - struct files_struct *files = NULL;
> struct task_struct *task;
> int ret = -ENOENT;
>
> task = get_proc_task(d_inode(dentry));
> if (task) {
> - files = get_files_struct(task);
> - put_task_struct(task);
> - }
> -
> - if (files) {
> unsigned int fd = proc_fd(d_inode(dentry));
> struct file *fd_file;
>
> - spin_lock(&files->file_lock);
> - fd_file = fcheck_files(files, fd);
> + rcu_read_lock();
> + fd_file = fcheck_task(task, fd);
> if (fd_file) {
> *path = fd_file->f_path;
> path_get(&fd_file->f_path);
> ret = 0;
> }
> - spin_unlock(&files->file_lock);
> - put_files_struct(files);
> + rcu_read_unlock();
> + put_task_struct(task);
> }
>
> return ret;
> --
> 2.25.0
>
On Mon, Aug 17, 2020 at 05:04:15PM -0500, Eric W. Biederman wrote:
> 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 fcheck_task instead of get_files_struct clarifies tid_fd_mode by
> removing a step.
>
> [1] https://lkml.kernel.org/r/[email protected]
> Suggested-by: Oleg Nesterov <[email protected]>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:14PM -0500, Eric W. Biederman wrote:
> As a companion to fget_task implement fcheck_task for use for querying
> a process about a specific file.
>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:21PM -0500, Eric W. Biederman wrote:
> 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.
>
> Now that get_files_struct has no more users and can not cause the
> problems for posix file locking and fget_light remove get_files_struct
> so that it does not gain any new users.
>
> [1] https://lkml.kernel.org/r/[email protected]
> Suggested-by: Oleg Nesterov <[email protected]>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 05:04:20PM -0500, Eric W. Biederman wrote:
> 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.
>
> Instead hold task_lock for the duration that task->files needs to be
> stable in seq_show. The task_lock was already taken in
> get_files_struct, and so skipping get_files_struct performs less work
> overall, and avoids the problems with the files_struct reference
> count.
>
> [1] https://lkml.kernel.org/r/[email protected]
> Suggested-by: Oleg Nesterov <[email protected]>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
Acked-by: Christian Brauner <[email protected]>
On Mon, Aug 17, 2020 at 06:17:35PM -0700, Linus Torvalds wrote:
> On Mon, Aug 17, 2020 at 6:06 PM Eric W. Biederman <[email protected]> wrote:
> >
> > I struggle with the fcheck name as I have not seen or at least not
> > registed on the the user that just checks to see if the result is NULL.
> > So the name fcheck never made a bit of sense to me.
>
> Yeah, that name is not great. I just don't want to make things even worse.
>
> > I will see if I can come up with some good descriptive comments around
> > these functions. Along with describing what these things are doing I am
> > thinking maybe I should put "_rcu" in their names and have a debug check
> > that verifies "_rcu" is held.
>
> Yeah, something along the lines of "rcu_lookup_fd_task(tsk,fd)" would
> be a *lot* more descriptive than fcheck_task().
>
> And I think "fnext_task()" could be "rcu_lookup_next_fd_task(tsk,fd)".
>
> Yes, those are much longer names, but it's not like you end up typing
> them all that often, and I think being descriptive would be worth it.
>
> And "fcheck()" and "fcheck_files()" would be good to rename too along
> the same lines.
>
> Something like "rcu_lookup_fd()" and "rcu_lookup_fd_files()" respectively?
>
> I'm obviously trying to go for a "rcu_lookup_fd*()" kind of pattern,
> and I'm not married to _that_ particular pattern but I think it would
> be better than what we have now.
In fs/inode.c and a few other places we have the *_rcu suffix pattern
already so maybe:
fcheck() -> fd_file_rcu() or lookup_fd_rcu()
fcheck_files() -> fd_files_rcu() or lookup_fd_files_rcu()
fnext_task() -> fd_file_from_task_rcu() or lookup_next_fd_from_task_rcu()
rather than as prefix or sm.
Christian
Please kill off ksys_close as well while you're at it.
Christoph Hellwig <[email protected]> writes:
> Please kill off ksys_close as well while you're at it.
Good point. ksys_close is just a trivial wrapper around close_fd. So
the one caller of ksys_close autofs_dev_ioctl_closemount can be
trivially changed to call close_fd.
Eric
kernel test robot <[email protected]> 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 <[email protected]>
>
> 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/[email protected]
On Wed, Aug 19, 2020 at 6:25 AM Eric W. Biederman <[email protected]> wrote:
>
> The bug in the existing code is that bpf_iter does get_file instead
> of get_file_rcu. Does anyone have any sense of how to add debugging
> to get_file to notice when it is being called in the wrong context?
That bug is already fixed in bpf tree.
See commit cf28f3bbfca0 ("bpf: Use get_file_rcu() instead of
get_file() for task_file iterator")
On Wed, Aug 19, 2020 at 6:25 AM Eric W. Biederman <[email protected]> wrote:
>
> So I sat down and played with it and here is what I wound up with is:
>
> __fcheck_files -> files_lookup_fd_raw
> fcheck_files -> files_lookup_fd_locked
> fcheck_files -> files_lookup_fd_rcu
> fcheck -> lookup_fd_rcu
> ...
> fcheck_task -> task_lookup_fd_fcu
> fnext_task -> task_lookup_next_fd_rcu
This certainly looks fine to me. No confusion about what it does. So Ack.
Linus
On Mon, Aug 17, 2020 at 05:04:12PM -0500, Eric W. Biederman wrote:
> Use the helper fget_task and simplify the code.
>
> As well as simplifying the code this removes one unnecessary increment of
> struct files_struct. This unnecessary increment of files_struct.count can
> result in exec unnecessarily unsharing files_struct and breaking posix
> locks, and it can result in fget_light having to fallback to fget reducing
> performance.
>
> Suggested-by: Oleg Nesterov <[email protected]>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
Reviewed-by: Cyrill Gorcunov <[email protected]>
I really like this simplification!
On Mon, Aug 17, 2020 at 05:04:17PM -0500, Eric W. Biederman wrote:
> As a companion to fget_task and fcheck_task implement fnext_task that
> will return the struct file for the first file descriptor show number
> is equal or greater than the fd argument value, or NULL if there is
> no such struct file.
>
> This allows file descriptors of foreign processes to be iterated through
> safely, without needed to increment the count on files_struct.
>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
> fs/file.c | 21 +++++++++++++++++++++
> include/linux/fdtable.h | 1 +
> 2 files changed, 22 insertions(+)
>
> diff --git a/fs/file.c b/fs/file.c
> index 8d4b385055e9..88f9f78869f8 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -876,6 +876,27 @@ struct file *fcheck_task(struct task_struct *task, unsigned int fd)
> return file;
> }
>
> +struct file *fnext_task(struct task_struct *task, unsigned int *ret_fd)
> +{
> + /* Must be called with rcu_read_lock held */
> + struct files_struct *files;
> + unsigned int fd = *ret_fd;
> + struct file *file = NULL;
> +
> + task_lock(task);
> + files = task->files;
> + if (files) {
> + for (; fd < files_fdtable(files)->max_fds; fd++) {
> + file = fcheck_files(files, fd);
> + if (file)
> + break;
> + }
> + }
> + task_unlock(task);
> + *ret_fd = fd;
> + return file;
> +}
Eric, if only I'm not missing something obvious you could
escape @fd/@ret_fd operations in case if task->files = NULL,
iow
if (files) {
unsigned int fd = *ret_fd;
for (; fd < files_fdtable(files)->max_fds; fd++) {
file = fcheck_files(files, fd);
if (file)
break;
}
*ret_fd = fd;
}
On Fri, Aug 21, 2020 at 8:26 AM Eric W. Biederman <[email protected]> wrote:
>
> Alexei Starovoitov <[email protected]> writes:
>
> > On Wed, Aug 19, 2020 at 6:25 AM Eric W. Biederman <[email protected]> wrote:
> >>
> >> The bug in the existing code is that bpf_iter does get_file instead
> >> of get_file_rcu. Does anyone have any sense of how to add debugging
> >> to get_file to notice when it is being called in the wrong context?
> >
> > That bug is already fixed in bpf tree.
> > See commit cf28f3bbfca0 ("bpf: Use get_file_rcu() instead of
> > get_file() for task_file iterator")
>
> I wished you had based that change on -rc1 instead of some random
> looking place in David's Millers net tree.
random?
It's a well documented process. Please see:
Documentation/bpf/bpf_devel_QA.rst
> I am glad to see that our existing debug checks can catch that
> kind of problem when the code is exercised enough.
They did not. Please see the commit log of the fix.
It was a NULL pointer dereference.
> I am going to pull this change into my tree on top of -rc1 so we won't
> have unnecessary conflicts. Hopefully this will show up in -rc2 so the
> final version of this patchset can use an easily describable base.
Please do not cherry pick fixes from other trees. You need to wait
until the bpf tree gets merged into net tree and net into Linus's tree.
It's only a couple days away. Hopefully it's there by -rc2,
but I cannot speak for Dave's schedule.
We'll send bpf tree pull-req to Dave today.