Skip to contentSkip to conversation

target-iSCSI: Adjustments for several function implementations

PATCH In review SF Markus Elfring 3 participants 18 messages Dec 12, 2017 – Feb 23, 2018

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover target-iSCSI: Adjustments for several function implementations

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 22:22:11 +0100

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (8):
Less function calls in chap_server_compute_md5() after error detection Move resetting of seven variables in chap_server_compute_md5() Delete 36 error messages for a failed memory allocation
Delete an unnecessary variable initialisation in iscsit_allocate_ooo_cmdsn() Delete an unnecessary variable initialisation in iscsi_copy_param_list() Delete an unnecessary variable initialisation in iscsi_create_default_params() Delete an unnecessary variable initialisation in iscsi_set_default_param() Improve 16 size determinations

 drivers/target/iscsi/iscsi_target.c               |   2 -
 drivers/target/iscsi/iscsi_target_auth.c          | 110 +++++++++++-----------
 drivers/target/iscsi/iscsi_target_datain_values.c |   6 +-
 drivers/target/iscsi/iscsi_target_erl1.c          |  14 +--
 drivers/target/iscsi/iscsi_target_erl2.c          |   8 +-
 drivers/target/iscsi/iscsi_target_login.c         |  29 ++----
 drivers/target/iscsi/iscsi_target_nego.c          |   4 +-
 drivers/target/iscsi/iscsi_target_parameters.c    |  58 +++++-------
 drivers/target/iscsi/iscsi_target_seq_pdu_list.c  |  34 +++----
 drivers/target/iscsi/iscsi_target_tpg.c           |  13 +--
 drivers/target/iscsi/iscsi_target_util.c          |  11 +--
 11 files changed, 118 insertions(+), 171 deletions(-)
Signature · 1 line
2.15.1
#1 lore raw

PATCH 1/8 target/iscsi: Less function calls in chap_server_compute_md5() after error detection

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 18:00:41 +0100

The functions "crypto_free_shash", "kfree" and "kzfree" were called in a few cases by the chap_server_compute_md5() function during error handling even if the passed variable contained a null pointer.

* Adjust jump targets according to the Linux coding style convention.

* Delete initialisations for the variables "challenge", "challenge_binhex", "desc" and "tfm" at the beginning which became unnecessary with this refactoring.

Fixes 69110e3cedbb8aad1c70d91ed58a9f4f0ed9eec6 ("iscsi-target: Use shash and ahash")
Fixes e48354ce078c079996f89d715dfa44814b4eba01 ("iscsi-target: Add iSCSI fabric support for target v4.1")
---
 drivers/target/iscsi/iscsi_target_auth.c | 71 +++++++++++++++++---------------
 1 file changed, 37 insertions(+), 34 deletions(-)
