nlas: Add type for AfSpecBridge which allows links to configure various bridge flags with SETLINK calls

[flouthoc]

Following PR allows handle.link().set( or RTM_SETLINK calls to configure various bridge attributes like.

• IFLA_BRIDGE_FLAGS
• IFLA_BRIDGE_VLAN_INFO

With this PR its easy to implement functionality like

bridge vlan add ...

This PR also corrects some changes made in
#212 and #213

[flouthoc]

@little-dude @cathay4t PTAL. Let me know if any example file is needed with this, however simple example like this works.

    /* bridge vlan add ... */
    let mut links = handle.link().get().match_name("my-bridge-1".to_string().clone()).execute();
    if let Some(link) = links.try_next().await? {

        let mut request = handle.link().set(link.header.index);
        request.message_mut().header.interface_family = AF_BRIDGE as u8;
        request.message_mut().header.flags = 0;
        let my_struct = BridgeVlanInfo { id: 6u16, data: 100u16};
        let bytes: &[u8] = unsafe { any_as_u8_slice(&my_struct) };

        let bridge_flags = AfSpecBridge::Flags(BRIDGE_FLAGS_SELF);
        let bridge_vlan_info = AfSpecBridge::VlanInfo(bytes.to_vec());
        request.message_mut().nlas.push(Nla::AfSpecBridge(vec![bridge_flags, bridge_vlan_info]));
        request.execute().await.map_err(|e| format!("{}", e));
    } else {
        println!("no link link found");
    }
    Ok(())

[flouthoc]

@cathay4t @little-dude Gentle reminder. Were you guys able to take a look at this. No rush.

[flouthoc]

Even if this is merged I would be not able to use quickly due to version dependency across all netlink crates will have to wait for release of netlink-packet-route and that has to be updated in rtnetlink master.

So no rush here.

[flouthoc]

Even if this is merged I would be not able to use quickly due to version dependency across all netlink crates will have to wait for release of netlink-packet-route and that has to be updated in rtnetlink master.

So no rush here.

Not related to this PR but we could avoid this drift it if all crates share same version and are released together. But it has its own problems. I'll open a separate GH issue to discuss this.

[little-dude]

Hey @flouthoc,

Just a heads up, I'm not ignoring you. I've had covid symptoms since last Friday. It's getting better but I'm not going to look at pending PRs for a couple more days.

[flouthoc]

Hey @flouthoc,

Just a heads up, I'm not ignoring you. I've had covid symptoms since last Friday. It's getting better but I'm not going to look at pending PRs for a couple more days.

Ah Please take care. This PR is not urgent we can do this later. Get well soon.

[little-dude]

Even if this is merged I would be not able to use quickly due to version dependency across all netlink crates will have to wait for release of netlink-packet-route and that has to be updated in rtnetlink master.

So no rush here.

I can make a release once this is merged. I'll try to make more smaller releases as we go. Big releases are a pain.

[little-dude]

@little-dude @cathay4t PTAL. Let me know if any example file is needed with this, however simple example like this works.

    /* bridge vlan add ... */
    let mut links = handle.link().get().match_name("my-bridge-1".to_string().clone()).execute();
    if let Some(link) = links.try_next().await? {

        let mut request = handle.link().set(link.header.index);
        request.message_mut().header.interface_family = AF_BRIDGE as u8;
        request.message_mut().header.flags = 0;
        let my_struct = BridgeVlanInfo { id: 6u16, data: 100u16};
        let bytes: &[u8] = unsafe { any_as_u8_slice(&my_struct) };

        let bridge_flags = AfSpecBridge::Flags(BRIDGE_FLAGS_SELF);
        let bridge_vlan_info = AfSpecBridge::VlanInfo(bytes.to_vec());
        request.message_mut().nlas.push(Nla::AfSpecBridge(vec![bridge_flags, bridge_vlan_info]));
        request.execute().await.map_err(|e| format!("{}", e));
    } else {
        println!("no link link found");
    }
    Ok(())

Probably no need for an example in netlink-packet-route but an integration with rtnetlink and an example there would be awesome :)

[flouthoc]

