Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

Commit

Permalink
CORTX-34179 : Fixed panic in read path (#2147)
Browse files Browse the repository at this point in the history 
Problem:
Motr panic occurred while performing m0cat operation after 
corrupting the data block checksum.

Solution:
Removed all asserts in DI (Data Integrity) path from client side.
Converted all asserts and returned appropriate error to the client

Signed-off-by: Rajat Patil <rajat.r.patil@seagate.com>
  • Loading branch information
@rajatpatil98
rajatpatil98 authored Sep 15, 2022
1 parent a891ba4 commit f722cf3
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 31 deletions.
48 changes: 31 additions & 17 deletions motr/io_nw_xfer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,7 +824,8 @@ int m0_target_calculate_checksum(struct m0_op_io *ioo, uint8_t pi_type,
uint64_t u_idx;

u_idx = cs_idx->ci_unit_idx;
M0_ASSERT(cs_idx->ci_pg_idx < ioo->ioo_iomap_nr);
if (cs_idx->ci_pg_idx >= ioo->ioo_iomap_nr)
return -EINVAL;

pi = (struct m0_generic_pi *)chksm_buf;
map = ioo->ioo_iomaps[cs_idx->ci_pg_idx];
Expand All @@ -843,10 +844,12 @@ int m0_target_calculate_checksum(struct m0_op_io *ioo, uint8_t pi_type,
/* Select data pointer */
if (filter == PA_PARITY) {
data = map->pi_paritybufs;
M0_ASSERT(u_idx < layout_k(play));
if (u_idx >= layout_k(play))
return -EINVAL;
} else {
data = map->pi_databufs;
M0_ASSERT(u_idx < layout_n(play));
if (u_idx >= layout_n(play))
return -EINVAL;
}

rc = m0_bufvec_empty_alloc(&bvec, rows_nr(play, obj));
Expand Down Expand Up @@ -910,7 +913,8 @@ static int target_ioreq_prepare_checksum(struct m0_op_io *ioo,
/* Get checksum size and type */
cksum_size = m0__obj_di_cksum_size(ioo);
cksum_type = m0__obj_di_cksum_type(ioo);
M0_ASSERT(cksum_type < M0_PI_TYPE_MAX);
if (cksum_type >= M0_PI_TYPE_MAX)
return -EINVAL;

/* Number of units will not be zero as its already checked */
num_units = irfop->irf_cksum_data.cd_num_units;
Expand All @@ -929,8 +933,9 @@ static int target_ioreq_prepare_checksum(struct m0_op_io *ioo,
for (idx = 0; idx < num_units; idx++) {
cs_idx_data = &cs_data->cd_idx[idx];
/* Valid data should be populated */
M0_ASSERT(cs_idx_data->ci_pg_idx != UINT32_MAX &&
cs_idx_data->ci_unit_idx != UINT32_MAX);
if (cs_idx_data->ci_pg_idx == UINT32_MAX ||
cs_idx_data->ci_unit_idx == UINT32_MAX)
return -EINVAL;
/* For Parity Unit only Motr can generates checksum */
if (m0__obj_is_di_cksum_gen_enabled(ioo) ||
(irfop->irf_pattr == PA_PARITY)) {
Expand All @@ -950,12 +955,14 @@ static int target_ioreq_prepare_checksum(struct m0_op_io *ioo,
struct m0_pdclust_layout *play = pdlayout_get(ioo);
unit_off = cs_idx_data->ci_pg_idx * layout_n(play) +
cs_idx_data->ci_unit_idx;
M0_ASSERT(unit_off < ioo->ioo_attr.ov_vec.v_nr);
if (unit_off >= ioo->ioo_attr.ov_vec.v_nr)
return -EINVAL;
memcpy(b_addr + computed_cksm_nob,
ioo->ioo_attr.ov_buf[unit_off], cksum_size);
}
computed_cksm_nob += cksum_size;
M0_ASSERT(computed_cksm_nob <= rw_fop->crw_di_data_cksum.b_nob);
if (computed_cksm_nob > rw_fop->crw_di_data_cksum.b_nob)
return -EINVAL;
}
return rc;
}
Expand All @@ -965,7 +972,7 @@ static int target_ioreq_prepare_checksum(struct m0_op_io *ioo,
* DATA Units : Gob Offset - PGStart : PGStart + NxUS => PG Index - 0 : (N-1)
* PARITY Units : Gob Offset - PGStart : PGStart + KxUS => PG Index - 0 : (K-1)
*/
static void target_ioreq_calc_idx(struct m0_op_io *ioo,
static int target_ioreq_calc_idx(struct m0_op_io *ioo,
struct fop_cksum_idx_gbl_data *pgdata,
struct ioreq_fop *irfop,
struct m0_ivec_cursor *goff_cur,
Expand All @@ -989,8 +996,9 @@ static void target_ioreq_calc_idx(struct m0_op_io *ioo,
goff = m0_ivec_cursor_index(goff_cur);
cs_idx =
&fop_cs_data->cd_idx[fop_cs_data->cd_num_units];
M0_ASSERT(cs_idx->ci_pg_idx == UINT32_MAX &&
cs_idx->ci_unit_idx == UINT32_MAX);
if (cs_idx->ci_pg_idx != UINT32_MAX ||
cs_idx->ci_unit_idx != UINT32_MAX)
return -EINVAL;
grp0_idx = pgdata->fg_pgrp0_index;
/**
* There are scenarios where K > N in such case when the
Expand All @@ -1012,8 +1020,9 @@ static void target_ioreq_calc_idx(struct m0_op_io *ioo,
pgdata->fg_pgrp_sz;
cs_idx->ci_unit_idx = rem_pg_sz/pgdata->fg_unit_sz;
fop_cs_data->cd_num_units++;
M0_ASSERT(fop_cs_data->cd_num_units <=
fop_cs_data->cd_max_units);
if (fop_cs_data->cd_num_units >
fop_cs_data->cd_max_units)
return -EINVAL;
M0_LOG(M0_DEBUG,"FOP Unit Added Num:%d GOF:%" PRIi64
" [PG Idx:%" PRIu64 "][Unit Idx:%" PRIu64
"] Seg:%d", fop_cs_data->cd_num_units, goff,
Expand All @@ -1022,6 +1031,7 @@ static void target_ioreq_calc_idx(struct m0_op_io *ioo,
}
m0_ivec_cursor_move(goff_cur, pgdata->fg_seg_sz);
}
return M0_RC(0);
}

/**
Expand Down Expand Up @@ -1309,8 +1319,9 @@ static int target_ioreq_iofops_prepare(struct target_ioreq *ti,
* adjusted otherwise num_units will
* be 0
*/
M0_ASSERT(delta > (num_units_iter *
m0__obj_di_cksum_size(ioo)));
if (delta <= (num_units_iter *
m0__obj_di_cksum_size(ioo)))
return -EINVAL;
delta -= (num_units_iter *
m0__obj_di_cksum_size(ioo));

Expand All @@ -1336,13 +1347,16 @@ static int target_ioreq_iofops_prepare(struct target_ioreq *ti,
} else if (rc == 0) {
++bbsegs;
sz_added_to_fop += xfer_len;
if (di_enabled)
target_ioreq_calc_idx(ioo,
if (di_enabled) {
rc = target_ioreq_calc_idx(ioo,
&pgdata,
irfop,
&goff_curr,
seg_start,
seg_end);
if (rc != 0)
goto fini_fop;
}
}
} else if (di_enabled)
m0_ivec_cursor_move(&goff_curr, seg_sz);
Expand Down
50 changes: 36 additions & 14 deletions motr/io_req_fop.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,18 +145,24 @@ static int application_checksum_process(struct m0_op_io *ioo,
uint32_t num_units;
uint32_t cksum_size;
uint32_t cs_compared = 0;
void *compute_cs_buf;
void *compute_cs_buf = NULL;
enum m0_pi_algo_type cksum_type;
struct fop_cksum_data *cs_data;

cs_data = &irfop->irf_cksum_data;
/* Validate if FOP has unit count set */
num_units = cs_data->cd_num_units;
M0_ASSERT(num_units != 0);
if (num_units == 0) {
rc = -EINVAL;
goto fail;
}

/* FOP reply data should have pi type correctly set */
cksum_type = ((struct m0_pi_hdr *)rw_rep_cs_data->b_addr)->pih_type;
M0_ASSERT(cksum_type < M0_PI_TYPE_MAX);
if (cksum_type >= M0_PI_TYPE_MAX) {
rc = -EINVAL;
goto fail;
}
cksum_size = m0_cksum_get_size(cksum_type);
if (cksum_size == 0) {
M0_LOG(M0_WARN, "Skipping DI for PI Type: %d Size: %d",
Expand All @@ -170,20 +176,28 @@ static int application_checksum_process(struct m0_op_io *ioo,
* also confirm that user has correctly allocated buffer for checksum
* in ioo attr structure.
*/
M0_ASSERT(rw_rep_cs_data->b_nob == num_units * cksum_size);
if (rw_rep_cs_data->b_nob != num_units * cksum_size) {
rc = -EINVAL;
goto fail;
}

/* Allocate checksum buffer */
compute_cs_buf = m0_alloc(cksum_size);
if (compute_cs_buf == NULL )
return -ENOMEM;
if (compute_cs_buf == NULL) {
rc = -ENOMEM;
goto fail;
}

M0_LOG(M0_DEBUG, "RECEIVED CS b_nob: %d PiTyp:%d",
(int)rw_rep_cs_data->b_nob,cksum_type);

for (idx = 0; idx < num_units; idx++) {
struct fop_cksum_idx_data *cs_idx = &cs_data->cd_idx[idx];
M0_ASSERT(cs_idx->ci_pg_idx != UINT32_MAX &&
cs_idx->ci_unit_idx != UINT32_MAX);
if (cs_idx->ci_pg_idx == UINT32_MAX ||
cs_idx->ci_unit_idx == UINT32_MAX) {
rc = -EINVAL;
goto fail;
}

/* Calculate checksum for each unit */
rc = m0_target_calculate_checksum(ioo, cksum_type,
Expand All @@ -196,8 +210,7 @@ static int application_checksum_process(struct m0_op_io *ioo,
if (memcmp(rw_rep_cs_data->b_addr + cs_compared,
compute_cs_buf, cksum_size) != 0) {
/* Add error code to the target status */
rc = M0_RC(-EIO);
ioo->ioo_rc = M0_RC(-EIO);
rc = -EIO;
ioo->ioo_di_err_count++;

/* Log all info to locate unit */
Expand All @@ -218,6 +231,7 @@ static int application_checksum_process(struct m0_op_io *ioo,
cs_idx->ci_unit_idx);
print_pi(rw_rep_cs_data->b_addr, rw_rep_cs_data->b_nob);
print_pi(compute_cs_buf, cksum_size);
goto fail;
}
/* Copy checksum to application buffer */
if (!m0__obj_is_di_cksum_gen_enabled(ioo) &&
Expand All @@ -233,13 +247,21 @@ static int application_checksum_process(struct m0_op_io *ioo,
}

cs_compared += cksum_size;
M0_ASSERT(cs_compared <= rw_rep_cs_data->b_nob);
if (cs_compared > rw_rep_cs_data->b_nob) {
rc = -EINVAL;
goto fail;
}
}
/* All checksum expected from target should be received */
M0_ASSERT(cs_compared == rw_rep_cs_data->b_nob);

if (cs_compared != rw_rep_cs_data->b_nob) {
rc = -EINVAL;
goto fail;
}
m0_free(compute_cs_buf);
return rc;
fail:
m0_free(compute_cs_buf);
ioo->ioo_rc = rc;
return rc;
}

Expand Down Expand Up @@ -331,7 +353,7 @@ static void io_bottom_half(struct m0_sm_group *grp, struct m0_sm_ast *ast)
"sns state = %d", ioo, req_item,
req_item->ri_type->rit_opcode, rc, ioo->ioo_sns_state);
actual_bytes = rw_reply->rwr_count;
rc = gen_rep->gr_rc;
rc = rc ?: gen_rep->gr_rc;
rc = rc ?: rw_reply->rwr_rc;
irfop->irf_reply_rc = rc;

Expand Down

0 comments on commit f722cf3

Please sign in to comment.