drivers/target/iscsi/iscsi_target_auth.c +37 -34
index f9bc8ec6fb6b..94b011fe74e8 100644
@@ -186,15 +186,15 @@ static int chap_server_compute_md5(
 	unsigned char id_as_uchar;
 	unsigned char digest[MD5_SIGNATURE_SIZE];
 	unsigned char type, response[MD5_SIGNATURE_SIZE * 2 + 2];
-	unsigned char identifier[10], *challenge = NULL;
-	unsigned char *challenge_binhex = NULL;
+	unsigned char identifier[10], *challenge;
+	unsigned char *challenge_binhex;
 	unsigned char client_digest[MD5_SIGNATURE_SIZE];
 	unsigned char server_digest[MD5_SIGNATURE_SIZE];
 	unsigned char chap_n[MAX_CHAP_N_SIZE], chap_r[MAX_RESPONSE_LENGTH];
 	size_t compare_len;
 	struct iscsi_chap *chap = conn->auth_protocol;
-	struct crypto_shash *tfm = NULL;
-	struct shash_desc *desc = NULL;
+	struct crypto_shash *tfm;
+	struct shash_desc *desc;
 	int auth_ret = -1, ret, challenge_len;
 
 	memset(identifier, 0, 10);
@@ -208,13 +208,13 @@ static int chap_server_compute_md5(
 	challenge = kzalloc(CHAP_CHALLENGE_STR_LEN, GFP_KERNEL);
 	if (!challenge) {
 		pr_err("Unable to allocate challenge buffer\n");
-		goto out;
+		goto exit;
 	}
 
 	challenge_binhex = kzalloc(CHAP_CHALLENGE_STR_LEN, GFP_KERNEL);
 	if (!challenge_binhex) {
 		pr_err("Unable to allocate challenge_binhex buffer\n");
-		goto out;
+		goto free_challenge;
 	}
 	/*
 	 * Extract CHAP_N.
@@ -222,18 +222,18 @@ static int chap_server_compute_md5(
 	if (extract_param(nr_in_ptr, "CHAP_N", MAX_CHAP_N_SIZE, chap_n,
 				&type) < 0) {
 		pr_err("Could not find CHAP_N.\n");
-		goto out;
+		goto free_challenge_binhex;
 	}
 	if (type = HEX) {
 		pr_err("Could not find CHAP_N.\n");
-		goto out;
+		goto free_challenge_binhex;
 	}
 
 	/* Include the terminating NULL in the compare */
 	compare_len = strlen(auth->userid) + 1;
 	if (strncmp(chap_n, auth->userid, compare_len) != 0) {
 		pr_err("CHAP_N values do not match!\n");
-		goto out;
+		goto free_challenge_binhex;
 	}
 	pr_debug("[server] Got CHAP_N=%s\n", chap_n);
 	/*
@@ -242,11 +242,11 @@ static int chap_server_compute_md5(
 	if (extract_param(nr_in_ptr, "CHAP_R", MAX_RESPONSE_LENGTH, chap_r,
 				&type) < 0) {
 		pr_err("Could not find CHAP_R.\n");
-		goto out;
+		goto free_challenge_binhex;
 	}
 	if (type != HEX) {
 		pr_err("Could not find CHAP_R.\n");
-		goto out;
+		goto free_challenge_binhex;
 	}
 
 	pr_debug("[server] Got CHAP_R=%s\n", chap_r);
@@ -254,15 +254,14 @@ static int chap_server_compute_md5(
 
 	tfm = crypto_alloc_shash("md5", 0, 0);
 	if (IS_ERR(tfm)) {
-		tfm = NULL;
 		pr_err("Unable to allocate struct crypto_shash\n");
-		goto out;
+		goto free_challenge_binhex;
 	}
 
 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(tfm), GFP_KERNEL);
 	if (!desc) {
 		pr_err("Unable to allocate struct shash_desc\n");
-		goto out;
+		goto free_shash;
 	}
 
 	desc->tfm = tfm;
@@ -271,27 +270,27 @@ static int chap_server_compute_md5(
 	ret = crypto_shash_init(desc);
 	if (ret < 0) {
 		pr_err("crypto_shash_init() failed\n");
-		goto out;
+		goto free_desc;
 	}
 
 	ret = crypto_shash_update(desc, &chap->id, 1);
 	if (ret < 0) {
 		pr_err("crypto_shash_update() failed for id\n");
-		goto out;
+		goto free_desc;
 	}
 
 	ret = crypto_shash_update(desc, (char *)&auth->password,
 				  strlen(auth->password));
 	if (ret < 0) {
 		pr_err("crypto_shash_update() failed for password\n");
-		goto out;
+		goto free_desc;
 	}
 
 	ret = crypto_shash_finup(desc, chap->challenge,
 				 CHAP_CHALLENGE_LENGTH, server_digest);
 	if (ret < 0) {
 		pr_err("crypto_shash_finup() failed for challenge\n");
-		goto out;
+		goto free_desc;
 	}
 
 	chap_binaryhex_to_asciihex(response, server_digest, MD5_SIGNATURE_SIZE);
@@ -299,7 +298,7 @@ static int chap_server_compute_md5(
 
 	if (memcmp(server_digest, client_digest, MD5_SIGNATURE_SIZE) != 0) {
 		pr_debug("[server] MD5 Digests do not match!\n\n");
-		goto out;
+		goto free_desc;
 	} else
 		pr_debug("[server] MD5 Digests match, CHAP connection"
 				" successful.\n\n");
@@ -309,14 +308,14 @@ static int chap_server_compute_md5(
 	 */
 	if (!auth->authenticate_target) {
 		auth_ret = 0;
-		goto out;
+		goto free_desc;
 	}
 	/*
 	 * Get CHAP_I.
 	 */
 	if (extract_param(nr_in_ptr, "CHAP_I", 10, identifier, &type) < 0) {
 		pr_err("Could not find CHAP_I.\n");
-		goto out;
+		goto free_desc;
 	}
 
 	if (type = HEX)
@@ -326,11 +325,11 @@ static int chap_server_compute_md5(
 
 	if (ret < 0) {
 		pr_err("kstrtoul() failed for CHAP identifier: %d\n", ret);
-		goto out;
+		goto free_desc;
 	}
 	if (id > 255) {
 		pr_err("chap identifier: %lu greater than 255\n", id);
-		goto out;
+		goto free_desc;
 	}
 	/*
 	 * RFC 1994 says Identifier is no more than octet (8 bits).
@@ -342,23 +341,23 @@ static int chap_server_compute_md5(
 	if (extract_param(nr_in_ptr, "CHAP_C", CHAP_CHALLENGE_STR_LEN,
 			challenge, &type) < 0) {
 		pr_err("Could not find CHAP_C.\n");
-		goto out;
+		goto free_desc;
 	}
 
 	if (type != HEX) {
 		pr_err("Could not find CHAP_C.\n");
-		goto out;
+		goto free_desc;
 	}
 	pr_debug("[server] Got CHAP_C=%s\n", challenge);
 	challenge_len = chap_string_to_hex(challenge_binhex, challenge,
 				strlen(challenge));
 	if (!challenge_len) {
 		pr_err("Unable to convert incoming challenge\n");
-		goto out;
+		goto free_desc;
 	}
 	if (challenge_len > 1024) {
 		pr_err("CHAP_C exceeds maximum binary size of 1024 bytes\n");
-		goto out;
+		goto free_desc;
 	}
 	/*
 	 * During mutual authentication, the CHAP_C generated by the
@@ -368,7 +367,7 @@ static int chap_server_compute_md5(
 	if (!memcmp(challenge_binhex, chap->challenge, CHAP_CHALLENGE_LENGTH)) {
 		pr_err("initiator CHAP_C matches target CHAP_C, failing"
 		       " login attempt\n");
-		goto out;
+		goto free_desc;
 	}
 	/*
 	 * Generate CHAP_N and CHAP_R for mutual authentication.
@@ -376,7 +375,7 @@ static int chap_server_compute_md5(
 	ret = crypto_shash_init(desc);
 	if (ret < 0) {
 		pr_err("crypto_shash_init() failed\n");
-		goto out;
+		goto free_desc;
 	}
 
 	/* To handle both endiannesses */
@@ -384,7 +383,7 @@ static int chap_server_compute_md5(
 	ret = crypto_shash_update(desc, &id_as_uchar, 1);
 	if (ret < 0) {
 		pr_err("crypto_shash_update() failed for id\n");
-		goto out;
+		goto free_desc;
 	}
 
 	ret = crypto_shash_update(desc, auth->password_mutual,
@@ -392,7 +391,7 @@ static int chap_server_compute_md5(
 	if (ret < 0) {
 		pr_err("crypto_shash_update() failed for"
 				" password_mutual\n");
-		goto out;
+		goto free_desc;
 	}
 	/*
 	 * Convert received challenge to binary hex.
@@ -401,7 +400,7 @@ static int chap_server_compute_md5(
 				 digest);
 	if (ret < 0) {
 		pr_err("crypto_shash_finup() failed for ma challenge\n");
-		goto out;
+		goto free_desc;
 	}
 
 	/*
@@ -419,11 +418,15 @@ static int chap_server_compute_md5(
 	*nr_out_len += 1;
 	pr_debug("[server] Sending CHAP_R=0x%s\n", response);
 	auth_ret = 0;
-out:
+free_desc:
 	kzfree(desc);
+free_shash:
 	crypto_free_shash(tfm);
-	kfree(challenge);
+free_challenge_binhex:
 	kfree(challenge_binhex);
+free_challenge:
+	kfree(challenge);
+exit:
 	return auth_ret;
 }
 
Signature · 1 line
2.15.1
#2 lore raw

PATCH 2/8 target/iscsi: Move resetting of seven variables in chap_server_compute_md5()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 19:43:47 +0100

Move the resetting of these array variables so that this operation will be performed only if memory allocations succeeded before in this function.

---
 drivers/target/iscsi/iscsi_target_auth.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)
drivers/target/iscsi/iscsi_target_auth.c +14 -8
index 94b011fe74e8..d837fcbdbaf2 100644
@@ -197,14 +197,6 @@ static int chap_server_compute_md5(
 	struct shash_desc *desc;
 	int auth_ret = -1, ret, challenge_len;
 
-	memset(identifier, 0, 10);
-	memset(chap_n, 0, MAX_CHAP_N_SIZE);
-	memset(chap_r, 0, MAX_RESPONSE_LENGTH);
-	memset(digest, 0, MD5_SIGNATURE_SIZE);
-	memset(response, 0, MD5_SIGNATURE_SIZE * 2 + 2);
-	memset(client_digest, 0, MD5_SIGNATURE_SIZE);
-	memset(server_digest, 0, MD5_SIGNATURE_SIZE);
-
 	challenge = kzalloc(CHAP_CHALLENGE_STR_LEN, GFP_KERNEL);
 	if (!challenge) {
 		pr_err("Unable to allocate challenge buffer\n");
@@ -216,6 +208,9 @@ static int chap_server_compute_md5(
 		pr_err("Unable to allocate challenge_binhex buffer\n");
 		goto free_challenge;
 	}
+
+	memset(chap_n, 0, MAX_CHAP_N_SIZE);
+
 	/*
 	 * Extract CHAP_N.
 	 */
@@ -236,6 +231,8 @@ static int chap_server_compute_md5(
 		goto free_challenge_binhex;
 	}
 	pr_debug("[server] Got CHAP_N=%s\n", chap_n);
+	memset(chap_r, 0, MAX_RESPONSE_LENGTH);
+
 	/*
 	 * Extract CHAP_R.
 	 */
@@ -250,6 +247,7 @@ static int chap_server_compute_md5(
 	}
 
 	pr_debug("[server] Got CHAP_R=%s\n", chap_r);
+	memset(client_digest, 0, MD5_SIGNATURE_SIZE);
 	chap_string_to_hex(client_digest, chap_r, strlen(chap_r));
 
 	tfm = crypto_alloc_shash("md5", 0, 0);
@@ -286,6 +284,7 @@ static int chap_server_compute_md5(
 		goto free_desc;
 	}
 
+	memset(server_digest, 0, MD5_SIGNATURE_SIZE);
 	ret = crypto_shash_finup(desc, chap->challenge,
 				 CHAP_CHALLENGE_LENGTH, server_digest);
 	if (ret < 0) {
@@ -293,6 +292,7 @@ static int chap_server_compute_md5(
 		goto free_desc;
 	}
 
+	memset(response, 0, MD5_SIGNATURE_SIZE * 2 + 2);
 	chap_binaryhex_to_asciihex(response, server_digest, MD5_SIGNATURE_SIZE);
 	pr_debug("[server] MD5 Server Digest: %s\n", response);
 
@@ -310,6 +310,9 @@ static int chap_server_compute_md5(
 		auth_ret = 0;
 		goto free_desc;
 	}
+
+	memset(identifier, 0, ARRAY_SIZE(identifier));
+
 	/*
 	 * Get CHAP_I.
 	 */
@@ -393,6 +396,9 @@ static int chap_server_compute_md5(
 				" password_mutual\n");
 		goto free_desc;
 	}
+
+	memset(digest, 0, MD5_SIGNATURE_SIZE);
+
 	/*
 	 * Convert received challenge to binary hex.
 	 */
Signature · 1 line
2.15.1
#3 lore raw

PATCH 3/8 target/iscsi: Delete 36 error messages for a failed memory allocation

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 21:07:16 +0100

Omit extra messages for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

---
 drivers/target/iscsi/iscsi_target.c               |  2 --
 drivers/target/iscsi/iscsi_target_auth.c          | 17 +++------
 drivers/target/iscsi/iscsi_target_datain_values.c |  6 ++--
 drivers/target/iscsi/iscsi_target_erl1.c          | 12 +++----
 drivers/target/iscsi/iscsi_target_erl2.c          |  6 ++--
 drivers/target/iscsi/iscsi_target_login.c         | 23 +++----------
 drivers/target/iscsi/iscsi_target_nego.c          |  4 +--
 drivers/target/iscsi/iscsi_target_parameters.c    | 42 +++++++----------------
 drivers/target/iscsi/iscsi_target_seq_pdu_list.c  | 23 +++++--------
 drivers/target/iscsi/iscsi_target_tpg.c           |  9 ++---
 drivers/target/iscsi/iscsi_target_util.c          | 11 +++---
 11 files changed, 46 insertions(+), 109 deletions(-)
drivers/target/iscsi/iscsi_target.c +0 -2
index 9eb10d34682c..f3c6ea556ea8 100644
@@ -812,7 +812,6 @@ int iscsit_add_reject(
 
 	cmd->buf_ptr = kmemdup(buf, ISCSI_HDR_LEN, GFP_KERNEL);
 	if (!cmd->buf_ptr) {
-		pr_err("Unable to allocate memory for cmd->buf_ptr\n");
 		iscsit_free_cmd(cmd, false);
 		return -1;
 	}
@@ -849,7 +848,6 @@ static int iscsit_add_reject_from_cmd(
 
 	cmd->buf_ptr = kmemdup(buf, ISCSI_HDR_LEN, GFP_KERNEL);
 	if (!cmd->buf_ptr) {
-		pr_err("Unable to allocate memory for cmd->buf_ptr\n");
 		iscsit_free_cmd(cmd, false);
 		return -1;
 	}
drivers/target/iscsi/iscsi_target_auth.c +5 -12
index d837fcbdbaf2..3a17343f43ed 100644
@@ -80,10 +80,9 @@ static int chap_check_algorithm(const char *a_str)
 	char *tmp, *orig, *token;
 
 	tmp = kstrdup(a_str, GFP_KERNEL);
-	if (!tmp) {
-		pr_err("Memory allocation failed for CHAP_A temporary buffer\n");
+	if (!tmp)
 		return CHAP_DIGEST_UNKNOWN;
-	}
+
 	orig = tmp;
 
 	token = strsep(&tmp, "=");
@@ -198,16 +197,12 @@ static int chap_server_compute_md5(
 	int auth_ret = -1, ret, challenge_len;
 
 	challenge = kzalloc(CHAP_CHALLENGE_STR_LEN, GFP_KERNEL);
-	if (!challenge) {
-		pr_err("Unable to allocate challenge buffer\n");
+	if (!challenge)
 		goto exit;
-	}
 
 	challenge_binhex = kzalloc(CHAP_CHALLENGE_STR_LEN, GFP_KERNEL);
-	if (!challenge_binhex) {
-		pr_err("Unable to allocate challenge_binhex buffer\n");
+	if (!challenge_binhex)
 		goto free_challenge;
-	}
 
 	memset(chap_n, 0, MAX_CHAP_N_SIZE);
 
@@ -257,10 +252,8 @@ static int chap_server_compute_md5(
 	}
 
 	desc = kmalloc(sizeof(*desc) + crypto_shash_descsize(tfm), GFP_KERNEL);
-	if (!desc) {
-		pr_err("Unable to allocate struct shash_desc\n");
+	if (!desc)
 		goto free_shash;
-	}
 
 	desc->tfm = tfm;
 	desc->flags = 0;
drivers/target/iscsi/iscsi_target_datain_values.c +2 -4
index 173ddd93c757..c591165f9b1b 100644
@@ -30,11 +30,9 @@ struct iscsi_datain_req *iscsit_allocate_datain_req(void)
 	struct iscsi_datain_req *dr;
 
 	dr = kmem_cache_zalloc(lio_dr_cache, GFP_ATOMIC);
-	if (!dr) {
-		pr_err("Unable to allocate memory for"
-				" struct iscsi_datain_req\n");
+	if (!dr)
 		return NULL;
-	}
+
 	INIT_LIST_HEAD(&dr->cmd_datain_node);
 
 	return dr;
drivers/target/iscsi/iscsi_target_erl1.c +4 -8
index 5efa42b939a1..ff3e08b6d4e1 100644
@@ -59,11 +59,9 @@ int iscsit_dump_data_payload(
 	length = min(buf_len, OFFLOAD_BUF_SIZE);
 
 	buf = kzalloc(length, GFP_ATOMIC);
-	if (!buf) {
-		pr_err("Unable to allocate %u bytes for offload"
-				" buffer.\n", length);
+	if (!buf)
 		return -1;
-	}
+
 	memset(&iov, 0, sizeof(struct kvec));
 
 	while (offset < buf_len) {
@@ -787,11 +785,9 @@ static struct iscsi_ooo_cmdsn *iscsit_allocate_ooo_cmdsn(void)
 	struct iscsi_ooo_cmdsn *ooo_cmdsn = NULL;
 
 	ooo_cmdsn = kmem_cache_zalloc(lio_ooo_cache, GFP_ATOMIC);
-	if (!ooo_cmdsn) {
-		pr_err("Unable to allocate memory for"
-			" struct iscsi_ooo_cmdsn.\n");
+	if (!ooo_cmdsn)
 		return NULL;
-	}
+
 	INIT_LIST_HEAD(&ooo_cmdsn->ooo_list);
 
 	return ooo_cmdsn;
drivers/target/iscsi/iscsi_target_erl2.c +2 -4
index 8df9c90f3db3..87c27e8d4f49 100644
@@ -325,11 +325,9 @@ int iscsit_prepare_cmds_for_reallegiance(struct iscsi_conn *conn)
 	 * connection's command list for connection recovery.
 	 */
 	cr = kzalloc(sizeof(struct iscsi_conn_recovery), GFP_KERNEL);
-	if (!cr) {
-		pr_err("Unable to allocate memory for"
-			" struct iscsi_conn_recovery.\n");
+	if (!cr)
 		return -1;
-	}
+
 	INIT_LIST_HEAD(&cr->cr_list);
 	INIT_LIST_HEAD(&cr->conn_recovery_cmd_list);
 	spin_lock_init(&cr->conn_recovery_cmd_lock);
drivers/target/iscsi/iscsi_target_login.c +5 -18
index 64c5a57b92e4..cfaf564825e0 100644
@@ -47,32 +47,24 @@ static struct iscsi_login *iscsi_login_init_conn(struct iscsi_conn *conn)
 	struct iscsi_login *login;
 
 	login = kzalloc(sizeof(struct iscsi_login), GFP_KERNEL);
-	if (!login) {
-		pr_err("Unable to allocate memory for struct iscsi_login.\n");
+	if (!login)
 		return NULL;
-	}
+
 	conn->login = login;
 	login->conn = conn;
 	login->first_request = 1;
 
 	login->req_buf = kzalloc(MAX_KEY_VALUE_PAIRS, GFP_KERNEL);
-	if (!login->req_buf) {
-		pr_err("Unable to allocate memory for response buffer.\n");
+	if (!login->req_buf)
 		goto out_login;
-	}
 
 	login->rsp_buf = kzalloc(MAX_KEY_VALUE_PAIRS, GFP_KERNEL);
-	if (!login->rsp_buf) {
-		pr_err("Unable to allocate memory for request buffer.\n");
+	if (!login->rsp_buf)
 		goto out_req_buf;
-	}
 
 	conn->conn_ops = kzalloc(sizeof(struct iscsi_conn_ops), GFP_KERNEL);
-	if (!conn->conn_ops) {
-		pr_err("Unable to allocate memory for"
-			" struct iscsi_conn_ops.\n");
+	if (!conn->conn_ops)
 		goto out_rsp_buf;
-	}
 
 	init_waitqueue_head(&conn->queues_wq);
 	INIT_LIST_HEAD(&conn->conn_list);
@@ -306,7 +298,6 @@ static int iscsi_login_zero_tsih_s1(
 	if (!sess) {
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
 				ISCSI_LOGIN_STATUS_NO_RESOURCES);
-		pr_err("Could not allocate memory for session\n");
 		return -ENOMEM;
 	}
 
@@ -363,8 +354,6 @@ static int iscsi_login_zero_tsih_s1(
 	if (!sess->sess_ops) {
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
 				ISCSI_LOGIN_STATUS_NO_RESOURCES);
-		pr_err("Unable to allocate memory for"
-				" struct iscsi_sess_ops.\n");
 		kfree(sess);
 		return -ENOMEM;
 	}
@@ -1257,8 +1246,6 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
 
 	conn = kzalloc(sizeof(struct iscsi_conn), GFP_KERNEL);
 	if (!conn) {
-		pr_err("Could not allocate memory for"
-			" new connection\n");
 		/* Get another socket */
 		return 1;
 	}
drivers/target/iscsi/iscsi_target_nego.c +1 -3
index b686e2ce9c0e..694842c772eb 100644
@@ -1075,10 +1075,8 @@ int iscsi_target_locate_portal(
 	payload_length = ntoh24(login_req->dlength);
 
 	tmpbuf = kzalloc(payload_length + 1, GFP_KERNEL);
-	if (!tmpbuf) {
-		pr_err("Unable to allocate memory for tmpbuf.\n");
+	if (!tmpbuf)
 		return -1;
-	}
 
 	memcpy(tmpbuf, login->req_buf, payload_length);
 	tmpbuf[payload_length] = '\0';
drivers/target/iscsi/iscsi_target_parameters.c +13 -29

98 lines not on this page · raw message

drivers/target/iscsi/iscsi_target_seq_pdu_list.c +8 -15
index f65e5e584212..3a6e619bb30e 100644
@@ -138,11 +138,9 @@ static int iscsit_randomize_pdu_lists(
 			continue;
 		}
 		array = kcalloc(seq_count, sizeof(u32), GFP_KERNEL);
-		if (!array) {
-			pr_err("Unable to allocate memory"
-				" for random array.\n");
+		if (!array)
 			return -ENOMEM;
-		}
+
 		iscsit_create_random_array(array, seq_count);
 
 		for (i = 0; i < seq_count; i++)
@@ -158,11 +156,9 @@ static int iscsit_randomize_pdu_lists(
 
 	if (seq_count) {
 		array = kcalloc(seq_count, sizeof(u32), GFP_KERNEL);
-		if (!array) {
-			pr_err("Unable to allocate memory for"
-				" random array.\n");
+		if (!array)
 			return -ENOMEM;
-		}
+
 		iscsit_create_random_array(array, seq_count);
 
 		for (i = 0; i < seq_count; i++)
@@ -190,10 +186,9 @@ static int iscsit_randomize_seq_lists(
 		return 0;
 
 	array = kcalloc(seq_count, sizeof(u32), GFP_KERNEL);
-	if (!array) {
-		pr_err("Unable to allocate memory for random array.\n");
+	if (!array)
 		return -ENOMEM;
-	}
+
 	iscsit_create_random_array(array, seq_count);
 
 	for (i = 0; i < cmd->seq_count; i++) {
@@ -544,10 +539,9 @@ int iscsit_build_pdu_and_seq_lists(
 
 	if (!conn->sess->sess_ops->DataSequenceInOrder) {
 		seq = kcalloc(seq_count, sizeof(struct iscsi_seq), GFP_ATOMIC);
-		if (!seq) {
-			pr_err("Unable to allocate struct iscsi_seq list\n");
+		if (!seq)
 			return -ENOMEM;
-		}
+
 		cmd->seq_list = seq;
 		cmd->seq_count = seq_count;
 	}
@@ -555,7 +549,6 @@ int iscsit_build_pdu_and_seq_lists(
 	if (!conn->sess->sess_ops->DataPDUInOrder) {
 		pdu = kcalloc(pdu_count, sizeof(struct iscsi_pdu), GFP_ATOMIC);
 		if (!pdu) {
-			pr_err("Unable to allocate struct iscsi_pdu list.\n");
 			kfree(seq);
 			return -ENOMEM;
 		}
drivers/target/iscsi/iscsi_target_tpg.c +2 -7
index 4b34f71547c6..c3a607b3ccc8 100644
@@ -35,10 +35,8 @@ struct iscsi_portal_group *iscsit_alloc_portal_group(struct iscsi_tiqn *tiqn, u1
 	struct iscsi_portal_group *tpg;
 
 	tpg = kzalloc(sizeof(struct iscsi_portal_group), GFP_KERNEL);
-	if (!tpg) {
-		pr_err("Unable to allocate struct iscsi_portal_group\n");
+	if (!tpg)
 		return NULL;
-	}
 
 	tpg->tpgt = tpgt;
 	tpg->tpg_state = TPG_STATE_FREE;
@@ -477,11 +475,8 @@ struct iscsi_tpg_np *iscsit_tpg_add_network_portal(
 	}
 
 	tpg_np = kzalloc(sizeof(struct iscsi_tpg_np), GFP_KERNEL);
-	if (!tpg_np) {
-		pr_err("Unable to allocate memory for"
-				" struct iscsi_tpg_np.\n");
+	if (!tpg_np)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	np = iscsit_add_np(sockaddr, network_transport);
 	if (IS_ERR(np)) {
drivers/target/iscsi/iscsi_target_util.c +4 -7
index 4435bf374d2d..30175e1f4672 100644
@@ -69,10 +69,9 @@ int iscsit_add_r2t_to_list(
 	struct iscsi_r2t *r2t;
 
 	r2t = kmem_cache_zalloc(lio_r2t_cache, GFP_ATOMIC);
-	if (!r2t) {
-		pr_err("Unable to allocate memory for struct iscsi_r2t.\n");
+	if (!r2t)
 		return -1;
-	}
+
 	INIT_LIST_HEAD(&r2t->r2t_list);
 
 	r2t->recovery_r2t = recovery;
@@ -577,11 +576,9 @@ int iscsit_add_cmd_to_response_queue(
 	struct iscsi_queue_req *qr;
 
 	qr = kmem_cache_zalloc(lio_qr_cache, GFP_ATOMIC);
-	if (!qr) {
-		pr_err("Unable to allocate memory for"
-			" struct iscsi_queue_req\n");
+	if (!qr)
 		return -ENOMEM;
-	}
+
 	INIT_LIST_HEAD(&qr->qr_list);
 	qr->cmd = cmd;
 	qr->state = state;
Signature · 1 line
2.15.1
#4 lore raw

PATCH 4/8 target/iscsi: Delete an unnecessary variable initialisation in iscsit_allocate_ooo_cmdsn()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 21:13:49 +0100

The local variable "ooo_cmdsn" will be reassigned by a following statement. Thus omit the explicit initialisation at the beginning.

---
 drivers/target/iscsi/iscsi_target_erl1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/target/iscsi/iscsi_target_erl1.c +1 -1
index ff3e08b6d4e1..c4c550128161 100644
@@ -782,7 +782,7 @@ int iscsit_recover_dataout_sequence(
 
 static struct iscsi_ooo_cmdsn *iscsit_allocate_ooo_cmdsn(void)
 {
-	struct iscsi_ooo_cmdsn *ooo_cmdsn = NULL;
+	struct iscsi_ooo_cmdsn *ooo_cmdsn;
 
 	ooo_cmdsn = kmem_cache_zalloc(lio_ooo_cache, GFP_ATOMIC);
 	if (!ooo_cmdsn)
Signature · 1 line
2.15.1
#5 lore raw

PATCH 5/8 target/iscsi: Delete an unnecessary variable initialisation in iscsi_copy_param_list()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 21:15:55 +0100

The variable "param_list" will be reassigned by a following statement. Thus omit the explicit initialisation at the beginning.

---
 drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/target/iscsi/iscsi_target_parameters.c +1 -1
index 06310b2c4e26..25a3a9550488 100644
@@ -566,7 +566,7 @@ int iscsi_copy_param_list(
 {
 	struct iscsi_param *param = NULL;
 	struct iscsi_param *new_param = NULL;
-	struct iscsi_param_list *param_list = NULL;
+	struct iscsi_param_list *param_list;
 
 	param_list = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
 	if (!param_list)
Signature · 1 line
2.15.1
#6 lore raw

PATCH 6/8 target/iscsi: Delete an unnecessary variable initialisation in iscsi_create_default_params()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 21:18:39 +0100

The variable "param" will eventually be set to an appropriate pointer a bit later. Thus omit the explicit initialisation at the beginning.

---
 drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/target/iscsi/iscsi_target_parameters.c +1 -1
index 25a3a9550488..151269b8816d 100644
@@ -196,7 +196,7 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
 /* #warning Add extension keys */
 int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
 {
-	struct iscsi_param *param = NULL;
+	struct iscsi_param *param;
 	struct iscsi_param_list *pl;
 
 	pl = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
Signature · 1 line
2.15.1
#7 lore raw

PATCH 7/8 target/iscsi: Delete an unnecessary variable initialisation in iscsi_set_default_param()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 21:21:31 +0100

The local variable "param" will be reassigned by a following statement. Thus omit the explicit initialisation at the beginning.

---
 drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/target/iscsi/iscsi_target_parameters.c +1 -1
index 151269b8816d..a49b0b2a4f6c 100644
@@ -127,7 +127,7 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
 		char *name, char *value, u8 phase, u8 scope, u8 sender,
 		u16 type_range, u8 use)
 {
-	struct iscsi_param *param = NULL;
+	struct iscsi_param *param;
 
 	param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
 	if (!param)
Signature · 1 line
2.15.1
#8 lore raw

PATCH 8/8 target/iscsi: Improve 16 size determinations

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Tue, 12 Dec 2017 22:06:00 +0100

Replace the specification of data types by pointer dereferences as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

---
 drivers/target/iscsi/iscsi_target_erl2.c         |  2 +-
 drivers/target/iscsi/iscsi_target_login.c        |  6 +++---
 drivers/target/iscsi/iscsi_target_parameters.c   | 10 +++++-----
 drivers/target/iscsi/iscsi_target_seq_pdu_list.c | 11 ++++++-----
 drivers/target/iscsi/iscsi_target_tpg.c          |  4 ++--
 5 files changed, 17 insertions(+), 16 deletions(-)
drivers/target/iscsi/iscsi_target_erl2.c +1 -1
index 87c27e8d4f49..80b8297f4b05 100644
@@ -324,7 +324,7 @@ int iscsit_prepare_cmds_for_reallegiance(struct iscsi_conn *conn)
 	 * (struct iscsi_cmd->cr) so we need to allocate this before preparing the
 	 * connection's command list for connection recovery.
 	 */
-	cr = kzalloc(sizeof(struct iscsi_conn_recovery), GFP_KERNEL);
+	cr = kzalloc(sizeof(*cr), GFP_KERNEL);
 	if (!cr)
 		return -1;
 
drivers/target/iscsi/iscsi_target_login.c +3 -3
index cfaf564825e0..095651e48f36 100644
@@ -46,7 +46,7 @@ static struct iscsi_login *iscsi_login_init_conn(struct iscsi_conn *conn)
 {
 	struct iscsi_login *login;
 
-	login = kzalloc(sizeof(struct iscsi_login), GFP_KERNEL);
+	login = kzalloc(sizeof(*login), GFP_KERNEL);
 	if (!login)
 		return NULL;
 
@@ -294,7 +294,7 @@ static int iscsi_login_zero_tsih_s1(
 	struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf;
 	int ret;
 
-	sess = kzalloc(sizeof(struct iscsi_session), GFP_KERNEL);
+	sess = kzalloc(sizeof(*sess), GFP_KERNEL);
 	if (!sess) {
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
 				ISCSI_LOGIN_STATUS_NO_RESOURCES);
@@ -1244,7 +1244,7 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
 	}
 	spin_unlock_bh(&np->np_thread_lock);
 
-	conn = kzalloc(sizeof(struct iscsi_conn), GFP_KERNEL);
+	conn = kzalloc(sizeof(*conn), GFP_KERNEL);
 	if (!conn) {
 		/* Get another socket */
 		return 1;
drivers/target/iscsi/iscsi_target_parameters.c +5 -5
index a49b0b2a4f6c..110e81a0c97d 100644
@@ -129,7 +129,7 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para
 {
 	struct iscsi_param *param;
 
-	param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
+	param = kzalloc(sizeof(*param), GFP_KERNEL);
 	if (!param)
 		goto out;
 
@@ -199,7 +199,7 @@ int iscsi_create_default_params(struct iscsi_param_list **param_list_ptr)
 	struct iscsi_param *param;
 	struct iscsi_param_list *pl;
 
-	pl = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
+	pl = kzalloc(sizeof(*pl), GFP_KERNEL);
 	if (!pl)
 		return -ENOMEM;
 
@@ -568,7 +568,7 @@ int iscsi_copy_param_list(
 	struct iscsi_param *new_param = NULL;
 	struct iscsi_param_list *param_list;
 
-	param_list = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
+	param_list = kzalloc(sizeof(*param_list), GFP_KERNEL);
 	if (!param_list)
 		return -ENOMEM;
 
@@ -583,7 +583,7 @@ int iscsi_copy_param_list(
 				continue;
 		}
 
-		new_param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL);
+		new_param = kzalloc(sizeof(*new_param), GFP_KERNEL);
 		if (!new_param)
 			goto err_out;
 
@@ -714,7 +714,7 @@ static int iscsi_add_notunderstood_response(
 		return -1;
 	}
 
-	extra_response = kzalloc(sizeof(struct iscsi_extra_response), GFP_KERNEL);
+	extra_response = kzalloc(sizeof(*extra_response), GFP_KERNEL);
 	if (!extra_response)
 		return -ENOMEM;
 
drivers/target/iscsi/iscsi_target_seq_pdu_list.c +6 -5
index 3a6e619bb30e..0fd914d92f4f 100644
@@ -137,7 +137,8 @@ static int iscsit_randomize_pdu_lists(
 			seq_count++;
 			continue;
 		}
-		array = kcalloc(seq_count, sizeof(u32), GFP_KERNEL);
+
+		array = kcalloc(seq_count, sizeof(*array), GFP_KERNEL);
 		if (!array)
 			return -ENOMEM;
 
@@ -155,7 +156,7 @@ static int iscsit_randomize_pdu_lists(
 	}
 
 	if (seq_count) {
-		array = kcalloc(seq_count, sizeof(u32), GFP_KERNEL);
+		array = kcalloc(seq_count, sizeof(*array), GFP_KERNEL);
 		if (!array)
 			return -ENOMEM;
 
@@ -185,7 +186,7 @@ static int iscsit_randomize_seq_lists(
 	if (!seq_count)
 		return 0;
 
-	array = kcalloc(seq_count, sizeof(u32), GFP_KERNEL);
+	array = kcalloc(seq_count, sizeof(*array), GFP_KERNEL);
 	if (!array)
 		return -ENOMEM;
 
@@ -538,7 +539,7 @@ int iscsit_build_pdu_and_seq_lists(
 	iscsit_determine_counts_for_list(cmd, &bl, &seq_count, &pdu_count);
 
 	if (!conn->sess->sess_ops->DataSequenceInOrder) {
-		seq = kcalloc(seq_count, sizeof(struct iscsi_seq), GFP_ATOMIC);
+		seq = kcalloc(seq_count, sizeof(*seq), GFP_ATOMIC);
 		if (!seq)
 			return -ENOMEM;
 
@@ -547,7 +548,7 @@ int iscsit_build_pdu_and_seq_lists(
 	}
 
 	if (!conn->sess->sess_ops->DataPDUInOrder) {
-		pdu = kcalloc(pdu_count, sizeof(struct iscsi_pdu), GFP_ATOMIC);
+		pdu = kcalloc(pdu_count, sizeof(*pdu), GFP_ATOMIC);
 		if (!pdu) {
 			kfree(seq);
 			return -ENOMEM;
drivers/target/iscsi/iscsi_target_tpg.c +2 -2
index c3a607b3ccc8..3298145f74f5 100644
@@ -34,7 +34,7 @@ struct iscsi_portal_group *iscsit_alloc_portal_group(struct iscsi_tiqn *tiqn, u1
 {
 	struct iscsi_portal_group *tpg;
 
-	tpg = kzalloc(sizeof(struct iscsi_portal_group), GFP_KERNEL);
+	tpg = kzalloc(sizeof(*tpg), GFP_KERNEL);
 	if (!tpg)
 		return NULL;
 
@@ -474,7 +474,7 @@ struct iscsi_tpg_np *iscsit_tpg_add_network_portal(
 		}
 	}
 
-	tpg_np = kzalloc(sizeof(struct iscsi_tpg_np), GFP_KERNEL);
+	tpg_np = kzalloc(sizeof(*tpg_np), GFP_KERNEL);
 	if (!tpg_np)
 		return ERR_PTR(-ENOMEM);
 
Signature · 1 line
2.15.1
#10 lore raw
In reply to SF Markus Elfring
Quoted 15 lines

Date: Tue, 12 Dec 2017 22:22:11 +0100

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (8):
Less function calls in chap_server_compute_md5() after error detection Move resetting of seven variables in chap_server_compute_md5() Delete 36 error messages for a failed memory allocation
Delete an unnecessary variable initialisation in iscsit_allocate_ooo_cmdsn() Delete an unnecessary variable initialisation in iscsi_copy_param_list() Delete an unnecessary variable initialisation in iscsi_create_default_params() Delete an unnecessary variable initialisation in iscsi_set_default_param() Improve 16 size determinations

Quoted 12 lines
 drivers/target/iscsi/iscsi_target.c               |   2 -
 drivers/target/iscsi/iscsi_target_auth.c          | 110 +++++++++++-----------
 drivers/target/iscsi/iscsi_target_datain_values.c |   6 +-
 drivers/target/iscsi/iscsi_target_erl1.c          |  14 +--
 drivers/target/iscsi/iscsi_target_erl2.c          |   8 +-
 drivers/target/iscsi/iscsi_target_login.c         |  29 ++----
 drivers/target/iscsi/iscsi_target_nego.c          |   4 +-
 drivers/target/iscsi/iscsi_target_parameters.c    |  58 +++++-------
 drivers/target/iscsi/iscsi_target_seq_pdu_list.c  |  34 +++----
 drivers/target/iscsi/iscsi_target_tpg.c           |  13 +--
 drivers/target/iscsi/iscsi_target_util.c          |  11 +--
 11 files changed, 118 insertions(+), 171 deletions(-)

One of these update suggestions resulted in the commit “target: avoid NULL dereference in CHAP auth error path� which is considered for integration into Linux stable versions now.
https://patchwork.kernel.org/patch/10110459/
https://lkml.kernel.org/r/<[email protected]>
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/target/iscsi/iscsi_target_auth.c?idÎ512d79d0466a604793addb6b769d12ee326822

Now I am curious if more remaining change possibilities can be picked up from this patch series.

* Would you like to improve any more implementation details?

* Do you need additional explanations for further benefits?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#11 lore raw
In reply to SF Markus Elfring

[cc list trimmed]

On Wed, 21 Feb 2018 17:45:35 +0100, SF Markus Elfring wrote:

...

Quoted 6 lines

One of these update suggestions resulted in the commit “target: avoid NULL dereference in CHAP auth error path� which is considered for integration into Linux stable versions now.
https://patchwork.kernel.org/patch/10110459/
https://lkml.kernel.org/r/<[email protected]>
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/target/iscsi/iscsi_target_auth.c?idÎ512d79d0466a604793addb6b769d12ee326822

You're 1/8 patch had an actual bug fix hidden amongst the style churn. I don't see any such fixes in the other patches.

Quoted 6 lines

Now I am curious if more remaining change possibilities can be picked up from this patch series.

* Would you like to improve any more implementation details?

* Do you need additional explanations for further benefits?

My opinion from https://www.spinics.net/lists/target-devel/msg16342.html
hasn't changed. FWIW, I'd prefer to see LIO adopt a policy similar to:
https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#How_not_to_start

Cheers, David
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#12 lore raw
Quoted 1 line

You're 1/8 patch had an actual bug fix hidden amongst the style churn.

It showed the general possibility to adjust the source code structure for the function “chap_server_compute_md5” also because of the usage of the single jump label “out” before.

Quoted 1 line

I don't see any such fixes in the other patches.

This view is appropriate.

Further update steps show different transformation possibilities.

Quoted 3 lines

My opinion from https://www.spinics.net/lists/target-devel/msg16342.html
hasn't changed. FWIW, I'd prefer to see LIO adopt a policy similar to:
https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#How_not_to_start

It seems that you express a few aspects for general change resistance. Will the circumstances evolve for similar software improvements?

Regards,
Markus

#13 lore raw
In reply to SF Markus Elfring
Quoted 12 lines · David Disseldorp

[cc list trimmed]

On Wed, 21 Feb 2018 17:45:35 +0100, SF Markus Elfring wrote:

...

Quoted 6 lines

One of these update suggestions resulted in the commit “target: avoid NULL dereference in CHAP auth error path� which is considered for integration into Linux stable versions now.
https://patchwork.kernel.org/patch/10110459/
https://lkml.kernel.org/r/<[email protected]>
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/target/iscsi/iscsi_target_auth.c?idÎ512d79d0466a604793addb6b769d12ee326822

Quoted 3 lines

You're 1/8 patch had an actual bug fix hidden amongst the style churn. I don't see any such fixes in the other patches.

Calling crypto_free_shash(NULL) is actually fine. It doesn't dereference the parameter, it just does pointer math on it in crypto_shash_tfm() and returns if it's NULL in crypto_destroy_tfm().

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#14 lore raw
Quoted 1 line

Calling crypto_free_shash(NULL) is actually fine.

Really?

Quoted 2 lines

It doesn't dereference the parameter, it just does pointer math on it in crypto_shash_tfm() and returns if it's NULL in crypto_destroy_tfm().

Can a passed null pointer really work in this function?

https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id9da844d87796ac31b04e81ee95e155e9043132#n751

static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)

{
	return &tfm->base;
}

Regards,
Markus

#15 lore raw
Quoted 10 lines · SF Markus Elfring
Quoted 1 line

Calling crypto_free_shash(NULL) is actually fine.

Really?

Quoted 2 lines

It doesn't dereference the parameter, it just does pointer math on it in crypto_shash_tfm() and returns if it's NULL in crypto_destroy_tfm().

Can a passed null pointer really work in this function?

Quoted 7 lines

https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id9da844d87796ac31b04e81ee95e155e9043132#n751

static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)

{
	return &tfm->base;
}

Yes. It's not a dereference, it's just doing pointer math to get the address.

regards,
dan carpenter

#16 lore raw
Quoted 11 lines
Quoted 9 lines

Can a passed null pointer really work in this function?

https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id9da844d87796ac31b04e81ee95e155e9043132#n751

static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)

{
	return &tfm->base;
}

Yes. It's not a dereference,

Do any processors treat the zero address still special there?

Quoted 1 line

it's just doing pointer math to get the address.

Can eventually happen anything unexpected?

Can it be nicer to avoid such a software behaviour concern generally just by adjusting a few jump labels (as I proposed it)?

Regards,
Markus

#17 lore raw
In reply to SF Markus Elfring

On Thu, 22 Feb 2018 16:56:00 +0300, Dan Carpenter wrote:

...

Quoted 3 lines

Calling crypto_free_shash(NULL) is actually fine. It doesn't dereference the parameter, it just does pointer math on it in crypto_shash_tfm() and returns if it's NULL in crypto_destroy_tfm().

Bleh, you're right. I read the crypto_shash_tfm() code path incorrectly.