diff --git a/src/mctpd.c b/src/mctpd.c index a1d2728..93e7c30 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -459,6 +459,22 @@ static struct peer *find_peer_by_addr(struct ctx *ctx, mctp_eid_t eid, return NULL; } +static struct peer *find_bridge_for_peer(struct ctx *ctx, + const struct peer *peer) +{ + for (size_t i = 0; i < ctx->num_peers; i++) { + struct peer *p = ctx->peers[i]; + if (p->pool_size == 0) + continue; + if (p->net != peer->net) + continue; + if (peer->eid >= p->pool_start && + peer->eid < (mctp_eid_t)(p->pool_start + p->pool_size)) + return p; + } + return NULL; +} + static int find_local_eids_by_net(struct net *net, size_t *local_eid_cnt, mctp_eid_t *ret_eids) { @@ -2948,7 +2964,7 @@ static int query_get_peer_uuid_by_phys(struct ctx *ctx, const dest_phys *dest, return rc; } -static int query_get_peer_uuid(struct peer *peer) +static int query_get_peer_uuid(struct peer *peer, uint8_t uuid[16]) { struct mctp_ctrl_resp_get_uuid *resp = NULL; struct mctp_ctrl_cmd_get_uuid req; @@ -2979,10 +2995,7 @@ static int query_get_peer_uuid(struct peer *peer) goto out; resp = cmd.resp; - rc = peer_set_uuid(peer, resp->uuid); - if (rc < 0) - goto out; - rc = 0; + memcpy(uuid, resp->uuid, 16); out: mctp_ctrl_cmd_free(&cmd); @@ -3452,10 +3465,19 @@ static int query_peer_properties(struct peer *peer) } } - rc = query_get_peer_uuid(peer); - if (rc < 0 && peer->ctx->verbose) { - errno = -rc; - warn("Error getting UUID for %s", peer_tostr(peer)); + { + uint8_t uuid[16]; + memset(uuid, 0, sizeof(uuid)); + rc = query_get_peer_uuid(peer, uuid); + if (rc < 0) { + if (peer->ctx->verbose) { + errno = -rc; + warn("Error getting UUID for %s", + peer_tostr(peer)); + } + } else { + rc = peer_set_uuid(peer, uuid); + } } // TODO: emit property changed? Though currently they are all const. @@ -3747,8 +3769,13 @@ static int peer_endpoint_recover(sd_event_source *s, uint64_t usec, * Test if we still have connectivity to the endpoint. If we do, we will get a * response reporting the current EID. This is the test recommended by 8.17.6 * of DSP0236 v1.3.1. + + * Determine whether this is a downstream (bridged) endpoint before + * probing, so we can check bridge's connectivity first. */ - rc = query_get_endpoint_id(ctx, &peer->phys, &peer->recovery.eid, + struct peer *bridge = find_bridge_for_peer(ctx, peer); + rc = query_get_endpoint_id(ctx, bridge ? &bridge->phys : &peer->phys, + &peer->recovery.eid, &peer->recovery.endpoint_type, &peer->recovery.medium_spec, /*peer=*/NULL, /*retry=*/false); @@ -3757,26 +3784,53 @@ static int peer_endpoint_recover(sd_event_source *s, uint64_t usec, } /* - * If we've got a response there are two scenarios: - * - * 1. The device responds with the EID that we expect it to have - * 2. The device responds with an unexpected EID, e.g. 0 - * - * For scenario 1 we're done as the device is responsive and has the expected - * address. For scenario 2, we may not yet consider the EID assignment as - * expired, so check the UUID for a match. If the UUID matches we reassign the - * expected EID to the device. If the UUID does not match we allocate a new - * EID for the exchanged device, given it is responsive. - */ - if (peer->recovery.eid != peer->eid) { + * Directly-connected endpoint: If we've got a response there + * are two scenarios: + * + * 1. The device responds with the EID that we expect it to have + * 2. The device responds with an unexpected EID, e.g. 0 + * + * For scenario 1 we're done as the device is responsive and has + * the expected address. For scenario 2, we may not yet consider + * the EID assignment as expired, so check the UUID for a match. + * If the UUID matches we reassign the expected EID to the + * device. If the UUID does not match we allocate a new EID for + * the exchanged device, given it is responsive. + * + * Bridged endpoint: If bridge connectivity is intact and we get a response, + * from downstream device, we can confirm the EID assignment is still valid + * since EID for downstreams endpoints are managed by birdge itself. + * If UUID doesn't match, bridge has re-assigned the EID to the new device, + * we discover the new device while recovering previously assigned EID. + */ + + if (bridge) { static const uint8_t nil_uuid[16] = { 0 }; bool uuid_matches_peer = false; bool uuid_matches_nil = false; uint8_t uuid[16] = { 0 }; - mctp_eid_t new_eid; - rc = query_get_peer_uuid_by_phys(ctx, &peer->phys, uuid); - if (!rc && peer->uuid) { + /* + * Re-probe via EID routing so recovery.eid reflects the + * downstream device's response, not the bridge's EID. + */ + rc = query_get_endpoint_id(ctx, &peer->phys, + &peer->recovery.eid, + &peer->recovery.endpoint_type, + &peer->recovery.medium_spec, peer, + /*retry=*/false); + if (rc < 0) + goto reschedule; + + /* + * UUID via EID routing to identify the downstream device + * specifically, not the bridge. + */ + rc = query_get_peer_uuid(peer, uuid); + if (rc < 0) + goto reclaim; + + if (peer->uuid) { static_assert(sizeof(uuid) == sizeof(nil_uuid), "Unsynchronized UUID sizes"); uuid_matches_peer = @@ -3785,33 +3839,78 @@ static int peer_endpoint_recover(sd_event_source *s, uint64_t usec, memcmp(uuid, nil_uuid, sizeof(uuid)) == 0; } - if (rc || !uuid_matches_peer || + if (!uuid_matches_peer || (uuid_matches_nil && !MCTPD_RECOVER_NIL_UUID)) { - /* It's not known to be the same device, allocate a new EID */ + /* + * The bridge has re-assigned this EID to a new device. + * Remove the old peer and register the new one at the + * same EID. + */ dest_phys phys = peer->phys; + mctp_eid_t eid = peer->eid; + uint32_t net = peer->net; + struct peer *new_peer; assert(sd_event_source_get_enabled( peer->recovery.source, NULL) == 0); remove_peer(peer); - /* - * The representation of the old peer is now gone. Set up the new peer, - * after which we immediately return as there's no old peer state left to - * maintain. - */ - return endpoint_assign_eid(ctx, NULL, &phys, &peer, 0, - false); + rc = add_peer(ctx, &phys, eid, net, &new_peer, + /*allow_bridged=*/true); + if (rc < 0) + return rc; + return setup_added_peer(new_peer); } - /* Confirmation of the same device, apply its already allocated EID */ - rc = endpoint_send_set_endpoint_id(peer, &new_eid, NULL); - if (rc < 0) { - goto reschedule; - } + } else { + if (peer->recovery.eid != peer->eid) { + static const uint8_t nil_uuid[16] = { 0 }; + bool uuid_matches_peer = false; + bool uuid_matches_nil = false; + uint8_t uuid[16] = { 0 }; + mctp_eid_t new_eid; + + rc = query_get_peer_uuid_by_phys(ctx, &peer->phys, + uuid); + if (!rc && peer->uuid) { + static_assert(sizeof(uuid) == sizeof(nil_uuid), + "Unsynchronized UUID sizes"); + uuid_matches_peer = memcmp(uuid, peer->uuid, + sizeof(uuid)) == 0; + uuid_matches_nil = memcmp(uuid, nil_uuid, + sizeof(uuid)) == 0; + } + + if (rc || !uuid_matches_peer || + (uuid_matches_nil && !MCTPD_RECOVER_NIL_UUID)) { + /* It's not known to be the same device, allocate a new EID */ + dest_phys phys = peer->phys; - if (new_eid != peer->eid) { - rc = change_peer_eid(peer, new_eid); + assert(sd_event_source_get_enabled( + peer->recovery.source, NULL) == + 0); + remove_peer(peer); + /* + * The representation of the old peer is now + * gone. Set up the new peer, after which we + * immediately return as there's no old peer + * state left to maintain. + */ + return endpoint_assign_eid(ctx, NULL, &phys, + &peer, 0, false); + } + + /* Confirmation of the same device, apply its already allocated EID */ + rc = endpoint_send_set_endpoint_id(peer, &new_eid, + NULL); if (rc < 0) { - goto reclaim; + goto reschedule; + } + + if (new_eid != peer->eid) { + rc = change_peer_eid(peer, new_eid); + if (rc < 0) { + goto reclaim; + } } } } diff --git a/tests/test_mctpd.py b/tests/test_mctpd.py index 7853794..56af87b 100644 --- a/tests/test_mctpd.py +++ b/tests/test_mctpd.py @@ -29,6 +29,7 @@ MCTPD_MCTP_I = 'au.com.codeconstruct.MCTP.BusOwner1' MCTPD_ENDPOINT_I = 'au.com.codeconstruct.MCTP.Endpoint1' MCTPD_ENDPOINT_BRIDGE_I = 'au.com.codeconstruct.MCTP.Bridge1' +OPENBMC_UUID_I = 'xyz.openbmc_project.Common.UUID' DBUS_OBJECT_MANAGER_I = 'org.freedesktop.DBus.ObjectManager' DBUS_PROPERTIES_I = 'org.freedesktop.DBus.Properties' @@ -2339,3 +2340,296 @@ async def test_iface_config_match_path_none(dbus, sysnet, nursery): res = await mctpd.stop_mctpd() assert res == 0 + + +async def _setup_bridge_with_downstream( + dbus, sysnet, nursery, num_downstream=1 +): + """Start mctpd, assign a bridge endpoint, wait for downstream discovery. + + Returns (mctpd, bridge_ep, downstream_eps, downstream_paths). + """ + _DOWNSTREAM_POLL_MS = 2500 + config = f""" + [bus-owner] + endpoint_poll_ms = {_DOWNSTREAM_POLL_MS} + """ + mctpd = MctpdWrapper(dbus, sysnet, config=config) + await mctpd.start_mctpd(nursery) + + iface = mctpd.system.interfaces[0] + bridge_ep = mctpd.network.endpoints[0] + mctp = await mctpd_mctp_iface_obj(dbus, iface) + + downstream_eps = [] + for _ in range(num_downstream): + bep = Endpoint(iface, bytes(), types=[0]) + mctpd.network.add_endpoint(bep) + bridge_ep.add_bridged_ep(bep) + downstream_eps.append(bep) + + # Subscribe to InterfacesAdded AFTER assign_endpoint returns so we don't + # accidentally match the bridge's own Endpoint1 signal (emitted before its + # separate Bridge1 signal, so it would pass the downstream filter). + (_, _, _, new) = await mctp.call_assign_endpoint(bridge_ep.lladdr) + assert new + + mctp_obj = await dbus.get_proxy_object(MCTPD_C, MCTPD_MCTP_P) + mctp_objmgr = await mctp_obj.get_interface(DBUS_OBJECT_MANAGER_I) + + downstream_paths = [] + discovered = trio.Semaphore(initial_value=0) + + def ep_added(ep_path, content): + if ( + MCTPD_ENDPOINT_I in content + and MCTPD_ENDPOINT_BRIDGE_I not in content + ): + downstream_paths.append(ep_path) + discovered.release() + + await mctp_objmgr.on_interfaces_added(ep_added) + + with trio.move_on_after(_DOWNSTREAM_POLL_MS / 1000 * 3) as scope: + for _ in range(num_downstream): + await discovered.acquire() + + assert not scope.cancelled_caught, ( + "Timed out waiting for downstream EP discovery" + ) + + return mctpd, bridge_ep, downstream_eps, downstream_paths + + +async def test_recover_downstream_present( + dbus, sysnet, nursery, autojump_clock +): + """Downstream peer still alive: recovery transitions Connectivity to Available. + + Physical probe targets the bridge (Step 1), EID-routed probe reaches the + downstream device (Step 2), UUID via EID routing matches stored UUID (Step 3). + """ + _DOWNSTREAM_TRECLAIM = 5 + ( + mctpd, + bridge_ep, + downstream_eps, + downstream_paths, + ) = await _setup_bridge_with_downstream(dbus, sysnet, nursery) + + ds_path = downstream_paths[0] + ep_obj = await dbus.get_proxy_object(MCTPD_C, ds_path) + ep_props = await ep_obj.get_interface(DBUS_PROPERTIES_I) + ep_cc = await ep_obj.get_interface(MCTPD_ENDPOINT_I) + + recovered = trio.Semaphore(initial_value=0) + + def on_connectivity_changed(iface, changed, _invalidated): + if iface == MCTPD_ENDPOINT_I and 'Connectivity' in changed: + if changed['Connectivity'].value == 'Available': + recovered.release() + + await ep_props.on_properties_changed(on_connectivity_changed) + await ep_cc.call_recover() + + with trio.move_on_after(4 * _DOWNSTREAM_TRECLAIM) as scope: + await recovered.acquire() + + assert not scope.cancelled_caught, ( + "Downstream peer did not recover to Available" + ) + + res = await mctpd.stop_mctpd() + assert res == 0 + + +async def test_recover_downstream_removed( + dbus, sysnet, nursery, autojump_clock +): + """Downstream peer gone while bridge is up: peer is removed after all retries. + + Physical probe reaches the bridge (Step 1); EID-routed probe times out + repeatedly (Step 2 fails), so the peer is removed. + """ + _DOWNSTREAM_TRECLAIM = 5 + ( + mctpd, + bridge_ep, + downstream_eps, + downstream_paths, + ) = await _setup_bridge_with_downstream(dbus, sysnet, nursery) + + ds_path = downstream_paths[0] + ep_obj = await dbus.get_proxy_object(MCTPD_C, ds_path) + ep_props = await ep_obj.get_interface(DBUS_PROPERTIES_I) + ep_cc = await ep_obj.get_interface(MCTPD_ENDPOINT_I) + + degraded = trio.Semaphore(initial_value=0) + removed = trio.Semaphore(initial_value=0) + + def on_connectivity_changed(iface, changed, _invalidated): + if iface == MCTPD_ENDPOINT_I and 'Connectivity' in changed: + if changed['Connectivity'].value == 'Degraded': + degraded.release() + + await ep_props.on_properties_changed(on_connectivity_changed) + + mctp_obj = await dbus.get_proxy_object(MCTPD_C, MCTPD_MCTP_P) + mctp_objmgr = await mctp_obj.get_interface(DBUS_OBJECT_MANAGER_I) + + def on_ep_removed(ep_path, interfaces): + if ep_path == ds_path and MCTPD_ENDPOINT_I in interfaces: + removed.release() + + await mctp_objmgr.on_interfaces_removed(on_ep_removed) + + # Disconnect downstream — bridge mock stops routing EID-addressed packets to it. + bridge_ep.bridged_eps.remove(downstream_eps[0]) + + await ep_cc.call_recover() + + with trio.move_on_after(4 * _DOWNSTREAM_TRECLAIM) as scope: + await removed.acquire() + await degraded.acquire() + + assert not scope.cancelled_caught, ( + "Downstream peer was not removed after losing connectivity" + ) + + res = await mctpd.stop_mctpd() + assert res == 0 + + +async def test_recover_downstream_bridge_unreachable( + dbus, sysnet, nursery, autojump_clock +): + """Bridge unreachable: downstream peer is removed when physical probe fails. + + Removing the bridge from the network causes Step 1 to time out on every + attempt; the downstream peer is removed without reaching Step 2 or 3. + """ + _DOWNSTREAM_TRECLAIM = 5 + ( + mctpd, + bridge_ep, + downstream_eps, + downstream_paths, + ) = await _setup_bridge_with_downstream(dbus, sysnet, nursery) + + ds_path = downstream_paths[0] + ep_obj = await dbus.get_proxy_object(MCTPD_C, ds_path) + ep_props = await ep_obj.get_interface(DBUS_PROPERTIES_I) + ep_cc = await ep_obj.get_interface(MCTPD_ENDPOINT_I) + + degraded = trio.Semaphore(initial_value=0) + removed = trio.Semaphore(initial_value=0) + + def on_connectivity_changed(iface, changed, _invalidated): + if iface == MCTPD_ENDPOINT_I and 'Connectivity' in changed: + if changed['Connectivity'].value == 'Degraded': + degraded.release() + + await ep_props.on_properties_changed(on_connectivity_changed) + + mctp_obj = await dbus.get_proxy_object(MCTPD_C, MCTPD_MCTP_P) + mctp_objmgr = await mctp_obj.get_interface(DBUS_OBJECT_MANAGER_I) + + def on_ep_removed(ep_path, interfaces): + if ep_path == ds_path and MCTPD_ENDPOINT_I in interfaces: + removed.release() + + await mctp_objmgr.on_interfaces_removed(on_ep_removed) + + mctpd.network.endpoints.remove(bridge_ep) + + await ep_cc.call_recover() + + with trio.move_on_after(4 * _DOWNSTREAM_TRECLAIM) as scope: + await removed.acquire() + await degraded.acquire() + + assert not scope.cancelled_caught, ( + "Downstream peer was not removed after bridge became unreachable" + ) + + res = await mctpd.stop_mctpd() + assert res == 0 + + +async def test_recover_downstream_exchange( + dbus, sysnet, nursery, autojump_clock +): + """Bridge reassigns pool EID to a new device: old peer removed, new peer added. + + UUID mismatch detected via EID routing (Step 3) triggers InterfacesRemoved + for the old peer and InterfacesAdded for the replacement. + """ + _DOWNSTREAM_TRECLAIM = 5 + ( + mctpd, + bridge_ep, + downstream_eps, + downstream_paths, + ) = await _setup_bridge_with_downstream(dbus, sysnet, nursery) + + ds_path = downstream_paths[0] + ep_obj = await dbus.get_proxy_object(MCTPD_C, ds_path) + ep_props = await ep_obj.get_interface(DBUS_PROPERTIES_I) + ep_cc = await ep_obj.get_interface(MCTPD_ENDPOINT_I) + + degraded = trio.Semaphore(initial_value=0) + removed = trio.Semaphore(initial_value=0) + added = trio.Semaphore(initial_value=0) + added_uuid = [] + + def on_connectivity_changed(iface, changed, _invalidated): + if iface == MCTPD_ENDPOINT_I and 'Connectivity' in changed: + if changed['Connectivity'].value == 'Degraded': + degraded.release() + + await ep_props.on_properties_changed(on_connectivity_changed) + + mctp_obj = await dbus.get_proxy_object(MCTPD_C, MCTPD_MCTP_P) + mctp_objmgr = await mctp_obj.get_interface(DBUS_OBJECT_MANAGER_I) + + def on_ep_removed(ep_path, interfaces): + if ep_path == ds_path and MCTPD_ENDPOINT_I in interfaces: + removed.release() + + def on_ep_added(ep_path, content): + if ep_path == ds_path and MCTPD_ENDPOINT_I in content: + if OPENBMC_UUID_I in content and 'UUID' in content[OPENBMC_UUID_I]: + added_uuid.append(content[OPENBMC_UUID_I]['UUID'].value) + added.release() + + await mctp_objmgr.on_interfaces_removed(on_ep_removed) + await mctp_objmgr.on_interfaces_added(on_ep_added) + + old_ds = downstream_eps[0] + new_ds = Endpoint(old_ds.iface, bytes(), types=[0]) + new_ds.eid = old_ds.eid # same pool EID, fresh UUID + + bridge_ep.bridged_eps.remove(old_ds) + bridge_ep.bridged_eps.append(new_ds) + mctpd.network.add_endpoint(new_ds) + + await ep_cc.call_recover() + + with trio.move_on_after(4 * _DOWNSTREAM_TRECLAIM) as scope: + await removed.acquire() + await added.acquire() + await degraded.acquire() + + assert not scope.cancelled_caught, ( + "Downstream peer exchange did not emit Remove + Add" + ) + + # Verify the re-added peer carries the new device's UUID, not the old one. + assert added_uuid, "InterfacesAdded did not include a UUID property" + assert added_uuid[0] == str(new_ds.uuid), ( + f"New peer UUID {added_uuid[0]!r} does not match " + f"replacement device UUID {new_ds.uuid!r}" + ) + + res = await mctpd.stop_mctpd() + assert res == 0