From 58123b479a192ad8a2c00861d027c634db6cd66a Mon Sep 17 00:00:00 2001 From: Wang Shilong Date: Wed, 3 Sep 2025 12:17:51 +0800 Subject: [PATCH 1/3] DAOS-17948 object: fix bugs for sgl duplicate logic 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 --- src/object/cli_obj.c | 46 +++++++++++-------- src/tests/suite/daos_obj_array.c | 78 ++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 18 deletions(-) diff --git a/src/object/cli_obj.c b/src/object/cli_obj.c index d2ff2f951c1..cabbe7c5fc1 100644 --- a/src/object/cli_obj.c +++ b/src/object/cli_obj.c @@ -4846,17 +4846,19 @@ obj_sgls_dup(struct obj_auxi_args *obj_auxi, daos_obj_update_t *args, bool updat setbits64(ctx.alloc_bitmaps[i], sgl_idx, 1); /* Copy data from original IOVs to merged buffer */ - offset = 0; - for (k = merge_start; k < j; k++) { - uint64_t merge_len; + if (update) { + offset = 0; + for (k = merge_start; k < j; k++) { + uint64_t merge_len; - iov = &sg->sg_iovs[k]; - if (skip_sgl_iov(update, iov)) - continue; - D_ASSERT(offset < merged_buf_size); - merge_len = update ? iov->iov_len : iov->iov_buf_len; - memcpy(merged_buf + offset, iov->iov_buf, merge_len); - offset += merge_len; + iov = &sg->sg_iovs[k]; + if (skip_sgl_iov(update, iov)) + continue; + D_ASSERT(offset < merged_buf_size); + merge_len = update ? iov->iov_len : iov->iov_buf_len; + memcpy(merged_buf + offset, iov->iov_buf, merge_len); + offset += merge_len; + } } /* Create merged IOV entry */ @@ -4910,6 +4912,7 @@ obj_dup_sgls_free(struct obj_auxi_args *obj_auxi) d_sg_list_t *sg_orig = &ctx->sgls_orig[i]; uint32_t dup_sg_idx = 0; uint32_t dup_buf_size = 0; + uint32_t dup_data_len = 0; uint32_t sg_nr_out = 0; char *dup_buf; @@ -4923,33 +4926,39 @@ obj_dup_sgls_free(struct obj_auxi_args *obj_auxi) if (skip_sgl_iov(false, iov)) { j++; + sg_nr_out++; continue; } /* Direct copy if entry wasn't modified */ if (!isset_range((uint8_t *)ctx->merged_bitmaps[i], j, j)) { *iov = *iov_dup; - D_ASSERT(dup_buf_size == 0); + D_ASSERT(dup_data_len == 0); j++; sg_nr_out++; dup_sg_idx++; continue; } - if (dup_buf_size == 0) { + if (dup_data_len == 0) { + dup_data_len = iov_dup->iov_len; dup_buf_size = iov_dup->iov_buf_len; dup_buf = (char *)iov_dup->iov_buf; } + /* Update iov_len for short read */ + if (dup_data_len < iov->iov_len) + iov->iov_len = dup_data_len; /* Copy data from duplicate buffer to original buffer */ - D_ASSERT(dup_buf_size >= iov->iov_buf_len); - memcpy((char *)iov->iov_buf, dup_buf, iov->iov_buf_len); - 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); + dup_data_len -= iov->iov_len; + dup_buf += iov->iov_len; + dup_buf_size -= iov->iov_len; sg_nr_out++; j++; /* When current duplicate buffer is exhausted, get next entry */ - if (dup_buf_size == 0) + if (dup_data_len == 0) dup_sg_idx++; } sg_orig->sg_nr_out = sg_nr_out; @@ -4977,7 +4986,8 @@ obj_reasb_io_fini(struct obj_auxi_args *obj_auxi, bool retry) } obj_bulk_fini(obj_auxi); obj_auxi_free_failed_tgt_list(obj_auxi); - obj_dup_sgls_free(obj_auxi); + if (!retry) + obj_dup_sgls_free(obj_auxi); obj_reasb_req_fini(&obj_auxi->reasb_req, obj_auxi->iod_nr); obj_auxi->req_reasbed = false; diff --git a/src/tests/suite/daos_obj_array.c b/src/tests/suite/daos_obj_array.c index a29b21e6fa7..fc8b897a457 100644 --- a/src/tests/suite/daos_obj_array.c +++ b/src/tests/suite/daos_obj_array.c @@ -1729,6 +1729,83 @@ array_recx_write_mixed_iov_read_single(void **state) daos_obj_close(oh, NULL); } +#define ONE_KB (1024) +#define TWO_MB (2 * 1024 * 1024) + +void +test_sgl_merge_short_read(void **state) +{ + test_arg_t *arg = *state; + daos_obj_id_t oid; + daos_handle_t oh; + d_iov_t dkey; + d_sg_list_t sgl; + daos_iod_t iod; + daos_recx_t recx; + char *wbuf, *rbuf; + d_iov_t *sg_iovs = NULL; + int total_iovs = TWO_MB / ONE_KB; + int data_offset = 0; + int rc, i; + + oid = daos_test_oid_gen(arg->coh, OC_SX, 0, 0, arg->myrank); + rc = daos_obj_open(arg->coh, oid, DAOS_OO_RW, &oh, NULL); + assert_rc_equal(rc, 0); + + /* Buffer allocation with guard pages */ + wbuf = malloc(ONE_MB); + assert_true(wbuf != NULL); + dts_buf_render(wbuf, ONE_MB); + + /* Allocate 2MB read buffer with guard pages */ + rbuf = calloc(1, TWO_MB + FOUR_KB); + assert_true(rbuf != NULL); + + /* Build scatter-gather list for 1KB chunks */ + sg_iovs = calloc(total_iovs, sizeof(d_iov_t)); + assert_true(sg_iovs != NULL); + + /* 1MB data read in 1KB chunks */ + for (i = 0; i < total_iovs; i++) { + d_iov_set(&sg_iovs[data_offset], rbuf + i * ONE_KB, ONE_KB); + data_offset++; + } + + /* Configure scatter-gather list */ + sgl = (d_sg_list_t){.sg_nr = total_iovs, .sg_nr_out = 0, .sg_iovs = sg_iovs}; + + /* Prepare I/O descriptor */ + d_iov_set(&dkey, "short_key", 9); + d_iov_set(&iod.iod_name, "big_akey", 8); + iod.iod_nr = 1; + iod.iod_size = 1; + iod.iod_type = DAOS_IOD_ARRAY; + iod.iod_recxs = &recx; + recx.rx_idx = 0; + recx.rx_nr = ONE_MB - 4; + + /* Write initial data with single IOV */ + d_sg_list_t write_sgl = { + .sg_nr = 1, + .sg_iovs = &(d_iov_t){.iov_buf = wbuf, .iov_len = ONE_MB - 4, .iov_buf_len = ONE_MB}}; + rc = daos_obj_update(oh, DAOS_TX_NONE, 0, &dkey, 1, &iod, &write_sgl, NULL); + assert_rc_equal(rc, 0); + + /* Read back using 1KB chunks */ + rc = daos_obj_fetch(oh, DAOS_TX_NONE, 0, &dkey, 1, &iod, &sgl, NULL, NULL); + assert_rc_equal(rc, 0); + assert_rc_equal(sgl.sg_nr_out, 1024); + assert_rc_equal(sg_iovs[1023].iov_len, 1020); + + /* Verify only 1MB - 4 was read successfully */ + assert_memory_equal(wbuf, rbuf, ONE_MB - 4); + + free(wbuf); + free(rbuf); + free(sg_iovs); + daos_obj_close(oh, NULL); +} + static const struct CMUnitTest array_tests[] = { {"ARRAY0: small_sgl", small_sgl, NULL, test_case_teardown}, {"ARRAY1: byte array with buffer on stack", byte_array_simple_stack, NULL, test_case_teardown}, @@ -1759,6 +1836,7 @@ static const struct CMUnitTest array_tests[] = { {"ARRAY20: recx read mixed iov", array_recx_read_mixed_iov, NULL, test_case_teardown}, {"ARRAY21: recx write mixed iov", array_recx_write_mixed_iov_read_single, NULL, test_case_teardown}, + {"ARRAY22: recx short read", test_sgl_merge_short_read, NULL, test_case_teardown}, }; static int From 2a2ce08bcc2333fd7993eee874155f81dddc2d0a Mon Sep 17 00:00:00 2001 From: Wang Shilong Date: Thu, 4 Sep 2025 16:00:05 +0800 Subject: [PATCH 2/3] Fix test failure Signed-off-by: Wang Shilong --- src/object/cli_obj.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/object/cli_obj.c b/src/object/cli_obj.c index cabbe7c5fc1..c0fd3766bfb 100644 --- a/src/object/cli_obj.c +++ b/src/object/cli_obj.c @@ -4846,19 +4846,17 @@ obj_sgls_dup(struct obj_auxi_args *obj_auxi, daos_obj_update_t *args, bool updat setbits64(ctx.alloc_bitmaps[i], sgl_idx, 1); /* Copy data from original IOVs to merged buffer */ - if (update) { - offset = 0; - for (k = merge_start; k < j; k++) { - uint64_t merge_len; + offset = 0; + for (k = merge_start; k < j; k++) { + uint64_t merge_len; - iov = &sg->sg_iovs[k]; - if (skip_sgl_iov(update, iov)) - continue; - D_ASSERT(offset < merged_buf_size); - merge_len = update ? iov->iov_len : iov->iov_buf_len; - memcpy(merged_buf + offset, iov->iov_buf, merge_len); - offset += merge_len; - } + iov = &sg->sg_iovs[k]; + if (skip_sgl_iov(update, iov)) + continue; + D_ASSERT(offset < merged_buf_size); + merge_len = update ? iov->iov_len : iov->iov_buf_len; + memcpy(merged_buf + offset, iov->iov_buf, merge_len); + offset += merge_len; } /* Create merged IOV entry */ From 2913eedf90bfe6563eced91c410ecea6a21a5655 Mon Sep 17 00:00:00 2001 From: Wang Shilong Date: Mon, 22 Sep 2025 10:04:29 +0800 Subject: [PATCH 3/3] cleanup codes Signed-off-by: Wang Shilong --- src/object/cli_obj.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/object/cli_obj.c b/src/object/cli_obj.c index c0fd3766bfb..aaa5c7e2256 100644 --- a/src/object/cli_obj.c +++ b/src/object/cli_obj.c @@ -4911,29 +4911,23 @@ obj_dup_sgls_free(struct obj_auxi_args *obj_auxi) uint32_t dup_sg_idx = 0; uint32_t dup_buf_size = 0; uint32_t dup_data_len = 0; - uint32_t sg_nr_out = 0; char *dup_buf; if (!ctx->alloc_bitmaps || !ctx->alloc_bitmaps[i]) continue; D_ASSERT(ctx->merged_bitmaps[i] != NULL); - for (j = 0; j < sg_orig->sg_nr && dup_sg_idx < sg_dup->sg_nr_out;) { + for (j = 0; j < sg_orig->sg_nr && dup_sg_idx < sg_dup->sg_nr_out; j++) { iov = &sg_orig->sg_iovs[j]; iov_dup = &sg_dup->sg_iovs[dup_sg_idx]; - if (skip_sgl_iov(false, iov)) { - j++; - sg_nr_out++; + if (skip_sgl_iov(false, iov)) continue; - } /* Direct copy if entry wasn't modified */ if (!isset_range((uint8_t *)ctx->merged_bitmaps[i], j, j)) { *iov = *iov_dup; D_ASSERT(dup_data_len == 0); - j++; - sg_nr_out++; dup_sg_idx++; continue; } @@ -4952,14 +4946,12 @@ obj_dup_sgls_free(struct obj_auxi_args *obj_auxi) dup_data_len -= iov->iov_len; dup_buf += iov->iov_len; dup_buf_size -= iov->iov_len; - sg_nr_out++; - j++; /* When current duplicate buffer is exhausted, get next entry */ if (dup_data_len == 0) dup_sg_idx++; } - sg_orig->sg_nr_out = sg_nr_out; + sg_orig->sg_nr_out = j; } }