From 669ca6fac365c0799ca352cd6cbed2af0e1a7b01 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 3f0f2f196e5..a7de97d6679 100644 --- a/src/object/cli_obj.c +++ b/src/object/cli_obj.c @@ -4843,17 +4843,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 */ @@ -4907,6 +4909,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; @@ -4920,33 +4923,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; @@ -4974,7 +4983,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 00742f54991..858b70e6ff2 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 68feac62468d9f0ed1cab538ebc6bd6b803ba9c9 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 a7de97d6679..530a0d6a18a 100644 --- a/src/object/cli_obj.c +++ b/src/object/cli_obj.c @@ -4843,19 +4843,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 a94c5e801195798dac67897189f8ba94e4fa2437 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 530a0d6a18a..2a4d6e89313 100644 --- a/src/object/cli_obj.c +++ b/src/object/cli_obj.c @@ -4908,29 +4908,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; } @@ -4949,14 +4943,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; } }