From dc818988dad1c7b42f6ea806f43414497e9c2ddb Mon Sep 17 00:00:00 2001 From: "viktor.iarmola" Date: Wed, 27 Nov 2024 00:05:15 +0200 Subject: [PATCH] Support update operation for link objects When bridge interfaces are in vlan-aware mode, one of the rtnetlink updates they produce was not handled correctly. When the set of vlans active on a vlan-aware bridge itself it being changed it produces RTM_NEWLINK message with AF_BRIDGE family (e.g. with `bridge vlan add dev br0 vid 3 self`). Unlike other messages arriving with AF_BRIDGE family - this one doesn't refer to one of the member interfaces, but to the bridge itself as if it was its own member. This message doesn't contain full information about bridge - mostly about vlans in IFLA_AF_SPEC attribute. It does, however, refer to the bridge interface itself via ifi_index parameter. `libnl` parses such message into a link object with (AF_BRIDGE, ) key. At the same time, the regular rtnetlink message describing bridge interface itself (one that arrives with AF_UNSPEC family and IFLA_LINKINFO/IFLA_INFO_KIND == bridge) will also be parsed into link object with the same key (AF_BRIDGE, ). That is caused by the libnl classifying it internally as bridge (via IFLA_INFO_KIND) and overriding its family to AF_BRIDGE. Such collision results in an unexpected behavior when we have a cache with the information about bridge (from AF_UNSPEC message) and then receive notification produced by `bridge vlan add/del ... self`. As both link entries have the same key - new one will override the old - and we will have a misleading entry in cache, which doesn't contain most of the information we've had about the bridge. It also defies some established expectations about bridge interfaces in libnl. E.g. `rtnl_link_is_bridge` is 1 for such entries, but `rtnl_link_get_type` is NULL. This situation can be amended by implementing update mechanism for link objects. With that, it is possible to properly handle such special rtnetlink messages and merge information available in them into full bridge description. --- lib/route/link.c | 15 +++++++++++++++ lib/route/link/bridge.c | 24 ++++++++++++++++++++++++ lib/route/link/link-api.h | 12 ++++++++++++ 3 files changed, 51 insertions(+) diff --git a/lib/route/link.c b/lib/route/link.c index 9b72574ad..d5fcdb0c6 100644 --- a/lib/route/link.c +++ b/lib/route/link.c @@ -1126,6 +1126,20 @@ static void link_keygen(struct nl_object *obj, uint32_t *hashkey, return; } +static int link_update(struct nl_object *old_obj, struct nl_object *new_obj) +{ + struct rtnl_link *src = (struct rtnl_link *) new_obj; + struct rtnl_link *dst = (struct rtnl_link *) old_obj; + + if (!dst->l_af_ops) + return -NLE_OPNOTSUPP; + + if (dst->l_af_ops->ao_update) + return dst->l_af_ops->ao_update(dst, src); + + return -NLE_OPNOTSUPP; +} + static uint64_t link_compare(struct nl_object *_a, struct nl_object *_b, uint64_t attrs, int flags) { @@ -3215,6 +3229,7 @@ static struct nl_object_ops link_obj_ops = { }, .oo_compare = link_compare, .oo_keygen = link_keygen, + .oo_update = link_update, .oo_attrs2str = link_attrs2str, .oo_id_attrs = LINK_ATTR_IFINDEX | LINK_ATTR_FAMILY, }; diff --git a/lib/route/link/bridge.c b/lib/route/link/bridge.c index 2df8d85e8..9d629b582 100644 --- a/lib/route/link/bridge.c +++ b/lib/route/link/bridge.c @@ -973,6 +973,29 @@ static int bridge_compare(struct rtnl_link *_a, struct rtnl_link *_b, } /** @endcond */ +static int bridge_update(struct rtnl_link *old_obj, struct rtnl_link *new_obj) +{ + struct bridge_data *dst = bridge_data(old_obj); + struct bridge_data *src = bridge_data(new_obj); + + /* + * Update only when we're dealing with a case of bridge + * notification on changes to itself as a bridge port. + * E.g. after `bridge vlan add dev br0 vid 3 self` + * Such notifications don't have full bridge details + * and thus have to be merged with parent rather than replace it. + * Reliable criterion to detect the case we're interested in + * is to look if there is IFLA_MASTER attr equal to ifi_index + * in the same message. + */ + if (new_obj->l_index != new_obj->l_master) + return -NLE_OPNOTSUPP; + + *dst = *src; + + return NLE_SUCCESS; +} + /** * Allocate link object of type bridge * @@ -1851,6 +1874,7 @@ static struct rtnl_link_af_ops bridge_ops = { .ao_parse_protinfo = &bridge_parse_protinfo, .ao_dump[NL_DUMP_DETAILS] = &bridge_dump_details, .ao_compare = &bridge_compare, + .ao_update = &bridge_update, .ao_parse_af_full = &bridge_parse_af_full, .ao_get_af = &bridge_get_af, .ao_fill_af = &bridge_fill_af, diff --git a/lib/route/link/link-api.h b/lib/route/link/link-api.h index 0e54057d2..746ee024a 100644 --- a/lib/route/link/link-api.h +++ b/lib/route/link/link-api.h @@ -141,6 +141,18 @@ struct rtnl_link_af_ops int (*ao_compare)(struct rtnl_link *, struct rtnl_link *, int, uint32_t, int); + /** + * Update function + * + * Will be called when af data of the link object given by first argument + * needs to be updated with the af data of the second supplied link object + * + * The function must return 0 for success and error for failure + * to update. In case of failure its assumed that the original + * object is not touched + */ + int (*ao_update)(struct rtnl_link *, struct rtnl_link *); + /* RTM_NEWLINK override * * Called if a change link request is set to the kernel. If this returns