linux-kernel - Re: [PATCH 3/9] pci/switchtec: Don't abuse completion wait queue for poll

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: <1adfff2f-86a5-d692-bac6-c87a15f24d67@deltatee.com>
Date:   Mon, 16 Mar 2020 19:15:53 -0600
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-kernel@...r.kernel.org
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>, Will Deacon <will@...nel.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Kurt Schwemmer <kurt.schwemmer@...rosemi.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org
Subject: Re: [PATCH 3/9] pci/switchtec: Don't abuse completion wait queue for
 poll

On 2020-03-16 6:17 p.m., Thomas Gleixner wrote:
> That's a good question, but that question simply arises due to the fact
> that C does not provide proper privatizing or if you want to work around
> that it forces you to come up with really horrible constructs.

Well, we do have the underscore convention. Though, I concede this code
could potentially predate that. Had there been a preceding underscore, I
definitely would have thought twice before touching it.

> That's not a made up nightmare scenario. This happened in reality and
> caused me to mop up 50+ interrupt chip implementations in order to be
> able to make an urgently needed 10 line change to the core interrupt
> infrastructure. Guess what, the vast majority of instances fiddling with
> the core internals were either voodoo programming or plain bugs. There
> were a few legitimate issues, but they had been better solved in the
> core code upfront.  Even after that cleanup a driver got merged which
> had #include "../../../../kernel/irg/internal.h" inside just because the
> code which was developed out of tree before this change had be made to
> "work".

I get where your coming from, and it sucks having to clean up so many
instances in an urgent situation. But I see this kind of cleanup work as
routine, a necessary thing that happens all the time. I've done it
myself a couple times before in the kernel. The hard trick is to get
developers to do more of it, before it becomes a problem like the one
you faced.

In my experience, what makes clean up work even harder is where
developers see an interface, notice it's not a perfect fit and so open
code the whole thing themselves. Then you have random buggy primitives
open coded all over the place that are impossible to find. And the
primitives themselves never improve or grow new interfaces because
nobody knows there's a bunch of instances that require the improvement.
That's a much bigger mess.

> I really prefer when people come along and show the problem they want to
> solve - I'm completely fine with the POC hack which uses internals for
> that purpose - so that this can be discussed and eventually integrated
> into core infrastructure in one way or the other or better suitable
> solutions can be found.

Yes, and this code was a prototype at one point and went through review
from a number of people in the community, and nobody complained about
this. I've also been in the situation where I submitted a POC and
somebody pointed out a better way (though with a few swears thrown in
for good measure); but in that case, I was actually changing a primitive
so it got their attention more easily.

It's impossible for the maintainer of a primitive to review all the use
cases of every primitive when new code gets merged. But at least if new
code uses/abuses the primitive they will eventually come to light and
can be cleaned up as appropriate with a holistic view.

> I hope that clarifies where I'm coming from.

Yes, I understood your point. And I concede that a completion is a
pretty trivial primitive to open code and the change is not really worth
any further fight. If the patch gets merged (preferably with a reworked
commit message), I will not complain.

> This has nothing to do with you personally, you just triggered a few
> sensible fuses while understandably defending your admittedly smart
> solution.

Thank you.

Logan