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.

Sign up for GitHub

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

Jump to bottom

DAOS-16931 container: fix oid iv race with using invalid on_update ptr #15714

Merged
merged 1 commit into from
Jan 12, 2025
Merged

DAOS-16931 container: fix oid iv race with using invalid on_update ptr #15714

merged 1 commit into from
Jan 12, 2025

Conversation

mchaarawi
Copy link
Contributor

the src pointer for on_update() callback is on the stack and should not be saved beyond that call to detect the same current request, because in some cases we can end up with 2 different requests sharing getting the same pointer value. Use the private pointer allocated by the oid iv on_alloc operation instead.

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

@mchaarawi
DAOS-16931 container: fix oid iv race with using invalid on update ptr
ff83047 
the src pointer for on_update() callback is on the stack and should not
be saved beyond that call to detect the same current request, because in
some cases we can end up with 2 different requests sharing getting the
same pointer value. Use the private pointer allocated by the oid iv
on_alloc operation instead.

Signed-off-by: Mohamad Chaarawi <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

Ticket title is 'oid IV assertion on server with mutex unlock'
Status is 'Open'
Labels: 'triaged'
https://daosio.atlassian.net/browse/DAOS-16931

@mchaarawi mchaarawi marked this pull request as ready for review January 9, 2025 23:25
@mchaarawi mchaarawi requested review from a team as code owners January 9, 2025 23:25
@mchaarawi mchaarawi requested review from liuxuezhao, wangdi1 and frostedcmos and removed request for a team January 9, 2025 23:25
@mchaarawi mchaarawi changed the title DAOS-16931 container: fix oid iv race with using invalid on update ptr DAOS-16931 container: fix oid iv race with using invalid on_update ptr Jan 9, 2025
frostedcmos
frostedcmos reviewed Jan 10, 2025
View reviewed changes
src/container/oid_iv.c
@@ -133,10 +134,10 @@ oid_iv_ent_update(struct ds_iv_entry *ns_entry, struct ds_iv_key *iv_key,
entry = ns_entry->iv_value.sg_iovs[0].iov_buf;
rc = ABT_mutex_trylock(entry->lock);
/** For retry requests, from _iv_op(), the lock may not be released in some cases. */
if (rc == ABT_ERR_MUTEX_LOCKED && entry->current_req != src)
if (rc == ABT_ERR_MUTEX_LOCKED && entry->current_req != priv)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double-check -- does on_get() implementation always return a valid priv or are there cases when priv returned can be NULL? (iv framework just passes whatever priv was filled with, we dont enforce it to be non-null). if latter, then two requests might have same null priv causing issues with this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the oid iv, the on get returns a valid ptr always. see here:
https://github.com/daos-stack/daos/pull/15714/files#diff-9396dfee0370217c952706fe6a29b126a7ec9f73f9d23078a7b82659bc94df41R214

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the "entry->current_req == priv" can never be true, as every time it will allocate a new one though oid_iv_ent_get(). So why need this check in the original place?

Copy link
Contributor Author

@mchaarawi mchaarawi Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure TBH. @wangdi1 had added this one a while back. according to wangdi's comment, on an IV retry operation, we can re-enter the on-update call. but are you saying on a retry, we are calling on_get() again and so we have a new priv pointer allocated?

Copy link
Contributor

@liuxuezhao liuxuezhao Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If the "retry" you mean here -
static int
_iv_op()'s
retry:
rc = iv_op_internal(ns, key, value, sync, shortcut, opc);

Then yes, I think the retry of iv_op_internal() -> crt_iv_update()/crt_iv_fetch(), it will cause the oid_iv_ent_get() be called in the path and provide a new priv pointer.
So the check "entry->current_req != priv" will be always true.

@wangdi1 is above correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

the original code was added by @wangdi1 's PR #13632
the original code's "src" pointer is ds_cont_oid_alloc_handler() -> cont_oid_alloc()'s sgl pointer,
using src instead of priv looks more reasonable to me, because the src will not be changed when retry the IV operation, but priv will be changed.

Copy link
Contributor Author

@mchaarawi mchaarawi Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

using src is just wrong.. it causes assertions. check the ticket.
src value is on the stack. a totally different request can get the same src pointer value.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm perhaps you may want to use "entry->current_req = src->sg_iovs[0].iov_buf"

while 'src' (temp iv value) can be on the stack, depending where iv calls it from, the underlying iov buffer will be provided by on_get(), so should remain valid until a corresponding on_put()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea that was my thought as well. thanks Alex, i'll post a follow on PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

"while 'src' (temp iv value) can be on the stack, depending where iv calls it from, the underlying iov buffer will be provided by on_get(),"
I think it is not true. let's discuss it more on the new PR #15727

frostedcmos
frostedcmos reviewed Jan 10, 2025
View reviewed changes
src/container/oid_iv.c
@@ -133,10 +134,10 @@ oid_iv_ent_update(struct ds_iv_entry *ns_entry, struct ds_iv_key *iv_key,
entry = ns_entry->iv_value.sg_iovs[0].iov_buf;
rc = ABT_mutex_trylock(entry->lock);
/** For retry requests, from _iv_op(), the lock may not be released in some cases. */
Copy link
Contributor

@frostedcmos frostedcmos Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

is it guaranteed that priv going to be the same for retry requests? i am not sure what daos stores in it or how it manages it across requests

Copy link
Contributor Author

@mchaarawi mchaarawi Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yes i believe so. the priv is allocated on the on-get CB. so it will not change on retry afaik.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15714/1/execution/node/1420/log

@frostedcmos frostedcmos self-requested a review January 10, 2025 17:32
wangdi1
wangdi1 approved these changes Jan 11, 2025
View reviewed changes
Copy link
Contributor

@wangdi1 wangdi1 left a comment

Choose a reason for hiding this comment

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

good catch!

@mchaarawi mchaarawi merged commit 6dda9d7 into master Jan 12, 2025
58 checks passed
@mchaarawi mchaarawi deleted the mschaara/16931 branch January 12, 2025 05:04
mchaarawi added a commit that referenced this pull request Jan 12, 2025
@mchaarawi
DAOS-16931 container: fix oid iv race with using invalid on update ptr (
ac60cab 
#15714)

the src pointer for on_update() callback is on the stack and should not
be saved beyond that call to detect the same request, because in
some cases we can end up with 2 different requests sharing getting the
same pointer value. Use the private pointer allocated by the oid iv
on_get operation instead.

Signed-off-by: Mohamad Chaarawi <[email protected]>
@mchaarawi mchaarawi mentioned this pull request Jan 12, 2025
Merged
18 tasks
@mchaarawi mchaarawi mentioned this pull request Jan 20, 2025
Closed
mchaarawi added a commit that referenced this pull request Jan 27, 2025
@mchaarawi
DAOS-16931 container: fix oid iv race with using invalid on update ptr (
594bc3f 
#15714)

the src pointer for on_update() callback is on the stack and should not
be saved beyond that call to detect the same request, because in
some cases we can end up with 2 different requests sharing getting the
same pointer value. Use the private pointer allocated by the oid iv
on_get operation instead.

Signed-off-by: Mohamad Chaarawi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frostedcmos frostedcmos frostedcmos approved these changes

@wangdi1 wangdi1 wangdi1 approved these changes

@liuxuezhao liuxuezhao Awaiting requested review from liuxuezhao

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mchaarawi @daosbuild1 @liuxuezhao @wangdi1 @frostedcmos