linux-kernel - Re: [RFC][PATCHSET] epoll cleanups

lists.openwall.net		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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201004121329.GG20115@casper.infradead.org>
Date:   Sun, 4 Oct 2020 13:13:29 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Marc Zyngier <maz@...nel.org>
Subject: Re: [RFC][PATCHSET] epoll cleanups

On Sun, Oct 04, 2020 at 03:36:08AM +0100, Al Viro wrote:
> 	Finally, there's the mess with reverse path check.  When we insert
> a new file into a epoll, we need to check that there won't be excessively long
> (or excessively many) wakeup chains.  If the target is not an epoll, we need
> to check that for the target alone, but if it's another epoll we need the same
> check done to everything reachable from that epoll.  The way it's currently
> done is Not Nice(tm): we collect the set of files to check and, once we have
> verified the absence of loops, created a new epitem and inserted it into
> the epoll data structures, we run reverse_path_check_proc() for every file
> in the set.  The set is maintained as a (global) cyclic list threaded through
> some struct file.  Everything is serialized on epmutex, so the list can be
> global.  Unfortunately, each struct file ends up with list_head embedded into
> it, all for the sake of operation that is rare *and* involves a small fraction
> of all struct file instances on the system.
> 	Worse, files can end up disappearing while we are collecting the set;
> explicit EPOLL_CTL_DEL does not grab epmutex, and final fput() won't touch
> epmutex if all epitem refering to that file have already been removed.  This
> had been worked around this cycle (by grabbing temporary references to struct
> file added to the set), but that's not a good solution.
> 	What we need is to separate the head of epitem list (->f_ep_links)
> from struct file; all we need in struct file is a reference to that head.
> We could thread the list representing the set of files through those objects
> (getting rid of 3 pointers in each struct file) and have epitem removal
> free those objects if there's no epitems left *and* they are not included
> into the set.  Reference from struct file would be cleared as soon as there's
> no epitems left.  Dissolving the set would free those that have no epitems
> left and thus would've been freed by ep_remove() if they hadn't been in the
> set.
> 	With some massage it can be done; we end up with
> * 3 pointers gone from each struct file
> * one pointer added to struct eventpoll
> * two-pointer object kept for each non-epoll file that is currently watched by
> some epoll.

Have you considered just storing a pointer to each struct file in an
epoll set in an XArray?  Linked lists suck for modern CPUs, and there'd
be no need to store any additional data in each struct file.  Using
xa_alloc() to store the pointer and throw away the index the pointer
got stored at would leave you with something approximating a singly
linked list, except it's an array.  Which does zero memory allocations
for a single entry and will then allocate a single node for your first
64 entries.