Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: sockets: services: Don't modify pollfd array from other threads #75525

Conversation

rlubos
Copy link
Contributor

@rlubos rlubos commented Jul 5, 2024

pollfd array used with zsock_poll() should not be modified while inside zsock_poll() function as this could lead to unexpected results. For instance, k_poll already monitoring some kernel primitive could report an event, but it will not be processed if the monitored socket file descriptor in the pollfd array was set to -1. In result, zsock_poll() may unexpectedly quit prematurely, returning 0 events, even if it was requested to wait infinitely.

The pollfd arrays used by zsock_poll() (ctx.events) is reinitialized when the service thread is restarted so modifying it directly when registering/unregistering service is not really needed. It's enough if those functions notify the eventfd socket used to restart the services thread.

Fixes #75474

pollfd array used with zsock_poll() should not be modified while inside
zsock_poll() function as this could lead to unexpected results. For
instance, k_poll already monitoring some kernel primitive could report
an event, but it will not be processed if the monitored socket file
descriptor in the pollfd array was set to -1. In result,
zsock_poll() may unexpectedly quit prematurely, returning 0 events, even
if it was requested to wait infinitely.

The pollfd arrays used by zsock_poll() (ctx.events) is reinitialized
when the service thread is restarted so modifying it directly when
registering/unregistering service is not really needed. It's enough if
those functions notify the eventfd socket used to restart the services
thread.

Signed-off-by: Robert Lubos <[email protected]>
@rlubos rlubos added this to the v3.7.0 milestone Jul 5, 2024
@pdgendt pdgendt requested a review from katgiadla July 5, 2024 15:02
jukkar added a commit to jukkar/zephyr that referenced this pull request Jul 7, 2024
As found in PR zephyrproject-rtos#75525, we should not modify the polled fd array
in multiple places. Because of this fix, the async version of
the socket service could start to trigger while it is being handled
by the async handler. This basically means that the async version
cannot work as intended so remove its support.

Signed-off-by: Jukka Rissanen <[email protected]>
Copy link
Collaborator

@katgiadla katgiadla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on Nordic targets

@aescolar aescolar merged commit e5d67ad into zephyrproject-rtos:main Jul 9, 2024
28 checks passed
aescolar pushed a commit that referenced this pull request Jul 10, 2024
As found in PR #75525, we should not modify the polled fd array
in multiple places. Because of this fix, the async version of
the socket service could start to trigger while it is being handled
by the async handler. This basically means that the async version
cannot work as intended so remove its support.

Signed-off-by: Jukka Rissanen <[email protected]>
AlienSarlak pushed a commit to AlienSarlak/zephyr that referenced this pull request Jul 13, 2024
As found in PR zephyrproject-rtos#75525, we should not modify the polled fd array
in multiple places. Because of this fix, the async version of
the socket service could start to trigger while it is being handled
by the async handler. This basically means that the async version
cannot work as intended so remove its support.

Signed-off-by: Jukka Rissanen <[email protected]>
CZKikin pushed a commit to nxp-upstream/zephyr that referenced this pull request Jul 18, 2024
As found in PR zephyrproject-rtos#75525, we should not modify the polled fd array
in multiple places. Because of this fix, the async version of
the socket service could start to trigger while it is being handled
by the async handler. This basically means that the async version
cannot work as intended so remove its support.

Signed-off-by: Jukka Rissanen <[email protected]>
Devansh0210 pushed a commit to Devansh0210/zephyr that referenced this pull request Jul 20, 2024
As found in PR zephyrproject-rtos#75525, we should not modify the polled fd array
in multiple places. Because of this fix, the async version of
the socket service could start to trigger while it is being handled
by the async handler. This basically means that the async version
cannot work as intended so remove its support.

Signed-off-by: Jukka Rissanen <[email protected]>
mpenate-ellenbytech pushed a commit to mpenate-ellenbytech/zephyr that referenced this pull request Aug 19, 2024
As found in PR zephyrproject-rtos#75525, we should not modify the polled fd array
in multiple places. Because of this fix, the async version of
the socket service could start to trigger while it is being handled
by the async handler. This basically means that the async version
cannot work as intended so remove its support.

Signed-off-by: Jukka Rissanen <[email protected]>
Chenhongren pushed a commit to Chenhongren/zephyr that referenced this pull request Aug 26, 2024
As found in PR zephyrproject-rtos#75525, we should not modify the polled fd array
in multiple places. Because of this fix, the async version of
the socket service could start to trigger while it is being handled
by the async handler. This basically means that the async version
cannot work as intended so remove its support.

(cherry picked from commit 794d7cf)

Original-Signed-off-by: Jukka Rissanen <[email protected]>
GitOrigin-RevId: 794d7cf
Change-Id: I049890a7827d3a03f245abf171877f4ab0304e15
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5691930
Tested-by: Keith Short <[email protected]>
Reviewed-by: Keith Short <[email protected]>
Commit-Queue: Keith Short <[email protected]>
Tested-by: ChromeOS Prod (Robot) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

net: socket: service: net.socket.service fails
6 participants