@little-dude thanks for the review. I'll fix nits and add example by this weekend.

[little-dude]

@flouthoc gentle ping :) Are you planning to finish this one off? Fwiw I'm up to date with releases so I can make one as soon as this is merged.

[flouthoc]

@little-dude Sorry I got stuck in few other things. But I need this PR so i'll get back to this, and would probably close this in this week. So if not in this release we can get this in by next release.

So please don't stop release because of this but I'll make sure that it will not miss this in another release.

[flouthoc]

@little-dude I didn't add example yet but other than that resolved all the nits you requested.

[stbuehler]

I'm a little worried that IFLA_AF_SPEC seems to be used with two different protocols.

• one is described in uapi/linux/if_bridge.h and contains the bridge values "flat".
• the second is described in uapi/linux/if_link.h and describes it as a map from address family (as "attribute type") to (nested) af specific data (for AF_BRIDGE probably the same as the first one, just nested one level deeper? not sure.)

I think the first one is only to be used if rtgen_family == AF_BRIDGE (I'm guessing this becomes interface_family in the parser), and the second one for everything else. The current implementation seems completely broken; especially emit would need to consider the current family too.

[flouthoc]

@stbuehler Yeah but issue is i'm not able to use AfSpecBridge at all, do you have any suggestions for emit block ?

[stbuehler]

Most attributes (and IFLA_AF_SPEC too) only can occur once, but the nested content forms a list/map again. So you probably should start with something like this:

pub enum AfSpecElem {
    AfSpecIpv4(AfSpecInet),
    AfSpecIpv6(AfSpecInet),
    AfSpecBridge(AfSpecBridge),
    AfSpecUnknown {
        family: u8,
        data: Vec<u8>,
    }
}

/// rtnl::link::nlas::Nla
pub enum Nla {
    ...
    AfSpec(Vec<AfSpecElem>),
}

(I'm not a fan of these Vec<SomeNla> things and would rather use something like this:

pub struct AfSpec {
    ipv4: Option<AfSpecInet>,
    ipv6: Option<AfSpecInet>,
    bridge: Option<AfSpecBridge>,
    unknown: Vec<(u8, Vec<u8>)>,
}

/// rtnl::link::nlas::Nla
pub enum Nla {
    ...
    AfSpec(AfSpec),
}

But that is not how it is done in this library, and more work to implement the encode/decode things for.)

When parsing you could now map the rtgen_family == AF_BRIDGE case by parsing an inner AfSpecBridge and just put it into the nested data as it would have been an inner nested entry. Then you'll have to undo that on emit by somehow passing the rtgen_family - the code for emitting nested Nlas is not that complex, so you should be able to duplicate it with local functions (instead of traits, or build a generic trait variant similar to the parser case).

Or you could add a different Nla variant for the bridge family:

/// rtnl::link::nlas::Nla
pub enum Nla {
    ...
    // only with `rtgen_family != AF_BRIDGE`
    AfSpec(Vec<AfSpecElem>),
    // only with `rtgen_family == AF_BRIDGE`
    AfSpecBridge(AfSpecBridge),
}

On parsing you should have the family as context to distinguish the two cases, and on emit it would be the users responsibility to choose the correct one.

(Note that I'm still not sure whether the contained AfSpecBridge data is actually the same in both cases; you should probably check the kernel for that.)

[stbuehler]

Hm. It seems I didn't look closely enough at AfSpecInet. It actually already has the nested parts for the inner mapping (although it is quite generic for lots of families, and only three are actually implemented in the kernel afaict: AF_INET, AF_INET6, AF_MCTP. AF_BRIDGE isn't used here.). I think the name is a little bit misleading though :)

This AfSpecInet format is (again afaict) used for messages for all families but AF_BRIDGE; in this case the bridge specific format is used.

So this does look alright after all.

[flouthoc]

Hm. It seems I didn't look closely enough at AfSpecInet. It actually already has the nested parts for the inner mapping (although it is quite generic for lots of families, and only three are actually implemented in the kernel afaict: AF_INET, AF_INET6, AF_MCTP. AF_BRIDGE isn't used here.). I think the name is a little bit misleading though :)

