net: socket: packet: Fix memory leak

[jukkar]

The conn_raw_input() in connection.c will clone the incoming
packet so that it is possible to receive socket data in
multiple packet sockets. This is all fine except that if the
socket is never calling recv(), then the cloned net_pkt is never
processed and we will have a memory leak.

What this all means in practice, is that we should call recv()
to every packet socket in order to flush the socket for any
incoming data even if the socket is just sending data.

Fixes #34462

Signed-off-by: Jukka Rissanen jukka.rissanen@linux.intel.com

[mniestroj]

I have search through packet(7) and raw(7) man pages, as I was never using those before. I haven't found any logic about which socket (if multiple were created) should be able to receive data. Adding a logic that socket will be receiving data only after first recv() doesn't seem to be described/documented. As it is undefined (not documented), then current implementation of samples/net/sockets/packet/src/packet.c is also undefined (which #34462 was filed as a result). I think that this sample implementation should be reworked to use single socket to send and receive. But then, why should we receive on this socket data that we have just sent? Just wondering if this is a valid application use case and if the same can be implemented on Linux.

But still, I am no expert with AF_PACKET and stuff.

[jukkar]

I think that this sample implementation should be reworked to use single socket to send and receive.

The original problem was that if there was both AF_PACKET socket and normal UDP recv socket open in the system, the received data was only given to AF_PACKET socket. This is not good as both sockets should receive the data in this case. This issue was fixed in #30934

That fix lead to memory leak in packet socket sample as the received data was cloned to both sockets that are opened in the packet socket sample. Because the packet socket sample application was not reading the sending socket (why would it), the system was slowly running out of RX buffers. The fix is not to give received data to a socket that is only sending. The sample application is quite artificial but should still work just fine as nothing really prevents developer to use the socket API like this. It is actually good that the sample app caught this issue.
I will need to rework the checks a bit in next version, to avoid any race conditions.

[mniestroj]

The original problem was that if there was both AF_PACKET socket and normal UDP recv socket open in the system, the received data was only given to AF_PACKET socket. This is not good as both sockets should receive the data in this case.

Thanks for context. Is it specified somewhere in man pages of Linux socket or recv APIs, that all listening sockets should receive data? So far my feeling was that data should be received by only single socket at a time.

Because the packet socket sample application was not reading the sending socket (why would it), the system was slowly running out of RX buffers. The fix is not to give received data to a socket that is only sending.

So the question is how to make send-only socket. I didn't found information about that myself. My initial thought was to use bind() for that, similar as we do it for UDP sockets in

zephyr/subsys/net/lib/sockets/sockets.c

Lines 386 to 400 in 38126f9

	int zsock_bind_ctx(struct net_context *ctx, const struct sockaddr *addr,
	socklen_t addrlen)
	{
	SET_ERRNO(net_context_bind(ctx, addr, addrlen));
	/* For DGRAM socket, we expect to receive packets after call to
	* bind(), but for STREAM socket, next expected operation is
	* listen(), which doesn't work if recv callback is set.
	*/
	if (net_context_get_type(ctx) == SOCK_DGRAM) {
	SET_ERRNO(net_context_recv(ctx, zsock_received_cb, K_NO_WAIT,
	ctx->user_data));
	}
	return 0;
	}

.

The sample application is quite artificial but should still work just fine as nothing really prevents developer to use the socket API like this.

If all created AF_PACKET sockets are receiving by default, which is what I understand from Linux man pages, then application should be able to receive data on both sockets. This would be also in line with "This is not good as both sockets should receive the data in this case." that you have written above about AF_PACKET and UDP sockets. The fact that application does not call recv() on one of the socket and this results in using all the network packets, sounds like a desirable thing to do from network stack perspective.

Maybe calling recv() periodically to drain all waiting packets on a "sending" socket would be the way to "fix"? I mean we need a mechanism to say "do not receive any packets on this socket" and I think that this should be explicit, not implicit to the first call of recv().

[jukkar]

Is it specified somewhere in man pages of Linux socket or recv APIs, that all listening sockets should receive data? So far my feeling was that data should be received by only single socket at a time.

I have not see that but I think it is a common sense that if there is a socket listener, any data sent to it is received.

My initial thought was to use bind() for that, similar as we do it for UDP sockets in

bind() can be called for sending socket too

Maybe calling recv() periodically to drain all waiting packets on a "sending" socket would be the way to "fix"?

Edit: yep, reading the "sending" socket is the fix here.

I actually did it like this first but then realized that if application is only meant to send stuff, then it is not expected to call recv().

[jukkar]

Maybe calling recv() periodically to drain all waiting packets on a "sending" socket would be the way to "fix"?

I actually did it like this first but then realized that if application is only meant to send stuff, then it is not expected to call recv().

After debugging this more, I realized that we actually install receive callback when doing a bind. So if a bind is called for a socket, then it can always receive data. This means that we cannot actually check the net_context state value like I had in the patches.
To fix this memory leak, the only option seems to be to also read the sending socket in the application as it might get data too.

[jukkar]

Reworked the patch and made the packet socket sample to read the sending socket so that we do not leak any received data there.

[github-actions]

Unit Test Results

     10 files    41 suites   12m 30s ⏱️
1 279 tests 394 ✔️ 885 💤 0 ❌

Results for commit 81e38b8c.

[jukkar]

Updated according to comments.