From b8920647617433ff85a9ad1b603fa25d40782d9b Mon Sep 17 00:00:00 2001 From: Andreas Dilger Date: Fri, 9 Mar 2018 16:18:53 -0700 Subject: [PATCH] LU-10264 mdc: fix possible NULL pointer dereference Fix two static analysis errors. lustre/mdc/mdc_dev.c: in mdc_enqueue_send(), pointer 'matched' return from call to function 'ldlm_handle2lock' at line 704 may be NULL and will be dereferenced at line 705. If client is evicted between ldlm_lock_match() and ldlm_handle2lock() the lock pointer could be NULL. lustre/lov/lov_dev.c:488 in lov_process_config, sscanf format specification '%d' expects type 'int' for 'd', but parameter 3 has a different type '__u32'. Converting to kstrtou32() requires changing the "index" variable type from __u32 to u32, which is fine since it is only used internally, fix up the few functions that are also passing "__u32 index" and the resulting checkpatch.pl warnings. Test-Parameters: trivial Signed-off-by: Andreas Dilger Change-Id: I3cc80d66bbb537161a561f4f2ba7830ddebcab07 Reviewed-on: https://review.whamcloud.com/31621 Tested-by: Jenkins Tested-by: Maloo Reviewed-by: Dmitry Eremin Reviewed-by: Bob Glossman Reviewed-by: James Simmons Reviewed-by: Oleg Drokin --- lustre/lov/lov_dev.c | 7 +- lustre/lov/lov_internal.h | 10 +-- lustre/lov/lov_obd.c | 196 ++++++++++++++++++++++++---------------------- lustre/mdc/mdc_dev.c | 2 +- 4 files changed, 111 insertions(+), 104 deletions(-) diff --git a/lustre/lov/lov_dev.c b/lustre/lov/lov_dev.c index 40f333b..acd868a 100644 --- a/lustre/lov/lov_dev.c +++ b/lustre/lov/lov_dev.c @@ -447,7 +447,7 @@ static int lov_process_config(const struct lu_env *env, int cmd; int rc; int gen; - __u32 index; + u32 index; obd_getref(obd); @@ -479,8 +479,9 @@ static int lov_process_config(const struct lu_env *env, obd_str2uuid(&tgt_uuid, lustre_cfg_buf(cfg, 1)); - if (sscanf(lustre_cfg_buf(cfg, 2), "%d", &index) != 1) - GOTO(out, rc = -EINVAL); + rc = kstrtou32(lustre_cfg_buf(cfg, 2), 10, &index); + if (rc) + GOTO(out, rc); mdc = class_find_client_obd(&tgt_uuid, LUSTRE_MDC_NAME, &obd->obd_uuid); diff --git a/lustre/lov/lov_internal.h b/lustre/lov/lov_internal.h index bba4a8d..54c07a8 100644 --- a/lustre/lov/lov_internal.h +++ b/lustre/lov/lov_internal.h @@ -280,13 +280,13 @@ void lov_fix_desc_pattern(__u32 *val); void lov_fix_desc_qos_maxage(__u32 *val); __u16 lov_get_stripe_count(struct lov_obd *lov, __u32 magic, __u16 stripe_count); -int lov_connect_obd(struct obd_device *obd, __u32 index, int activate, - struct obd_connect_data *data); +int lov_connect_obd(struct obd_device *obd, u32 index, int activate, + struct obd_connect_data *data); int lov_setup(struct obd_device *obd, struct lustre_cfg *lcfg); int lov_process_config_base(struct obd_device *obd, struct lustre_cfg *lcfg, - __u32 *indexp, int *genp); -int lov_del_target(struct obd_device *obd, __u32 index, - struct obd_uuid *uuidp, int gen); + u32 *indexp, int *genp); +int lov_del_target(struct obd_device *obd, u32 index, + struct obd_uuid *uuidp, int gen); /* lov_pack.c */ ssize_t lov_lsm_pack(const struct lov_stripe_md *lsm, void *buf, diff --git a/lustre/lov/lov_obd.c b/lustre/lov/lov_obd.c index c5ac570..51e14f5 100644 --- a/lustre/lov/lov_obd.c +++ b/lustre/lov/lov_obd.c @@ -100,21 +100,21 @@ static void lov_putref(struct obd_device *obd) list_for_each_entry_safe(tgt, n, &kill, ltd_kill) { list_del(&tgt->ltd_kill); - /* Disconnect */ - __lov_del_obd(obd, tgt); - } - } else { + /* Disconnect */ + __lov_del_obd(obd, tgt); + } + } else { mutex_unlock(&lov->lov_lock); - } + } } static int lov_set_osc_active(struct obd_device *obd, struct obd_uuid *uuid, - enum obd_notify_event ev); + enum obd_notify_event ev); static int lov_notify(struct obd_device *obd, struct obd_device *watched, enum obd_notify_event ev); -int lov_connect_obd(struct obd_device *obd, __u32 index, int activate, - struct obd_connect_data *data) +int lov_connect_obd(struct obd_device *obd, u32 index, int activate, + struct obd_connect_data *data) { struct lov_obd *lov = &obd->u.lov; struct obd_uuid *tgt_uuid; @@ -316,40 +316,42 @@ static int lov_disconnect_obd(struct obd_device *obd, struct lov_tgt_desc *tgt) static int lov_disconnect(struct obd_export *exp) { - struct obd_device *obd = class_exp2obd(exp); - struct lov_obd *lov = &obd->u.lov; - int i, rc; - ENTRY; - - if (!lov->lov_tgts) - goto out; + struct obd_device *obd = class_exp2obd(exp); + struct lov_obd *lov = &obd->u.lov; + u32 index; + int rc; - /* Only disconnect the underlying layers on the final disconnect. */ - lov->lov_connects--; - if (lov->lov_connects != 0) { - /* why should there be more than 1 connect? */ - CERROR("disconnect #%d\n", lov->lov_connects); - goto out; - } + ENTRY; + if (!lov->lov_tgts) + goto out; + + /* Only disconnect the underlying layers on the final disconnect. */ + lov->lov_connects--; + if (lov->lov_connects != 0) { + /* why should there be more than 1 connect? */ + CWARN("%s: unexpected disconnect #%d\n", + obd->obd_name, lov->lov_connects); + goto out; + } - /* Let's hold another reference so lov_del_obd doesn't spin through - putref every time */ - obd_getref(obd); + /* hold another ref so lov_del_obd() doesn't spin in putref each time */ + obd_getref(obd); - for (i = 0; i < lov->desc.ld_tgt_count; i++) { - if (lov->lov_tgts[i] && lov->lov_tgts[i]->ltd_exp) { - /* Disconnection is the last we know about an obd */ - lov_del_target(obd, i, NULL, lov->lov_tgts[i]->ltd_gen); - } - } - obd_putref(obd); + for (index = 0; index < lov->desc.ld_tgt_count; index++) { + if (lov->lov_tgts[index] && lov->lov_tgts[index]->ltd_exp) { + /* Disconnection is the last we know about an OBD */ + lov_del_target(obd, index, NULL, + lov->lov_tgts[index]->ltd_gen); + } + } + obd_putref(obd); if (lov->targets_proc_entry != NULL) lprocfs_remove(&lov->targets_proc_entry); out: - rc = class_disconnect(exp); /* bz 9811 */ - RETURN(rc); + rc = class_disconnect(exp); /* bz 9811 */ + RETURN(rc); } /* Error codes: @@ -487,37 +489,37 @@ out_notify_lock: } static int lov_add_target(struct obd_device *obd, struct obd_uuid *uuidp, - __u32 index, int gen, int active) + u32 index, int gen, int active) { - struct lov_obd *lov = &obd->u.lov; - struct lov_tgt_desc *tgt; - struct obd_device *tgt_obd; - int rc; - ENTRY; + struct lov_obd *lov = &obd->u.lov; + struct lov_tgt_desc *tgt; + struct obd_device *tgt_obd; + int rc; - CDEBUG(D_CONFIG, "uuid:%s idx:%d gen:%d active:%d\n", - uuidp->uuid, index, gen, active); + ENTRY; + CDEBUG(D_CONFIG, "uuid:%s idx:%u gen:%d active:%d\n", + uuidp->uuid, index, gen, active); - if (gen <= 0) { - CERROR("request to add OBD %s with invalid generation: %d\n", - uuidp->uuid, gen); - RETURN(-EINVAL); - } + if (gen <= 0) { + CERROR("%s: request to add '%s' with invalid generation: %d\n", + obd->obd_name, uuidp->uuid, gen); + RETURN(-EINVAL); + } - tgt_obd = class_find_client_obd(uuidp, LUSTRE_OSC_NAME, - &obd->obd_uuid); - if (tgt_obd == NULL) - RETURN(-EINVAL); + tgt_obd = class_find_client_obd(uuidp, LUSTRE_OSC_NAME, &obd->obd_uuid); + if (tgt_obd == NULL) + RETURN(-EINVAL); mutex_lock(&lov->lov_lock); - if ((index < lov->lov_tgt_size) && (lov->lov_tgts[index] != NULL)) { - tgt = lov->lov_tgts[index]; - CERROR("UUID %s already assigned at LOV target index %d\n", - obd_uuid2str(&tgt->ltd_uuid), index); + if ((index < lov->lov_tgt_size) && (lov->lov_tgts[index] != NULL)) { + tgt = lov->lov_tgts[index]; + rc = -EEXIST; + CERROR("%s: UUID %s already assigned at index %d: rc = %d\n", + obd->obd_name, obd_uuid2str(&tgt->ltd_uuid), index, rc); mutex_unlock(&lov->lov_lock); - RETURN(-EEXIST); - } + RETURN(rc); + } if (index >= lov->lov_tgt_size) { /* We need to reallocate the lov target array. */ @@ -608,17 +610,17 @@ static int lov_add_target(struct obd_device *obd, struct obd_uuid *uuidp, active ? OBD_NOTIFY_CONNECT : OBD_NOTIFY_INACTIVE); out: - if (rc) { - CERROR("add failed (%d), deleting %s\n", rc, - obd_uuid2str(&tgt->ltd_uuid)); + if (rc) { + CERROR("%s: add failed, deleting %s: rc = %d\n", + obd->obd_name, obd_uuid2str(&tgt->ltd_uuid), rc); lov_del_target(obd, index, NULL, 0); - } - obd_putref(obd); - RETURN(rc); + } + obd_putref(obd); + RETURN(rc); } /* Schedule a target for deletion */ -int lov_del_target(struct obd_device *obd, __u32 index, +int lov_del_target(struct obd_device *obd, u32 index, struct obd_uuid *uuidp, int gen) { struct lov_obd *lov = &obd->u.lov; @@ -892,43 +894,47 @@ static int lov_cleanup(struct obd_device *obd) } int lov_process_config_base(struct obd_device *obd, struct lustre_cfg *lcfg, - __u32 *indexp, int *genp) + u32 *indexp, int *genp) { - struct obd_uuid obd_uuid; - int cmd; - int rc = 0; - ENTRY; + struct obd_uuid obd_uuid; + int cmd; + int rc = 0; - switch(cmd = lcfg->lcfg_command) { + ENTRY; + switch (cmd = lcfg->lcfg_command) { case LCFG_ADD_MDC: case LCFG_DEL_MDC: break; case LCFG_LOV_ADD_OBD: - case LCFG_LOV_ADD_INA: - case LCFG_LOV_DEL_OBD: { - __u32 index; - int gen; - /* lov_modify_tgts add 0:lov_mdsA 1:ost1_UUID 2:0 3:1 */ - if (LUSTRE_CFG_BUFLEN(lcfg, 1) > sizeof(obd_uuid.uuid)) - GOTO(out, rc = -EINVAL); - - obd_str2uuid(&obd_uuid, lustre_cfg_buf(lcfg, 1)); - - if (sscanf(lustre_cfg_buf(lcfg, 2), "%u", indexp) != 1) - GOTO(out, rc = -EINVAL); - if (sscanf(lustre_cfg_buf(lcfg, 3), "%d", genp) != 1) - GOTO(out, rc = -EINVAL); - index = *indexp; - gen = *genp; - if (cmd == LCFG_LOV_ADD_OBD) - rc = lov_add_target(obd, &obd_uuid, index, gen, 1); - else if (cmd == LCFG_LOV_ADD_INA) - rc = lov_add_target(obd, &obd_uuid, index, gen, 0); - else - rc = lov_del_target(obd, index, &obd_uuid, gen); - GOTO(out, rc); - } - case LCFG_PARAM: { + case LCFG_LOV_ADD_INA: + case LCFG_LOV_DEL_OBD: { + u32 index; + int gen; + + /* lov_modify_tgts add 0:lov_mdsA 1:ost1_UUID 2:0 3:1 */ + if (LUSTRE_CFG_BUFLEN(lcfg, 1) > sizeof(obd_uuid.uuid)) + GOTO(out, rc = -EINVAL); + + obd_str2uuid(&obd_uuid, lustre_cfg_buf(lcfg, 1)); + + rc = kstrtou32(lustre_cfg_buf(lcfg, 2), 10, indexp); + if (rc) + GOTO(out, rc); + rc = kstrtoint(lustre_cfg_buf(lcfg, 3), 10, genp); + if (rc) + GOTO(out, rc); + index = *indexp; + gen = *genp; + if (cmd == LCFG_LOV_ADD_OBD) + rc = lov_add_target(obd, &obd_uuid, index, gen, 1); + else if (cmd == LCFG_LOV_ADD_INA) + rc = lov_add_target(obd, &obd_uuid, index, gen, 0); + else + rc = lov_del_target(obd, index, &obd_uuid, gen); + + GOTO(out, rc); + } + case LCFG_PARAM: { struct lov_desc *desc = &(obd->u.lov.desc); if (!desc) diff --git a/lustre/mdc/mdc_dev.c b/lustre/mdc/mdc_dev.c index 0ebc4ef..3672ed1 100644 --- a/lustre/mdc/mdc_dev.c +++ b/lustre/mdc/mdc_dev.c @@ -702,7 +702,7 @@ int mdc_enqueue_send(const struct lu_env *env, struct obd_export *exp, RETURN(ELDLM_OK); matched = ldlm_handle2lock(&lockh); - if (ldlm_is_kms_ignore(matched)) + if (!matched || ldlm_is_kms_ignore(matched)) goto no_match; if (mdc_set_dom_lock_data(env, matched, einfo->ei_cbdata)) { -- 1.8.3.1