Skip to content

Miscellaneous improvements to mctpd #81

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Miscellaneous improvements to mctpd #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
850e103
tests: handle RTM_NEWADDR and RTM_DELADDR
khangng-ampere Jun 3, 2025
a3297d9
tests: deallocate iid on response
khangng-ampere Jun 4, 2025
31832d2
mctp-netlink: use ifindex instead of name
khangng-ampere Mar 24, 2025
9c2adf0
mctp-netlink: add addr utils
khangng-ampere Mar 24, 2025
51e76e4
mctpd: always reply to physical addresses
khangng-ampere Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 75 additions & 16 deletions src/mctp-netlink.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -1140,6 +1145,68 @@ 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_add(struct mctp_nl *nl, mctp_eid_t eid, int ifindex)
{
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_NEWADDR;

return mctp_nl_send(nl, &msg.nh);
}

int mctp_nl_addr_del(struct mctp_nl *nl, mctp_eid_t eid, int ifindex)
{
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_DELADDR;

return mctp_nl_send(nl, &msg.nh);
}
Comment on lines +1182 to +1208
Copy link
Member

Choose a reason for hiding this comment

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

With the only difference being the type here, could we simplify this into a parameter? This would also mean you don't need the rtm_command check in mctp.c to choose a function...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think API design wise for mctp-netlink.h, separate mctp_nl_addr_add and mctp_nl_addr_del should be superior. I don't think there is any common case that need adding or deleting parameterized. Or i.e, I think

mctp_nl_addr_add(...)

is better than

mctp_nl_addr(..., RTM_DELADDR)

It is also less error-prone to pass something else to it.

I think the problem might be mctp.c. Currently it fuses addr add and addr del into one code path, and then split the code path again with the rtm_command. Let me also clean that up by separating the code path.


/* Common parts of RTM_NEWROUTE and RTM_DELROUTE */
struct mctp_rtalter_msg {
struct nlmsghdr nh;
Expand All @@ -1151,19 +1218,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;

Expand All @@ -1190,14 +1250,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;
}
Expand All @@ -1223,20 +1284,18 @@ 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;
}
msg.nh.nlmsg_type = RTM_DELROUTE;

return mctp_nl_send(nl, &msg.nh);
}

11 changes: 8 additions & 3 deletions src/mctp-netlink.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -93,10 +94,14 @@ 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_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, 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 */

Expand Down
43 changes: 18 additions & 25 deletions src/mctp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -786,21 +780,9 @@ 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 rtm_command == RTM_NEWADDR ?
mctp_nl_addr_add(ctx->nl, eid, ifindex) :
mctp_nl_addr_del(ctx->nl, eid, ifindex);
}

static int cmd_addr_add(struct ctx *ctx, int argc, const char **argv)
Expand Down Expand Up @@ -874,6 +856,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)) {
Expand Down Expand Up @@ -906,18 +889,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;

Expand All @@ -936,14 +925,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)
Expand Down
39 changes: 17 additions & 22 deletions src/mctpd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,20 +570,18 @@ static int read_message(struct ctx *ctx, int sd, uint8_t **ret_buf, size_t *ret_
return rc;
}

/* 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)
{
ssize_t len;
struct sockaddr_mctp reply_addr;
struct sockaddr_mctp_ext reply_addr = *addr;

memcpy(&reply_addr, &addr->smctp_base, sizeof(reply_addr));
reply_addr.smctp_tag &= ~MCTP_TAG_OWNER;
reply_addr.smctp_base.smctp_tag &= ~MCTP_TAG_OWNER;

if (reply_addr.smctp_addr.s_addr == 0 ||
reply_addr.smctp_addr.s_addr == 0xff) {
if (reply_addr.smctp_base.smctp_addr.s_addr == 0 ||
reply_addr.smctp_base.smctp_addr.s_addr == 0xff) {
bug_warn("reply_message can't take EID %d",
reply_addr.smctp_addr.s_addr);
reply_addr.smctp_base.smctp_addr.s_addr);
return -EPROTO;
}

Expand Down Expand Up @@ -1545,26 +1543,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;
}
Expand Down Expand Up @@ -2254,19 +2251,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);
Expand Down
Loading