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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/container/oid_iv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/**
* (C) Copyright 2017-2024 Intel Corporation.
* (C) Copyright 2025 Hewlett Packard Enterprise Development LP
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*/
Expand Down Expand Up @@ -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.

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

return -DER_BUSY;

entry->current_req = src;
entry->current_req = priv;
avail = &entry->rg;

oids = src->sg_iovs[0].iov_buf;
Expand Down
Loading