bridge: Restore ABI compatibility with 14.x
ClosedPublic
Actions

Authored by ivy on Tue, Aug 19, 4:47 PM.

Details

Reviewers

kevans
flo
zlei

Group Reviewers

bridge
network

Commits

rG110a2fa97d51: bridge: Restore ABI compatibility with 14.x

Summary

When new fields were added to struct ifbreq in 15.0, the decision was
made to not use the padding to preserve binary compatibility. However
this causes some issues, including the inability for 14.x jails to use
bridges, and a failure to bring up networking when booting a 15 kernel
with a 14.x userland, e.g. during upgrade.

Restore the old size of struct ifbreq by eating some of the padding.
This only requires 4 bytes of padding; we have 28 bytes left, and in
the medium term this problem will be solved with a netlink interface,
so running out of padding should not be a concern.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 66350
Build 63233: arc lint + arc unit

Event Timeline

ivy created this revision.Tue, Aug 19, 4:47 PM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptTue, Aug 19, 4:47 PM

ivy requested review of this revision.Tue, Aug 19, 4:47 PM

Harbormaster completed remote builds in B66349: Diff 160614.Tue, Aug 19, 4:47 PM

add an UPDATING entry

Harbormaster completed remote builds in B66350: Diff 160615.Tue, Aug 19, 4:50 PM

Please also give this a few Fixes tags:

At the very least, this is sufficient to give someone downstream the information they need to avoid/defer breakage from the first commit until they're ready to pickup the second and this fix at the same time.

This revision is now accepted and ready to land.Tue, Aug 19, 5:43 PM

This patch fixes my reported upgrade case. Thanks.

zlei accepted this revision as: zlei.Thu, Aug 21, 2:26 AM

zlei added a subscriber: zlei.

zlei added inline comments.

sys/net/if_bridgevar.h
162	Given `struct ifbreq` is actually part of ABI, I'd recommend you adding `_Static_assert()` to assert its size, so the compiler will complain if forget to change the pad.

ivy added inline comments.Thu, Aug 21, 3:12 AM

sys/net/if_bridgevar.h
162	since layout/padding of the struct may change between architectures, what would be the usual way to do this? should it be under `#ifdef __amd64__`? `__packed` might be a better solution, but that risks changing the ABI again.

zlei added inline comments.Thu, Aug 21, 3:55 AM

sys/net/if_bridgevar.h

162

The current version can be landed as is.

The IFNAMSIZ is 16, and struct ifbreq has used the fixed sized vibrant of integers, say uint32_t and uint16_t, so that the size is architecture independent.

Well currently there're holes in struct ifbreq. The struct has 5 uint8_t members which are not properly aligned. So you're right that __packed will break ABI again.

I think it is time to explicitly add the holes members.

--- a/sys/net/if_bridgevar.h
+++ b/sys/net/if_bridgevar.h
@@ -154,13 +154,16 @@ struct ifbreq {
        uint8_t         ifbr_proto;             /* member if STP protocol */
        uint8_t         ifbr_role;              /* member if STP role */
        uint8_t         ifbr_state;             /* member if STP state */
+       uint8_t         _hole[3];               /* holes */
        uint32_t        ifbr_addrcnt;           /* member if addr number */
        uint32_t        ifbr_addrmax;           /* member if addr max */
        uint32_t        ifbr_addrexceeded;      /* member if addr violations */
        ether_vlanid_t  ifbr_pvid;              /* member if PVID */
        uint16_t        ifbr_vlanproto;         /* member if VLAN protocol */
-       uint8_t         pad[32];
+       uint8_t         pad[28];
 };
+_Static_assert(sizeof(struct ifbreq) == 80,
+    "size of struct ifbreq is wrong");

That is only tested on amd64 platform, I think that should also be tested on big-endian platforms.

zlei added inline comments.Thu, Aug 21, 4:30 AM

sys/net/if_bridgevar.h
162	The current version can be landed as is. The `IFNAMSIZ` is 16, and `struct ifbreq` has used the fixed sized vibrant of integers, say `uint32_t` and `uint16_t`, so that the size is architecture independent. Well currently there're holes in `struct ifbreq`. The struct has 5 `uint8_t` members which are not properly aligned. So you're right that `__packed` will break ABI again. I think it is time to explicitly add the holes members. --- a/sys/net/if_bridgevar.h +++ b/sys/net/if_bridgevar.h @@ -154,13 +154,16 @@ struct ifbreq { uint8_t ifbr_proto; /* member if STP protocol / uint8_t ifbr_role; / member if STP role / uint8_t ifbr_state; / member if STP state / + uint8_t _hole[3]; / holes / Well, this again breaks ABI, if user pass option say `-fpack-struct` to the compiler, or users build world with different compilers. uint32_t ifbr_addrcnt; / member if addr number / uint32_t ifbr_addrmax; / member if addr max / uint32_t ifbr_addrexceeded; / member if addr violations / ether_vlanid_t ifbr_pvid; / member if PVID / uint16_t ifbr_vlanproto; / member if VLAN protocol */ uint8_t pad[32]; + uint8_t pad[28]; }; +_Static_assert(sizeof(struct ifbreq) == 80, + "size of struct ifbreq is wrong"); That is only tested on amd64 platform, I think that should also be tested on big-endian platforms. Please go ahead and forget asserting the size of `struct ifbreq`.

kevans added inline comments.Thu, Aug 21, 5:20 AM

sys/net/if_bridgevar.h
162	Ideally she'll subsequently make it a moot point anyways; netlink comes in and `struct ifbreq` just remains frozen in time more or less as it is today.