linux-kernel - Re: drm pull for v5.3-rc1

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: <CAHk-=wiD6a189CXj-ugRzCxA9r1+siSCA0eP_eoZ_bk_bLTRMw@mail.gmail.com>
Date:   Mon, 15 Jul 2019 11:00:12 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Dave Airlie <airlied@...il.com>
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jason Gunthorpe <jgg@...lanox.com>,
        Jerome Glisse <jglisse@...hat.com>,
        Thomas Hellstrom <thellstrom@...are.com>
Subject: Re: drm pull for v5.3-rc1

On Mon, Jul 15, 2019 at 10:37 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> I'm not pulling this. Why did you merge it into your tree, when
> apparently you were aware of how questionable it is judging by the drm
> pull request.

Looking at some of the fallout, I also see that you then added that
"adjust apply_to_pfn_range interface for dropped token" patch that
seems to be for easier merging of this all.

But you remove the 'token' entirely in one place, and in another you
keep it and just say "whatever, it's unused, pass in NULL". WHAA?

As part of looking at this all, I also note that some of this is also
very non-kernely.

The whole thing with trying to implement a "closure" in C is simply
not how we do things in the kernel (although I've admittedly seen
signs of it in some drivers).

If this should be done at all (and that's questionable), at least do
it in the canonical kernel way: pass in a separate "actor" function
pointer and an argument block, don't try to mix function pointers and
argument data and call it a "closure".

We try to keep data and functions separate. It's not even for security
concerns (although those have caused some splits in the past - avoid
putting function pointers in structures that you then can't mark
read-only!), it's a more generic issue of just keeping arguments as
arguments - even if you then make a structure of them in order to not
make the calling convention very complicated.

(Yes, we do have the pattern of sometimes mixing function pointers
with "describing data", ie the "struct file_operations" structure
isn't _just_ actual function pointers, it also contains the module
owner, for example. But those aren't about mixing function pointers
with their arguments, it's about basically "describing" an object
interface with more than just the operation pointers).

So some of this code is stuff that I would have let go if it was in
some individual driver ("Closures? C doesn't have closures! But
whatever - that driver writer came from some place that taught lamda
calculus before techning C").

But in the core mm code, I want reviews. And I want the code to follow
normal kernel conventions.

                   Linus