From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=mur.at Authentication-Results: passt.top; dkim=pass (3072-bit key; unprotected) header.d=mur.at header.i=@mur.at header.a=rsa-sha256 header.s=dkim2 header.b=rIydt5+T; dkim-atps=neutral Received: from efeu.mur.at (efeu.mur.at [IPv6:2a02:3e0::42]) by passt.top (Postfix) with ESMTPS id 4A5555A0274 for ; Sat, 19 Sep 2026 14:56:38 +0200 (CEST) Received: from [192.168.4.117] (lan1.raspi.ma39.ffgraz.net [10.12.1.243]) by efeu.mur.at (Postfix) with ESMTPSA id B6F3C46080; Sat, 19 Sep 2026 14:56:36 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mur.at; s=dkim2; t=1789822596; bh=2FI56rzlPX11XVn+1HD7C5ZhjywgrReUQtLMI99ZFYA=; h=Date:From:Subject:To:Cc:References:In-Reply-To:From; b=rIydt5+TIh0cUkW59PWjhM2PMbUUo7+5U6476jM6c/MpEhY9dKLMTOey7opS944aY GTRVgRGKXlZoUKdxrcH9kQ+V7S3cx7yNcnHNrYsMTRn1XQvAlADXEVE3W55XZCKPHh 4avRMDj0orfRqMu3P60oYuKB5426OhFqOlHaJaj0fx/OJ0zMLcOlsyveg5sFrb3fBu o+qY/T8PQN+SQ72ul0Sdm8czRBGjYkI/+D0DErX8W5h2gxjTLrUPKF9eFraR+ZlN8L t8/a7x1T0U+TkhffHEHOy7W1QjHY++j0o1M6Cl7yCR5THwdDwrYM/kSUXzhF1cl08M vOy56EhZ0J0D4s8ZthfRMrRUKIM0yVpzRXLbaAdjvQRLYqZ4+Sg+lY1VlprVIu+wlM +2qdVEuw3Nbr/x0q6fYjeIOlQ5XWrKv7/mDOCfiXTYNpJjgzX1VRgem1ZekTgg8QGG xsJwrSANL8/Ia/j+4xPcRqG4TvPO1zbDF8Kh941Z/07gpi+sbG9 Message-ID: Date: Sat, 19 Sep 2026 14:56:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Martin Schitter Subject: Re: [PATCH v2 1/3] Fix handling of netlink multipart route dumps. To: Stefano Brivio References: <20260908181631.537802-1-ms+git@mur.at> <20260908181631.537802-2-ms+git@mur.at> <20260917210812.44e10453@elisabeth> Content-Language: en-US In-Reply-To: <20260917210812.44e10453@elisabeth> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-MailFrom: ms+git@mur.at X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation Message-ID-Hash: YN5GJENN2LALEROWFVX5DCEIIUEOLNHG X-Message-ID-Hash: YN5GJENN2LALEROWFVX5DCEIIUEOLNHG X-Mailman-Approved-At: Sun, 20 Sep 2026 11:27:02 +0200 CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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-types) > 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.