Skip to content

DAOS-17948 object: fix bugs for sgl duplicate logic - #16795

Merged
gnailzenh merged 5 commits into
masterfrom
shilongw/DAOS-17948
Sep 24, 2025
Merged

gnailzenh merged 5 commits into
masterfrom
shilongw/DAOS-17948

Conversation

@wangshilong

@wangshilong wangshilong commented Sep 2, 2025 •

Copy link
Copy Markdown
Contributor
  1. When IO retries occur, a use-after-free scenario may arise during
    scatter-gather list (sgl) merging. This happens because retried I/O's
    sgl references dup_sgls, which could be prematurely freed in obj_reasb_io_fini().

    To fix above issue, processed_sgls must not be freed during retry operations
    and should be called only once per I/O operation.

  2. If both short reads and duplicated SGLs occur, the iov_len of the original
    SGLs must be updated properly to ensure the correct number of read data bytes
    is returned.

  3. A test case has been added to verify that short reads function as expected during SGL merging.

Steps for the author:

  • Commit message follows the guidelines.
  • Appropriate Features or Test-tag pragmas were used.
  • Appropriate Functional Test Stages were run.
  • At least two positive code reviews including at least one code owner from each category referenced in the PR.
  • Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

  • Gatekeeper requested (daos-gatekeeper added as a reviewer).

@wangshilong
wangshilong requested review from a team September 2, 2025 01:49
@github-actions

github-actions Bot commented Sep 2, 2025 •

Copy link
Copy Markdown

Ticket title is 'Potential use-after-free for ec retry IO'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-17948

@wangshilong wangshilong changed the title DAOS-17948 object: fix potential use-after-free in EC retry path DAOS-17948 object: fix potential use-after-free in io retry path Sep 2, 2025
@daosbuild3

Copy link
Copy Markdown
Collaborator

1. When IO retries occur, a use-after-free scenario may arise during
scatter-gather list (sgl) merging. This happens because retried I/O's
sgl references dup_sgls, which could be prematurely freed in obj_reasb_io_fini().

To fix above issue, processed_sgls must not be freed during retry operations
and should be called only once per I/O operation.

2. If both short reads and duplicated SGLs occur, the iov_len of the original
SGLs must be updated properly to ensure the correct number of read data bytes
is returned.

3. Since only fetched data bytes are copied back, it is unnecessary to copy
the original buffer to the merged buffer before fetching.

4. A test case has been added to verify that short reads function as expected
during SGL merging.

Signed-off-by: Wang Shilong <shilong.wang@hpe.com>
@wangshilong wangshilong changed the title DAOS-17948 object: fix potential use-after-free in io retry path DAOS-17948 object: fix bugs for sgl duplicate logic Sep 3, 2025
@daosbuild3

Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Medium Verbs Provider MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16795/2/testReport/

Signed-off-by: Wang Shilong <shilong.wang@hpe.com>
@daosbuild3

Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Large MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16795/5/execution/node/1465/log

Comment thread src/object/cli_obj.c Outdated
Comment thread src/object/cli_obj.c
dup_buf_size -= iov->iov_buf_len;
dup_buf += iov->iov_buf_len;
D_ASSERT(dup_buf_size >= iov->iov_len);
memcpy((char *)iov->iov_buf, dup_buf, iov->iov_len);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some confused here, for read case, the output lov_len depend on the read result, right? Why not copy as much data as possible as long as the output buffer is large enough? something like that:

if (iov->iov_buf_len > dup_data_len)
   iov->iov_len = dup_data_len;
else
   iov->iov_len = iov->iov_buf_len;
memcpy((char *)iov->iov_buf, dup_buf, iov->iov_len);

On the other hand, it the application requires only copy specified length to the given buffer which real size is large than the required length (iov->iov_len < iov->iov_buf_len), then how can we guarantee that all the fetched data can be filled into the given SGL buffer? I may miss something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is what I currently follow: I think daos will not return more data than requested iov_len, iov_buf_len shall be equal or larger than iov_len, and when copying data back, real fetched data might be less than user requested iov_len. and it will not return more data then requested iov_len.

Signed-off-by: Wang Shilong <shilong.wang@hpe.com>
@daosbuild3

Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16795/8/execution/node/1379/log

@wangshilong

Copy link
Copy Markdown
Contributor Author

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16795/8/execution/node/1379/log

There is a failure https://daosio.atlassian.net/browse/DAOS-18011 I opened a ticket, it looks like a MD-on-SSD related bug.

Comment thread src/object/cli_obj.c
processed_sgls_free(ctx, obj_auxi->iod_nr);
obj_auxi->rw_args.merge_ctx = NULL;
api_args = dc_task_get_args(obj_auxi->obj_task);
api_args = dc_task_get_args(obj_auxi->obj_task);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not from this patch, this line is a dup

@gnailzenh
gnailzenh merged commit 5f2e416 into master Sep 24, 2025
42 of 44 checks passed
@gnailzenh
gnailzenh deleted the shilongw/DAOS-17948 branch September 24, 2025 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants