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

pkg/lwip: Add thread safety check when using DEVELHELP #16259

Merged
merged 2 commits into from
Jul 20, 2021

Conversation

yarrick
Copy link
Contributor

@yarrick yarrick commented Mar 30, 2021

Contribution description

When DEVELHELP is enabled, add checks that unprotected lwip functions are called correctly:

  • Not be called from interrupts.
  • If called from outside the tcpip thread, the lock must be held

The check is a macro to preserve the file/line of where it fired, to simplify debugging.

Adds locking to some calls in sock implementation to make it safe.
Having this enabled will make it harder to add new unsafe code.

Netif handling has already been made safe in the switch to netifapi functions for dhcp and netif link up/down.

Testing procedure

All lwip tests still work.

When calling dhcp_start instead of netifapi version in pkg/lwip/contrib/lwip.c, the following failure occurs (when IPv4 enabled) on native:
Assertion "Core lock held" failed at [...]/RIOT/build/pkg/lwip/src/core/ipv4/dhcp.c:742

Sending data with lwip sock still works when manually using test commands on tests/lwip

Issues/PRs references

None

@yarrick yarrick requested a review from miri64 as a code owner March 30, 2021 18:19
@benpicco benpicco added Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Mar 30, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@yarrick
Copy link
Contributor Author

yarrick commented Jul 9, 2021

Still looking for a review of this.

@miri64
Copy link
Member

miri64 commented Jul 12, 2021

Still looking for a review of this.

Sorry, was very busy the last few month. I will have a look ASAP!

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

The doc says LWIP_TCPIP_CORE_LOCKING needs to be set to 1 for this feature to be used. So wouldn't it be more sensible to make the #ifdef DEVELHELPs #if IS_ACTIVE(LWIP_TCPIP_CORE_LOCKING) instead and set LWIP_TCPIP_CORE_LOCKING == 1 when DEVELHELP is defined in lwipopts.h?

pkg/lwip/contrib/sys_arch.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Area: pkg Area: External package ports label Jul 13, 2021
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jul 15, 2021
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. I ran tests/lwip on native. The remaining tests should run on Murdock.

@miri64
Copy link
Member

miri64 commented Jul 15, 2021

Please squash

@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 20, 2021
@miri64 miri64 merged commit e1449e2 into RIOT-OS:master Jul 20, 2021
@yarrick yarrick deleted the safe_lwip branch July 29, 2021 17:33
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
@fjmolinas
Copy link
Contributor

I bisected the failing test on native(#17115 (comment)), I'm not sure if this PIR is to blame or if starting from this PR we have the mechanism to identify the underlying issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants