net: sockets: Update msg_controllen in recvmsg properly

[decsny]

If adding any control data like timestamping or pkt_info fails, the msg_controllen wasn't properly cleared.
This commit aims to fix this by clearing the msg_controllen only when adding control data is successful.

Fixes #77303

Author: @axelnxp
(submitted on behalf because of some nxp github policies delaying his ability to post a PR)

[axelnxp]

The change from this PR is causing a regression is some socket tests.
I tested on native_sim and I confirmed the change is the culprit.

when running run_ancillary_recvmsg_test, after the change, the second call to comm_sendmsg_recvmsg returns a server_msg with no control data, so it looks like we clear msg_controllen but we shouldn't in this case.

@jukkar @awojasinski what do you think?

[axelnxp]

comm_sendmsg_recvmsg seems to be re-using the cmsgbuf without clearing it.
@jukkar is this a valid use of cmsgbuf ? I would expect the caller to clear it before calling recvmsg.

I tried to clear the buffer, and the test can pass.

I push it so you can review.

[jukkar]

I tried to find answer when the msg_controllen should be cleared.
The https://www.man7.org/linux/man-pages/man3/cmsg.3.html says

Finally, the msg_controllen field of the msghdr
should be set to the sum of the CMSG_SPACE() of the length of all
control messages in the buffer. 

Also recvmsg(2) says

 When  recvmsg() is called, msg_controllen should contain the length of the available buffer in msg_control; upon return from a successful call it will contain the length of the control message sequence.

So if I am reading this correctly, if the call is successfull, we should not clear the value.
The original code cleared the controllen when there was no control data involved. The commit ee06da5 added more socket option checks but now it looks like the change was not correct.
With this in mind, I did following change and the tests started to pass. Please take a look if this looks sane.

