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/20180915160423.ga31...@redhat.com
> Suggested-by: Oleg Nesterov <o...@redhat.com>
> Signed-off-by: "Eric W. Biederman" <ebied...@xmission.com>
> ---
Acked-by: Christian Brauner <christian.brau...@ubuntu.com>
> 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
>
