Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

[LibOS] test program for sigsuspend and alarm #452

Conversation

yamahata
Copy link
Contributor

@yamahata yamahata commented Feb 2, 2019

This test program shows race condition between thread_sleep() and
thread_wakeup() and DkThreadResume(thread->pal_handle).

With graphene, sigsuspend doesn't return or results in assert.

assert failed shim_syscalls.c:260 preempt == get_cur_preempt() (value:0)

Signed-off-by: Isaku Yamahata isaku.yamahata@gmail.com

Please fill in the following form before submitting this PR:

  • Affected components

    • README and global configurations
    • Linux PAL
    • SGX PAL
    • FreeBSD PAL
    • Library OS (i.e., SHIM), including GLIBC
  • A brief description of this PR (describe the reasons and measures)

  • How to test this PR?

    • Documentation-only; no need to test
      no need to test because this is a test program

Please follow the coding style guidelines. Do not submit PRs which violate these rules.

  • Comments and commit messages: no spelling or grammatical errors
  • 4 spaces per indentation level, at most 100 chars per line
  • Asterisks (*) next to types: int* pointer; (one pointer each line)
  • Braces ({) begin in the same line as function names, if, else, while, do, for, switch, union, and struct keywords.
  • Naming: Macros, constants - NAMED_THIS_WAY; global variables - g_named_this_way; others - named_this_way.
  • Other styling issues may be pointed out by reviewers.

Please preserve the following checklist for reviewing:

  • Pass all CI unit tests
  • Resolve all discussions/requests from reviewers
  • Reviewer approval (select one):
    • 3 approving reviews
    • 2 approving reviews and 5 days since the PR was created
    • The PR is from a maintainer; 1 approving review and 10 days since the PR was created

This change is Reviewable

This test program shows race condition between thread_sleep() and
thread_wakeup() and DkThreadResume(thread->pal_handle).

With graphene, sigsuspend doesn't return or results in assert.
> assert failed shim_syscalls.c:260 preempt == get_cur_preempt() (value:0)

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
@therealoscarbot
Copy link

Can one of the admins verify this patch?

2 similar comments
@therealoscarbot
Copy link

Can one of the admins verify this patch?

@therealoscarbot
Copy link

Can one of the admins verify this patch?

yamahata added a commit to yamahata/graphene that referenced this pull request Feb 2, 2019
There are race condition on thread_sleep() and DkThreadResume().
To avoid it, wake up the thread.
test program is can be found at
gramineproject#452.
NOTE: there are still other race conditions. this patch
only address one of them.

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
@yamahata
Copy link
Contributor Author

yamahata commented Feb 2, 2019

#453

yamahata added a commit to yamahata/graphene that referenced this pull request Feb 10, 2019
Now append_signal only send signal to the target thread.
There are race condition among PalEventWait() and PalThreadResume().
So before sending host signal by DkThreadResume(), wake up the thread
to avoid the race condition.

The test program is at gramineproject#452

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

It is unclear how to incorporate this test into regression suite: the data race is non-deterministic (it takes up to several minutes on my machine to trigger the race). I suggest to move this test case under LibOS/shim/test/native, so interested people can play with it.

Note: This data race is visible only in non-debug mode in manifest: loader.debug_type = none.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @yamahata)


LibOS/shim/test/regression/sigsuspend.c, line 52 at r1 (raw file):

        struct timespec t = {
            .tv_sec = 1,

If we change from 1 to say 5, the data race disappears but Graphene-SGX hangs deterministically. This behavior is explained in #453 (comment). We can implement a regression test by changing this line. This bug will require a separate fix (again, see the link).

Copy link
Contributor

@donporter donporter left a comment

Choose a reason for hiding this comment

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

Thanks for the test case. This also needs a python wrapper that can automatically run this test as part of 'make regression' and tell if there is a failure or success. Since this is a separate PR from the fix, my suggestion would be to go ahead and write the test and have it fail, but mask the error code until the fix lands.

After reading @dimakuv's comments, I think he is advocating we do some other fixes first.

Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @yamahata)

yamahata added a commit to yamahata/graphene that referenced this pull request Mar 22, 2019
Now append_signal only send signal to the target thread.
There are race condition among PalEventWait() and PalThreadResume().
So before sending host signal by DkThreadResume(), wake up the thread
to avoid the race condition.

The test program is at gramineproject#452

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
yamahata added a commit to yamahata/graphene that referenced this pull request Mar 26, 2019
Now append_signal only send signal to the target thread.
There are race condition among PalEventWait() and PalThreadResume().
So before sending host signal by DkThreadResume(), wake up the thread
to avoid the race condition.

The test program is at gramineproject#452

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
donporter pushed a commit to yamahata/graphene that referenced this pull request Mar 29, 2019
There are race condition on thread_sleep() and DkThreadResume().
To avoid it, wake up the thread.
test program is can be found at
gramineproject#452.
NOTE: there are still other race conditions. this patch
only address one of them.

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
yamahata added a commit to yamahata/graphene that referenced this pull request Mar 29, 2019
Now append_signal only send signal to the target thread.
There are race condition among PalEventWait() and PalThreadResume().
So before sending host signal by DkThreadResume(), wake up the thread
to avoid the race condition.

The test program is at gramineproject#452

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
donporter pushed a commit that referenced this pull request Apr 1, 2019
There are race condition on thread_sleep() and DkThreadResume().
To avoid it, wake up the thread.
test program is can be found at
#452.
NOTE: there are still other race conditions. this patch
only address one of them.

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
yamahata added a commit to yamahata/graphene that referenced this pull request Apr 5, 2019
Now append_signal only send signal to the target thread.
There are race condition among PalEventWait() and PalThreadResume().
So before sending host signal by DkThreadResume(), wake up the thread
to avoid the race condition.

The test program is at gramineproject#452

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
yamahata added a commit to yamahata/graphene that referenced this pull request Apr 19, 2019
Now append_signal only send signal to the target thread.
There are race condition among PalEventWait() and PalThreadResume().
So before sending host signal by DkThreadResume(), wake up the thread
to avoid the race condition.

The test program is at gramineproject#452

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
yamahata added a commit to yamahata/graphene that referenced this pull request Jul 15, 2019
Now append_signal only send signal to the target thread.
There are race condition among PalEventWait() and PalThreadResume().
So before sending host signal by DkThreadResume(), wake up the thread
to avoid the race condition.

The test program is at gramineproject#452

Signed-off-by: Isaku Yamahata <isaku.yamahata@gmail.com>
@stefanberger
Copy link
Contributor

Would we still need this test case now since we have LTP's rt_sigsuspend01 and sigsuspend01?

@dimakuv
Copy link

dimakuv commented Jun 12, 2020

I would suggest to close this PR. @yamahata What do you think?

@dimakuv
Copy link

dimakuv commented Jul 15, 2020

This PR is outdated, the tests work correctly on current master.

@dimakuv dimakuv closed this Jul 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants