bridge: add a netlink interface

[ivy]

Summary

For easier review of the basic scaffolding, only two commands are
supported: add member and remove member, which have the same semantics
as the existing add and delete ioctls.

Update ifconfig to use netlink for these operations.

The netlink structure used for add member is called "modifymember",
because this structure will also be used for the modify member operation
once it's added; this allows interface parameters to be set at the same
time the interface is added to the bridge.

[ivy]

i've never really used netlink before and there appears to be very little documention on it, so this might be completely wrong; it's pieced together from various other examples of netlink in sys/.

[ivy]

fix a comment typo

[ivy]

fix setting MTU on new members; remove an unused variable

[kp]

sbin/ifconfig/ifbridge.c

287

Ideally don't terminate the process in a library. Return an error instead, and let ifconfig terminate the process.

The intent behind libifconfig is for it to be used by other code that needs to interact with the network stack, and we don't know how that code will want to handle errors.

335–339

Would it make sense to have a bridge-netlink handle in the if_ctx? That way we'd only have to initialise this once, rather than for every request.

sys/net/if_bridge.c

4550

We do this same lookup in the ioctl variant. Could we move it into bridge_cmd_add() to reduce duplication?

[ivy]

sbin/ifconfig/ifbridge.c

287

this is in ifconfig, not libifconfig. possibly it should be moved to libifconfig, but the old ioctl-based code wasn't there either, so i put the new code in the same place.

sys/net/if_bridge.c

4550

it could be moved, but eventually the netlink addmember will support doing more things (like setting interface vlan configuration) that the ioctl interface doesn't do, so it'll probably need the ifnet anyway.

on the other hand, perhaps a better way to handle that is to have bridge_cmd_add return the new bridge_iflist... i'll have a look at whether that works.

[kp]

sbin/ifconfig/ifbridge.c

287

*headdesk*. I'm going to blame it on not having been awake enough yet.

[melifaro]

Thank you for working on that!

Would it be possible to be compatible with Linux? Their version has generic way of marking an interface a "child" interface (aka IFLA_PARENT_DEV_NAME attribute in RTM_NEWLINK in rtnetlink ) and an extensive set of bridge member attributes: https://github.com/torvalds/linux/blob/379f604cc3dc2c865dc2b13d81faa166b6df59ec/include/uapi/linux/if_link.h#L467 / https://github.com/CumulusNetworks/ifupdown2/blob/7034177eeb034a0b1ea9c6792cd3af59f1fc39d0/ifupdown2/addons/bridge.py#L462

[ivy]

i don't think we can be compatible with Linux due to semantic differences. to take two examples i noticed, their version of VLAN filtering says if it's disabled, the bridge won't process VLANs at all, but our bridge does (it just doesn't filter them), and their vlan protocol mode (802.1q/802.1ad) seems to be for the bridge as a whole, while ours is per-interface.

i think trying to match the Linux API might just end up being more confusing, since it'll appear to work but actually not work in subtle ways.

[adrian]

Would you mind considering whether to add version numbers to your netlink APIs before you start landing them?
I know right now it looks pretty simple because they're just strings, but once you start crafting more complicated things that aren't strings/integers...

[ivy]

In D51238#1170850, @adrian wrote:

Would you mind considering whether to add version numbers to your netlink APIs before you start landing them?
I know right now it looks pretty simple because they're just strings, but once you start crafting more complicated things that aren't strings/integers...

i assume you mean setting the third argument, family_version, when calling genl_register_family? i notice i've set this is 2 here which is probably wrong.

do we need to define a constant like BRIDGE_NL_VERSION_1 to use as the family version?

[ivy]

move ifunit_ref() to bridge_cmd_add()

make bridge_cmd_add return the new bridge_iflist since Netlink will need this,
but currently it's not used.

[ivy]

cache the bridge netlink socket across calls

do this in ifbridge rather than putting it in if_ctx since it's specific to
bridge.

it seems to me that every call to snl_init_writer() here will leak memory,
but i don't see any way to avoid this in the snl API.

[kp]

In D51238#1170996, @ivy wrote:

it seems to me that every call to snl_init_writer() here will leak memory,
but i don't see any way to avoid this in the snl API.

It's a little deceptive. snl_allocz() (as called in snl_init_writer()) doesn't actually allocate memory. It uses the buffer from the snl_state, so if we free the snl_state (snl_free()) we also free the writer's memory.
I've not gone looking where the appropriate place to do so would be in ifconfig, but in libifconfig it's fairly straightforward: we'd attach the snl_state to the ifconfig_handle_t, and free it in ifconfig_close().

Perhaps we could call bridge_init_netlink() in bridge_ctor(). There's no equivalent destructor, but it may be fine to not bother for ifconfig. That's always short-lived anyway, and memory leaks stop when the missile explo^W^W^W the process stops.

[melifaro]

In D51238#1170774, @ivy wrote:

i don't think we can be compatible with Linux due to semantic differences. to take two examples i noticed, their version of VLAN filtering says if it's disabled, the bridge won't process VLANs at all, but our bridge does (it just doesn't filter them), and their vlan protocol mode (802.1q/802.1ad) seems to be for the bridge as a whole, while ours is per-interface.

i think trying to match the Linux API might just end up being more confusing, since it'll appear to work but actually not work in subtle ways.

There are multiple "levels" of compatibility to consider. I'd start with general interface configuration. Bridge interface is not significantly different from any other interface - and I don't think we should be creating generic netlink interface for each network interface we convert. In that sense I'd really prefer we follow the current netlink model, where we have per-interface custom properties (see vlan(4) for partial implementation) in rtnetlink.

The semantics indeed differ. I don't think we need to be fully compatible - however, it's worth reusing the already existing bits. The overall goal for netlink is to make larger app infrastructure support FreeBSD easier. Having the similar model, but with some custom variables makes it really low-cost to support FreeBSD. Having totally incompatible interface won't be much different from existing ioctl mess.
So I'd suggest considering the approach where we support the basic config and put the rest to the custom FreeBSD-specific sub-attribute as it's done in the other places in FreeBSD rtnetlink.

[ivy]

in an attempt to manage the number of open bridge diffs, i'm going to close this one and open a new one once the Linux-compatible netlink interface is ready, which won't be until 16.0.

ty everyone for feedback.