Re: [PATCH 5.10 000/122] 5.10.211-rc1 review

[Dominique Martinet]

Kees Cook wrote on Wed, Feb 28, 2024 at 12:39:42PM -0800:
> > > This commit breaks build of some 3rd party wireless module we use here
> > > (because sizeof(sa->sa_data) no longer works and needs to use
> > > sa_data_min)
> 
> Just FYI, it's possible that things using sizeof(sa->sa_data) were buggy
> to begin with since the struct size isn't actually dictated by that size
> (it's only the minimum possible size).

Yes, I definitely agree with this.
As it's "vendor stuff" I just replaced with sa_data_min because that
preserves the values, but it ought to get a second look.
I'd love to pretend that driver's upstream will do the right thing and
use proper values here on newer kernel but upon checking its >6.2 tree
support now they apparently did the same instead of getting the size
properly.

> > We NEVER preserve in-kernel APIs for any out-of-tree code as obviously,
> > we have no idea what out-of-tree code is actually using, so it would be
> > impossible to do so.

Right, I just don't see much "common struct" changes in stable tree
patches -- stuff like livepatches or weak modules and whatsnot don't
like these so some downstreams (redhat to name them) try very hard to
keep these constants for the lifetime of a given stable release... iirc
they go as far as adding some padding fields to some structs that are
likely to need fiddling so they can do this while preserving binary
compatibility.

I understand the upstream stable kernels don't make such promise (and
given the amount of work that probably goes into it, rightfully so! I
wouldn't exect you or anyone to do this here), just pointed it out
as part of my usual test round for anyone else who'd care.

> Out of curiosity, which drivers broke and what's needed to get them into
> upstream (or at least staging)?

Sure, it was NXP's wifi chips driver:
https://github.com/nxp-imx/mwifiex/

It's mostly based on drivers/net/wireless/marvell/mwifiex but has since
been quite extensively modified, so it'd take quite a bit of effort to
upstream as a separate entity (changing a few names to avoid conflcts so
both can be built together... Add to that the requirement for a
compatible firmware with a restrictive license... And that NXP isn't
exactly focused on upstreaming); I have little hope of seeing it
upstream at this point unfortunately and gave up on it as part of
maintaining an embedded kernel "port" as it's sadly far from being
only one :/

(I especially don't get it as I consider maintaining a bunch of
spaghetti ifdef on kernel versions to be much more work than getting the
driver upstream once, but I guess I'm barking at the wrong tree here)


Thanks,
-- 
Dominique Martinet | Asmadeus