From 850e103c9bcb3ac570269f20533431567bbb6a56 Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Tue, 3 Jun 2025 15:09:23 +0700 Subject: [PATCH 1/5] tests: handle RTM_NEWADDR and RTM_DELADDR Currently, when mctpd adds new addresses to the routing table, unit tests will not receive it. This adds the missing test infrastructure to handle new addresses added to the routing table. Signed-off-by: Khang D Nguyen --- tests/mctpd/__init__.py | 43 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/mctpd/__init__.py b/tests/mctpd/__init__.py index 67469cc..54955c8 100644 --- a/tests/mctpd/__init__.py +++ b/tests/mctpd/__init__.py @@ -670,6 +670,10 @@ async def handle_send(self, addr, data): await self._handle_getlink(msg) elif typ == rtnl.RTM_GETADDR: await self._handle_getaddr(msg) + elif typ == rtnl.RTM_NEWADDR: + await self._handle_newaddr(msg) + elif typ == rtnl.RTM_DELADDR: + await self._handle_deladdr(msg) elif typ == rtnl.RTM_GETROUTE: await self._handle_getroute(msg) @@ -783,6 +787,45 @@ async def _handle_getaddr(self, msg): await self._send_msg(buf) + async def _handle_newaddr(self, msg): + # reparse as ifaddrmsg + msg = ifaddrmsg_mctp(msg.data) + msg.decode() + + ifindex = msg["index"] + eid = msg.get_attr("IFA_LOCAL") + + iface = self.system.find_interface_by_ifindex(ifindex) + address = System.Address(iface, eid) + try: + await self.system.add_address(address) + except NetlinkError as nle: + msg = nle.to_nlmsg() + msg.encode() + await self._send_msg(msg.data) + return + if msg['header']['flags'] & netlink.NLM_F_ACK: + await self._nlmsg_ack(msg) + + async def _handle_deladdr(self, msg): + msg = ifaddrmsg_mctp(msg.data) + msg.decode() + + ifindex = msg["index"] + eid = msg.get_attr("IFA_LOCAL") + + iface = self.system.find_interface_by_ifindex(ifindex) + addr = System.Address(iface, eid) + try: + await self.system.del_address(addr) + except NetlinkError as nle: + msg = nle.to_nlmsg() + msg.encode() + await self._send_msg(msg.data) + return + if msg["header"]["flags"] & netlink.NLM_F_ACK: + await self._nlmsg_ack(msg) + async def _notify_addr(self, addr, typ): msg = self._create_msg(ifaddrmsg_mctp, typ, 0) self._format_addr(msg, addr) From a3297d9de9c2f322263b7a20a98f63a26578a863 Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Wed, 4 Jun 2025 15:53:09 +0700 Subject: [PATCH 2/5] tests: deallocate iid on response This adds missing code to deallocate an iid after being used for an response. Signed-off-by: Khang D Nguyen --- tests/mctpd/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mctpd/__init__.py b/tests/mctpd/__init__.py index 54955c8..3e1da70 100644 --- a/tests/mctpd/__init__.py +++ b/tests/mctpd/__init__.py @@ -303,7 +303,7 @@ async def handle_mctp_control(self, sock, addr, data): iid = flags & 0x1f if not rq: - cmd = self.commands.get((0, iid), None) + cmd = self.commands.pop((0, iid), None) assert cmd is not None, "unexpected response?" await cmd.complete(data) From 31832d29d8e7e28c4b04d620107b8273b9cd1532 Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Mon, 24 Mar 2025 02:58:13 +0000 Subject: [PATCH 3/5] mctp-netlink: use ifindex instead of name Currently, mctp-netlink requires passing interface name to its API. (due to mctp.c being written first) However, in mctpd, we always get ifindex from sockaddr. This leads to mctpd having to convert ifindex to interface name, and then mctp-netlink will convert the interface name back to ifindex. This change removes the unnecessary roundtrip. ifindex is now the main thing to pass around. mctp.c will do its own conversion. Signed-off-by: Khang D Nguyen --- src/mctp-netlink.c | 29 +++++++++++++---------------- src/mctp-netlink.h | 7 ++++--- src/mctp.c | 19 +++++++++++++++---- src/mctpd.c | 27 ++++++++++++--------------- 4 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/mctp-netlink.c b/src/mctp-netlink.c index b05cd9f..3f87acc 100644 --- a/src/mctp-netlink.c +++ b/src/mctp-netlink.c @@ -1096,6 +1096,11 @@ int *mctp_nl_if_list(const mctp_nl *nl, size_t *ret_num_ifs) return ifs; } +bool mctp_nl_if_exists(const mctp_nl *nl, int ifindex) +{ + return entry_byindex(nl, ifindex) != NULL; +} + static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info, const char *ifname, size_t ifname_len, uint32_t net, bool up, uint32_t min_mtu, uint32_t max_mtu, size_t hwaddr_len) @@ -1151,19 +1156,12 @@ struct mctp_rtalter_msg { ]; }; static int fill_rtalter_args(struct mctp_nl *nl, struct mctp_rtalter_msg *msg, - struct rtattr **prta, size_t *prta_len, - mctp_eid_t eid, const char* linkstr) + struct rtattr **prta, size_t *prta_len, + mctp_eid_t eid, int ifindex) { - int ifindex; struct rtattr *rta; size_t rta_len; - ifindex = mctp_nl_ifindex_byname(nl, linkstr); - if (!ifindex) { - warnx("invalid device %s", linkstr); - return -1; - } - memset(msg, 0x0, sizeof(*msg)); msg->nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK; @@ -1190,14 +1188,15 @@ static int fill_rtalter_args(struct mctp_nl *nl, struct mctp_rtalter_msg *msg, return 0; } -int mctp_nl_route_add(struct mctp_nl *nl, uint8_t eid, const char* ifname, - uint32_t mtu) { +int mctp_nl_route_add(struct mctp_nl *nl, uint8_t eid, int ifindex, + uint32_t mtu) +{ struct mctp_rtalter_msg msg; struct rtattr *rta; size_t rta_len; int rc; - rc = fill_rtalter_args(nl, &msg, &rta, &rta_len, eid, ifname); + rc = fill_rtalter_args(nl, &msg, &rta, &rta_len, eid, ifindex); if (rc) { return -1; } @@ -1223,15 +1222,14 @@ int mctp_nl_route_add(struct mctp_nl *nl, uint8_t eid, const char* ifname, } return mctp_nl_send(nl, &msg.nh); - } -int mctp_nl_route_del(struct mctp_nl *nl, uint8_t eid, const char* ifname) +int mctp_nl_route_del(struct mctp_nl *nl, uint8_t eid, int ifindex) { struct mctp_rtalter_msg msg; int rc; - rc = fill_rtalter_args(nl, &msg, NULL, NULL, eid, ifname); + rc = fill_rtalter_args(nl, &msg, NULL, NULL, eid, ifindex); if (rc) { return rc; } @@ -1239,4 +1237,3 @@ int mctp_nl_route_del(struct mctp_nl *nl, uint8_t eid, const char* ifname) return mctp_nl_send(nl, &msg.nh); } - diff --git a/src/mctp-netlink.h b/src/mctp-netlink.h index ce0d981..4a265f3 100644 --- a/src/mctp-netlink.h +++ b/src/mctp-netlink.h @@ -78,6 +78,7 @@ void mctp_nl_linkmap_dump(const mctp_nl *nl); uint32_t *mctp_nl_net_list(const mctp_nl *nl, size_t *ret_num_nets); /* Returns an allocated list of ifindex, caller to free */ int *mctp_nl_if_list(const mctp_nl *nl, size_t *ret_num_if); +bool mctp_nl_if_exists(const mctp_nl *nl, int ifindex); /* Get/set userdata for a link. The userdata is attached to a link * with index @ifindex. Userdata will also be populated into @@ -94,9 +95,9 @@ void *mctp_nl_get_link_userdata(const mctp_nl *nl, int ifindex); void *mctp_nl_get_link_userdata_byname(const mctp_nl *nl, const char *ifname); /* MCTP route helper */ -int mctp_nl_route_add(struct mctp_nl *nl, uint8_t eid, const char* ifname, - uint32_t mtu); -int mctp_nl_route_del(struct mctp_nl *nl, uint8_t eid, const char* ifname); +int mctp_nl_route_add(struct mctp_nl *nl, uint8_t eid, int ifindex, + uint32_t mtu); +int mctp_nl_route_del(struct mctp_nl *nl, uint8_t eid, int ifindex); /* Helpers */ diff --git a/src/mctp.c b/src/mctp.c index 91cb660..3516453 100644 --- a/src/mctp.c +++ b/src/mctp.c @@ -874,6 +874,7 @@ static int cmd_route_add(struct ctx *ctx, int argc, const char **argv) { const char *eidstr = NULL, *linkstr = NULL, *mtustr = NULL; uint32_t mtu = 0, eid = 0; + int ifindex = 0; int rc = 0; if (!(argc == 4 || argc == 6)) { @@ -906,18 +907,24 @@ static int cmd_route_add(struct ctx *ctx, int argc, const char **argv) warnx("Bad eid"); rc = -EINVAL; } + ifindex = mctp_nl_ifindex_byname(ctx->nl, linkstr); + if (!ifindex) { + warnx("add: invalid device %s", linkstr); + rc = -EINVAL; + } if (rc) { warnx("add: invalid command line arguments"); return -1; } - return mctp_nl_route_add(ctx->nl, eid, linkstr, mtu); + return mctp_nl_route_add(ctx->nl, eid, ifindex, mtu); } static int cmd_route_del(struct ctx *ctx, int argc, const char **argv) { - const char *linkstr = NULL, *eidstr = NULL; + const char *eidstr = NULL; uint32_t tmp = 0; + int ifindex = 0; uint8_t eid; int rc = 0; @@ -936,14 +943,18 @@ static int cmd_route_del(struct ctx *ctx, int argc, const char **argv) warnx("Bad eid"); rc = -EINVAL; } + ifindex = mctp_nl_ifindex_byname(ctx->nl, argv[3]); + if (!ifindex) { + warnx("del: invalid device %s", argv[3]); + rc = -EINVAL; + } if (rc) { warnx("del: invalid command line arguments"); return -1; } eid = tmp & 0xff; - linkstr = argv[3]; - return mctp_nl_route_del(ctx->nl, eid, linkstr); + return mctp_nl_route_del(ctx->nl, eid, ifindex); } static int cmd_route(struct ctx *ctx, int argc, const char **argv) diff --git a/src/mctpd.c b/src/mctpd.c index 8234fb4..373d89f 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -1545,26 +1545,25 @@ static int change_peer_eid(struct peer *peer, mctp_eid_t new_eid) return 0; } -static int peer_set_mtu(struct ctx *ctx, struct peer *peer, uint32_t mtu) { - const char* ifname = NULL; +static int peer_set_mtu(struct ctx *ctx, struct peer *peer, uint32_t mtu) +{ int rc; - ifname = mctp_nl_if_byindex(ctx->nl, peer->phys.ifindex); - if (!ifname) { + if (!mctp_nl_if_exists(peer->ctx->nl, peer->phys.ifindex)) { bug_warn("%s: no interface for ifindex %d", __func__, peer->phys.ifindex); return -EPROTO; } - rc = mctp_nl_route_del(ctx->nl, peer->eid, ifname); + rc = mctp_nl_route_del(ctx->nl, peer->eid, peer->phys.ifindex); if (rc < 0 && rc != -ENOENT) { warnx("%s, Failed removing existing route for eid %d %s", - __func__, - peer->phys.ifindex, ifname); + __func__, peer->phys.ifindex, + mctp_nl_if_byindex(ctx->nl, peer->phys.ifindex)); // Continue regardless, route_add will likely fail with EEXIST } - rc = mctp_nl_route_add(ctx->nl, peer->eid, ifname, mtu); + rc = mctp_nl_route_add(ctx->nl, peer->eid, peer->phys.ifindex, mtu); if (rc >= 0) { peer->mtu = mtu; } @@ -2254,19 +2253,17 @@ static int peer_neigh_update(struct peer *peer, uint16_t type) // type is RTM_NEWROUTE or RTM_DELROUTE static int peer_route_update(struct peer *peer, uint16_t type) { - const char * link; - - link = mctp_nl_if_byindex(peer->ctx->nl, peer->phys.ifindex); - if (!link) { + if (!mctp_nl_if_exists(peer->ctx->nl, peer->phys.ifindex)) { bug_warn("%s: Unknown ifindex %d", __func__, peer->phys.ifindex); return -ENODEV; } if (type == RTM_NEWROUTE) { - return mctp_nl_route_add(peer->ctx->nl, - peer->eid, link, peer->mtu); + return mctp_nl_route_add(peer->ctx->nl, peer->eid, + peer->phys.ifindex, peer->mtu); } else if (type == RTM_DELROUTE) { - return mctp_nl_route_del(peer->ctx->nl, peer->eid, link); + return mctp_nl_route_del(peer->ctx->nl, peer->eid, + peer->phys.ifindex); } bug_warn("%s: bad type %d", __func__, type); From e630ff5583748a28cbe74aa159f4b6e67a4098bd Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Mon, 24 Mar 2025 02:58:13 +0000 Subject: [PATCH 4/5] mctp-netlink: add addr utils Extracts netlink addresses utils from mctp.c into mctp-netlink.c for reuse in mctpd Set EID. Signed-off-by: Khang D Nguyen --- src/mctp-netlink.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++ src/mctp-netlink.h | 5 ++++ src/mctp.c | 22 +----------------- 3 files changed, 64 insertions(+), 21 deletions(-) diff --git a/src/mctp-netlink.c b/src/mctp-netlink.c index 3f87acc..92871aa 100644 --- a/src/mctp-netlink.c +++ b/src/mctp-netlink.c @@ -1145,6 +1145,64 @@ static int linkmap_add_entry(mctp_nl *nl, struct ifinfomsg *info, return 0; } +/* Common parts of RTM_NEWADDR and RTM_DELADDR */ +struct mctp_addralter_msg { + struct nlmsghdr nh; + struct ifaddrmsg ifmsg; + struct rtattr rta; + uint8_t data[4]; +}; +static int fill_addralter_args(struct mctp_nl *nl, + struct mctp_addralter_msg *msg, + struct rtattr **prta, size_t *prta_len, + mctp_eid_t eid, int ifindex) +{ + memset(msg, 0x0, sizeof(*msg)); + + msg->nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK; + + msg->ifmsg.ifa_index = ifindex; + msg->ifmsg.ifa_family = AF_MCTP; + + msg->rta.rta_type = IFA_LOCAL; + msg->rta.rta_len = RTA_LENGTH(sizeof(eid)); + memcpy(RTA_DATA(&msg->rta), &eid, sizeof(eid)); + + msg->nh.nlmsg_len = + NLMSG_LENGTH(sizeof(msg->ifmsg)) + RTA_SPACE(sizeof(eid)); + + if (prta) + *prta = &msg->rta; + if (prta_len) + *prta_len = msg->rta.rta_len; + + return 0; +} + +int mctp_nl_addr(struct mctp_nl *nl, mctp_eid_t eid, int ifindex, int rtm_command) +{ + struct mctp_addralter_msg msg; + int rc; + + rc = fill_addralter_args(nl, &msg, NULL, NULL, eid, ifindex); + if (rc) + return -1; + + msg.nh.nlmsg_type = rtm_command; + + return mctp_nl_send(nl, &msg.nh); +} + +int mctp_nl_addr_add(struct mctp_nl *nl, mctp_eid_t eid, int ifindex) +{ + return mctp_nl_addr(nl, eid, ifindex, RTM_NEWADDR); +} + +int mctp_nl_addr_del(struct mctp_nl *nl, mctp_eid_t eid, int ifindex) +{ + return mctp_nl_addr(nl, eid, ifindex, RTM_DELADDR); +} + /* Common parts of RTM_NEWROUTE and RTM_DELROUTE */ struct mctp_rtalter_msg { struct nlmsghdr nh; diff --git a/src/mctp-netlink.h b/src/mctp-netlink.h index 4a265f3..5b3a946 100644 --- a/src/mctp-netlink.h +++ b/src/mctp-netlink.h @@ -94,6 +94,11 @@ void *mctp_nl_get_link_userdata(const mctp_nl *nl, int ifindex); /* Returns NULL if the link does not exist */ void *mctp_nl_get_link_userdata_byname(const mctp_nl *nl, const char *ifname); +/* MCTP addr helper */ +int mctp_nl_addr(struct mctp_nl *nl, uint8_t eid, int ifindex, int rtm_command); +int mctp_nl_addr_add(struct mctp_nl *nl, uint8_t eid, int ifindex); +int mctp_nl_addr_del(struct mctp_nl *nl, uint8_t eid, int ifindex); + /* MCTP route helper */ int mctp_nl_route_add(struct mctp_nl *nl, uint8_t eid, int ifindex, uint32_t mtu); diff --git a/src/mctp.c b/src/mctp.c index 3516453..76f9e01 100644 --- a/src/mctp.c +++ b/src/mctp.c @@ -748,12 +748,6 @@ static int cmd_addr_addremove(struct ctx *ctx, const char* cmdname, int rtm_command, int argc, const char **argv) { - struct { - struct nlmsghdr nh; - struct ifaddrmsg ifmsg; - struct rtattr rta; - uint8_t data[4]; - } msg = {0}; const char *eidstr, *linkstr; uint32_t tmp; uint8_t eid; @@ -786,21 +780,7 @@ static int cmd_addr_addremove(struct ctx *ctx, } eid = tmp & 0xff; - msg.nh.nlmsg_type = rtm_command; - // request an error status since there's no other reply - msg.nh.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK; - - msg.ifmsg.ifa_index = ifindex; - msg.ifmsg.ifa_family = AF_MCTP; - - msg.rta.rta_type = IFA_LOCAL; - msg.rta.rta_len = RTA_LENGTH(sizeof(eid)); - memcpy(RTA_DATA(&msg.rta), &eid, sizeof(eid)); - - msg.nh.nlmsg_len = NLMSG_LENGTH(sizeof(msg.ifmsg)) + - RTA_SPACE(sizeof(eid)); - - return mctp_nl_send(ctx->nl, &msg.nh); + return mctp_nl_addr(ctx->nl, eid, ifindex, rtm_command); } static int cmd_addr_add(struct ctx *ctx, int argc, const char **argv) From ebb19f48f8e8e0925745c7e44d8de8e4709650c3 Mon Sep 17 00:00:00 2001 From: Khang D Nguyen Date: Mon, 24 Mar 2025 02:58:13 +0000 Subject: [PATCH 5/5] mctpd: respond to Get EID using physical addressing Currently, mctpd running as an endpoint with no EID set cannot reply to Get EID message, due to having no routing knowledge and we currently use EID-based routing for the response. This uses physical addressing for Get Endpoint message. Signed-off-by: Khang D Nguyen --- src/mctpd.c | 26 +++++++++++++++++++++++++- tests/test_mctpd_endpoint.py | 25 +++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/mctpd.c b/src/mctpd.c index 373d89f..171e6ba 100644 --- a/src/mctpd.c +++ b/src/mctpd.c @@ -570,6 +570,29 @@ static int read_message(struct ctx *ctx, int sd, uint8_t **ret_buf, size_t *ret_ return rc; } +/* Replies to a physical address */ +static int reply_message_phys(struct ctx *ctx, int sd, const void *resp, size_t resp_len, + const struct sockaddr_mctp_ext *addr) +{ + ssize_t len; + struct sockaddr_mctp_ext reply_addr = *addr; + + reply_addr.smctp_base.smctp_tag &= ~MCTP_TAG_OWNER; + + len = mctp_ops.mctp.sendto(sd, resp, resp_len, 0, + (struct sockaddr *)&reply_addr, + sizeof(reply_addr)); + if (len < 0) { + return -errno; + } + + if ((size_t)len != resp_len) { + bug_warn("short sendto %zd, expected %zu", len, resp_len); + return -EPROTO; + } + return 0; +} + /* Replies to a real EID, not physical addressing */ static int reply_message(struct ctx *ctx, int sd, const void *resp, size_t resp_len, const struct sockaddr_mctp_ext *addr) @@ -694,7 +717,8 @@ static int handle_control_get_endpoint_id(struct ctx *ctx, SET_ENDPOINT_ID_TYPE(resp->eid_type, 2); // TODO: medium specific information - return reply_message(ctx, sd, resp, sizeof(*resp), addr); + // Get Endpoint ID is typically send and reply using physical addressing. + return reply_message_phys(ctx, sd, resp, sizeof(*resp), addr); } static int handle_control_get_endpoint_uuid(struct ctx *ctx, diff --git a/tests/test_mctpd_endpoint.py b/tests/test_mctpd_endpoint.py index 49e8f0c..14b2fe7 100644 --- a/tests/test_mctpd_endpoint.py +++ b/tests/test_mctpd_endpoint.py @@ -2,14 +2,35 @@ from mctp_test_utils import * from mctpd import * -@pytest.fixture(name="config") -def endpoint_config(): +@pytest.fixture +def config(): return """ mode = "endpoint" """ +@pytest.fixture +async def sysnet(): + system = System() + iface = System.Interface("mctp0", 1, 1, bytes([0x1D]), 68, 254, True) + await system.add_interface(iface) + network = Network() + network.add_endpoint(Endpoint(iface, bytes([0x10]), eid=8)) + return Sysnet(system, network) + + """ Test if mctpd is running as an endpoint """ async def test_endpoint_role(dbus, mctpd): obj = await mctpd_mctp_iface_control_obj(dbus, mctpd.system.interfaces[0]) role = await obj.get_role() assert str(role) == "Endpoint" + + +""" mctpd returns null EID on no EID """ +async def test_respond_get_eid_with_no_eid(dbus, mctpd): + bo = mctpd.network.endpoints[0] + + assert len(mctpd.system.addresses) == 0 + + # no EID yet + rsp = await bo.send_control(mctpd.network.mctp_socket, 0x02) + assert rsp.hex(' ') == '00 02 00 00 02 00'