From: David Gibson <david@gibson.dropbear.id.au>
To: passt-dev@passt.top, Stefano Brivio <sbrivio@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Subject: [PATCH v2 10/17] netlink: Add nl_do() helper for simple operations with error checking
Date: Thu, 3 Aug 2023 17:19:49 +1000 [thread overview]
Message-ID: <20230803071956.3198452-11-david@gibson.dropbear.id.au> (raw)
In-Reply-To: <20230803071956.3198452-1-david@gibson.dropbear.id.au>
So far we never checked for errors reported on netlink operations via
NLMSG_ERROR messages. This has led to several subtle and tricky to debug
situations which would have been obvious if we knew that certain netlink
operations had failed.
Introduce a nl_do() helper that performs netlink "do" operations (that is
making a single change without retreiving complex information) with much
more thorough error checking. As well as returning an error code if we
get an NLMSG_ERROR message, we also check for unexpected behaviour in
several places. That way if we've made a mistake in our assumptions about
how netlink works it should result in a clear error rather than some subtle
misbehaviour.
We update those calls to nl_req() that can use the new wrapper to do so.
We will extend those to better handle errors in future. We don't touch
non-"do" operations for now, those are a bit trickier.
Link: https://bugs.passt.top/show_bug.cgi?id=60
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
netlink.c | 59 ++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 12 deletions(-)
diff --git a/netlink.c b/netlink.c
index 5f008ce..9ff93a4 100644
--- a/netlink.c
+++ b/netlink.c
@@ -148,6 +148,47 @@ static ssize_t nl_req(int s, char *buf, void *req,
return n;
}
+/**
+ * nl_do() - Send netlink "do" request, and wait for acknowledgement
+ * @s: Netlink socket
+ * @req: Request (will fill netlink header)
+ * @type: Request type
+ * @flags: Extra request flags (NLM_F_REQUEST and NLM_F_ACK assumed)
+ * @len: Request length
+ *
+ * Return: 0 on success, negative error code on error
+ */
+static int nl_do(int s, void *req, uint16_t type, uint16_t flags, ssize_t len)
+{
+ struct nlmsghdr *nh;
+ char buf[NLBUFSIZ];
+ uint16_t seq;
+ ssize_t n;
+
+ n = nl_req(s, buf, req, type, flags, len);
+ seq = ((struct nlmsghdr *)req)->nlmsg_seq;
+
+ for (nh = (struct nlmsghdr *)buf;
+ NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) {
+ struct nlmsgerr *errmsg;
+
+ if (nh->nlmsg_seq != seq)
+ die("netlink: Unexpected response sequence number");
+
+ switch (nh->nlmsg_type) {
+ case NLMSG_DONE:
+ return 0;
+ case NLMSG_ERROR:
+ errmsg = (struct nlmsgerr *)NLMSG_DATA(nh);
+ return errmsg->error;
+ default:
+ warn("netlink: Unexpected response message");
+ }
+ }
+
+ die("netlink: Missing acknowledgement of request");
+}
+
/**
* nl_get_ext_if() - Get interface index supporting IP version being probed
* @s: Netlink socket
@@ -289,7 +330,6 @@ void nl_route_set_def(int s, unsigned int ifi, sa_family_t af, void *gw)
.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)),
.ifi = ifi,
};
- char buf[NLBUFSIZ];
ssize_t len;
if (af == AF_INET6) {
@@ -316,7 +356,7 @@ void nl_route_set_def(int s, unsigned int ifi, sa_family_t af, void *gw)
req.set.r4.rta_gw.rta_len = rta_len;
}
- nl_req(s, buf, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
+ nl_do(s, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
}
/**
@@ -385,12 +425,11 @@ void nl_route_dup(int s_src, unsigned int ifi_src,
NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE;
nh = NLMSG_NEXT(nh, n)) {
uint16_t flags = nh->nlmsg_flags;
- char resp[NLBUFSIZ];
if (nh->nlmsg_type != RTM_NEWROUTE)
continue;
- nl_req(s_dst, resp, nh, RTM_NEWROUTE,
+ nl_do(s_dst, nh, RTM_NEWROUTE,
(flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
nh->nlmsg_len);
}
@@ -489,7 +528,6 @@ void nl_addr_set(int s, unsigned int ifi, sa_family_t af,
.ifa.ifa_prefixlen = prefix_len,
.ifa.ifa_scope = RT_SCOPE_UNIVERSE,
};
- char buf[NLBUFSIZ];
ssize_t len;
if (af == AF_INET6) {
@@ -518,7 +556,7 @@ void nl_addr_set(int s, unsigned int ifi, sa_family_t af,
req.set.a4.rta_a.rta_type = IFA_ADDRESS;
}
- nl_req(s, buf, &req, RTM_NEWADDR, NLM_F_CREATE | NLM_F_EXCL, len);
+ nl_do(s, &req, RTM_NEWADDR, NLM_F_CREATE | NLM_F_EXCL, len);
}
/**
@@ -550,7 +588,6 @@ void nl_addr_dup(int s_src, unsigned int ifi_src,
NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE;
nh = NLMSG_NEXT(nh, n)) {
struct ifaddrmsg *ifa;
- char resp[NLBUFSIZ];
struct rtattr *rta;
size_t na;
@@ -571,7 +608,7 @@ void nl_addr_dup(int s_src, unsigned int ifi_src,
rta->rta_type = IFA_UNSPEC;
}
- nl_req(s_dst, resp, nh, RTM_NEWADDR,
+ nl_do(s_dst, nh, RTM_NEWADDR,
(nh->nlmsg_flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
nh->nlmsg_len);
}
@@ -639,11 +676,10 @@ void nl_link_set_mac(int s, unsigned int ifi, void *mac)
.rta.rta_type = IFLA_ADDRESS,
.rta.rta_len = RTA_LENGTH(ETH_ALEN),
};
- char buf[NLBUFSIZ];
memcpy(req.mac, mac, ETH_ALEN);
- nl_req(s, buf, &req, RTM_NEWLINK, 0, sizeof(req));
+ nl_do(s, &req, RTM_NEWLINK, 0, sizeof(req));
}
/**
@@ -669,11 +705,10 @@ void nl_link_up(int s, unsigned int ifi, int mtu)
.mtu = mtu,
};
ssize_t len = sizeof(req);
- char buf[NLBUFSIZ];
if (!mtu)
/* Shorten request to drop MTU attribute */
len = offsetof(struct req_t, rta);
- nl_req(s, buf, &req, RTM_NEWLINK, 0, len);
+ nl_do(s, &req, RTM_NEWLINK, 0, len);
}
--
@@ -148,6 +148,47 @@ static ssize_t nl_req(int s, char *buf, void *req,
return n;
}
+/**
+ * nl_do() - Send netlink "do" request, and wait for acknowledgement
+ * @s: Netlink socket
+ * @req: Request (will fill netlink header)
+ * @type: Request type
+ * @flags: Extra request flags (NLM_F_REQUEST and NLM_F_ACK assumed)
+ * @len: Request length
+ *
+ * Return: 0 on success, negative error code on error
+ */
+static int nl_do(int s, void *req, uint16_t type, uint16_t flags, ssize_t len)
+{
+ struct nlmsghdr *nh;
+ char buf[NLBUFSIZ];
+ uint16_t seq;
+ ssize_t n;
+
+ n = nl_req(s, buf, req, type, flags, len);
+ seq = ((struct nlmsghdr *)req)->nlmsg_seq;
+
+ for (nh = (struct nlmsghdr *)buf;
+ NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) {
+ struct nlmsgerr *errmsg;
+
+ if (nh->nlmsg_seq != seq)
+ die("netlink: Unexpected response sequence number");
+
+ switch (nh->nlmsg_type) {
+ case NLMSG_DONE:
+ return 0;
+ case NLMSG_ERROR:
+ errmsg = (struct nlmsgerr *)NLMSG_DATA(nh);
+ return errmsg->error;
+ default:
+ warn("netlink: Unexpected response message");
+ }
+ }
+
+ die("netlink: Missing acknowledgement of request");
+}
+
/**
* nl_get_ext_if() - Get interface index supporting IP version being probed
* @s: Netlink socket
@@ -289,7 +330,6 @@ void nl_route_set_def(int s, unsigned int ifi, sa_family_t af, void *gw)
.rta.rta_len = RTA_LENGTH(sizeof(unsigned int)),
.ifi = ifi,
};
- char buf[NLBUFSIZ];
ssize_t len;
if (af == AF_INET6) {
@@ -316,7 +356,7 @@ void nl_route_set_def(int s, unsigned int ifi, sa_family_t af, void *gw)
req.set.r4.rta_gw.rta_len = rta_len;
}
- nl_req(s, buf, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
+ nl_do(s, &req, RTM_NEWROUTE, NLM_F_CREATE | NLM_F_EXCL, len);
}
/**
@@ -385,12 +425,11 @@ void nl_route_dup(int s_src, unsigned int ifi_src,
NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE;
nh = NLMSG_NEXT(nh, n)) {
uint16_t flags = nh->nlmsg_flags;
- char resp[NLBUFSIZ];
if (nh->nlmsg_type != RTM_NEWROUTE)
continue;
- nl_req(s_dst, resp, nh, RTM_NEWROUTE,
+ nl_do(s_dst, nh, RTM_NEWROUTE,
(flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
nh->nlmsg_len);
}
@@ -489,7 +528,6 @@ void nl_addr_set(int s, unsigned int ifi, sa_family_t af,
.ifa.ifa_prefixlen = prefix_len,
.ifa.ifa_scope = RT_SCOPE_UNIVERSE,
};
- char buf[NLBUFSIZ];
ssize_t len;
if (af == AF_INET6) {
@@ -518,7 +556,7 @@ void nl_addr_set(int s, unsigned int ifi, sa_family_t af,
req.set.a4.rta_a.rta_type = IFA_ADDRESS;
}
- nl_req(s, buf, &req, RTM_NEWADDR, NLM_F_CREATE | NLM_F_EXCL, len);
+ nl_do(s, &req, RTM_NEWADDR, NLM_F_CREATE | NLM_F_EXCL, len);
}
/**
@@ -550,7 +588,6 @@ void nl_addr_dup(int s_src, unsigned int ifi_src,
NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE;
nh = NLMSG_NEXT(nh, n)) {
struct ifaddrmsg *ifa;
- char resp[NLBUFSIZ];
struct rtattr *rta;
size_t na;
@@ -571,7 +608,7 @@ void nl_addr_dup(int s_src, unsigned int ifi_src,
rta->rta_type = IFA_UNSPEC;
}
- nl_req(s_dst, resp, nh, RTM_NEWADDR,
+ nl_do(s_dst, nh, RTM_NEWADDR,
(nh->nlmsg_flags & ~NLM_F_DUMP_FILTERED) | NLM_F_CREATE,
nh->nlmsg_len);
}
@@ -639,11 +676,10 @@ void nl_link_set_mac(int s, unsigned int ifi, void *mac)
.rta.rta_type = IFLA_ADDRESS,
.rta.rta_len = RTA_LENGTH(ETH_ALEN),
};
- char buf[NLBUFSIZ];
memcpy(req.mac, mac, ETH_ALEN);
- nl_req(s, buf, &req, RTM_NEWLINK, 0, sizeof(req));
+ nl_do(s, &req, RTM_NEWLINK, 0, sizeof(req));
}
/**
@@ -669,11 +705,10 @@ void nl_link_up(int s, unsigned int ifi, int mtu)
.mtu = mtu,
};
ssize_t len = sizeof(req);
- char buf[NLBUFSIZ];
if (!mtu)
/* Shorten request to drop MTU attribute */
len = offsetof(struct req_t, rta);
- nl_req(s, buf, &req, RTM_NEWLINK, 0, len);
+ nl_do(s, &req, RTM_NEWLINK, 0, len);
}
--
2.41.0
next prev parent reply other threads:[~2023-08-03 7:20 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-03 7:19 [PATCH v2 00/17] netlink fixes and cleanups David Gibson
2023-08-03 7:19 ` [PATCH v2 01/17] netlink: Split up functionality of nl_link() David Gibson
2023-08-03 7:19 ` [PATCH v2 02/17] netlink: Split nl_addr() into separate operation functions David Gibson
2023-08-03 7:19 ` [PATCH v2 03/17] netlink: Split nl_route() " David Gibson
2023-08-03 7:19 ` [PATCH v2 04/17] netlink: Use struct in_addr for IPv4 addresses, not bare uint32_t David Gibson
2023-08-03 7:19 ` [PATCH v2 05/17] netlink: Explicitly pass netlink sockets to operations David Gibson
2023-08-03 7:19 ` [PATCH v2 06/17] netlink: Make nl_*_dup() use a separate datagram for each request David Gibson
2023-08-03 7:19 ` [PATCH v2 07/17] netlink: Start sequence number from 1 instead of 0 David Gibson
2023-08-03 7:19 ` [PATCH v2 08/17] netlink: Treat send() or recv() errors as fatal David Gibson
2023-08-03 7:19 ` [PATCH v2 09/17] netlink: Fill in netlink header fields from nl_req() David Gibson
2023-08-03 7:19 ` David Gibson [this message]
2023-08-03 7:19 ` [PATCH v2 11/17] netlink: Clearer reasoning about the netlink response buffer size David Gibson
2023-08-03 7:19 ` [PATCH v2 12/17] netlink: Split nl_req() to allow processing multiple response datagrams David Gibson
2023-08-03 7:19 ` [PATCH v2 13/17] netlink: Add nl_foreach_oftype to filter response message types David Gibson
2023-08-03 7:19 ` [PATCH v2 14/17] netlink: Propagate errors for "set" operations David Gibson
2023-08-03 7:19 ` [PATCH v2 15/17] netlink: Always process all responses to a netlink request David Gibson
2023-08-03 7:19 ` [PATCH v2 16/17] netlink: Propagate errors for "dump" operations David Gibson
2023-08-03 7:19 ` [PATCH v2 17/17] netlink: Propagate errors for "dup" operations David Gibson
2023-08-04 7:04 ` [PATCH v2 00/17] netlink fixes and cleanups Stefano Brivio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230803071956.3198452-11-david@gibson.dropbear.id.au \
--to=david@gibson.dropbear.id.au \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).