Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

route/link: support update operation for link objects #414

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

viktor-iarmola
Copy link

@viktor-iarmola viktor-iarmola commented Nov 26, 2024

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, br-ifindex) 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, br-ifindex). 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.

Addresses #377

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, <br-ifindex>) 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, <br-ifindex>).
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.
@viktor-iarmola viktor-iarmola changed the title Support update operation for link objects route/link: support update operation for link objects Nov 26, 2024
@viktor-iarmola
Copy link
Author

I must add that current update implementation handles rather narrow edge-case and doesn't cover full scope of things that one could expect from update implementation - e.g. we lose bridge vlan data in cache when new AF_UNSPEC message with full bridge info arrives.

@thom311
Copy link
Owner

thom311 commented Nov 27, 2024

It seems hard to understand all the consequence of this patch. But the reasoning and the patch looks good to me, so unless somebody objects, I'll take it... (let's wait a bit).

if (dst->l_af_ops->ao_update)
return dst->l_af_ops->ao_update(dst, src);

return -NLE_OPNOTSUPP;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's return early, so we handle failure (OPNOTSUPP) cases first. Also, add a code comment like:

        struct rtnl_link *src = (struct rtnl_link *) new_obj;
        struct rtnl_link *dst = (struct rtnl_link *) old_obj;
        int r;

        if (!dst->l_af_ops)
		return -NLE_OPNOTSUPP;

	if (!dst->l_af_ops->ao_update)
		return -NLE_OPNOTSUPP;

        r = dst->l_af_ops->ao_update(dst, src);
        if (r < 0)
            return r;

        /* dst was already modified. We can no longer fail at this point. */

	return 0;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants