net: sockets: Cleanup socket properly if POSIX API is enabled

[jukkar]

The sock_obj_core_dealloc() was not called if close() is called instead of zsock_close(). This happens if POSIX API is enabled.

Fix this by calling zvfs_close() from zsock_close() and then pass the socket number to zsock_close_ctx() so that the cleanup can be done properly.

Reported-by: Andreas Ålgård [email protected]

Fixes #80997

[jukkar]

Using the ZVFS_MODE_IFSOCK mode flag for calling close2(void *, int) instead of close(int) is a bit problematic as that flag could be set for example in offloaded drivers (there are total 23 places in upstream alone setting this mode flag). Only couple of places out of those calls need to change to close2() where the socket descriptor is needed.
Also using this flag to determine whether we call close2 vs close is error prone because then out-of-tree drivers using this flag would not work any more without changes. So I think we cannot change the semantics of that flag here just like that.
If we could add new values for the mode list, then we could use that value to determine whether to call close2 vs close.

[jukkar]

• Introduced a new mode flag for the needed close2 call. We cannot really use the IFSOCK flag to determine if we need to call close2 as that could affect out of tree drivers (typically offloaded network drivers need to set these).

[cfriedt]

To me, this seems like it should be unnecessary

[jukkar]

I removed the extra flag, but this can be super confusing to a future developer looking at this code.
What I mean by this is that some of the networking code is now using close(void*) callback variant and some (currently sockets_inet.c, sockets_tls.c and sockets_packet.c) are using the close2(void*, int) callback variant in order to fix the resource leak. In both cases the fdtable will call the close2(void*, int) if IFSOCK is set but the 2nd parameter is ignored most of the cases (I would have guessed compiler to give a warning in this case). For a future self looking this code in couple of years, this might probably look like a bug rather than a feature 😕

[cfriedt]

I think there will be a fair bit of refactorization here between now and a couple of years from now, mainly because FS also needs to get a bump in the near term for PSE52.

So consumers of close() can be upgraded to close2(), and the older version can be deprecated. Additionally, that would be a good time to introduce something like a k_off_t type and deprecate the function signatures that use ssize_t and off_t.

[jukkar]

@icsys-aal I tested the code in this PR and did not spot any leaks, but if you have time, please try the PR to verify that your use case works too.

[cfriedt]

LGTM - thanks for this @jukkar

[icsys-aal]

@icsys-aal I tested the code in this PR and did not spot any leaks, but if you have time, please try the PR to verify that your use case works too.

Hello, I apologize for not getting back to you sooner. Your fix looks good, however in our use case we are using a strange offloaded device, which seems to return a nonzero return code on offload_put, which means this never runs and the leak persists. Should we call sock_obj_core_dealloc regardless of the return value?

[jukkar]

Your fix looks good, however in our use case we are using a strange offloaded device

@icsys-aal what offloaded driver is that? Perhaps there is a bug in it.

[icsys-aal]

@icsys-aal what offloaded driver is that? Perhaps there is a bug in it.

The driver in question is the winc1500 driver.

[jukkar]

however in our use case we are using a strange offloaded device, which seems to return a nonzero return code on offload_put, which means this never runs and the leak persists. Should we call sock_obj_core_dealloc regardless of the return value?

I briefly looked the Atmel HAL and its socket close implementation in
https://github.com/zephyrproject-rtos/hal_atmel/blob/56d60ebc909ad065bf6554cee73487969857614b/asf/common/components/wifi/winc1500/socket/source/socket.c#L908
and followed the call flow to hif_send() here
https://github.com/zephyrproject-rtos/hal_atmel/blob/56d60ebc909ad065bf6554cee73487969857614b/asf/common/components/wifi/winc1500/driver/source/m2m_hif.c#L273
According to the documentation there, the function should return 0 for successful operation. In the offloaded put function in

zephyr/drivers/wifi/winc1500/wifi_winc1500.c

Line 588 in d4b7bf9

static int winc1500_put(struct net_context *context)

we just return the value from the HAL.
I think we should do the cleanup in Zephyr socket implementation only if offloaded/native code is also ok with that (so the net_context_put returns 0). The offloaded driver could decide to return 0 if the HAL is giving some bogus value if we want to fix this properly.

[jukkar]

@mmahadevan108 / @dkalowsk Could we get this merged to 4.0?

[mmahadevan108]

@jukkar , waiting for approval from the PR assignees @nashif , @andyross