This AfSpecInet format is (again afaict) used for messages for all families but AF_BRIDGE; in this case the bridge specific format is used.

So this does look alright after all.

@stbuehler Thank you so much for spending time on this and confirming. I really appreciate it 😄 . Just waiting for reviewers to take final look @cathay4t @little-dude PTAL

[cathay4t]

I need more time on kernel code reading to decide whether current code is correct or not.

[cathay4t]

To reply #222 (comment)

The kernel code net/core/rtnetlink.c function rtnl_fill_link_af explained IFLA_AF_SPEC is two layer nested netlink message, like:

[ AF_BRIDGE: [ AfSpecBridge::VlanInfo],
  AF_INET: [ AfSpecInet::IpmrTableId] ]

This is the snippet of kernel code, in case you would like to read C:

	af_spec = nla_nest_start_noflag(skb, IFLA_AF_SPEC);
	if (!af_spec)
		return -EMSGSIZE;

	list_for_each_entry_rcu(af_ops, &rtnl_af_ops, list) {
		struct nlattr *af;
		int err;

		if (!af_ops->fill_link_af)
			continue;

		af = nla_nest_start_noflag(skb, af_ops->family);

Just some notes on kernel code reading.

[cathay4t]

This is the partial output of sudo strace -s 1024 ip netconf show dev wifi0, indicating the IFLA_AF_SPEC is two layers nested NLA.

But, apparently , this is too complex for this PR to fix. I intend to merge this PR and continue the fix in new PR for this IFLA_AF_SPEC stuff and also finish off the VLAN filtering of bridge.

[{nla_len=408, nla_type=IFLA_AF_SPEC},
    [
        [{nla_len=140, nla_type=AF_INET}, [
            {nla_len=136, nla_type=IFLA_INET_CONF}, [
                [IPV4_DEVCONF_FORWARDING-1] = 0,
                <omitted>
                [IPV4_DEVCONF_ARP_EVICT_NOCARRIER-1] = 1]]],
        [{nla_len=264, nla_type=AF_INET6}, [
            [{nla_len=8, nla_type=IFLA_INET6_FLAGS}, IF_READY],
            [{nla_len=20, nla_type=IFLA_INET6_CACHEINFO},
                {
                    max_reasm_len=65535,
                    tstamp=3794,
                    reachable_time=37584,
                    retrans_time=1000}],
            [{nla_len=232, nla_type=IFLA_INET6_CONF},
                [[DEVCONF_FORWARDING] = 0,
                <omitted>
                [DEVCONF_NDISC_EVICT_NOCARRIER] = 1]]]]]]

[cathay4t]

Looks good to me. Will continue the work in another PR.

[flouthoc]

Thanks @cathay4t for the review and findings, let me know if there is anything i can help with.

[cathay4t]

Hi @flouthoc , we can merge this once we fix https://github.com/little-dude/netlink/pull/222/files#r858245460

[cathay4t]

Conclusion on this IFLA_AF_SPEC:

• For PF_UNSPEC/AF_UNSPEC, the IFLA_AF_SPEC is two layer nested NLA as mentioned nlas: Add type for AfSpecBridge which allows links to configure various bridge flags with SETLINK calls #222 (comment) , The br_af_ops does not register the fill_link_af function, hence bridge interface will not append vlan filtering information in this query or dump.
• For PF_BRIDGE, the IFLA_AF_SPEC is single layer nested NLA for IFLA_BRIDGE_VLAN_INFO, IFLA_BRIDGE_MRP and etc. And this is only available for NLM_F_DUMP.

For kernel code reference: rtnetlink_init() of /net/core/rtnetlink.c has:

	rtnl_register(PF_UNSPEC, RTM_GETLINK, rtnl_getlink,
		      rtnl_dump_ifinfo, 0);
        // omitted
	rtnl_register(PF_BRIDGE, RTM_GETLINK, NULL, rtnl_bridge_getlink, 0);

[cathay4t]

I have amended the incorrect error message mentioned in #222 (comment) and merged. Thanks for the contribution!

[flouthoc]

@cathay4t My bad sorry for the delay and thanks for merging this :)