From d73a5d692b3a25cf6946ce2ec4c940efcc6d2cd9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Feb 2023 10:11:46 +0100 Subject: [PATCH 1/9] platform/netlink: assert for valid string in nla_get_string() --- src/libnm-platform/nm-netlink.h | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/libnm-platform/nm-netlink.h b/src/libnm-platform/nm-netlink.h index cdb8a81c32..2919d432d8 100644 --- a/src/libnm-platform/nm-netlink.h +++ b/src/libnm-platform/nm-netlink.h @@ -238,7 +238,18 @@ nla_get_be64(const struct nlattr *nla) static inline char * nla_get_string(const struct nlattr *nla) { - return nla_data(nla); + char *s; + + /* nla_get_string() requires that nla contains a NUL terminated string. + * It cannot return NULL. Only use it with attributes that validate as NLA_STRING. */ + + nm_assert(nla_len(nla) > 0); + + s = nla_data(nla); + + nm_assert(memchr(s, 0, nla_len(nla))); + + return s; } size_t nla_strlcpy(char *dst, const struct nlattr *nla, size_t dstsize); From 6f854ecaeb3467b0e196a98919251499d013fd3e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Feb 2023 11:04:32 +0100 Subject: [PATCH 2/9] platform/netlink: cleanup nla_strlcpy() to not wipe remaining buffer strlcpy()/g_strlcpy() has a well understood behavior. nla_strlcpy() did not behave like that. Instead, it also used to always wipe the remainder of the string, similar to what strncpy() would do. True, if we do nla_strlcpy(obj->link.name, tb[IFLA_IFNAME], IFNAMSIZ); then we might want to clear the remainder and don't care about the overhead of writing up to 14 bytes unnecessarily... However, actually all callers of nla_strlcpy() either operate on a buffer that is already pre-inialized with zero, or they really don't care about the uninitialized memory after the string. So this was nowhere the desired behavior. Change nla_strlcpy() to not wipe the remainder of the buffer, so it behaves mostly like strlcpy()/g_strlcpy() and as one would expect. Add nla_strlcpy_wipe(), which on top of it also clears the remaining buffer. In that aspect, it bears some similarities with strncpy(), but it differs in other regards from strncpy (always NUL terminating and returning the srclen). Yes, the name nla_strlcpy_wipe() is maybe unfamiliar to the user, but it really is like nla_strlcpy() with the addition to clear the buffer. That seems simple enough to understand based on the name. Note that all existing callers of nla_strlcpy() do not care about clearing the memory, and the change in behavior is fine for them. --- src/libnm-platform/nm-netlink.c | 58 +++++++++++++++++++-------------- src/libnm-platform/nm-netlink.h | 17 +++++++++- 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/src/libnm-platform/nm-netlink.c b/src/libnm-platform/nm-netlink.c index fc70422642..de042f8e90 100644 --- a/src/libnm-platform/nm-netlink.c +++ b/src/libnm-platform/nm-netlink.c @@ -488,44 +488,52 @@ nlmsg_put(struct nl_msg *n, } size_t -nla_strlcpy(char *dst, const struct nlattr *nla, size_t dstsize) +_nla_strlcpy_full(char *dst, const struct nlattr *nla, size_t dstsize, gboolean wipe_remainder) { - const char *src; + const char *src = NULL; size_t srclen; - size_t len; + size_t cpylen; - /* - Always writes @dstsize bytes to @dst - * - Copies the first non-NUL characters to @dst. - * Any characters after the first NUL bytes in @nla are ignored. - * - If the string @nla is longer than @dstsize, the string - * gets truncated. @dst will always be NUL terminated. */ + /* Behaves like strlcpy(): + * + * - returns the length of the string in nla (how much it wanted to copy). + * - will always NUL terminate dst (unless dstsize is zero). + * - if @wipe_remainder, the remaining bytes after the string are set to NUL, + * similar to what strncpy() would do. Otherwise the bytes are undefined. + * - nla is not required to contain a NUL terminated string (unlike nla_get_string()). + * - the function copies the bytes up to the first NUL character in nla. + * any remainder in nla is ignored. + * - nla may be NULL, which is treated the same as an empty string (copying zero bytes). + */ - if (G_UNLIKELY(dstsize <= 1)) { - if (dstsize == 1) - dst[0] = '\0'; - if (nla && (srclen = nla_len(nla)) > 0) - return strnlen(nla_data(nla), srclen); - return 0; - } - - nm_assert(dst); + nm_assert(dstsize == 0 || dst); if (nla) { srclen = nla_len(nla); if (srclen > 0) { src = nla_data(nla); srclen = strnlen(src, srclen); - if (srclen > 0) { - len = NM_MIN(dstsize - 1, srclen); - memcpy(dst, src, len); - memset(&dst[len], 0, dstsize - len); - return srclen; - } } + } else + srclen = 0; + + if (dstsize == 0) { + /* we cannot NUL terminate. This is potentially dangerous, maybe + * we should assert against this case. */ + return srclen; } - memset(dst, 0, dstsize); - return 0; + cpylen = NM_MIN(dstsize - 1u, srclen); + + nm_memcpy(dst, src, cpylen); + + if (wipe_remainder) { + /* like strncpy() would do, wipe the rest. */ + memset(&dst[cpylen], 0, dstsize - cpylen); + } else + dst[cpylen] = '\0'; + + return srclen; } size_t diff --git a/src/libnm-platform/nm-netlink.h b/src/libnm-platform/nm-netlink.h index 2919d432d8..117525f944 100644 --- a/src/libnm-platform/nm-netlink.h +++ b/src/libnm-platform/nm-netlink.h @@ -252,7 +252,22 @@ nla_get_string(const struct nlattr *nla) return s; } -size_t nla_strlcpy(char *dst, const struct nlattr *nla, size_t dstsize); +size_t +_nla_strlcpy_full(char *dst, const struct nlattr *nla, size_t dstsize, gboolean wipe_remainder); + +static inline size_t +nla_strlcpy(char *dst, const struct nlattr *nla, size_t dstsize) +{ + return _nla_strlcpy_full(dst, nla, dstsize, FALSE); +} + +static inline size_t +nla_strlcpy_wipe(char *dst, const struct nlattr *nla, size_t dstsize) +{ + /* Behaves exactly like nla_strlcpy(), but (similar to strncpy()) it fills the + * remaining @dstsize bytes with NUL. */ + return _nla_strlcpy_full(dst, nla, dstsize, TRUE); +} size_t nla_memcpy(void *dst, const struct nlattr *nla, size_t dstsize); From 6ca537fa6a1af8338da4d4ca0e872445e9bb9f53 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Feb 2023 10:34:54 +0100 Subject: [PATCH 3/9] platform: rename variables for extack message Consistently name those variables and parameters "extack_msg". The previous term "errmsg"/"msg" was not used consistently, and it is also not clear what message this really is. For netlink, it is well understood what Extended ACK means. --- src/libnm-platform/nm-linux-platform.c | 78 +++++++++++++------------- 1 file changed, 39 insertions(+), 39 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 30ad1275d8..d99c7e3005 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -465,7 +465,7 @@ typedef enum _nm_packed { typedef struct { WaitForNlResponseResult *out_seq_result; - char **out_errmsg; + char **out_extack_msg; union { int *out_refresh_all_in_progress; NMPObject **out_route_get; @@ -709,7 +709,7 @@ wait_for_nl_response_to_nmerr(WaitForNlResponseResult seq_result) static const char * wait_for_nl_response_to_string(WaitForNlResponseResult seq_result, - const char *errmsg, + const char *extack_msg, char *buf, gsize buf_size) { @@ -735,8 +735,8 @@ wait_for_nl_response_to_string(WaitForNlResponseResult seq_result, "failure %d (%s%s%s)", -((int) seq_result), nm_strerror_native(-((int) seq_result)), - errmsg ? " - " : "", - errmsg ?: ""); + extack_msg ? " - " : "", + extack_msg ?: ""); } else nm_strbuf_append(&buf, &buf_size, "internal failure %d", (int) seq_result); break; @@ -6994,7 +6994,7 @@ delayed_action_schedule_WAIT_FOR_RESPONSE(NMPlatform *pla NMPNetlinkProtocol netlink_protocol, guint32 seq_number, WaitForNlResponseResult *out_seq_result, - char **out_errmsg, + char **out_extack_msg, DelayedActionWaitForNlResponseType response_type, gpointer response_out_data) { @@ -7003,7 +7003,7 @@ delayed_action_schedule_WAIT_FOR_RESPONSE(NMPlatform *pla .timeout_abs_nsec = nm_utils_get_monotonic_timestamp_nsec() + (200 * (NM_UTILS_NSEC_PER_SEC / 1000)), .out_seq_result = out_seq_result, - .out_errmsg = out_errmsg, + .out_extack_msg = out_extack_msg, .response_type = response_type, .response.out_data = response_out_data, }; @@ -7362,7 +7362,7 @@ static int _nl_send_nlmsghdr(NMPlatform *platform, struct nlmsghdr *nlhdr, WaitForNlResponseResult *out_seq_result, - char **out_errmsg, + char **out_extack_msg, DelayedActionWaitForNlResponseType response_type, gpointer response_out_data) { @@ -7410,7 +7410,7 @@ again: NMP_NETLINK_ROUTE, seq, out_seq_result, - out_errmsg, + out_extack_msg, response_type, response_out_data); return 0; @@ -7421,7 +7421,7 @@ _netlink_send_nlmsg(NMPlatform *platform, NMPNetlinkProtocol netlink_protocol, struct nl_msg *nlmsg, WaitForNlResponseResult *out_seq_result, - char **out_errmsg, + char **out_extack_msg, DelayedActionWaitForNlResponseType response_type, gpointer response_out_data) { @@ -7444,7 +7444,7 @@ _netlink_send_nlmsg(NMPlatform *platform, netlink_protocol, seq, out_seq_result, - out_errmsg, + out_extack_msg, response_type, response_out_data); return 0; @@ -7454,13 +7454,13 @@ static int _netlink_send_nlmsg_rtnl(NMPlatform *platform, struct nl_msg *nlmsg, WaitForNlResponseResult *out_seq_result, - char **out_errmsg) + char **out_extack_msg) { return _netlink_send_nlmsg(platform, NMP_NETLINK_ROUTE, nlmsg, out_seq_result, - out_errmsg, + out_extack_msg, DELAYED_ACTION_RESPONSE_TYPE_VOID, NULL); } @@ -7731,7 +7731,7 @@ event_seq_check(NMPlatform *platform, NMPNetlinkProtocol netlink_protocol, guint32 seq_number, WaitForNlResponseResult seq_result, - const char *msg) + const char *extack_msg) { NMLinuxPlatformPrivate *priv = NM_LINUX_PLATFORM_GET_PRIVATE(platform); guint i; @@ -7759,8 +7759,8 @@ event_seq_check(NMPlatform *platform, } else if (seq_result != WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_UNKNOWN || data->seq_result == WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN) data->seq_result = seq_result; - if (data->out_errmsg && !*data->out_errmsg) - *data->out_errmsg = g_strdup(msg); + if (data->out_extack_msg && !*data->out_extack_msg) + *data->out_extack_msg = g_strdup(extack_msg); return; } } @@ -8011,14 +8011,14 @@ do_add_link_with_lookup(NMPlatform *platform, { const NMPObject *obj = NULL; WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; - gs_free char *errmsg = NULL; + gs_free char *extack_msg = NULL; int nle; char s_buf[256]; NMPCache *cache = nm_platform_get_cache(platform); event_handler_read_netlink(platform, NMP_NETLINK_ROUTE, FALSE); - nle = _netlink_send_nlmsg_rtnl(platform, nlmsg, &seq_result, &errmsg); + nle = _netlink_send_nlmsg_rtnl(platform, nlmsg, &seq_result, &extack_msg); if (nle < 0) { _LOGE("do-add-link[%s/%s]: failed sending netlink request \"%s\" (%d)", name, @@ -8037,7 +8037,7 @@ do_add_link_with_lookup(NMPlatform *platform, "do-add-link[%s/%s]: %s", name, nm_link_type_to_string(link_type), - wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf))); + wait_for_nl_response_to_string(seq_result, extack_msg, s_buf, sizeof(s_buf))); if (out_link) { obj = nmp_cache_lookup_link_full(cache, 0, name, FALSE, link_type, NULL, NULL); @@ -8055,7 +8055,7 @@ do_add_addrroute(NMPlatform *platform, { char sbuf1[NM_UTILS_TO_STRING_BUFFER_SIZE]; WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; - gs_free char *errmsg = NULL; + gs_free char *extack_msg = NULL; int nle; char s_buf[256]; @@ -8067,7 +8067,7 @@ do_add_addrroute(NMPlatform *platform, event_handler_read_netlink(platform, NMP_NETLINK_ROUTE, FALSE); - nle = _netlink_send_nlmsg_rtnl(platform, nlmsg, &seq_result, &errmsg); + nle = _netlink_send_nlmsg_rtnl(platform, nlmsg, &seq_result, &extack_msg); if (nle < 0) { _LOGE("do-add-%s[%s]: failure sending netlink request \"%s\" (%d)", NMP_OBJECT_GET_CLASS(obj_id)->obj_type_name, @@ -8088,7 +8088,7 @@ do_add_addrroute(NMPlatform *platform, "do-add-%s[%s]: %s", NMP_OBJECT_GET_CLASS(obj_id)->obj_type_name, nmp_object_to_string(obj_id, NMP_OBJECT_TO_STRING_ID, sbuf1, sizeof(sbuf1)), - wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf))); + wait_for_nl_response_to_string(seq_result, extack_msg, s_buf, sizeof(s_buf))); if (NMP_OBJECT_GET_TYPE(obj_id) == NMP_OBJECT_TYPE_IP6_ADDRESS) { /* In rare cases, the object is not yet ready as we received the ACK from @@ -8110,7 +8110,7 @@ do_delete_object(NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *n { char sbuf1[NM_UTILS_TO_STRING_BUFFER_SIZE]; WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; - gs_free char *errmsg = NULL; + gs_free char *extack_msg = NULL; int nle; char s_buf[256]; gboolean success; @@ -8118,7 +8118,7 @@ do_delete_object(NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *n event_handler_read_netlink(platform, NMP_NETLINK_ROUTE, FALSE); - nle = _netlink_send_nlmsg_rtnl(platform, nlmsg, &seq_result, &errmsg); + nle = _netlink_send_nlmsg_rtnl(platform, nlmsg, &seq_result, &extack_msg); if (nle < 0) { _LOGE("do-delete-%s[%s]: failure sending netlink request \"%s\" (%d)", NMP_OBJECT_GET_CLASS(obj_id)->obj_type_name, @@ -8155,7 +8155,7 @@ do_delete_object(NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *n "do-delete-%s[%s]: %s%s", NMP_OBJECT_GET_CLASS(obj_id)->obj_type_name, nmp_object_to_string(obj_id, NMP_OBJECT_TO_STRING_ID, sbuf1, sizeof(sbuf1)), - wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf)), + wait_for_nl_response_to_string(seq_result, extack_msg, s_buf, sizeof(s_buf)), log_detail); if (NM_IN_SET(NMP_OBJECT_GET_TYPE(obj_id), @@ -8186,7 +8186,7 @@ do_change_link(NMPlatform *platform, nm_auto_pop_netns NMPNetns *netns = NULL; int nle; WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; - gs_free char *errmsg = NULL; + gs_free char *extack_msg = NULL; char s_buf[256]; int result; NMLogLevel log_level; @@ -8207,7 +8207,7 @@ retry: log_detail = ""; nm_clear_g_free(&log_detail_free); - nle = _netlink_send_nlmsg_rtnl(platform, nlmsg, &seq_result, &errmsg); + nle = _netlink_send_nlmsg_rtnl(platform, nlmsg, &seq_result, &extack_msg); if (nle < 0) { log_level = LOGL_ERR; log_detail_free = @@ -8267,7 +8267,7 @@ out: _NMLOG(log_level, "do-change-link[%d]: %s%s", ifindex, - wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf)), + wait_for_nl_response_to_string(seq_result, extack_msg, s_buf, sizeof(s_buf)), log_detail); if (result == -EAGAIN) @@ -9751,7 +9751,7 @@ routing_rule_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformRoutin { WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; nm_auto_nlmsg struct nl_msg *msg = NULL; - gs_free char *errmsg = NULL; + gs_free char *extack_msg = NULL; char s_buf[256]; int nle; @@ -9759,7 +9759,7 @@ routing_rule_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformRoutin event_handler_read_netlink(platform, NMP_NETLINK_ROUTE, FALSE); - nle = _netlink_send_nlmsg_rtnl(platform, msg, &seq_result, &errmsg); + nle = _netlink_send_nlmsg_rtnl(platform, msg, &seq_result, &extack_msg); if (nle < 0) { _LOGE("do-add-rule: failed sending netlink request \"%s\" (%d)", nm_strerror(nle), -nle); return -NME_PL_NETLINK; @@ -9771,7 +9771,7 @@ routing_rule_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformRoutin _NMLOG(seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK ? LOGL_DEBUG : LOGL_WARN, "do-add-rule: %s", - wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf))); + wait_for_nl_response_to_string(seq_result, extack_msg, s_buf, sizeof(s_buf))); if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK) return 0; @@ -9786,7 +9786,7 @@ static int qdisc_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformQdisc *qdisc) { WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; - gs_free char *errmsg = NULL; + gs_free char *extack_msg = NULL; int nle; char s_buf[256]; nm_auto_nlmsg struct nl_msg *msg = NULL; @@ -9798,7 +9798,7 @@ qdisc_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformQdisc *qdisc) event_handler_read_netlink(platform, NMP_NETLINK_ROUTE, FALSE); - nle = _netlink_send_nlmsg_rtnl(platform, msg, &seq_result, &errmsg); + nle = _netlink_send_nlmsg_rtnl(platform, msg, &seq_result, &extack_msg); if (nle < 0) { _LOGE("do-add-qdisc: failed sending netlink request \"%s\" (%d)", nm_strerror(nle), -nle); return -NME_PL_NETLINK; @@ -9810,7 +9810,7 @@ qdisc_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformQdisc *qdisc) _NMLOG(seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK ? LOGL_DEBUG : LOGL_WARN, "do-add-qdisc: %s", - wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf))); + wait_for_nl_response_to_string(seq_result, extack_msg, s_buf, sizeof(s_buf))); if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK) return 0; @@ -9827,7 +9827,7 @@ tc_delete(NMPlatform *platform, gboolean log_error) { WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; - gs_free char *errmsg = NULL; + gs_free char *extack_msg = NULL; int nle; char s_buf[256]; const char *log_tag; @@ -9856,7 +9856,7 @@ tc_delete(NMPlatform *platform, event_handler_read_netlink(platform, NMP_NETLINK_ROUTE, FALSE); - nle = _netlink_send_nlmsg_rtnl(platform, msg, &seq_result, &errmsg); + nle = _netlink_send_nlmsg_rtnl(platform, msg, &seq_result, &extack_msg); if (nle < 0) { _NMLOG(log_error ? LOGL_ERR : LOGL_DEBUG, "%s: failed sending netlink request \"%s\" (%d)", @@ -9874,7 +9874,7 @@ tc_delete(NMPlatform *platform, : LOGL_WARN, "%s: %s", log_tag, - wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf))); + wait_for_nl_response_to_string(seq_result, extack_msg, s_buf, sizeof(s_buf))); if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK) return 0; @@ -9896,7 +9896,7 @@ static int tfilter_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformTfilter *tfilter) { WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; - gs_free char *errmsg = NULL; + gs_free char *extack_msg = NULL; int nle; char s_buf[256]; nm_auto_nlmsg struct nl_msg *msg = NULL; @@ -9908,7 +9908,7 @@ tfilter_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformTfilter *tf event_handler_read_netlink(platform, NMP_NETLINK_ROUTE, FALSE); - nle = _netlink_send_nlmsg_rtnl(platform, msg, &seq_result, &errmsg); + nle = _netlink_send_nlmsg_rtnl(platform, msg, &seq_result, &extack_msg); if (nle < 0) { _LOGE("do-add-tfilter: failed sending netlink request \"%s\" (%d)", nm_strerror(nle), -nle); return -NME_PL_NETLINK; @@ -9920,7 +9920,7 @@ tfilter_add(NMPlatform *platform, NMPNlmFlags flags, const NMPlatformTfilter *tf _NMLOG(seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK ? LOGL_DEBUG : LOGL_WARN, "do-add-tfilter: %s", - wait_for_nl_response_to_string(seq_result, errmsg, s_buf, sizeof(s_buf))); + wait_for_nl_response_to_string(seq_result, extack_msg, s_buf, sizeof(s_buf))); if (seq_result == WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK) return 0; From 1d69b41db92b084ac248e25d45d850c457833bd0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Feb 2023 11:35:19 +0100 Subject: [PATCH 4/9] platform: log extack warning messages for netlink requests The extack can also be returned on success. In that case, they are warnings. Log them, it might be useful. --- src/libnm-platform/nm-linux-platform.c | 16 ++++++++++++---- src/libnm-platform/nm-netlink.c | 15 ++++++++++----- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index d99c7e3005..04ac89cc89 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -10176,9 +10176,16 @@ continue_reading: int errsv; errsv = nlmsg_parse_error(msg.nm_nlh, &extack_msg); - if (errsv == 0) + if (errsv == 0) { seq_result = WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_OK; - else { + if (extack_msg) { + _LOGD("%s: recvmsg: warning message from kernel: %s%s%s for request %d", + log_prefix, + NM_PRINT_FMT_QUOTE_STRING(extack_msg), + msg.nm_nlh->nlmsg_seq); + extack_msg = NULL; + } + } else { _LOGD("%s: recvmsg: error message from kernel: %s (%d)%s%s%s for request %d", log_prefix, nm_strerror(errsv), @@ -10636,9 +10643,10 @@ mptcp_addr_update(NMPlatform *platform, NMOptionBool add, const NMPlatformMptcpA return nle; } - _LOGT("mptcp: %s address %s: success", + _LOGT("mptcp: %s address %s: success%s%s%s", cmd_str, - nm_platform_mptcp_addr_to_string(addr, sbuf, sizeof(sbuf))); + nm_platform_mptcp_addr_to_string(addr, sbuf, sizeof(sbuf)), + NM_PRINT_FMT_QUOTED(extack_msg[0] != '\0', " Warning: \"", extack_msg, "\"", "")); return 0; diff --git a/src/libnm-platform/nm-netlink.c b/src/libnm-platform/nm-netlink.c index de042f8e90..e95768465a 100644 --- a/src/libnm-platform/nm-netlink.c +++ b/src/libnm-platform/nm-netlink.c @@ -313,9 +313,6 @@ nlmsg_parse_error(const struct nlmsghdr *nlh, const char **out_extack_msg) e = nlmsg_data(nlh); - if (!e->error) - return 0; - if (NM_FLAGS_HAS(nlh->nlmsg_flags, NLM_F_ACK_TLVS) && out_extack_msg && nlh->nlmsg_len >= sizeof(*e) + e->msg.nlmsg_len) { static const struct nla_policy policy[] = { @@ -328,11 +325,19 @@ nlmsg_parse_error(const struct nlmsghdr *nlh, const char **out_extack_msg) tlvs = NM_CAST_ALIGN(struct nlattr, (((char *) e) + sizeof(*e) + e->msg.nlmsg_len - NLMSG_HDRLEN)); if (nla_parse_arr(tb, tlvs, nlh->nlmsg_len - sizeof(*e) - e->msg.nlmsg_len, policy) >= 0) { - if (tb[NLMSGERR_ATTR_MSG]) - *out_extack_msg = nla_get_string(tb[NLMSGERR_ATTR_MSG]); + if (tb[NLMSGERR_ATTR_MSG]) { + const char *s; + + s = nla_get_string(tb[NLMSGERR_ATTR_MSG]); + if (s[0] != '\0') + *out_extack_msg = s; + } } } + if (!e->error) + return 0; + return -nm_errno_from_native(e->error); } From bb9894abecb7f52f904d98fe6ed42fb8292a9eb3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Feb 2023 11:42:45 +0100 Subject: [PATCH 5/9] platform: minor cleanup of event_seq_check() - unindent the code by "continue" the loop for the irrelevant case. - fix indentation of comments. - avoid unnecessary g_strdup() call if the extack message is NULL. --- src/libnm-platform/nm-linux-platform.c | 29 ++++++++++++++------------ 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 04ac89cc89..4423b75ddb 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -7750,19 +7750,22 @@ event_seq_check(NMPlatform *platform, DelayedActionWaitForNlResponseData *data = delayed_action_get_list_wait_for_resonse(priv, netlink_protocol, i); - if (data->seq_number == seq_number) { - /* We potentially receive many parts partial responses for the same sequence number. - * Thus, we only remember the result, and collect it later. */ - if (data->seq_result < 0) { - /* we already saw an error for this sequence number. - * Preserve it. */ - } else if (seq_result != WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_UNKNOWN - || data->seq_result == WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN) - data->seq_result = seq_result; - if (data->out_extack_msg && !*data->out_extack_msg) - *data->out_extack_msg = g_strdup(extack_msg); - return; - } + if (data->seq_number != seq_number) + continue; + + /* We potentially receive many parts partial responses for the same sequence number. + * Thus, we only remember the result, and collect it later. */ + if (data->seq_result < 0) { + /* we already saw an error for this sequence number. + * Preserve it. */ + } else if (seq_result != WAIT_FOR_NL_RESPONSE_RESULT_RESPONSE_UNKNOWN + || data->seq_result == WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN) + data->seq_result = seq_result; + + if (extack_msg && data->out_extack_msg && !*data->out_extack_msg) + *data->out_extack_msg = g_strdup(extack_msg); + + return; } out: From 61388fd9c799ef92363ea73bde886a766157d1f3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 22 Feb 2023 11:27:48 +0100 Subject: [PATCH 6/9] platform: drop logging for unexpected sequence number It is not clear how that information is relevant. Since it is also only logged when building with a non-default configure option, this doesn't seem useful. Drop it. --- src/libnm-platform/nm-linux-platform.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 4423b75ddb..9a671d0856 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -482,9 +482,6 @@ typedef struct { typedef struct { guint32 nlh_seq_next; guint32 nlh_seq_last_seen; -#if NM_MORE_LOGGING - guint32 nlh_seq_last_handled; -#endif } NetlinkProtocolPrivData; typedef struct { @@ -7742,7 +7739,7 @@ event_seq_check(NMPlatform *platform, if (!NM_FLAGS_ANY( priv->delayed_action.flags, nmp_netlink_protocol_info(netlink_protocol)->delayed_action_type_wait_for_response)) - goto out; + return; nm_assert(priv->delayed_action.list_wait_for_response_x[netlink_protocol]->len > 0); @@ -7767,16 +7764,6 @@ event_seq_check(NMPlatform *platform, return; } - -out: - -#if NM_MORE_LOGGING - if (seq_number != priv->proto_data_x[netlink_protocol].nlh_seq_last_handled) - _LOGt("netlink: recvmsg: unwaited sequence number %u", seq_number); - priv->proto_data_x[netlink_protocol].nlh_seq_last_handled = seq_number; -#else - (void) 0; -#endif } static void From d755b50808eccbb4ae5e29895c23b4e41837dca1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 17 Feb 2023 12:07:05 +0100 Subject: [PATCH 7/9] platform: return extack message from add address/route operations --- src/core/nm-netns.c | 2 +- src/core/platform/nm-fake-platform.c | 30 ++++++++++----- src/core/platform/tests/test-cleanup.c | 4 +- src/core/platform/tests/test-common.c | 6 ++- src/core/platform/tests/test-route.c | 9 +++-- src/libnm-platform/nm-linux-platform.c | 20 ++++++---- src/libnm-platform/nm-platform.c | 50 +++++++++++++++++-------- src/libnm-platform/nm-platform.h | 23 +++++++++--- src/libnm-platform/nmp-global-tracker.c | 2 +- 9 files changed, 100 insertions(+), 46 deletions(-) diff --git a/src/core/nm-netns.c b/src/core/nm-netns.c index 12ca850898..dfdaaa06c7 100644 --- a/src/core/nm-netns.c +++ b/src/core/nm-netns.c @@ -815,7 +815,7 @@ nm_netns_ip_route_ecmp_commit(NMNetns *self, if (changed || is_reapply) { _LOGT("ecmp-route: multi-hop %s", nmp_object_to_string(route_obj, NMP_OBJECT_TO_STRING_PUBLIC, sbuf, sizeof(sbuf))); - nm_platform_ip_route_add(priv->platform, NMP_NLM_FLAG_APPEND, route_obj); + nm_platform_ip_route_add(priv->platform, NMP_NLM_FLAG_APPEND, route_obj, NULL); } } } diff --git a/src/core/platform/nm-fake-platform.c b/src/core/platform/nm-fake-platform.c index c92d9aef56..c5ecd90ddf 100644 --- a/src/core/platform/nm-fake-platform.c +++ b/src/core/platform/nm-fake-platform.c @@ -102,7 +102,9 @@ static gboolean ip6_address_add(NMPlatform *platform, struct in6_addr peer_addr, guint32 lifetime, guint32 preferred, - guint flags); + guint flags, + char **out_extack_msg); + static gboolean ip6_address_delete(NMPlatform *platform, int ifindex, struct in6_addr addr, guint8 plen); @@ -542,7 +544,7 @@ link_changed(NMPlatform *platform, nm_platform_cache_update_emit_signal(platform, cache_op, obj_old, device->obj); if (!IN6_IS_ADDR_UNSPECIFIED(&device->ip6_lladdr)) { - if (device->obj->link.connected) + if (device->obj->link.connected) { ip6_address_add(platform, device->obj->link.ifindex, device->ip6_lladdr, @@ -550,8 +552,9 @@ link_changed(NMPlatform *platform, in6addr_any, NM_PLATFORM_LIFETIME_PERMANENT, NM_PLATFORM_LIFETIME_PERMANENT, - 0); - else + 0, + NULL); + } else ip6_address_delete(platform, device->obj->link.ifindex, device->ip6_lladdr, 64); } @@ -865,7 +868,10 @@ mesh_set_ssid(NMPlatform *platform, int ifindex, const guint8 *ssid, gsize len) /*****************************************************************************/ static gboolean -ipx_address_add(NMPlatform *platform, int addr_family, const NMPlatformObject *address) +ipx_address_add(NMPlatform *platform, + int addr_family, + const NMPlatformObject *address, + char **out_extack_msg) { nm_auto_nmpobj NMPObject *obj = NULL; NMPCacheOpsType cache_op; @@ -874,6 +880,7 @@ ipx_address_add(NMPlatform *platform, int addr_family, const NMPlatformObject *a NMPCache *cache = nm_platform_get_cache(platform); g_assert(NM_IN_SET(addr_family, AF_INET, AF_INET6)); + g_assert(!out_extack_msg || !*out_extack_msg); obj = nmp_object_new(addr_family == AF_INET ? NMP_OBJECT_TYPE_IP4_ADDRESS : NMP_OBJECT_TYPE_IP6_ADDRESS, @@ -894,7 +901,8 @@ ip4_address_add(NMPlatform *platform, guint32 lifetime, guint32 preferred, guint32 flags, - const char *label) + const char *label, + char **out_extack_msg) { NMPlatformIP4Address address; @@ -914,7 +922,7 @@ ip4_address_add(NMPlatform *platform, if (label) g_strlcpy(address.label, label, sizeof(address.label)); - return ipx_address_add(platform, AF_INET, (const NMPlatformObject *) &address); + return ipx_address_add(platform, AF_INET, (const NMPlatformObject *) &address, out_extack_msg); } static gboolean @@ -925,7 +933,8 @@ ip6_address_add(NMPlatform *platform, struct in6_addr peer_addr, guint32 lifetime, guint32 preferred, - guint32 flags) + guint32 flags, + char **out_extack_msg) { NMPlatformIP6Address address; @@ -942,7 +951,7 @@ ip6_address_add(NMPlatform *platform, address.preferred = preferred; address.n_ifa_flags = flags; - return ipx_address_add(platform, AF_INET6, (const NMPlatformObject *) &address); + return ipx_address_add(platform, AF_INET6, (const NMPlatformObject *) &address, out_extack_msg); } static gboolean @@ -1092,7 +1101,7 @@ object_delete(NMPlatform *platform, const NMPObject *obj) } static int -ip_route_add(NMPlatform *platform, NMPNlmFlags flags, NMPObject *obj_stack) +ip_route_add(NMPlatform *platform, NMPNlmFlags flags, NMPObject *obj_stack, char **out_extack_msg) { NMDedupMultiIter iter; nm_auto_nmpobj NMPObject *obj = NULL; @@ -1114,6 +1123,7 @@ ip_route_add(NMPlatform *platform, NMPNlmFlags flags, NMPObject *obj_stack) g_assert(NM_IN_SET(NMP_OBJECT_GET_TYPE(obj_stack), NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE)); + g_assert(!out_extack_msg || !*out_extack_msg); addr_family = NMP_OBJECT_GET_ADDR_FAMILY(obj_stack); diff --git a/src/core/platform/tests/test-cleanup.c b/src/core/platform/tests/test-cleanup.c index 139a028001..c643e71c4a 100644 --- a/src/core/platform/tests/test-cleanup.c +++ b/src/core/platform/tests/test-cleanup.c @@ -77,6 +77,7 @@ test_cleanup_internal(void) lifetime, preferred, 0, + NULL, NULL)); g_assert(nm_platform_ip6_address_add(NM_PLATFORM_GET, ifindex, @@ -85,7 +86,8 @@ test_cleanup_internal(void) in6addr_any, lifetime, preferred, - flags)); + flags, + NULL)); nmtstp_ip4_route_add(NM_PLATFORM_GET, ifindex, NM_IP_CONFIG_SOURCE_USER, diff --git a/src/core/platform/tests/test-common.c b/src/core/platform/tests/test-common.c index a37982fc69..5a39d3aaf7 100644 --- a/src/core/platform/tests/test-common.c +++ b/src/core/platform/tests/test-common.c @@ -1809,7 +1809,8 @@ _ip_address_add(NMPlatform *platform, lifetime, preferred, flags, - label); + label, + NULL); } else { g_assert(label == NULL); success = nm_platform_ip6_address_add(platform, @@ -1819,7 +1820,8 @@ _ip_address_add(NMPlatform *platform, peer_address->addr6, lifetime, preferred, - flags); + flags, + NULL); } g_assert(success); } diff --git a/src/core/platform/tests/test-route.c b/src/core/platform/tests/test-route.c index bd8fdc271c..9aa21a9ac6 100644 --- a/src/core/platform/tests/test-route.c +++ b/src/core/platform/tests/test-route.c @@ -421,7 +421,8 @@ test_ip6_route(void) in6addr_any, NM_PLATFORM_LIFETIME_PERMANENT, NM_PLATFORM_LIFETIME_PERMANENT, - 0)); + 0, + NULL)); accept_signals(route_added, 0, 3); _wait_for_ipv6_addr_non_tentative(NM_PLATFORM_GET, 200, ifindex, 1, &pref_src); @@ -706,7 +707,8 @@ test_ip4_route_options(gconstpointer test_data) a->lifetime, a->preferred, a->n_ifa_flags, - a->label)); + a->label, + NULL)); if (a->peer_address == a->address) _wait_for_ipv4_addr_device_route(NM_PLATFORM_GET, 200, a->ifindex, a->address, a->plen); } @@ -878,7 +880,8 @@ test_ip6_route_options(gconstpointer test_data) addr[i].peer_address, addr[i].lifetime, addr[i].preferred, - addr[i].n_ifa_flags)); + addr[i].n_ifa_flags, + NULL)); } _wait_for_ipv6_addr_non_tentative(NM_PLATFORM_GET, 400, IFINDEX, addr_n, addr_in6); diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 9a671d0856..cf47fb35ee 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -8041,7 +8041,8 @@ static int do_add_addrroute(NMPlatform *platform, const NMPObject *obj_id, struct nl_msg *nlmsg, - gboolean suppress_netlink_failure) + gboolean suppress_netlink_failure, + char **out_extack_msg) { char sbuf1[NM_UTILS_TO_STRING_BUFFER_SIZE]; WaitForNlResponseResult seq_result = WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN; @@ -8049,6 +8050,7 @@ do_add_addrroute(NMPlatform *platform, int nle; char s_buf[256]; + nm_assert(!out_extack_msg || !*out_extack_msg); nm_assert(NM_IN_SET(NMP_OBJECT_GET_TYPE(obj_id), NMP_OBJECT_TYPE_IP4_ADDRESS, NMP_OBJECT_TYPE_IP6_ADDRESS, @@ -8064,6 +8066,7 @@ do_add_addrroute(NMPlatform *platform, nmp_object_to_string(obj_id, NMP_OBJECT_TO_STRING_ID, sbuf1, sizeof(sbuf1)), nm_strerror(nle), -nle); + NM_SET_OUT(out_extack_msg, g_steal_pointer(&extack_msg)); return -NME_PL_NETLINK; } @@ -9489,7 +9492,8 @@ ip4_address_add(NMPlatform *platform, guint32 lifetime, guint32 preferred, guint32 flags, - const char *label) + const char *label, + char **out_extack_msg) { NMPObject obj_id; nm_auto_nlmsg struct nl_msg *nlmsg = NULL; @@ -9509,7 +9513,7 @@ ip4_address_add(NMPlatform *platform, label); nmp_object_stackinit_id_ip4_address(&obj_id, ifindex, addr, plen, peer_addr); - return (do_add_addrroute(platform, &obj_id, nlmsg, FALSE) >= 0); + return (do_add_addrroute(platform, &obj_id, nlmsg, FALSE, out_extack_msg) >= 0); } static gboolean @@ -9520,7 +9524,8 @@ ip6_address_add(NMPlatform *platform, struct in6_addr peer_addr, guint32 lifetime, guint32 preferred, - guint32 flags) + guint32 flags, + char **out_extack_msg) { NMPObject obj_id; nm_auto_nlmsg struct nl_msg *nlmsg = NULL; @@ -9540,7 +9545,7 @@ ip6_address_add(NMPlatform *platform, NULL); nmp_object_stackinit_id_ip6_address(&obj_id, ifindex, &addr); - return (do_add_addrroute(platform, &obj_id, nlmsg, FALSE) >= 0); + return (do_add_addrroute(platform, &obj_id, nlmsg, FALSE, out_extack_msg) >= 0); } static gboolean @@ -9602,7 +9607,7 @@ ip6_address_delete(NMPlatform *platform, int ifindex, struct in6_addr addr, guin /*****************************************************************************/ static int -ip_route_add(NMPlatform *platform, NMPNlmFlags flags, NMPObject *obj_stack) +ip_route_add(NMPlatform *platform, NMPNlmFlags flags, NMPObject *obj_stack, char **out_extack_msg) { nm_auto_nlmsg struct nl_msg *nlmsg = NULL; @@ -9612,7 +9617,8 @@ ip_route_add(NMPlatform *platform, NMPNlmFlags flags, NMPObject *obj_stack) return do_add_addrroute(platform, obj_stack, nlmsg, - NM_FLAGS_HAS(flags, NMP_NLM_FLAG_SUPPRESS_NETLINK_FAILURE)); + NM_FLAGS_HAS(flags, NMP_NLM_FLAG_SUPPRESS_NETLINK_FAILURE), + out_extack_msg); } static gboolean diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index c80d964851..c4cd61cf47 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -3559,7 +3559,8 @@ nm_platform_ip4_address_add(NMPlatform *self, guint32 lifetime, guint32 preferred, guint32 flags, - const char *label) + const char *label, + char **out_extack_msg) { _CHECK_SELF(self, klass, FALSE); @@ -3569,6 +3570,7 @@ nm_platform_ip4_address_add(NMPlatform *self, g_return_val_if_fail(preferred <= lifetime, FALSE); g_return_val_if_fail(!label || strlen(label) < sizeof(((NMPlatformIP4Address *) NULL)->label), FALSE); + nm_assert(!out_extack_msg || !*out_extack_msg); if (_LOGD_ENABLED()) { char sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE]; @@ -3601,7 +3603,8 @@ nm_platform_ip4_address_add(NMPlatform *self, lifetime, preferred, flags, - label); + label, + out_extack_msg); } gboolean @@ -3612,7 +3615,8 @@ nm_platform_ip6_address_add(NMPlatform *self, struct in6_addr peer_address, guint32 lifetime, guint32 preferred, - guint32 flags) + guint32 flags, + char **out_extack_msg) { _CHECK_SELF(self, klass, FALSE); @@ -3620,6 +3624,7 @@ nm_platform_ip6_address_add(NMPlatform *self, g_return_val_if_fail(plen <= 128, FALSE); g_return_val_if_fail(lifetime > 0, FALSE); g_return_val_if_fail(preferred <= lifetime, FALSE); + nm_assert(!out_extack_msg || !*out_extack_msg); if (_LOGD_ENABLED()) { char sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE]; @@ -3640,8 +3645,15 @@ nm_platform_ip6_address_add(NMPlatform *self, nm_platform_ip6_dadfailed_set(self, ifindex, &address, FALSE); - return klass - ->ip6_address_add(self, ifindex, address, plen, peer_address, lifetime, preferred, flags); + return klass->ip6_address_add(self, + ifindex, + address, + plen, + peer_address, + lifetime, + preferred, + flags, + out_extack_msg); } gboolean @@ -4464,7 +4476,8 @@ next_plat:; NM_FLAGS_HAS(flags, NMP_IP_ADDRESS_SYNC_FLAGS_WITH_NOPREFIXROUTE) ? IFA_F_NOPREFIXROUTE : 0, - known_address->a4.label)) + known_address->a4.label, + NULL)) success = FALSE; } else { if (!nm_platform_ip6_address_add( @@ -4478,7 +4491,8 @@ next_plat:; (NM_FLAGS_HAS(flags, NMP_IP_ADDRESS_SYNC_FLAGS_WITH_NOPREFIXROUTE) ? IFA_F_NOPREFIXROUTE : 0) - | known_address->a6.n_ifa_flags)) + | known_address->a6.n_ifa_flags, + NULL)) success = FALSE; } } @@ -4975,7 +4989,8 @@ sync_route_add: r = nm_platform_ip_route_add(self, NMP_NLM_FLAG_APPEND | NMP_NLM_FLAG_SUPPRESS_NETLINK_FAILURE, - conf_o); + conf_o, + NULL); if (r < 0) { if (r == -EEXIST) { /* Don't fail for EEXIST. It's not clear that the existing route @@ -5085,7 +5100,8 @@ sync_route_add: r2 = nm_platform_ip_route_add(self, NMP_NLM_FLAG_APPEND | NMP_NLM_FLAG_SUPPRESS_NETLINK_FAILURE, - &oo); + &oo, + NULL); if (r2 < 0) { _LOG3D("route-sync: failure to add gateway IPv%c route: %s: %s", @@ -5250,7 +5266,7 @@ nm_platform_ip_route_normalize(int addr_family, NMPlatformIPRoute *route) } static int -_ip_route_add(NMPlatform *self, NMPNlmFlags flags, NMPObject *obj_stack) +_ip_route_add(NMPlatform *self, NMPNlmFlags flags, NMPObject *obj_stack, char **out_extack_msg) { char sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE]; int ifindex; @@ -5266,6 +5282,7 @@ _ip_route_add(NMPlatform *self, NMPNlmFlags flags, NMPObject *obj_stack) nm_assert(NM_IN_SET(NMP_OBJECT_GET_TYPE(obj_stack), NMP_OBJECT_TYPE_IP4_ROUTE, NMP_OBJECT_TYPE_IP6_ROUTE)); + nm_assert(!out_extack_msg || !*out_extack_msg); nm_assert(NMP_OBJECT_GET_TYPE(obj_stack) != NMP_OBJECT_TYPE_IP4_ROUTE || obj_stack->ip4_route.n_nexthops <= 1u || obj_stack->_ip4_route.extra_nexthops); @@ -5287,11 +5304,14 @@ _ip_route_add(NMPlatform *self, NMPNlmFlags flags, NMPObject *obj_stack) * is stack allocated (and the potential "extra_nexthops" array is * guaranteed to stay alive too). */ - return klass->ip_route_add(self, flags, obj_stack); + return klass->ip_route_add(self, flags, obj_stack, out_extack_msg); } int -nm_platform_ip_route_add(NMPlatform *self, NMPNlmFlags flags, const NMPObject *obj) +nm_platform_ip_route_add(NMPlatform *self, + NMPNlmFlags flags, + const NMPObject *obj, + char **out_extack_msg) { nm_auto_nmpobj const NMPObject *obj_keep_alive = NULL; NMPObject obj_stack; @@ -5309,7 +5329,7 @@ nm_platform_ip_route_add(NMPlatform *self, NMPNlmFlags flags, const NMPObject *o obj_stack._ip4_route.extra_nexthops = obj->_ip4_route.extra_nexthops; } - return _ip_route_add(self, flags, &obj_stack); + return _ip_route_add(self, flags, &obj_stack, out_extack_msg); } int @@ -5341,7 +5361,7 @@ nm_platform_ip4_route_add(NMPlatform *self, &extra_nexthops_free); } - return _ip_route_add(self, flags, &obj); + return _ip_route_add(self, flags, &obj, NULL); } int @@ -5350,7 +5370,7 @@ nm_platform_ip6_route_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformI NMPObject obj; nmp_object_stackinit(&obj, NMP_OBJECT_TYPE_IP6_ROUTE, (const NMPlatformObject *) route); - return _ip_route_add(self, flags, &obj); + return _ip_route_add(self, flags, &obj, NULL); } gboolean diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index 1cd9c6c6f9..39f8edabd8 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -1215,7 +1215,8 @@ typedef struct { guint32 lifetime, guint32 preferred_lft, guint32 flags, - const char *label); + const char *label, + char **out_extack_msg); gboolean (*ip6_address_add)(NMPlatform *self, int ifindex, struct in6_addr address, @@ -1223,7 +1224,8 @@ typedef struct { struct in6_addr peer_address, guint32 lifetime, guint32 preferred_lft, - guint32 flags); + guint32 flags, + char **out_extack_msg); gboolean (*ip4_address_delete)(NMPlatform *self, int ifindex, in_addr_t address, @@ -1234,7 +1236,11 @@ typedef struct { struct in6_addr address, guint8 plen); - int (*ip_route_add)(NMPlatform *self, NMPNlmFlags flags, NMPObject *obj_stack); + int (*ip_route_add)(NMPlatform *self, + NMPNlmFlags flags, + NMPObject *obj_stack, + char **out_extack_msg); + int (*ip_route_get)(NMPlatform *self, int addr_family, gconstpointer address, @@ -2130,7 +2136,8 @@ gboolean nm_platform_ip4_address_add(NMPlatform *self, guint32 lifetime, guint32 preferred_lft, guint32 flags, - const char *label); + const char *label, + char **out_extack_msg); gboolean nm_platform_ip6_address_add(NMPlatform *self, int ifindex, struct in6_addr address, @@ -2138,7 +2145,8 @@ gboolean nm_platform_ip6_address_add(NMPlatform *self, struct in6_addr peer_address, guint32 lifetime, guint32 preferred_lft, - guint32 flags); + guint32 flags, + char **out_extack_msg); gboolean nm_platform_ip4_address_delete(NMPlatform *self, int ifindex, in_addr_t address, @@ -2251,7 +2259,10 @@ nm_platform_ip_route_get_gateway(int addr_family, const NMPlatformIPRoute *route return &((NMPlatformIP6Route *) route)->gateway; } -int nm_platform_ip_route_add(NMPlatform *self, NMPNlmFlags flags, const NMPObject *route); +int nm_platform_ip_route_add(NMPlatform *self, + NMPNlmFlags flags, + const NMPObject *route, + char **out_extack_msg); int nm_platform_ip4_route_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatformIP4Route *route, diff --git a/src/libnm-platform/nmp-global-tracker.c b/src/libnm-platform/nmp-global-tracker.c index 3fd31e4e14..8f36c124ef 100644 --- a/src/libnm-platform/nmp-global-tracker.c +++ b/src/libnm-platform/nmp-global-tracker.c @@ -1101,7 +1101,7 @@ nmp_global_tracker_sync(NMPGlobalTracker *self, NMPObjectType obj_type, gboolean NMP_NLM_FLAG_ADD, NMP_OBJECT_CAST_ROUTING_RULE(obj_data->obj)); } else - nm_platform_ip_route_add(self->platform, NMP_NLM_FLAG_APPEND, obj_data->obj); + nm_platform_ip_route_add(self->platform, NMP_NLM_FLAG_APPEND, obj_data->obj, NULL); } } From eca8ebef185a539fc60b4ed09fd98558e50369a6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 22 Feb 2023 13:05:53 +0100 Subject: [PATCH 8/9] platform: get extack_msg innm_platform_ip_route_sync() Request the extack_msg for nm_platform_ip_route_add() call. Note that we (currently) don't do anything with it, however requesting it has no downsides. That is, the message already is heap allocated in the lower layers, so this only affects whether it will be returned up to nm_platform_ip_route_sync(). --- src/libnm-platform/nm-platform.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index c4cd61cf47..c9b7919a4c 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -4928,8 +4928,10 @@ nm_platform_ip_route_sync(NMPlatform *self, for (i_type = 0; routes && i_type < 2; i_type++) { for (i = 0; i < routes->len; i++) { - int r, r2; - gboolean gateway_route_added = FALSE; + gs_free char *extack_msg = NULL; + gboolean gateway_route_added = FALSE; + int r2; + int r; conf_o = routes->pdata[i]; @@ -4990,7 +4992,7 @@ sync_route_add: NMP_NLM_FLAG_APPEND | NMP_NLM_FLAG_SUPPRESS_NETLINK_FAILURE, conf_o, - NULL); + &extack_msg); if (r < 0) { if (r == -EEXIST) { /* Don't fail for EEXIST. It's not clear that the existing route From 9bb47d07d98a0c093c7f78abdf873aaf4a6fa5df Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 16 Feb 2023 18:27:32 +0100 Subject: [PATCH 9/9] platform: minor refactoring of temporary-not-available routes This will be used also for IPv4 addresses. Rename and make the function more generally useful. --- src/libnm-platform/nm-platform.c | 63 +++++++++++++++++++------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index c9b7919a4c..b1ac3132c0 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -4534,39 +4534,50 @@ nm_platform_ip_address_flush(NMPlatform *self, int addr_family, int ifindex) /*****************************************************************************/ static gboolean -_err_inval_due_to_ipv6_tentative_pref_src(NMPlatform *self, const NMPObject *obj) +_route_is_temporary_not_available(NMPlatform *self, + int err, + const NMPObject *obj, + const char **out_err_reason) { - const NMPlatformIP6Route *r; + const NMPlatformIPXRoute *rx; const NMPlatformIP6Address *a; nm_assert(NM_IS_PLATFORM(self)); nm_assert(NMP_OBJECT_IS_VALID(obj)); - /* trying to add an IPv6 route with pref-src fails, if the address is - * still tentative (rh#1452684). We need to hack around that. - * - * Detect it, by guessing whether that's the case. */ - - if (NMP_OBJECT_GET_TYPE(obj) != NMP_OBJECT_TYPE_IP6_ROUTE) + if (err != -EINVAL) return FALSE; - r = NMP_OBJECT_CAST_IP6_ROUTE(obj); + rx = NMP_OBJECT_CAST_IPX_ROUTE(obj); - /* we only allow this workaround for routes added manually by the user. */ - if (r->rt_source != NM_IP_CONFIG_SOURCE_USER) + if (rx->rx.rt_source != NM_IP_CONFIG_SOURCE_USER) { + /* we only allow this workaround for routes added manually by the user. */ return FALSE; + } - if (IN6_IS_ADDR_UNSPECIFIED(&r->pref_src)) + if (NMP_OBJECT_GET_TYPE(obj) == NMP_OBJECT_TYPE_IP4_ROUTE) { return FALSE; + } else { + const NMPlatformIP6Route *r = &rx->r6; - a = nm_platform_ip6_address_get(self, r->ifindex, &r->pref_src); - if (!a) - return FALSE; - if (!NM_FLAGS_HAS(a->n_ifa_flags, IFA_F_TENTATIVE) - || NM_FLAGS_HAS(a->n_ifa_flags, IFA_F_DADFAILED)) - return FALSE; + /* trying to add an IPv6 route with pref-src fails, if the address is + * still tentative (rh#1452684). We need to hack around that. + * + * Detect it, by guessing whether that's the case. */ - return TRUE; + if (IN6_IS_ADDR_UNSPECIFIED(&r->pref_src)) + return FALSE; + + a = nm_platform_ip6_address_get(self, r->ifindex, &r->pref_src); + if (!a) + return FALSE; + if (!NM_FLAGS_HAS(a->n_ifa_flags, IFA_F_TENTATIVE) + || NM_FLAGS_HAS(a->n_ifa_flags, IFA_F_DADFAILED)) + return FALSE; + + *out_err_reason = "tentative IPv6 src address not ready"; + return TRUE; + } } static guint @@ -4994,6 +5005,8 @@ sync_route_add: conf_o, &extack_msg); if (r < 0) { + const char *err_reason = NULL; + if (r == -EEXIST) { /* Don't fail for EEXIST. It's not clear that the existing route * is identical to the one that we were about to add. However, @@ -5032,15 +5045,15 @@ sync_route_add: sbuf1, sizeof(sbuf1)), nm_strerror(r)); - } else if (r == -EINVAL && out_temporary_not_available - && _err_inval_due_to_ipv6_tentative_pref_src(self, conf_o)) { - _LOG3D("route-sync: ignore failure to add IPv6 route with tentative IPv6 " - "pref-src: %s: %s", + } else if (out_temporary_not_available + && _route_is_temporary_not_available(self, r, conf_o, &err_reason)) { + _LOG3D("route-sync: ignore temporary failure to add route (%s, %s): %s", + nm_strerror(r), + err_reason, nmp_object_to_string(conf_o, NMP_OBJECT_TO_STRING_PUBLIC, sbuf1, - sizeof(sbuf1)), - nm_strerror(r)); + sizeof(sbuf1))); if (!*out_temporary_not_available) *out_temporary_not_available = g_ptr_array_new_full(0, (GDestroyNotify) nmp_object_unref);