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

[release/8.0] [NativeAOT] Missing memory fence before bulk move of objects #90941

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 22, 2023

Backport of #90890 to release/8.0

/cc @VSadov

Customer Impact

Per our memory model making objects available to other threads requires that all modifications to the object made prior to publishing are observable by other threads. This is normally guaranteed either by the strong memory ordering (i.e. x64) or via fences in GC barriers (ARM64).
The helper that performs bulk copying of objects is one of the scenarios that needs to ensure the invariant above and thus needs to have a fence on weak memory architectures. The NativeAOT version of the helper is missing the fence.
(similar helper on CoreCLR does have the fence).

The customer impact of the missing fence could be an occasional race or a GC hole on ARM64 in multithreaded apps. The frequency of the problem could depend on how far the memory weakness is exploited and thus hardware-dependent.

It is unclear if we have seen this kind of issues already and how often, since attributing such races to the cause could be challenging.
The issue was found by reviewing the code.

Testing

Regular tests indicate no regressions.

Risk

Very low. The added fence matches the pattern used in CoreCLR for many releases.

@ghost
Copy link

ghost commented Aug 22, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #90890 to release/8.0

/cc @VSadov

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. once you have a code review and green ci we can merge.

@jeffschwMSFT jeffschwMSFT added the Servicing-approved Approved for servicing release label Aug 22, 2023
@jeffschwMSFT jeffschwMSFT added this to the 8.0.0 milestone Aug 22, 2023
@VSadov VSadov requested a review from jkotas August 22, 2023 18:55
Copy link
Contributor

@IDisposable IDisposable left a comment

Choose a reason for hiding this comment

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

:shipit:

@VSadov VSadov force-pushed the backport/pr-90890-to-release/8.0 branch from 1b2acee to 84ef917 Compare August 22, 2023 23:50
@VSadov
Copy link
Member

VSadov commented Aug 22, 2023

It asked me to rebase the changes (after all tests have passed). Now it is running tests again.

@carlossanlop
Copy link
Member

It asked me to rebase the changes (after all tests have passed). Now it is running tests again.

Yes, unfortunately it's a protection that is causing confusion to a lot of people. If a rebase is not needed (for example, your change does not depend on something already merged), then the button should not be clicked, otherwise it restarts the CI. You can just ask me to merge it for you, I can bypass the protection.

@carlossanlop carlossanlop merged commit d4c6dd6 into release/8.0 Aug 23, 2023
76 of 105 checks passed
@carlossanlop carlossanlop deleted the backport/pr-90890-to-release/8.0 branch August 23, 2023 01:22
@VSadov
Copy link
Member

VSadov commented Aug 23, 2023

Makes sense.

I can bypass the protection.

I did not know about that. :-)

Thanks!!

@ghost ghost locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants