[PATCH v2 1/3] Fix handling of netlink multipart route dumps.
This patch adds a `nl_collect` function to store all datagram chunks of a multipart route dump in one single interim buffer for further processing and dependency resolution. --- netlink.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 23 deletions(-) diff --git a/netlink.c b/netlink.c index 225b666..650a6fd 100644 --- a/netlink.c +++ b/netlink.c @@ -229,6 +229,51 @@ static struct nlmsghdr *nl_next(int s, char *buf, struct nlmsghdr *nh, ssize_t * /* NOLINTNEXTLINE(readability-inconsistent-ifelse-braces) */\ } else +/** + * nl_collect() - Collect all respose Datagrams of a netlink multi part dump + * @s: Netlink socket + * @buf: Buffer for responses (at least NLBUFSIZ long) + * @buflen: Buffer size. + * @seq: Sequence number of request we're getting responses for + * + * Return: Length of collected responses in bytes or error code < 0 + */ +int nl_collect(int s, char *buf, size_t buflen, uint32_t seq) +{ + size_t collected = 0; + struct nlmsghdr *nh; + ssize_t n; + int res; + const struct nlmsghdr* tail; + + #define NLMSG_DONE_SIZE NLMSG_LENGTH(4) + + while(collected + NLBUFSIZ < buflen){ + nh = nl_next(s, &buf[collected], NULL, &n); + debug("collect: add chunk of size n=%ld at offset=%ld, seq=%d nlmsg_len=%d", + n, collected, seq, ((const struct nlmsghdr *)&buf[collected])->nlmsg_len); + res = nl_status(nh, n, seq); + if (res < 0) /* error */ + return res; + collected += n; + + /* look for NLMSG_DONE -- it's ussualy transmitted in a seperate datagram, + * but in some rare cases also placed at the end of a multipart chunk.*/ + tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]; + if (res == 0 || ( + collected >= NLMSG_DONE_SIZE + && (tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]) + && NLMSG_OK(tail, NLMSG_DONE_SIZE) + && (nl_status(tail, NLMSG_DONE_SIZE, seq) == 0)) + ) { + debug("collected: %ld", collected); + return collected; + } + } + err("netlink: Too many routes to duplicate"); + return -E2BIG; +} + /** * nl_do() - Send netlink "do" request, and wait for acknowledgement * @s: Netlink socket @@ -553,21 +598,21 @@ int nl_route_dup(int s_src, unsigned int ifi_src, .rta.rta_len = RTA_LENGTH(sizeof(unsigned int)), .ifi = ifi_src, }; - ssize_t nlmsgs_size, left, status; + ssize_t nlmsgs_size, left, status = 0; unsigned dup_routes = 0; struct nlmsghdr *nh; - char buf[NLBUFSIZ]; + char buf[NLBUFSIZ * 8]; uint32_t seq; unsigned i; seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req)); - /* nl_foreach() will step through multiple response datagrams, - * which we don't want here because we need to have all the - * routes in the buffer at once. - */ - nh = nl_next(s_src, buf, NULL, &nlmsgs_size); - for (left = nlmsgs_size; + /* collect all the chunks of a multi part dump in one buffer.*/ + nlmsgs_size = nl_collect(s_src, buf, sizeof(buf), seq); + if (nlmsgs_size < 0) + return nlmsgs_size; + + for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size; NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0; nh = NLMSG_NEXT(nh, left)) { struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh); @@ -648,24 +693,12 @@ int nl_route_dup(int s_src, unsigned int ifi_src, dup_routes++; } - if (!NLMSG_OK(nh, left)) { - /* Process any remaining datagrams in a different - * buffer so we don't overwrite the first one. - */ - char tail[NLBUFSIZ]; - unsigned extra = 0; - - nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE) - extra++; - - if (extra) { - err("netlink: Too many routes to duplicate"); - return -E2BIG; - } - } + /* status should be 0 (=NLMSG_DONE) */ if (status < 0) return status; + debug("found %d routes in dump", dup_routes); + /* Routes might have dependencies between each other, and the kernel * processes RTM_NEWROUTE messages sequentially. For n routes, we might * need to send the requests up to n times to get all of them inserted. -- 2.53.0
Here's my review, sorry for the delay.
I have mostly coding style remarks on this patch, and more substantial
comments in the following patches.
Conceptually, though, this patch entirely makes sense to me.
On Tue, 8 Sep 2026 18:13:12 +0000
Martin Schitter
This patch adds a `nl_collect` function to store all datagram chunks of a multipart route dump in one single interim buffer for further processing and dependency resolution.
Here, and in following patches: missing Signed-off-by: tag (see CONTRIBUTING.md).
--- netlink.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 23 deletions(-)
diff --git a/netlink.c b/netlink.c index 225b666..650a6fd 100644 --- a/netlink.c +++ b/netlink.c @@ -229,6 +229,51 @@ static struct nlmsghdr *nl_next(int s, char *buf, struct nlmsghdr *nh, ssize_t * /* NOLINTNEXTLINE(readability-inconsistent-ifelse-braces) */\ } else
+/** + * nl_collect() - Collect all respose Datagrams of a netlink multi part dump
Nits: 'response', 'datagrams'.
+ * @s: Netlink socket + * @buf: Buffer for responses (at least NLBUFSIZ long) + * @buflen: Buffer size.
"Buffer size"
+ * @seq: Sequence number of request we're getting responses for + * + * Return: Length of collected responses in bytes or error code < 0
Exceedingly minor, but for consistency (I strongly believe in boring code and comments): "length", ", negative error code" (see e.g. nl_do()).
+ */ +int nl_collect(int s, char *buf, size_t buflen, uint32_t seq)
Cppcheck (try 'make cppcheck') says: netlink.c:242:5: style: The function 'nl_collect' should have static linkage since it is not used outside of its translation unit. [staticFunction] int nl_collect(int s, char *buf, size_t buflen, uint32_t seq) ^
+{ + size_t collected = 0;
Coding style: we use tabs, see CONTRIBUTING.md. It's essentially the same coding style as the Linux kernel with the "netdev" variants.
+ struct nlmsghdr *nh;
Cppcheck reports that: netlink.c:245:22: style: The scope of the variable 'nh' can be reduced. [variableScope] struct nlmsghdr *nh; ^ and also: netlink.c:245:22: style: Variable 'nh' can be declared as pointer to const [constVariablePointer] struct nlmsghdr *nh; ^
+ ssize_t n; + int res; + const struct nlmsghdr* tail;
...and we also align variable declarations from the longest to the shortest (see references in CONTRIBUTING.md). Coding style: "struct nlmsghdr *tail". Same here for these two: netlink.c:247:9: style: The scope of the variable 'res' can be reduced. [variableScope] int res; ^ netlink.c:248:28: style: The scope of the variable 'tail' can be reduced. [variableScope] const struct nlmsghdr* tail; ^
+ + #define NLMSG_DONE_SIZE NLMSG_LENGTH(4)
It's just a header with no data, right? Shouldn't it be 0? Am I missing something? By the way we tend to group all the defines that are generally valid for a compilation unit towards the beginning of the relevant file (so that they can be used in all the functions), see #define directives earlier in netlink.c. Exceptions are macros that are only relevant or valid for a given function (see e.g. tap6_handler() in tap.c).
+ + while(collected + NLBUFSIZ < buflen){
Coding style: space before {. It's not really idiomatic to "do something as long as we have space" and, with this structure, we won't stop on errors or mismatching sequence numbers (see nl_status()). Would there be a way to encode this as a loop on nl_status() perhaps? I haven't tried but it would look cleaner and less bug-prone.
+ nh = nl_next(s, &buf[collected], NULL, &n); + debug("collect: add chunk of size n=%ld at offset=%ld, seq=%d nlmsg_len=%d", + n, collected, seq, ((const struct nlmsghdr *)&buf[collected])->nlmsg_len);
At some point, we should get a netlink monitor (see https://bugs.passt.top/show_bug.cgi?id=141) which might run this function quite frequently at runtime, and having this as debug() message would risk flooding debug logs and making them otherwise unusable. We have rate-limited versions of logging functions, but they wouldn't help debugging this function specifically, because once rate-limiting kicks in, the messages might become entirely useless. I think we could consider trace(). My preference would actually to drop this altogether because it seems to be specific to some particular phase of your development and not something of general relevance. If we want to keep this for some reason: "collect" isn't very descriptive (mind that it will just be printed among other messages coming from completely different parts of the code). I would rather use __func__.
+ res = nl_status(nh, n, seq); + if (res < 0) /* error */ + return res;
Looping on nl_status() directly would get rid of this.
+ collected += n; + + /* look for NLMSG_DONE -- it's ussualy transmitted in a seperate datagram,
usually, separate
+ * but in some rare cases also placed at the end of a multipart chunk.*/
* [...] chunk. */
+ tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE];
Cppcheck and clang-tidy (try also 'make clang-tidy') aren't particularly impressed here. Cppcheck says: netlink.c:266:22: style: Variable 'tail' is reassigned a value before the old one has been used. [redundantAssignment] && (tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]) ^ netlink.c:263:14: note: tail is assigned tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]; ^ netlink.c:266:22: note: tail is overwritten && (tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]) ^ and clang-tidy, more clearly, states: /home/sbrivio/passt/netlink.c:263:9: error: Value stored to 'tail' is never read [clang-analyzer-deadcode.DeadStores,-warnings-as-errors] 263 | tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]; | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/sbrivio/passt/netlink.c:263:9: note: Value stored to 'tail' is never read 263 | tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]; | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ if (res == 0 || ( + collected >= NLMSG_DONE_SIZE + && (tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]) + && NLMSG_OK(tail, NLMSG_DONE_SIZE) + && (nl_status(tail, NLMSG_DONE_SIZE, seq) == 0))
For readability: could we perhaps split this into different 'if' conditions with gotos dealing with specific errors (for example if !NLMSG_OK(tail, NLMSG_DONE_SIZE), we should report error and return early I guess)?
+ ) { + debug("collected: %ld", collected);
Same comments as the debug() call above.
+ return collected; + } + } + err("netlink: Too many routes to duplicate"); + return -E2BIG; +} + /** * nl_do() - Send netlink "do" request, and wait for acknowledgement * @s: Netlink socket @@ -553,21 +598,21 @@ int nl_route_dup(int s_src, unsigned int ifi_src, .rta.rta_len = RTA_LENGTH(sizeof(unsigned int)), .ifi = ifi_src, }; - ssize_t nlmsgs_size, left, status; + ssize_t nlmsgs_size, left, status = 0; unsigned dup_routes = 0; struct nlmsghdr *nh; - char buf[NLBUFSIZ]; + char buf[NLBUFSIZ * 8];
Reasonable, but perhaps it should be a #define just after #define NLBUFSIZ. If a change in a variable declaration changes its length (as displayed), you should move it so that we keep the "reverse pyramid" look.
uint32_t seq; unsigned i;
seq = nl_send(s_src, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req));
- /* nl_foreach() will step through multiple response datagrams, - * which we don't want here because we need to have all the - * routes in the buffer at once. - */ - nh = nl_next(s_src, buf, NULL, &nlmsgs_size); - for (left = nlmsgs_size; + /* collect all the chunks of a multi part dump in one buffer.*/
/* Collect [...] buffer */
+ nlmsgs_size = nl_collect(s_src, buf, sizeof(buf), seq); + if (nlmsgs_size < 0) + return nlmsgs_size; + + for (nh = (struct nlmsghdr *)buf, left = nlmsgs_size; NLMSG_OK(nh, left) && (status = nl_status(nh, left, seq)) > 0; nh = NLMSG_NEXT(nh, left)) { struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh); @@ -648,24 +693,12 @@ int nl_route_dup(int s_src, unsigned int ifi_src, dup_routes++; }
- if (!NLMSG_OK(nh, left)) { - /* Process any remaining datagrams in a different - * buffer so we don't overwrite the first one. - */ - char tail[NLBUFSIZ]; - unsigned extra = 0; - - nl_foreach_oftype(nh, status, s_src, tail, seq, RTM_NEWROUTE) - extra++; - - if (extra) { - err("netlink: Too many routes to duplicate"); - return -E2BIG; - } - } + /* status should be 0 (=NLMSG_DONE) */
This comment is redundant: if status is not 0, then clearly we hit some kind of issue.
if (status < 0) return status;
+ debug("found %d routes in dump", dup_routes);
Same comments as debug() calls above.
+ /* Routes might have dependencies between each other, and the kernel * processes RTM_NEWROUTE messages sequentially. For n routes, we might * need to send the requests up to n times to get all of them inserted.
-- Stefano
On 9/17/26 21:08, Stefano Brivio wrote:
Here's my review, sorry for the delay.
Thanks for your really extensive review! I'll try to respect and solve most of your remarks in my next patch series. In the meanwhile I also did a lot of additional recherche. At the end of this replay you'll see, that I'll simply have to rewrite everything again because this new insights.
I have mostly coding style remarks on this patch, and more substantial comments in the following patches.
Please don't hesitate to modify and improve all these suggested patches anytime. It's often more efficient to simply correct little style issues and docstring flaws on the fly in a well configured development environment than communicating all needed changes. Even in cases where I don't share your view, simply change it to your needs.
Here, and in following patches: missing Signed-off-by: tag (see CONTRIBUTING.md).
o.k. I'll add it -- although I'm not convinced that it makes much sense in a modern jj based patch description workflow.
+ #define NLMSG_DONE_SIZE NLMSG_LENGTH(4)
It's just a header with no data, right? Shouldn't it be 0? Am I missing something?
4 is correct -- it stands for header size + 4*8bit (int) error code (see: https://docs.kernel.org/userspace-api/netlink/intro.html#netlink-message-typ...)
By the way we tend to group all the defines that are generally valid for a compilation unit towards the beginning of the relevant file (so that they can be used in all the functions), see #define directives earlier in netlink.c.
Exceptions are macros that are only relevant or valid for a given function (see e.g. tap6_handler() in tap.c).
I wouldn't move it. It is a helpful hint to easily grasp this particular code section and isn't very useful outside this scope.
+ + while(collected + NLBUFSIZ < buflen){
Coding style: space before {.
It's not really idiomatic to "do something as long as we have space" and, with this structure, we won't stop on errors or mismatching sequence numbers (see nl_status()).
Would there be a way to encode this as a loop on nl_status() perhaps? I haven't tried but it would look cleaner and less bug-prone.
Hmm -- I'm also not very happy about this solution, but for much less cosmetic reasons. I simply don't like this arbitrary set hard-coded memory resp. route entry capacity limit that doesn't correspond to any limitation given by the operating system. Well -- it's at least an improvement, but it doesn't solve the fundamental issue as long as it doesn't handle routing tables of any size. But within the given constraints of this code base (i.e. strict avoiding any dynamic memory allocation and not using a more memory save programming language) I don't see any better solution until now.
+ nh = nl_next(s, &buf[collected], NULL, &n); + debug("collect: add chunk of size n=%ld at offset=%ld, seq=%d nlmsg_len=%d", + n, collected, seq, ((const struct nlmsghdr *)&buf[collected])->nlmsg_len);
At some point, we should get a netlink monitor (see https://bugs.passt.top/show_bug.cgi?id=141) which might run this function quite frequently at runtime, and having this as debug() message would risk flooding debug logs and making them otherwise unusable.
You can eliminate all my debug print commands just as you like. But as long as we do not have suitable unit tests to control at least a subset of common cases in advance, debug logs could be very helpful to iron out remaining mistakes and false assumptions. btw.: I don't think you should run this kind of routines frequently. Kernel-userspace-transfers are rather expensive. Perhaps you could try to utilize eBPF to handle these route table duplication resp. transfer between namespaces in a more efficient manner.
I think we could consider trace(). My preference would actually to drop this altogether because it seems to be specific to some particular phase of your development and not something of general relevance.
That's o.k. for me. But please consider, that we do not have much control about many details of the actual kernel behavior in respect to this complex netlink multipart dumps. It can be rather challenging to reproduce and solve all related issues without useful debug log capabilities.
If we want to keep this for some reason: "collect" isn't very descriptive (mind that it will just be printed among other messages coming from completely different parts of the code). I would rather use __func__.
__func__ and similar more precise code location prefixes would IMHO make the output even more dense, confusing, and cluttering. But again: just change it however you like!
+ res = nl_status(nh, n, seq); + if (res < 0) /* error */ + return res;
Looping on nl_status() directly would get rid of this.
Respecting the capacity limits of the given interim buffer is IMHO the most important aspect of this function. That's why I have chosen this particular loop design. Nevertheless, we have to catch a few other less critical error conditions resp. return conditions as well.
+ tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE];
Cppcheck and clang-tidy (try also 'make clang-tidy') aren't particularly impressed here. Cppcheck says:
netlink.c:266:22: style: Variable 'tail' is reassigned a value before the old one has been used. [redundantAssignment]
Yes -- this line is indeed wrong. I'll replace it by a simple declaration for '*tail'.
+ if (res == 0 || ( + collected >= NLMSG_DONE_SIZE + && (tail = (const struct nlmsghdr*)&buf[collected - NLMSG_DONE_SIZE]) + && NLMSG_OK(tail, NLMSG_DONE_SIZE) + && (nl_status(tail, NLMSG_DONE_SIZE, seq) == 0))
For readability: could we perhaps split this into different 'if' conditions with gotos dealing with specific errors (for example if !NLMSG_OK(tail, NLMSG_DONE_SIZE), we should report error and return early I guess)?
It's a rather simple logical chain of tests to look out for NLMSG_DONE at the end of an input chunk if not already found in the previous nl_status() call. In both cases it will stop collecting datagrams. Additionally, it would catch an illegal pointer arithmetic condition, although this case should never happen in practice.
- char buf[NLBUFSIZ]; + char buf[NLBUFSIZ * 8];
Reasonable, but perhaps it should be a #define just after #define NLBUFSIZ.
o.k. -- I'll use a macro (NLBUFBLOCKS) for the buffsize multiplier.
participants (2)
-
Martin Schitter
-
Stefano Brivio