Date:   Mon, 19 Sep 2022 10:20:52 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Wedson Almeida Filho <wedsonaf@...il.com>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Kees Cook <keescook@...omium.org>,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
        Konstantin Shelekhin <k.shelekhin@...ro.com>, ojeda@...nel.org,
        alex.gaynor@...il.com, ark.email@...il.com,
        bjorn3_gh@...tonmail.com, bobo1239@....de, bonifaido@...il.com,
        boqun.feng@...il.com, davidgow@...gle.com, dev@...lasmohrin.de,
        dsosnowski@...snowski.pl, foxhlchen@...il.com, gary@...yguo.net,
        geofft@...reload.com, gregkh@...uxfoundation.org,
        jarkko@...nel.org, john.m.baublitz@...il.com,
        leseulartichaut@...il.com, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, m.falkowski@...sung.com,
        me@...enk.de, milan@...verde.com, mjmouse9999@...il.com,
        patches@...ts.linux.dev, rust-for-linux@...r.kernel.org,
        thesven73@...il.com, viktor@...ar.de,
        Andreas Hindborg <andreas.hindborg@....com>
Subject: Re: [PATCH v9 12/27] rust: add `kernel` crate

On Mon, Sep 19, 2022 at 9:09 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> The whole "really know what context this code is running within" is
> really important. You may want to write very explicit comments about
> it.

Side note: a corollary of this is that people should avoid "dynamic
context" things like the plague, because it makes for such pain when
the context isn't statically obvious.

So things like conditional locking should generally be avoided as much
as humanly possible. Either you take the lock or you don't - don't
write code where the lock context depends on some argument value or
flag, for example.

Code like this is fine:

        if (some_condition) {
                spin_lock(&mylock);
                xyz();
                spin_unlock(&mylock);
        }

because 'xyz()' is always run in the same context. But avoid patterns like

        if (some_condition)
                spin_lock(&mylock);
        xyz();
        if (same_condition)
                spin_unlock(&mylock);

where now 'xyz()' sometimes does something with the lock held, and
sometimes not. That way lies insanity.

Now, obviously, the context for helper functions (like the Rust kernel
crate is, pretty much by definition) obviously depends on the context
of the callers of said helpers, so in that sense the above kind of
"sometimes in locked context, sometimes not" will always be the case.

So those kinds of helper functions will generally need to be either
insensitive to context and usable in all contexts (best), or
documented - and verify with debug code like 'might_sleep()' - that
they only run in certain contexts.

And then in the worst case there's a gfp_flag that says "you can only
do these kinds of allocations" or whatever, but even then you should
strive to never have other dynamic behavior (ie please try to avoid
behavior like having a "already locked" argument and then taking a
lock depending on that).

Because if you follow those rules, at least you can statically see the
context from a call chain (so, for example, the stack trace of an oops
will make the context unambiguous, because there's hopefully no lock
or interrupt disabling or similar that has some dynamic behavior like
in that second example of "xyz()".

Do we have places in the kernel that do conditional locking? Yes we
do. Examples like that second case do exist. It's bad. Sometimes you
can't avoid it. But you can always *strive* to avoid it, and
minimizing those kinds of "context depends on other things"
situations.

And we should strive very hard to make those kinds of contexts very
clear and explicit and not dynamic exactly because it's so important
in the kernel, and it has subtle implications wrt other locking, and
memory allocations.

             Linus

Powered by blists - more mailing lists