target-iSCSI: Adjustments for several function implementations
Subsystems (3)
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
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.
--- drivers/target/iscsi/iscsi_target_auth.c | 71 +++++++++++++++++--------------- 1 file changed, 37 insertions(+), 34 deletions(-)
drivers/target/iscsi/iscsi_target_auth.c +37 -34
@@ -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
PATCH 2/8 target/iscsi: Move resetting of seven variables in chap_server_compute_md5()
In reply to SF Markus ElfringFrom: 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
@@ -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
PATCH 3/8 target/iscsi: Delete 36 error messages for a failed memory allocation
In reply to SF Markus ElfringFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -35,10 +35,8 @@ struct iscsi_portal_group *iscsit_alloc_portal_group(struct iscsi_tiqn *tiqn, u1struct 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
@@ -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
PATCH 4/8 target/iscsi: Delete an unnecessary variable initialisation in iscsit_allocate_ooo_cmdsn()
In reply to SF Markus ElfringFrom: 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
@@ -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
PATCH 5/8 target/iscsi: Delete an unnecessary variable initialisation in iscsi_copy_param_list()
In reply to SF Markus ElfringFrom: 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
@@ -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
PATCH 6/8 target/iscsi: Delete an unnecessary variable initialisation in iscsi_create_default_params()
In reply to SF Markus ElfringFrom: 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
@@ -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
PATCH 7/8 target/iscsi: Delete an unnecessary variable initialisation in iscsi_set_default_param()
In reply to SF Markus ElfringFrom: 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
@@ -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
PATCH 8/8 target/iscsi: Improve 16 size determinations
In reply to SF Markus ElfringFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
Aside from the crypto_free_shash(NULL) error path fix in 1/8, I don't see a need for any of the style-based churn here.
Cheers, David
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 inchap_server_compute_md5()after error detection Move resetting of seven variables inchap_server_compute_md5()Delete 36 error messages for a failed memory allocation
Delete an unnecessary variable initialisation iniscsit_allocate_ooo_cmdsn()Delete an unnecessary variable initialisation iniscsi_copy_param_list()Delete an unnecessary variable initialisation iniscsi_create_default_params()Delete an unnecessary variable initialisation iniscsi_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
[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
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
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
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 incrypto_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
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 incrypto_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#n751static inline struct
crypto_tfm*crypto_shash_tfm(structcrypto_shash*tfm){ return &tfm->base; }
Yes. It's not a dereference, it's just doing pointer math to get the address.
regards,
dan carpenter
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#n751static inline struct
crypto_tfm*crypto_shash_tfm(structcrypto_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
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 incrypto_shash_tfm()and returns if it's NULL incrypto_destroy_tfm().
Bleh, you're right. I read the crypto_shash_tfm() code path incorrectly.