svn commit: r330451 - in stable/11/sys: dev/iwm dev/otus dev/usb/wlan net80211

[John Baldwin]

On Monday, March 05, 2018 07:13:59 PM Eitan Adler wrote:
> On 5 March 2018 at 10:08, John Baldwin <jhb at freebsd.org> wrote:
> > On Monday, March 05, 2018 07:54:58 AM Eitan Adler wrote:
> >> Author: eadler
> >> Date: Mon Mar  5 07:54:57 2018
> >> New Revision: 330451
> >> URL: https://svnweb.freebsd.org/changeset/base/330451
> >>
> >> Log:
> >>   MFC r306837:
> >>
> >>   [net80211] extend the ieee80211_rx_stats struct to include more information.
> >
> > Have you thought about the KBI implications of this change and some of the
> > other changes you've merged?
> 
> I do have a copy of the modules from 11.1 and have loaded them at
> various points in time after merging. That said, I am not perfect.
> Unfortunately, my -STABLE box did not have fully functioning drivers
> before these changes so its difficult to test anything beyond "loads
> and does not panic.". I havn't done so today yet, but that will happen
> soon.
> 
> Ensuring these things work through code inspection is certainly
> possible and I've skipped over several changes as a result.

Loading a module doesn't alone doesn't actually test for breakage.  That only
breaks if you remove symbols.  Changing the semantics of how functions work
or the layout of a structure that is passed between structures is an ABI change
even if it doesn't change the function signature.  In this case, this change
inserts new fields and changes the size of 'struct ieee80211_rx_stats'.  Thus
if a otus(4) or iwm(4) driver built against the prior revision (330450) is
used with a kernel built with this revision (330451), the driver will pass in
an 'rxs' structure in a call to ieee80211_input_mimo() that is smaller, and
in the first few lines of ieeee80211_input_mimo() these statements will
overflow that pointer and read stack garbage:

85	int
86	ieee80211_input_mimo(struct ieee80211_node *ni, struct mbuf *m,
87	    struct ieee80211_rx_stats *rx)
88	{
89	        struct ieee80211_rx_stats rxs;
90	
91	        if (rx) {
92	                memcpy(&rxs, rx, sizeof(*rx));
93	        } else {

Furthermore, the now garbage 'rxs' (since fields in *rx are in different
places so even the bits of 'rxs' that isn't stack garbage will be wrong)
will now be used by other routines in net80211.  Your next MFC commit then
causes another ABI breakage as it removes the 'rx' parameter from the
function.  You can't do that.  If you don't know how to recognize those types
of ABI breakages, then you probably shouldn't be MFC'ing changes without
getting some sort of review.  It also tends to be a lot harder to find these
breakages in code one didn't write.

The fact that there are back-to-back ABI breakages also suggests that it is
much better to consolidate MFCs into larger commits because you limit the
amount of compatibilty ABI shims you have to provide.

I think for net80211 you need to generate a diff of all of the wireless
related changes you have MFC'd to stable/11 and then review that for ABI
changes and come up with a plan for how to restore the ABI and get re@'s
approval.  (kib@ is a pretty good resource for devising ABI shims.)

I think that going forward you shouldn't MFC changes if you aren't certain
about the ABI implications until you have had someone review them.  Batching
up changes into a single diff is also helpful since if an API changes back
and forth in HEAD multiple times, collapsing them means that for stable you
may only need a single compat shim rather than several.

-- 
John Baldwin