Message-ID: <aCc_PSOPkLWTcTru@pollux>
Date: Fri, 16 May 2025 15:35:57 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Greg KH <gregkh@...uxfoundation.org>,
	John Hubbard <jhubbard@...dia.com>, Timur Tabi <timur@...nel.org>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH] rust: add basic ELF sections parser

On Fri, May 16, 2025 at 10:26:10PM +0900, Alexandre Courbot wrote:
> On Fri May 16, 2025 at 10:15 PM JST, Greg KH wrote:
> > On Thu, May 15, 2025 at 12:17:00PM -0700, John Hubbard wrote:
> >> On 5/15/25 7:30 AM, Timur Tabi wrote:
> >> > On Thu, May 15, 2025 at 6:43 AM Greg KH <gregkh@...uxfoundation.org> wrote:
> >> >>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
> >> >>
> >> >> The "problem" I'm not understanding is why does the kernel have to do
> >> >> any of this parsing at all?
> >> > 
> >> > Nova will need to parse ELF headers in order to properly load and boot
> >> > Nvidia firmware images.  Nouveau does this already:
> >> > 
> >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c#n2931
> >> 
> >> Hi Greg!
> >> 
> >> Nouveau influenced us heavily here, because having firmware that we
> >> can post once, and use everywhere (Nouveau and Nova), is very attractive.
> >> 
> >> Alex and Timur discuss other details that explain why the standard 
> >> user-space approach is less simple and clean than it might appear at
> >> first glance, but I wanted to emphasize that the firmware re-use point
> >> a little bit, too.
> >> 
> >> Oh, and also: the ELF images are going to remain extremely simple,
> >> because there is nothing now (nor can I see anything in the future)
> >> that would drive anyone to do complicated things. For example, if
> >> there is some exotic new thing in the future, it could be put into
> >> its own firmware image if necessary--because we understand that
> >> this parser here is intended to be a simple subset of ELF, and
> >> left alone really.
> >
> > Ok, then why not just bury this down in the driver that is going to
> > actually use it?  This patch series was adding it to ALL kernels, if you
> > need/want it or not, and as such would be seen as a generic way to
> > handle all ELF images.  But as that's not the case here, just copy what
> > you did in the existing C driver and make it private to your code, so
> > that no one else has to worry about accidentally thinking it would also
> > work for their code :)
> 
> Keeping this local to nova-core is perfectly fine if you think this is
> more acceptable. AFAIK there are no other users for it at the moment.

I'm not quite on board with that.

I think we should either we get to the conclusion that the desire of parsing (at
least part of) the firmware ELF is valid in the kernel and make it generic
infrastructure, or conclude that there really isn't a reasonable technical
reason to do that.

Please let's work out the exact technical reasons for doing this in the kernel,
such that we can either conclude one or the other.

> > And I still think that having the kernel do this is a mistake, firmware
> > should always just be a "pass through" otherwise you open yourself up to
> > all sorts of complexity and vulnerabilities in the kernel, both of which
> > is generally not a good idea.
> 
> I agree on principle, but I cannot think of a way to avoid doing this in
> the kernel without making things overly complex. We're happy to consider
> alternatives though, if they exist.

Powered by blists - more mailing lists