linux-kernel - Re: [PATCH] fs/dcache.c: re-add cond_resched() in shrink_dcache

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: <20180415204054.GA30522@ZenIV.linux.org.uk>
Date:   Sun, 15 Apr 2018 21:40:54 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Nikolay Borisov <nborisov@...e.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Khazhismel Kumykov <khazhy@...gle.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        David Rientjes <rientjes@...gle.com>,
        Goldwyn Rodrigues <rgoldwyn@...e.de>,
        Jeff Mahoney <jeffm@...e.com>,
        Davidlohr Bueso <dave@...olabs.net>
Subject: Re: [PATCH] fs/dcache.c: re-add cond_resched() in
 shrink_dcache_parent()

On Sun, Apr 15, 2018 at 11:34:17AM -0700, Linus Torvalds wrote:

> No, it's obviously not type-safe, but at least it's _legible_, and is
> a pattern, while that "let's randomly just do a cast in the middle of
> the code" is just nasty.

Sure, no problem...  I really wish there was a way to say

void foo(int (*f)(α *), α *data) ∀ α

and have the compiler verify that foo(f, v) is done only
when f(v) is well-typed, but that's C, not Haskell...  The best
approximation is something along the lines of

void __foo(int (*f)(void *), void *data);
#define foo(f, v) (sizeof((f)((v)), 0), __foo((f),(v)))

and that relies upon the identical calling sequence for all pointer
arguments.  AFAIK, it's true for all ABIs we support, but...
Worse, there's no way to get #define in macro expansion, so the
above would be impossible to hide behind anything convenient ;-/

> Side note: I do feel like "d_walk()" should be returning whether it
> terminated early or not. For example, this very same code in the
> caller does
> 
> +               struct dentry *victim = NULL;
> +               d_walk(dentry, &victim, find_submount);
> +               if (!victim) {
> 
> but in many ways it would be more natural to just check the exit condition, and
> 
> +               struct dentry *victim;
> +               if (!d_walk(dentry, &victim, find_submount)) {
> 
> don't you think? Because that matches the actual setting condition in
> the find_submount() callback.
> 
> There are other situations where the same thing is true: that
> path_check_mount() currently has that "info->mounted" flag, but again,
> it could be replaced by just checking what the quit condition was, and
> whether we terminated early or not. Because the two are 100%
> equivalent, and the return value in many ways would be more logical, I
> feel.
> 
> (I'm not sure if we should just return the actual exit condition -
> defaulting to D_WALK_CONTINUE if there was nothing to walk at all - or
> whether we should just return a boolean for "terminated early")
> 
> Hmm?

Not sure...   There are 5 callers:
	* do_one_tree(), d_genocide() - nothing to return
	* path_has_submounts(), d_invalidate() - could use your trick,
but d_invalidate() wants to look at victim if not buggering off, so
that one doesn't win much
	* shrink_dcache_parent() - no way to use that.  Here we normally
run the walk to completion and need to repeat it in all cases of early
termination *and* in some of the ran-to-completion cases.

BTW, the current placement of cond_resched() looks bogus; suppose we
have collected a lot of victims and ran into need_resched().  We leave
d_walk() and call shrink_dentry_list().  At that point there's a lot
of stuff on our shrink list and anybody else running into them will
have to keep scanning.  Giving up the timeslice before we take care
of any of those looks like a bad idea, to put it mildly, and that's
precisely what will happen.

What about doing that in the end of __dentry_kill() instead?  And to
hell with both existing call sites - dput() one (before going to
the parent) is obviously covered by that (dentry_kill() only returns
non-NULL after having called __dentry_kill()) and in shrink_dentry_list()
we'll get to it as soon as we go through all dentries that can be
immediately kicked off the shrink list.  Which, AFAICS, improves the
situation, now that shrink_lock_dentry() contains no trylock loops...

Comments?