lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 7 Dec 2020 23:29:00 +0000
From:   Al Viro <>
To:     "Eric W. Biederman" <>
        Linus Torvalds <>,
        Christian Brauner <>,
        Oleg Nesterov <>,
        Cyrill Gorcunov <>,
        Jann Horn <>, Kees Cook <>,
        Daniel P . Berrangé <>,
        Jeff Layton <>,
        Miklos Szeredi <>,
        Matthew Wilcox <>,
        "J. Bruce Fields" <>,
        Trond Myklebust <>,
        Chris Wright <>,
        Alexei Starovoitov <>,
        Daniel Borkmann <>,
        Martin KaFai Lau <>,
        Song Liu <>, Yonghong Song <>,
        Andrii Nakryiko <>,
        John Fastabend <>,
        KP Singh <>,
        Andy Lavr <>
Subject: Re: [PATCH v2 15/24] proc/fd: In proc_readfd_common use

On Fri, Nov 20, 2020 at 05:14:32PM -0600, 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 task_lookup_next_fd_rcu 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.
> As task_lookup_fd_rcu may update the fd ctx->pos has been changed
> to be the fd +2 after task_lookup_fd_rcu returns.

> +	for (fd = ctx->pos - 2;; fd++) {
>  		struct file *f;
>  		struct fd_data data;
>  		char name[10 + 1];
>  		unsigned int len;
> -		f = files_lookup_fd_rcu(files, fd);
> +		f = task_lookup_next_fd_rcu(p, &fd);

Ugh...  That makes for a massive cacheline pingpong on task_lock -
instead of grabbing/dropping task_lock() once in the beginning, we do
that for every damn descriptor.

I really don't like this one.  If anything, I would rather have
a helper that would collect a bunch of pairs (fd,mode) into an
array and have lookups batched into it.  With the loop in that
sucker grabbing a reasonable amount into a local array, then
doing proc_fill_cache() for each collected.

Powered by blists - more mailing lists