Skip to content

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

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
yarrick:safe_lwip
Jul 20, 2021
Merged

pkg/lwip: Add thread safety check when using DEVELHELP#16259
miri64 merged 2 commits intoRIOT-OS:masterfrom
yarrick:safe_lwip

Conversation

@yarrick
Copy link
Copy Markdown
Contributor

@yarrick yarrick commented Mar 30, 2021 •
edited
Loading

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
Copy Markdown
Contributor Author

yarrick commented Jul 9, 2021

Still looking for a review of this.

@miri64
Copy link
Copy Markdown
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!

miri64
miri64 reviewed Jul 13, 2021
View reviewed changes
Copy link
Copy Markdown
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?

@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
miri64
miri64 approved these changes Jul 15, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

fjmolinas commented Nov 5, 2021

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.

@fjmolinas fjmolinas mentioned this pull request Nov 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

Assignees

No one assigned

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

Milestone

Release 2021.10

Development

Successfully merging this pull request may close these issues.

5 participants

@yarrick @miri64 @fjmolinas @benpicco @MrKevinWeiss