diff --git a/subsys/net/lib/sockets/sockets.c b/subsys/net/lib/sockets/sockets.c
index b61683b0eec..c0ce03e1a16 100644
--- a/subsys/net/lib/sockets/sockets.c
+++ b/subsys/net/lib/sockets/sockets.c
@@ -1572,27 +1572,28 @@ static inline ssize_t zsock_recv_dgram(struct net_context *ctx,
 	if (msg != NULL) {
 		if (msg->msg_control != NULL) {
 			if (msg->msg_controllen > 0) {
-				bool clear_controllen = true;
+				bool clear_controllen = false;
 
 				if (IS_ENABLED(CONFIG_NET_CONTEXT_TIMESTAMPING)) {
 					if (add_timestamping(ctx, pkt, msg) < 0) {
 						msg->msg_flags |= ZSOCK_MSG_CTRUNC;
-					} else {
-						clear_controllen = false;
 					}
+				} else {
+					clear_controllen = true;
 				}
 
 				if (IS_ENABLED(CONFIG_NET_CONTEXT_RECV_PKTINFO) &&
 				    net_context_is_recv_pktinfo_set(ctx)) {
+					clear_controllen = false;
 					if (add_pktinfo(ctx, pkt, msg) < 0) {
 						msg->msg_flags |= ZSOCK_MSG_CTRUNC;
-					} else {
-						clear_controllen = false;
 					}
+				} else {
+					clear_controllen = true;
 				}
 
 				if (clear_controllen) {
-					msg->msg_controllen = 0;
+					msg->msg_controllen = 0U;
 				}
 			}
 		} else {

[jukkar]

comm_sendmsg_recvmsg seems to be re-using the cmsgbuf without clearing it. @jukkar is this a valid use of cmsgbuf ? I would expect the caller to clear it before calling recvmsg.

I tried to clear the buffer, and the test can pass.

Indeed, that could also be a problem.

[axelnxp]

I tried to find answer when the msg_controllen should be cleared. The https://www.man7.org/linux/man-pages/man3/cmsg.3.html says

Finally, the msg_controllen field of the msghdr
should be set to the sum of the CMSG_SPACE() of the length of all
control messages in the buffer. 

Also recvmsg(2) says

 When  recvmsg() is called, msg_controllen should contain the length of the available buffer in msg_control; upon return from a successful call it will contain the length of the control message sequence.

So if I am reading this correctly, if the call is successfull, we should not clear the value. The original code cleared the controllen when there was no control data involved. The commit ee06da5 added more socket option checks but now it looks like the change was not correct. With this in mind, I did following change and the tests started to pass. Please take a look if this looks sane.

diff --git a/subsys/net/lib/sockets/sockets.c b/subsys/net/lib/sockets/sockets.c
index b61683b0eec..c0ce03e1a16 100644
--- a/subsys/net/lib/sockets/sockets.c
+++ b/subsys/net/lib/sockets/sockets.c
@@ -1572,27 +1572,28 @@ static inline ssize_t zsock_recv_dgram(struct net_context *ctx,
 	if (msg != NULL) {
 		if (msg->msg_control != NULL) {
 			if (msg->msg_controllen > 0) {
-				bool clear_controllen = true;
+				bool clear_controllen = false;
 
 				if (IS_ENABLED(CONFIG_NET_CONTEXT_TIMESTAMPING)) {
 					if (add_timestamping(ctx, pkt, msg) < 0) {
 						msg->msg_flags |= ZSOCK_MSG_CTRUNC;
-					} else {
-						clear_controllen = false;
 					}
+				} else {
+					clear_controllen = true;
 				}
 
 				if (IS_ENABLED(CONFIG_NET_CONTEXT_RECV_PKTINFO) &&
 				    net_context_is_recv_pktinfo_set(ctx)) {
+					clear_controllen = false;
 					if (add_pktinfo(ctx, pkt, msg) < 0) {
 						msg->msg_flags |= ZSOCK_MSG_CTRUNC;
-					} else {
-						clear_controllen = false;
 					}
+				} else {
+					clear_controllen = true;
 				}
 
 				if (clear_controllen) {
-					msg->msg_controllen = 0;
+					msg->msg_controllen = 0U;
 				}
 			}
 		} else {

Are you sure this patch satisfies the following statement:

Finally, the msg_controllen field of the msghdr
should be set to the sum of the CMSG_SPACE() of the length of all
control messages in the buffer. 

I might be missing something, but I don't see where the sum of CMSG_SPACE is computed and set to msg_controllen.
I think with your patch, msg_controllen stays at the input value, but it should actually be the length of the control data sequence, which might be different (inferior or equal to the input len).

Maybe before returning from recvmsg, we should go through all the cmsg buffer and compute the sum of the different control data length.

Am I missing something ?

[jukkar]

Maybe before returning from recvmsg, we should go through all the cmsg buffer and compute the sum of the different control data length.

Yes, that is missing. The original code had only support for one ancillary message, and the caller should have been allocating space for it, but now there could be two so we should update the controllen correctly.

The fix needs more work

[axelnxp]

Maybe before returning from recvmsg, we should go through all the cmsg buffer and compute the sum of the different control data length.

Yes, that is missing. The original code had only support for one ancillary message, and the caller should have been allocating space for it, but now there could be two so we should update the controllen correctly.

I'll cook up something in my tomorrow, I got a rough patch which makes things pass and should correctly update the controllen, will keep you updated.

[axelnxp]

Fix updated, I updated insert_pktinfo to compute the proper msg_controllen when control data are added to the buffer.
I chose to implement this in this function to take advantage of the for loop which searches for available space in the buffer, this should be more optimized than adding the same loop after calling add_pktinfo in recvmsg.

With this version of the fix, the unit test are passing on my side, will get the full confirmation with the PR checks.

[awojasinski]

please fix compliance check errors

[axelnxp]

In last push I fixed the compliance checks and also pushed an update I forgot to add in the patch (the test would fail without it)

[jukkar]

LGTM

[axelnxp]

after some more thought, I think this clear_controllen logic is not working well.
If we have both TIMESTAMPING and RECV_PKTINFO enabled, we might end up in some corner cases when we clear msg_controllen even if we shouldn't.
The only safe way to implement this seems to compute the control data length in this function

[axelnxp]

as an example for my previous comment, if we have both timestamping and pkt_info enabled, we can have a case where we add timestamping info, then there are no pkt_info to add so we go to this branch and we would clear the control data, including the timestamping.

[axelnxp]

In the last push, I reworked the whole fix and now I think it covers all cases.
The idea was to handle the following cases:

• Combination of TIMESTAMPING and RECV_PKTINFO enabled/disabled
• Combination of TIMESTAMPING and RECV_PKTINFO present or not in the context
• Pre-exisiting control data in the control data buffer. The recvmsg man page doesn't exclude cases where the user reuses the same control data buffer between calls to recvmsg, so in those case, msg_controllen must be updated while taking into account pre-existing data in the buffer

To do so, I implemented next_context_is_timestamping_set to use the same logic than recv_pktinfo (please let me know if this is valid). This avoids calling add_timestamping which would return -ENOTSUP in some cases, but this wasn't handled correctly (we couldn't know if control_data was added or not).

I also implemented update_msg_controllen as an helper. This function goes through the control data buffer to compute the total length.
To me it was the only way to handle cases where the user re-uses the same control data buffer between calls to recvmsg, meaning if recvmsg doesn't add control data, then you can't set msg_controllen to 0 if control data are already present in the buffer, so you have to iterate through each data anyway.

[jukkar]

To me it was the only way to handle cases where the user re-uses the same control data buffer between calls to recvmsg, meaning if recvmsg doesn't add control data, then you can't set msg_controllen to 0 if control data are already present in the buffer, so you have to iterate through each data anyway.

I don't think this is a valid use case, what I mean by that is that the control data is always tied to the data that we just received, so the user should read all the control data after each recvmsg because it would be useless in the next recvmsg call.

[axelnxp]

To me it was the only way to handle cases where the user re-uses the same control data buffer between calls to recvmsg, meaning if recvmsg doesn't add control data, then you can't set msg_controllen to 0 if control data are already present in the buffer, so you have to iterate through each data anyway.

I don't think this is a valid use case, what I mean by that is that the control data is always tied to the data that we just received, so the user should read all the control data after each recvmsg because it would be useless in the next recvmsg call.

Ok that makes sense. But who is responsible to clear the control data buffer ? The user or recvmsg ? Couldn't find the answer.

[jukkar]

But who is responsible to clear the control data buffer ? The user or recvmsg ? Couldn't find the answer.

I think it can be only the caller, it only knows what to do with the data. So if we assume that, then your code is working fine by not overriding the control data.

[axelnxp]

I think it can be only the caller, it only knows what to do with the data. So if we assume that, then your code is working fine by not overriding the control data.

Then I think we need to update the unit tests as the control data buffer is not always cleared between calls to recvmsg.