[PATCH 02/17] target: fix memory corruption

[Joern Engel]

Found by coverity. l_conn cannot possible be NULL. It can very well
not match any connection, which is the logical equivalent of NULL. At
that point we would happily dereference the pointer and do heck knows
what with the random values we find. A NULL pointer dereference would
have been much better.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 30f4a7d42e32..f9d2b1255856 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4549,7 +4549,7 @@ static void iscsit_logout_post_handler_diffcid(
}
spin_unlock_bh(&sess->conn_lock);

- if (!l_conn)
+ if (&l_conn->conn_list == &sess->sess_conn_list)
return;

if (l_conn->sock)

[Joern Engel]

iscsi_release_param_list() cannot handle a NULL pointer. Found by
coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 02f9de26f38a..18c29260b4a2 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -601,7 +601,7 @@ int iscsi_copy_param_list(
param_list = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
if (!param_list) {
pr_err("Unable to allocate memory for struct iscsi_param_list.\n");
- goto err_out;
+ return -1;
}
INIT_LIST_HEAD(&param_list->param_list);
INIT_LIST_HEAD(&param_list->extra_response_list);

[Sagi Grimberg]

Post by Joern Engel
iscsi_release_param_list() cannot handle a NULL pointer. Found by
coverity.
---
drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c
index 02f9de26f38a..18c29260b4a2 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -601,7 +601,7 @@ int iscsi_copy_param_list(
param_list = kzalloc(sizeof(struct iscsi_param_list), GFP_KERNEL);
if (!param_list) {
pr_err("Unable to allocate memory for struct iscsi_param_list.\n");
- goto err_out;
+ return -1;
}
INIT_LIST_HEAD(&param_list->param_list);
INIT_LIST_HEAD(&param_list->extra_response_list);

Why won't you escalate correct error ENOMEM?
Although the caller don't care about it...

might be a good idea to follow up if return codes should propagate
better in this area...

Sagi.

[Jörn Engel]

Post by Sagi Grimberg

Post by Joern Engel
iscsi_release_param_list() cannot handle a NULL pointer. Found by
coverity.
---
drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/driver=

s/target/iscsi/iscsi_target_parameters.c

Post by Sagi Grimberg

Post by Joern Engel
index 02f9de26f38a..18c29260b4a2 100644
--- a/drivers/target/iscsi/iscsi_target_parameters.c
+++ b/drivers/target/iscsi/iscsi_target_parameters.c
@@ -601,7 +601,7 @@ int iscsi_copy_param_list(
param_list =3D kzalloc(sizeof(struct iscsi_param_list), GFP_KERNE=

L);

Post by Sagi Grimberg

Post by Joern Engel
if (!param_list) {
pr_err("Unable to allocate memory for struct iscsi_param_list.\n=

");

Post by Sagi Grimberg

Post by Joern Engel
- goto err_out;
+ return -1;
}
INIT_LIST_HEAD(&param_list->param_list);
INIT_LIST_HEAD(&param_list->extra_response_list);

=20
Why won't you escalate correct error ENOMEM?
Although the caller don't care about it...
=20
might be a good idea to follow up if return codes should propagate
better in this area...

I'm not even sure if we can do anything smarter than we do right now.
To the initiator we are just a disk and the subtle differences between
-ENOMEM, -ETIMEDOUT and -EIO are unlikely to matter after translation
to scsi sense codes.

Then again, I may be entirely wrong and this may be a good project for
someone else to work on.

J=C3=B6rn

--
The rabbit runs faster than the fox, because the rabbit is rinning for
his life while the fox is only running for his dinner.
-- Aesop

[Joern Engel]

Last user of buf was removed with c6037cc546ca. While at it,
free_cpumask_var() handles a NULL argument just fine, so remove the
conditionals.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target.c | 6 +-----
drivers/target/iscsi/iscsi_target_login.c | 3 +--
2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1f4c794f5fcc..30f4a7d42e32 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3709,7 +3709,6 @@ static inline void iscsit_thread_check_cpumask(
struct task_struct *p,
int mode)
{
- char buf[128];
/*
* mode == 1 signals iscsi_target_tx_thread() usage.
* mode == 0 signals iscsi_target_rx_thread() usage.
@@ -3728,8 +3727,6 @@ static inline void iscsit_thread_check_cpumask(
* both TX and RX kthreads are scheduled to run on the
* same CPU.
*/
- memset(buf, 0, 128);
- cpumask_scnprintf(buf, 128, conn->conn_cpumask);
set_cpus_allowed_ptr(p, conn->conn_cpumask);
}

@@ -4326,8 +4323,7 @@ int iscsit_close_connection(
if (conn->conn_tx_hash.tfm)
crypto_free_hash(conn->conn_tx_hash.tfm);

- if (conn->conn_cpumask)
- free_cpumask_var(conn->conn_cpumask);
+ free_cpumask_var(conn->conn_cpumask);

kfree(conn->conn_ops);
conn->conn_ops = NULL;
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 5e71ac609418..b1ae5cbe70f8 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1190,8 +1190,7 @@ old_sess_out:
if (!IS_ERR(conn->conn_tx_hash.tfm))
crypto_free_hash(conn->conn_tx_hash.tfm);

- if (conn->conn_cpumask)
- free_cpumask_var(conn->conn_cpumask);
+ free_cpumask_var(conn->conn_cpumask);

kfree(conn->conn_ops);

[Sagi Grimberg]

Post by Joern Engel
Last user of buf was removed with c6037cc546ca. While at it,
free_cpumask_var() handles a NULL argument just fine, so remove the
conditionals.
---
drivers/target/iscsi/iscsi_target.c | 6 +-----
drivers/target/iscsi/iscsi_target_login.c | 3 +--
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1f4c794f5fcc..30f4a7d42e32 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3709,7 +3709,6 @@ static inline void iscsit_thread_check_cpumask(
struct task_struct *p,
int mode)
{
- char buf[128];
/*
* mode == 1 signals iscsi_target_tx_thread() usage.
* mode == 0 signals iscsi_target_rx_thread() usage.
@@ -3728,8 +3727,6 @@ static inline void iscsit_thread_check_cpumask(
* both TX and RX kthreads are scheduled to run on the
* same CPU.
*/
- memset(buf, 0, 128);
- cpumask_scnprintf(buf, 128, conn->conn_cpumask);
set_cpus_allowed_ptr(p, conn->conn_cpumask);
}
@@ -4326,8 +4323,7 @@ int iscsit_close_connection(
if (conn->conn_tx_hash.tfm)
crypto_free_hash(conn->conn_tx_hash.tfm);
- if (conn->conn_cpumask)
- free_cpumask_var(conn->conn_cpumask);
+ free_cpumask_var(conn->conn_cpumask);
kfree(conn->conn_ops);
conn->conn_ops = NULL;
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 5e71ac609418..b1ae5cbe70f8 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
if (!IS_ERR(conn->conn_tx_hash.tfm))
crypto_free_hash(conn->conn_tx_hash.tfm);
- if (conn->conn_cpumask)
- free_cpumask_var(conn->conn_cpumask);
+ free_cpumask_var(conn->conn_cpumask);
kfree(conn->conn_ops);

Looks good to me.

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

Found by coverity. At this point sock is provably non-NULL.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target_login.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 0df2dcd0a273..d280d2f9cc29 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -978,8 +978,7 @@ int iscsit_setup_np(
return 0;
fail:
np->np_socket = NULL;
- if (sock)
- sock_release(sock);
+ sock_release(sock);
return ret;
}

[Sagi Grimberg]

Post by Joern Engel
Found by coverity. At this point sock is provably non-NULL.
---
drivers/target/iscsi/iscsi_target_login.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 0df2dcd0a273..d280d2f9cc29 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -978,8 +978,7 @@ int iscsit_setup_np(
return 0;
np->np_socket = NULL;
- if (sock)
- sock_release(sock);
+ sock_release(sock);
return ret;
}

Looks good to me.

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

The return statement cannot be reached without either recovery or dump
being set to 1. Therefore the condition always evaluates to true.
Found by Coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target_erl0.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c
index 0d1e6ee3e992..ed6c28872784 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
@@ -392,8 +392,7 @@ dump:
if (iscsit_dump_data_payload(conn, payload_length, 1) < 0)
return DATAOUT_CANNOT_RECOVER;

- return (recovery || dump) ? DATAOUT_WITHIN_COMMAND_RECOVERY :
- DATAOUT_NORMAL;
+ return DATAOUT_WITHIN_COMMAND_RECOVERY;
}

static int iscsit_dataout_pre_datapduinorder_yes(

[Sagi Grimberg]

Post by Joern Engel
The return statement cannot be reached without either recovery or dump
being set to 1. Therefore the condition always evaluates to true.
Found by Coverity.
---
drivers/target/iscsi/iscsi_target_erl0.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c
index 0d1e6ee3e992..ed6c28872784 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
if (iscsit_dump_data_payload(conn, payload_length, 1) < 0)
return DATAOUT_CANNOT_RECOVER;
- DATAOUT_NORMAL;
+ return DATAOUT_WITHIN_COMMAND_RECOVERY;
}
static int iscsit_dataout_pre_datapduinorder_yes(

Won't Coverity complain now that recovery and dump are meaningless now?

Sagi.

[Joern Engel]

Found by coverity. It appears as if the initiator can cause a kernel
NULL pointer dereference at will. Some might consider such behaviour
bad. My trivial patch will avoid such badness, at the cost of
potentially introducing unexpected behaviour - the internals of
iscsit_handle_nop_out() are complicated and don't always dereference
NULL.

Better patches are welcome. But in the absence of a better patch, this
at least doesn't leave trivial DoS vectors open to the public.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index f9d2b1255856..416824a168c2 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4007,9 +4007,9 @@ static int iscsi_target_rx_opcode(struct iscsi_conn *conn, unsigned char *buf)
cmd = NULL;
if (hdr->ttt == cpu_to_be32(0xFFFFFFFF)) {
cmd = iscsit_allocate_cmd(conn, TASK_INTERRUPTIBLE);
- if (!cmd)
- goto reject;
}
+ if (!cmd)
+ goto reject;
ret = iscsit_handle_nop_out(conn, cmd, buf);
break;
case ISCSI_OP_SCSI_TMFUNC:

[Sagi Grimberg]

Post by Joern Engel
Found by coverity. It appears as if the initiator can cause a kernel
NULL pointer dereference at will. Some might consider such behaviour
bad. My trivial patch will avoid such badness, at the cost of
potentially introducing unexpected behaviour - the internals of
iscsit_handle_nop_out() are complicated and don't always dereference
NULL.
Better patches are welcome. But in the absence of a better patch, this
at least doesn't leave trivial DoS vectors open to the public.
---
drivers/target/iscsi/iscsi_target.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index f9d2b1255856..416824a168c2 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4007,9 +4007,9 @@ static int iscsi_target_rx_opcode(struct iscsi_conn *conn, unsigned char *buf)
cmd = NULL;
if (hdr->ttt == cpu_to_be32(0xFFFFFFFF)) {
cmd = iscsit_allocate_cmd(conn, TASK_INTERRUPTIBLE);
- if (!cmd)
- goto reject;
}
+ if (!cmd)
+ goto reject;
ret = iscsit_handle_nop_out(conn, cmd, buf);
break;

Thanks Coverity.

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

last_intr_fail_name is a fixed-size array and could theoretically
overflow. In reality intrname->value doesn't seem to depend on
untrusted input or be anywhere near 224 characters, so the overflow is
pretty theoretical. But strlcpy is cheap enough. Found by coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target_util.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index fd90b28f1d94..5d611d7ba282 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -1479,8 +1479,9 @@ void iscsit_collect_login_stats(
if (conn->param_list)
intrname = iscsi_find_param_from_key(INITIATORNAME,
conn->param_list);
- strcpy(ls->last_intr_fail_name,
- (intrname ? intrname->value : "Unknown"));
+ strlcpy(ls->last_intr_fail_name,
+ (intrname ? intrname->value : "Unknown"),
+ sizeof(ls->last_intr_fail_name));

ls->last_intr_fail_ip_family = conn->login_family;

[Sagi Grimberg]

Post by Joern Engel
last_intr_fail_name is a fixed-size array and could theoretically
overflow. In reality intrname->value doesn't seem to depend on
untrusted input or be anywhere near 224 characters, so the overflow is
pretty theoretical. But strlcpy is cheap enough. Found by coverity.
---
drivers/target/iscsi/iscsi_target_util.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index fd90b28f1d94..5d611d7ba282 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -1479,8 +1479,9 @@ void iscsit_collect_login_stats(
if (conn->param_list)
intrname = iscsi_find_param_from_key(INITIATORNAME,
conn->param_list);
- strcpy(ls->last_intr_fail_name,
- (intrname ? intrname->value : "Unknown"));
+ strlcpy(ls->last_intr_fail_name,
+ (intrname ? intrname->value : "Unknown"),
+ sizeof(ls->last_intr_fail_name));
ls->last_intr_fail_ip_family = conn->login_family;

fair enough...

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

At this point in the function ret cannot possibly be -ENODEV. I
strongly suspect that comparing rc to -ENODEV was meant when writing the
code. Then again, we have lived with a technically non-existent
condition for a long time and maybe the right thing is to just remove
it.

Nick, any opinion on this matter?

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target_login.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index b1ae5cbe70f8..0df2dcd0a273 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1267,7 +1267,7 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
iscsit_put_transport(conn->conn_transport);
kfree(conn);
conn = NULL;
- if (ret == -ENODEV)
+ if (rc == -ENODEV)
goto out;
/* Get another socket */
return 1;

[Sagi Grimberg]

Post by Joern Engel
At this point in the function ret cannot possibly be -ENODEV. I
strongly suspect that comparing rc to -ENODEV was meant when writing the
code. Then again, we have lived with a technically non-existent
condition for a long time and maybe the right thing is to just remove
it.
Nick, any opinion on this matter?
---
drivers/target/iscsi/iscsi_target_login.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index b1ae5cbe70f8..0df2dcd0a273 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1267,7 +1267,7 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
iscsit_put_transport(conn->conn_transport);
kfree(conn);
conn = NULL;
- if (ret == -ENODEV)
+ if (rc == -ENODEV)
goto out;
/* Get another socket */
return 1;

iscsit_accept_np can definitely return ENODEV - in this case it is
pointless to continue with the login thread...

The patch is OK.

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

Found by coverity. InitiatorName and InitiatorAlias are static arrays
and therefore always non-NULL. At some point in the past they may have
been dynamically allocated, but for current code the condition is
useless. If the intent was to check InitiatorName[0] instead, I cannot
find a use for that either. Let's get rid of it.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target_configfs.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index ae03f3e5de1e..9059c1e0b26e 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -669,12 +669,10 @@ static ssize_t lio_target_nacl_show_info(
} else {
sess = se_sess->fabric_sess_ptr;

- if (sess->sess_ops->InitiatorName)
- rb += sprintf(page+rb, "InitiatorName: %s\n",
- sess->sess_ops->InitiatorName);
- if (sess->sess_ops->InitiatorAlias)
- rb += sprintf(page+rb, "InitiatorAlias: %s\n",
- sess->sess_ops->InitiatorAlias);
+ rb += sprintf(page+rb, "InitiatorName: %s\n",
+ sess->sess_ops->InitiatorName);
+ rb += sprintf(page+rb, "InitiatorAlias: %s\n",
+ sess->sess_ops->InitiatorAlias);

rb += sprintf(page+rb, "LIO Session ID: %u "
"ISID: 0x%02x %02x %02x %02x %02x %02x "

[Sagi Grimberg]

Post by Joern Engel
Found by coverity. InitiatorName and InitiatorAlias are static arrays
and therefore always non-NULL. At some point in the past they may have
been dynamically allocated, but for current code the condition is
useless. If the intent was to check InitiatorName[0] instead, I cannot
find a use for that either. Let's get rid of it.
---
drivers/target/iscsi/iscsi_target_configfs.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index ae03f3e5de1e..9059c1e0b26e 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -669,12 +669,10 @@ static ssize_t lio_target_nacl_show_info(
} else {
sess = se_sess->fabric_sess_ptr;
- if (sess->sess_ops->InitiatorName)
- rb += sprintf(page+rb, "InitiatorName: %s\n",
- sess->sess_ops->InitiatorName);
- if (sess->sess_ops->InitiatorAlias)
- rb += sprintf(page+rb, "InitiatorAlias: %s\n",
- sess->sess_ops->InitiatorAlias);
+ rb += sprintf(page+rb, "InitiatorName: %s\n",
+ sess->sess_ops->InitiatorName);
+ rb += sprintf(page+rb, "InitiatorAlias: %s\n",
+ sess->sess_ops->InitiatorAlias);
rb += sprintf(page+rb, "LIO Session ID: %u "
"ISID: 0x%02x %02x %02x %02x %02x %02x "

Looks good to me.

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

Each case of match_strdup could leak memory if the same argument was
present before. So "target_lun=foo target_lun=bar" will leak a few
bytes of memory and you have to be both root and somewhat stupid to
trigger the leak. We can live with that.

But arg_p is different, as it will always leak memory. Let's plug that
one. And while at it, replace some &args[0] with args.

Found by coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/target_core_configfs.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index bf55c5a04cfa..291dc711fbc3 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1263,7 +1263,7 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
{
unsigned char *i_fabric = NULL, *i_port = NULL, *isid = NULL;
unsigned char *t_fabric = NULL, *t_port = NULL;
- char *orig, *ptr, *arg_p, *opts;
+ char *orig, *ptr, *opts;
substring_t args[MAX_OPT_ARGS];
unsigned long long tmp_ll;
u64 sa_res_key = 0;
@@ -1295,14 +1295,14 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
token = match_token(ptr, tokens, args);
switch (token) {
case Opt_initiator_fabric:
- i_fabric = match_strdup(&args[0]);
+ i_fabric = match_strdup(args);
if (!i_fabric) {
ret = -ENOMEM;
goto out;
}
break;
case Opt_initiator_node:
- i_port = match_strdup(&args[0]);
+ i_port = match_strdup(args);
if (!i_port) {
ret = -ENOMEM;
goto out;
@@ -1316,7 +1316,7 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
}
break;
case Opt_initiator_sid:
- isid = match_strdup(&args[0]);
+ isid = match_strdup(args);
if (!isid) {
ret = -ENOMEM;
goto out;
@@ -1330,15 +1330,9 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
}
break;
case Opt_sa_res_key:
- arg_p = match_strdup(&args[0]);
- if (!arg_p) {
- ret = -ENOMEM;
- goto out;
- }
- ret = kstrtoull(arg_p, 0, &tmp_ll);
+ ret = kstrtoull(args->from, 0, &tmp_ll);
if (ret < 0) {
- pr_err("kstrtoull() failed for"
- " sa_res_key=\n");
+ pr_err("kstrtoull() failed for sa_res_key=\n");
goto out;
}
sa_res_key = (u64)tmp_ll;
@@ -1370,14 +1364,14 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
* PR APTPL Metadata for Target Port
*/
case Opt_target_fabric:
- t_fabric = match_strdup(&args[0]);
+ t_fabric = match_strdup(args);
if (!t_fabric) {
ret = -ENOMEM;
goto out;
}
break;
case Opt_target_node:
- t_port = match_strdup(&args[0]);
+ t_port = match_strdup(args);
if (!t_port) {
ret = -ENOMEM;
goto out;

[Sagi Grimberg]

Post by Joern Engel
Each case of match_strdup could leak memory if the same argument was
present before. So "target_lun=foo target_lun=bar" will leak a few
bytes of memory and you have to be both root and somewhat stupid to
trigger the leak. We can live with that.

I would choose a better phrasing here...

Post by Joern Engel
But arg_p is different, as it will always leak memory. Let's plug that
one. And while at it, replace some &args[0] with args.
Found by coverity.
---
drivers/target/target_core_configfs.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index bf55c5a04cfa..291dc711fbc3 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -1263,7 +1263,7 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
{
unsigned char *i_fabric = NULL, *i_port = NULL, *isid = NULL;
unsigned char *t_fabric = NULL, *t_port = NULL;
- char *orig, *ptr, *arg_p, *opts;
+ char *orig, *ptr, *opts;
substring_t args[MAX_OPT_ARGS];
unsigned long long tmp_ll;
u64 sa_res_key = 0;
@@ -1295,14 +1295,14 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
token = match_token(ptr, tokens, args);
switch (token) {
- i_fabric = match_strdup(&args[0]);
+ i_fabric = match_strdup(args);
if (!i_fabric) {
ret = -ENOMEM;
goto out;
}
break;
- i_port = match_strdup(&args[0]);
+ i_port = match_strdup(args);
if (!i_port) {
ret = -ENOMEM;
goto out;
@@ -1316,7 +1316,7 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
}
break;
- isid = match_strdup(&args[0]);
+ isid = match_strdup(args);
if (!isid) {
ret = -ENOMEM;
goto out;
@@ -1330,15 +1330,9 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
}
break;
- arg_p = match_strdup(&args[0]);
- if (!arg_p) {
- ret = -ENOMEM;
- goto out;
- }
- ret = kstrtoull(arg_p, 0, &tmp_ll);
+ ret = kstrtoull(args->from, 0, &tmp_ll);
if (ret < 0) {
- pr_err("kstrtoull() failed for"
- " sa_res_key=\n");
+ pr_err("kstrtoull() failed for sa_res_key=\n");
goto out;
}
sa_res_key = (u64)tmp_ll;
@@ -1370,14 +1364,14 @@ static ssize_t target_core_dev_pr_store_attr_res_aptpl_metadata(
* PR APTPL Metadata for Target Port
*/
- t_fabric = match_strdup(&args[0]);
+ t_fabric = match_strdup(args);
if (!t_fabric) {
ret = -ENOMEM;
goto out;
}
break;
- t_port = match_strdup(&args[0]);
+ t_port = match_strdup(args);
if (!t_port) {
ret = -ENOMEM;
goto out;

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Jörn Engel]

Post by Sagi Grimberg

Post by Joern Engel
Each case of match_strdup could leak memory if the same argument was
present before. So "target_lun=3Dfoo target_lun=3Dbar" will leak a =

few

Post by Sagi Grimberg

Post by Joern Engel
bytes of memory and you have to be both root and somewhat stupid to
trigger the leak. We can live with that.

=20
I would choose a better phrasing here...

Agreed. Do you have a suggestion?

J=C3=B6rn

--
What one programmer can do in one month, two programmers can do in two =
months.
-- Fred Brooks

[Joern Engel]

Found by coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/target_core_fabric_configfs.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 7de9f0475d05..ab8a27eff94c 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -381,7 +381,7 @@ static struct config_group *target_fabric_make_mappedlun(
if (!lacl_cg->default_groups) {
pr_err("Unable to allocate lacl_cg->default_groups\n");
ret = -ENOMEM;
- goto out;
+ goto out2;
}

config_group_init_type_name(&lacl->se_lun_group, name,
@@ -397,12 +397,14 @@ static struct config_group *target_fabric_make_mappedlun(
if (!ml_stat_grp->default_groups) {
pr_err("Unable to allocate ml_stat_grp->default_groups\n");
ret = -ENOMEM;
- goto out;
+ goto out2;
}
target_stat_setup_mappedlun_default_groups(lacl);

kfree(buf);
return &lacl->se_lun_group;
+out2:
+ kfree(lacl);
out:
if (lacl_cg)
kfree(lacl_cg->default_groups);

[Sagi Grimberg]

Post by Joern Engel
Found by coverity.
---
drivers/target/target_core_fabric_configfs.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 7de9f0475d05..ab8a27eff94c 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -381,7 +381,7 @@ static struct config_group *target_fabric_make_mappedlun(
if (!lacl_cg->default_groups) {
pr_err("Unable to allocate lacl_cg->default_groups\n");
ret = -ENOMEM;
- goto out;
+ goto out2;
}
config_group_init_type_name(&lacl->se_lun_group, name,
@@ -397,12 +397,14 @@ static struct config_group *target_fabric_make_mappedlun(
if (!ml_stat_grp->default_groups) {
pr_err("Unable to allocate ml_stat_grp->default_groups\n");
ret = -ENOMEM;
- goto out;
+ goto out2;
}
target_stat_setup_mappedlun_default_groups(lacl);
kfree(buf);
return &lacl->se_lun_group;
+ kfree(lacl);

1. Please use a meaningful tag name.
2. Not sure you need a tag here, lacl is either NULL or should
be freed - you can go ahead and free it under out is kfree is
NULL safe (I even think Coverity complains about it, or is it smatch?)

Post by Joern Engel
if (lacl_cg)
kfree(lacl_cg->default_groups);

[Jörn Engel]

Post by Sagi Grimberg

Post by Joern Engel
Found by coverity.
---
drivers/target/target_core_fabric_configfs.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/=

target/target_core_fabric_configfs.c

Post by Sagi Grimberg

Post by Joern Engel
index 7de9f0475d05..ab8a27eff94c 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -381,7 +381,7 @@ static struct config_group *target_fabric_make_m=

appedlun(

Post by Sagi Grimberg

Post by Joern Engel
if (!lacl_cg->default_groups) {
pr_err("Unable to allocate lacl_cg->default_groups\n");
ret =3D -ENOMEM;
- goto out;
+ goto out2;
}
config_group_init_type_name(&lacl->se_lun_group, name,
@@ -397,12 +397,14 @@ static struct config_group *target_fabric_make=

_mappedlun(

Post by Sagi Grimberg

Post by Joern Engel
if (!ml_stat_grp->default_groups) {
pr_err("Unable to allocate ml_stat_grp->default_groups\n");
ret =3D -ENOMEM;
- goto out;
+ goto out2;
}
target_stat_setup_mappedlun_default_groups(lacl);
kfree(buf);
return &lacl->se_lun_group;
+ kfree(lacl);

=20
1. Please use a meaningful tag name.

I'm not particularly creative nor do I care too much about meaningful
tag names. Which makes me part of a 10% minority, according to git
grep. Guess it might be time to change my ways and stick to majority
consensus.

Post by Sagi Grimberg
2. Not sure you need a tag here, lacl is either NULL or should
be freed - you can go ahead and free it under out is kfree is
NULL safe (I even think Coverity complains about it, or is it smatch?=

)

That requires initializing lacl to NULL before the first goto, which
grows .text by 16 bytes on x86_64. Remember there are "goto out"s
before the call to core_dev_init_initiator_node_lun_acl().

But I agree it makes the code a bit nicer and that's well worth it.

J=C3=B6rn

--
With a PC, I always felt limited by the software available. On Unix,
I am limited only by my knowledge.
-- Peter J. Schoenster

[Joern Engel]

Coverity complained that lun_cg has been dereferenced in all paths
leading to NULL check. It didn't mention that only a single path could
lead there and the code can be simplified even further.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/target_core_fabric_configfs.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index ab8a27eff94c..d6fda85c3441 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -912,16 +912,12 @@ static struct config_group *target_fabric_make_lun(
GFP_KERNEL);
if (!port_stat_grp->default_groups) {
pr_err("Unable to allocate port_stat_grp->default_groups\n");
- errno = -ENOMEM;
- goto out;
+ kfree(lun_cg->default_groups);
+ return ERR_PTR(-ENOMEM);
}
target_stat_setup_port_default_groups(lun);

return &lun->lun_group;
-out:
- if (lun_cg)
- kfree(lun_cg->default_groups);
- return ERR_PTR(errno);
}

static void target_fabric_drop_lun(

[Joern Engel]

Old code was obviously buggy and effectively ignored the high byte.
Found by coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/target_core_fabric_lib.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 0d1cf8b4f49f..35bfe77160d8 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -394,9 +394,9 @@ char *iscsi_parse_pr_out_transport_id(
* If the caller wants the TransportID Length, we set that value for the
* entire iSCSI Tarnsport ID now.
*/
- if (out_tid_len != NULL) {
- add_len = ((buf[2] >> 8) & 0xff);
- add_len |= (buf[3] & 0xff);
+ if (out_tid_len) {
+ /* The shift works thanks to integer promotion rules */
+ add_len = (buf[2] << 8) | buf[3];

tid_len = strlen(&buf[4]);
tid_len += 4; /* Add four bytes for iSCSI Transport ID header */

[Sagi Grimberg]

Post by Joern Engel
Old code was obviously buggy and effectively ignored the high byte.
Found by coverity.
---
drivers/target/target_core_fabric_lib.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
index 0d1cf8b4f49f..35bfe77160d8 100644
--- a/drivers/target/target_core_fabric_lib.c
+++ b/drivers/target/target_core_fabric_lib.c
@@ -394,9 +394,9 @@ char *iscsi_parse_pr_out_transport_id(
* If the caller wants the TransportID Length, we set that value for the
* entire iSCSI Tarnsport ID now.
*/
- if (out_tid_len != NULL) {
- add_len = ((buf[2] >> 8) & 0xff);
- add_len |= (buf[3] & 0xff);
+ if (out_tid_len) {
+ /* The shift works thanks to integer promotion rules */
+ add_len = (buf[2] << 8) | buf[3];
tid_len = strlen(&buf[4]);
tid_len += 4; /* Add four bytes for iSCSI Transport ID header */

Looks good to me.

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

Found by coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/target_core_file.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 7d6cddaec525..ab2c53b63cc3 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -762,7 +762,9 @@ static ssize_t fd_set_configfs_dev_params(struct se_device *dev,
fd_dev->fbd_flags |= FBDF_HAS_SIZE;
break;
case Opt_fd_buffered_io:
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
if (arg != 1) {
pr_err("bogus fd_buffered_io=%d value\n", arg);
ret = -EINVAL;

[Sagi Grimberg]

Post by Joern Engel
Found by coverity.
---
drivers/target/target_core_file.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 7d6cddaec525..ab2c53b63cc3 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -762,7 +762,9 @@ static ssize_t fd_set_configfs_dev_params(struct se_device *dev,
fd_dev->fbd_flags |= FBDF_HAS_SIZE;
break;
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
if (arg != 1) {
pr_err("bogus fd_buffered_io=%d value\n", arg);
ret = -EINVAL;

Looks good to me.

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

Clearly a right-shift was meant. Effectively doesn't make a difference,
as add_len is hard-coded to 8 and the high byte will be zero no matter
which way you shift. But I hate leaving bad examples for others to
copy.

Found by coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/target_core_pr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index df357862286e..281d52e3fe99 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3803,7 +3803,7 @@ core_scsi3_pri_report_capabilities(struct se_cmd *cmd)
if (!buf)
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;

- buf[0] = ((add_len << 8) & 0xff);
+ buf[0] = ((add_len >> 8) & 0xff);
buf[1] = (add_len & 0xff);
buf[2] |= 0x10; /* CRH: Compatible Reservation Hanlding bit. */
buf[2] |= 0x08; /* SIP_C: Specify Initiator Ports Capable bit */

[Sagi Grimberg]

Post by Joern Engel
Clearly a right-shift was meant. Effectively doesn't make a difference,
as add_len is hard-coded to 8 and the high byte will be zero no matter
which way you shift. But I hate leaving bad examples for others to
copy.
Found by coverity.
---
drivers/target/target_core_pr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index df357862286e..281d52e3fe99 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3803,7 +3803,7 @@ core_scsi3_pri_report_capabilities(struct se_cmd *cmd)
if (!buf)
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
- buf[0] = ((add_len << 8) & 0xff);
+ buf[0] = ((add_len >> 8) & 0xff);
buf[1] = (add_len & 0xff);
buf[2] |= 0x10; /* CRH: Compatible Reservation Hanlding bit. */
buf[2] |= 0x08; /* SIP_C: Specify Initiator Ports Capable bit */

Looks good to me.

Reviewed-by: Sagi Grimberg <***@mellanox.com>

[Joern Engel]

Found by coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/target_core_pscsi.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 943b1dbe859a..a1690a3fdd7f 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -749,14 +749,18 @@ static ssize_t pscsi_set_configfs_dev_params(struct se_device *dev,
ret = -EINVAL;
goto out;
}
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
pdv->pdv_host_id = arg;
pr_debug("PSCSI[%d]: Referencing SCSI Host ID:"
" %d\n", phv->phv_host_id, pdv->pdv_host_id);
pdv->pdv_flags |= PDF_HAS_VIRT_HOST_ID;
break;
case Opt_scsi_channel_id:
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
pdv->pdv_channel_id = arg;
pr_debug("PSCSI[%d]: Referencing SCSI Channel"
" ID: %d\n", phv->phv_host_id,
@@ -764,7 +768,9 @@ static ssize_t pscsi_set_configfs_dev_params(struct se_device *dev,
pdv->pdv_flags |= PDF_HAS_CHANNEL_ID;
break;
case Opt_scsi_target_id:
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
pdv->pdv_target_id = arg;
pr_debug("PSCSI[%d]: Referencing SCSI Target"
" ID: %d\n", phv->phv_host_id,
@@ -772,7 +778,9 @@ static ssize_t pscsi_set_configfs_dev_params(struct se_device *dev,
pdv->pdv_flags |= PDF_HAS_TARGET_ID;
break;
case Opt_scsi_lun_id:
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
pdv->pdv_lun_id = arg;
pr_debug("PSCSI[%d]: Referencing SCSI LUN ID:"
" %d\n", phv->phv_host_id, pdv->pdv_lun_id);

[Sagi Grimberg]

Post by Joern Engel
Found by coverity.
---
drivers/target/target_core_pscsi.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 943b1dbe859a..a1690a3fdd7f 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -749,14 +749,18 @@ static ssize_t pscsi_set_configfs_dev_params(struct se_device *dev,
ret = -EINVAL;
goto out;
}
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
pdv->pdv_host_id = arg;
pr_debug("PSCSI[%d]: Referencing SCSI Host ID:"
" %d\n", phv->phv_host_id, pdv->pdv_host_id);
pdv->pdv_flags |= PDF_HAS_VIRT_HOST_ID;
break;
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
pdv->pdv_channel_id = arg;
pr_debug("PSCSI[%d]: Referencing SCSI Channel"
" ID: %d\n", phv->phv_host_id,
@@ -764,7 +768,9 @@ static ssize_t pscsi_set_configfs_dev_params(struct se_device *dev,
pdv->pdv_flags |= PDF_HAS_CHANNEL_ID;
break;
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
pdv->pdv_target_id = arg;
pr_debug("PSCSI[%d]: Referencing SCSI Target"
" ID: %d\n", phv->phv_host_id,
@@ -772,7 +778,9 @@ static ssize_t pscsi_set_configfs_dev_params(struct se_device *dev,
pdv->pdv_flags |= PDF_HAS_TARGET_ID;
break;
- match_int(args, &arg);
+ ret = match_int(args, &arg);
+ if (ret)
+ goto out;
pdv->pdv_lun_id = arg;
pr_debug("PSCSI[%d]: Referencing SCSI LUN ID:"
" %d\n", phv->phv_host_id, pdv->pdv_lun_id);

Shouldn't this be squashed with 14/17?

[Jörn Engel]

Post by Sagi Grimberg
=20
Shouldn't this be squashed with 14/17?

Technically yes. But if that is your only complaint I would prefer
the lazy option and have my patch statistic artificially inflated.

Thank you for the review. I will rework and send out a new patchset.

J=C3=B6rn

--
When in doubt, punt. When somebody actually complains, go back and fix=
it...
The 90% solution is a good thing.
-- Rob Landley

[Joern Engel]

Code inspection indicates these are possible. At the very least we
should catch them and call a bug a bug.

While at it, this also changes iscsit_map_iovec() to take an index into
cmd->iov_data instead of a pointer. Also fixes an off-by-one leading to
leaked kmap()s. iscsit_send_datain() called iscsit_map_iovec() with and
index of 1, resulting in cmd->kmapped_nents being one less than required
and not unmapping the last element in the vector.

The fact that noone seems to have noticed this bug yet implies that
target code doesn't get a lot of testing on 32bit platforms.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/iscsi/iscsi_target.c | 173 +++++++++++++-----------------------
1 file changed, 64 insertions(+), 109 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 416824a168c2..edb32e955628 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -729,17 +729,29 @@ int iscsit_reject_cmd(struct iscsi_cmd *cmd, u8 reason, unsigned char *buf)
return iscsit_add_reject_from_cmd(cmd, reason, false, buf);
}

+static void iscsit_map_data_iovec(struct iscsi_cmd *cmd, int i, void *iov_base,
+ size_t iov_len)
+{
+ WARN_ON_ONCE(i > cmd->orig_iov_data_count);
+ cmd->iov_data[i].iov_base = iov_base;
+ cmd->iov_data[i].iov_len = iov_len;
+}
+
+static void iscsit_map_misc_iovec(struct iscsi_cmd *cmd, int i, void *iov_base,
+ size_t iov_len)
+{
+ WARN_ON_ONCE(i > ISCSI_MISC_IOVECS);
+ cmd->iov_misc[i].iov_base = iov_base;
+ cmd->iov_misc[i].iov_len = iov_len;
+}
+
/*
* Map some portion of the allocated scatterlist to an iovec, suitable for
* kernel sockets to copy data in/out.
*/
-static int iscsit_map_iovec(
- struct iscsi_cmd *cmd,
- struct kvec *iov,
- u32 data_offset,
- u32 data_length)
+static int iscsit_map_iovec(struct iscsi_cmd *cmd, int i, u32 data_offset,
+ u32 data_length)
{
- u32 i = 0;
struct scatterlist *sg;
unsigned int page_off;

@@ -755,13 +767,11 @@ static int iscsit_map_iovec(
while (data_length) {
u32 cur_len = min_t(u32, data_length, sg->length - page_off);

- iov[i].iov_base = kmap(sg_page(sg)) + sg->offset + page_off;
- iov[i].iov_len = cur_len;
+ iscsit_map_data_iovec(cmd, i++, kmap(sg_page(sg)) + sg->offset + page_off, cur_len);

data_length -= cur_len;
page_off = 0;
sg = sg_next(sg);
- i++;
}

cmd->kmapped_nents = i;
@@ -1396,32 +1406,24 @@ static int
iscsit_get_dataout(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
struct iscsi_data *hdr)
{
- struct kvec *iov;
- u32 checksum, iov_count = 0, padding = 0, rx_got = 0, rx_size = 0;
+ u32 checksum, iov_count, padding = 0, rx_got = 0, rx_size = 0;
u32 payload_length = ntoh24(hdr->dlength);
- int iov_ret, data_crc_failed = 0;
+ int data_crc_failed = 0;

rx_size += payload_length;
- iov = &cmd->iov_data[0];

- iov_ret = iscsit_map_iovec(cmd, iov, be32_to_cpu(hdr->offset),
+ iov_count = iscsit_map_iovec(cmd, 0, be32_to_cpu(hdr->offset),
payload_length);
- if (iov_ret < 0)
- return -1;
-
- iov_count += iov_ret;

padding = ((-payload_length) & 3);
if (padding != 0) {
- iov[iov_count].iov_base = cmd->pad_bytes;
- iov[iov_count++].iov_len = padding;
+ iscsit_map_data_iovec(cmd, iov_count++, cmd->pad_bytes, padding);
rx_size += padding;
pr_debug("Receiving %u padding bytes.\n", padding);
}

if (conn->conn_ops->DataDigest) {
- iov[iov_count].iov_base = &checksum;
- iov[iov_count++].iov_len = ISCSI_CRC_LEN;
+ iscsit_map_data_iovec(cmd, iov_count++, &checksum, ISCSI_CRC_LEN);
rx_size += ISCSI_CRC_LEN;
}

@@ -1648,7 +1650,6 @@ static int iscsit_handle_nop_out(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
{
unsigned char *ping_data = NULL;
struct iscsi_nopout *hdr = (struct iscsi_nopout *)buf;
- struct kvec *iov = NULL;
u32 payload_length = ntoh24(hdr->dlength);
int ret;

@@ -1670,21 +1671,17 @@ static int iscsit_handle_nop_out(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
goto out;
}

- iov = &cmd->iov_misc[0];
- iov[niov].iov_base = ping_data;
- iov[niov++].iov_len = payload_length;
+ iscsit_map_misc_iovec(cmd, niov++, ping_data, payload_length);

padding = ((-payload_length) & 3);
if (padding != 0) {
pr_debug("Receiving %u additional bytes"
" for padding.\n", padding);
- iov[niov].iov_base = &cmd->pad_bytes;
- iov[niov++].iov_len = padding;
+ iscsit_map_misc_iovec(cmd, niov++, &cmd->pad_bytes, padding);
rx_size += padding;
}
if (conn->conn_ops->DataDigest) {
- iov[niov].iov_base = &checksum;
- iov[niov++].iov_len = ISCSI_CRC_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, &checksum, ISCSI_CRC_LEN);
rx_size += ISCSI_CRC_LEN;
}

@@ -2411,29 +2408,21 @@ static int iscsit_handle_immediate_data(
struct iscsi_scsi_req *hdr,
u32 length)
{
- int iov_ret, rx_got = 0, rx_size = 0;
- u32 checksum, iov_count = 0, padding = 0;
+ int rx_got = 0, rx_size = 0;
+ u32 checksum, iov_count, padding = 0;
struct iscsi_conn *conn = cmd->conn;
- struct kvec *iov;

- iov_ret = iscsit_map_iovec(cmd, cmd->iov_data, cmd->write_data_done, length);
- if (iov_ret < 0)
- return IMMEDIATE_DATA_CANNOT_RECOVER;
+ iov_count = iscsit_map_iovec(cmd, 0, cmd->write_data_done, length);

rx_size = length;
- iov_count = iov_ret;
- iov = &cmd->iov_data[0];
-
padding = ((-length) & 3);
if (padding != 0) {
- iov[iov_count].iov_base = cmd->pad_bytes;
- iov[iov_count++].iov_len = padding;
+ iscsit_map_data_iovec(cmd, iov_count++, cmd->pad_bytes, padding);
rx_size += padding;
}

if (conn->conn_ops->DataDigest) {
- iov[iov_count].iov_base = &checksum;
- iov[iov_count++].iov_len = ISCSI_CRC_LEN;
+ iscsit_map_data_iovec(cmd, iov_count++, &checksum, ISCSI_CRC_LEN);
rx_size += ISCSI_CRC_LEN;
}

@@ -2640,9 +2629,8 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
struct iscsi_data_rsp *hdr = (struct iscsi_data_rsp *)&cmd->pdu[0];
struct iscsi_datain datain;
struct iscsi_datain_req *dr;
- struct kvec *iov;
u32 iov_count = 0, tx_size = 0;
- int eodr = 0, ret, iov_ret;
+ int eodr = 0, ret;
bool set_statsn = false;

memset(&datain, 0, sizeof(struct iscsi_datain));
@@ -2684,9 +2672,7 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)

iscsit_build_datain_pdu(cmd, conn, &datain, hdr, set_statsn);

- iov = &cmd->iov_data[0];
- iov[iov_count].iov_base = cmd->pdu;
- iov[iov_count++].iov_len = ISCSI_HDR_LEN;
+ iscsit_map_data_iovec(cmd, iov_count++, cmd->pdu, ISCSI_HDR_LEN);
tx_size += ISCSI_HDR_LEN;

if (conn->conn_ops->HeaderDigest) {
@@ -2695,25 +2681,19 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
iscsit_do_crypto_hash_buf(&conn->conn_tx_hash, cmd->pdu,
ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);

- iov[0].iov_len += ISCSI_CRC_LEN;
+ cmd->iov_data[0].iov_len += ISCSI_CRC_LEN;
tx_size += ISCSI_CRC_LEN;

pr_debug("Attaching CRC32 HeaderDigest"
" for DataIN PDU 0x%08x\n", *header_digest);
}

- iov_ret = iscsit_map_iovec(cmd, &cmd->iov_data[1],
- datain.offset, datain.length);
- if (iov_ret < 0)
- return -1;
-
- iov_count += iov_ret;
+ iov_count += iscsit_map_iovec(cmd, 1, datain.offset, datain.length);
tx_size += datain.length;

cmd->padding = ((-datain.length) & 3);
if (cmd->padding) {
- iov[iov_count].iov_base = cmd->pad_bytes;
- iov[iov_count++].iov_len = cmd->padding;
+ iscsit_map_data_iovec(cmd, iov_count++, cmd->pad_bytes, cmd->padding);
tx_size += cmd->padding;

pr_debug("Attaching %u padding bytes\n",
@@ -2723,8 +2703,7 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
cmd->data_crc = iscsit_do_crypto_hash_sg(&conn->conn_tx_hash, cmd,
datain.offset, datain.length, cmd->padding, cmd->pad_bytes);

- iov[iov_count].iov_base = &cmd->data_crc;
- iov[iov_count++].iov_len = ISCSI_CRC_LEN;
+ iscsit_map_data_iovec(cmd, iov_count++, &cmd->data_crc, ISCSI_CRC_LEN);
tx_size += ISCSI_CRC_LEN;

pr_debug("Attached CRC32C DataDigest %d bytes, crc"
@@ -2854,7 +2833,6 @@ EXPORT_SYMBOL(iscsit_build_logout_rsp);
static int
iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
{
- struct kvec *iov;
int niov = 0, tx_size, rc;

rc = iscsit_build_logout_rsp(cmd, conn,
@@ -2863,9 +2841,7 @@ iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
return rc;

tx_size = ISCSI_HDR_LEN;
- iov = &cmd->iov_misc[0];
- iov[niov].iov_base = cmd->pdu;
- iov[niov++].iov_len = ISCSI_HDR_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, cmd->pdu, ISCSI_HDR_LEN);

if (conn->conn_ops->HeaderDigest) {
u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
@@ -2873,7 +2849,7 @@ iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
iscsit_do_crypto_hash_buf(&conn->conn_tx_hash, &cmd->pdu[0],
ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);

- iov[0].iov_len += ISCSI_CRC_LEN;
+ cmd->iov_misc[0].iov_len += ISCSI_CRC_LEN;
tx_size += ISCSI_CRC_LEN;
pr_debug("Attaching CRC32C HeaderDigest to"
" Logout Response 0x%08x\n", *header_digest);
@@ -2962,16 +2938,13 @@ static int
iscsit_send_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
{
struct iscsi_nopin *hdr = (struct iscsi_nopin *)&cmd->pdu[0];
- struct kvec *iov;
u32 padding = 0;
int niov = 0, tx_size;

iscsit_build_nopin_rsp(cmd, conn, hdr, true);

tx_size = ISCSI_HDR_LEN;
- iov = &cmd->iov_misc[0];
- iov[niov].iov_base = cmd->pdu;
- iov[niov++].iov_len = ISCSI_HDR_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, cmd->pdu, ISCSI_HDR_LEN);

if (conn->conn_ops->HeaderDigest) {
u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
@@ -2979,7 +2952,7 @@ iscsit_send_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
iscsit_do_crypto_hash_buf(&conn->conn_tx_hash, hdr,
ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);

- iov[0].iov_len += ISCSI_CRC_LEN;
+ cmd->iov_misc[0].iov_len += ISCSI_CRC_LEN;
tx_size += ISCSI_CRC_LEN;
pr_debug("Attaching CRC32C HeaderDigest"
" to NopIn 0x%08x\n", *header_digest);
@@ -2990,8 +2963,7 @@ iscsit_send_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
* NOPOUT DataSegmentLength is at struct iscsi_cmd->buf_ptr_size.
*/
if (cmd->buf_ptr_size) {
- iov[niov].iov_base = cmd->buf_ptr;
- iov[niov++].iov_len = cmd->buf_ptr_size;
+ iscsit_map_misc_iovec(cmd, niov++, cmd->buf_ptr, cmd->buf_ptr_size);
tx_size += cmd->buf_ptr_size;

pr_debug("Echoing back %u bytes of ping"
@@ -2999,8 +2971,7 @@ iscsit_send_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn)

padding = ((-cmd->buf_ptr_size) & 3);
if (padding != 0) {
- iov[niov].iov_base = &cmd->pad_bytes;
- iov[niov++].iov_len = padding;
+ iscsit_map_misc_iovec(cmd, niov++, &cmd->pad_bytes, padding);
tx_size += padding;
pr_debug("Attaching %u additional"
" padding bytes.\n", padding);
@@ -3011,8 +2982,7 @@ iscsit_send_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
padding, (u8 *)&cmd->pad_bytes,
(u8 *)&cmd->data_crc);

- iov[niov].iov_base = &cmd->data_crc;
- iov[niov++].iov_len = ISCSI_CRC_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, &cmd->data_crc, ISCSI_CRC_LEN);
tx_size += ISCSI_CRC_LEN;
pr_debug("Attached DataDigest for %u"
" bytes of ping data, CRC 0x%08x\n",
@@ -3219,16 +3189,13 @@ EXPORT_SYMBOL(iscsit_build_rsp_pdu);
static int iscsit_send_response(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
{
struct iscsi_scsi_rsp *hdr = (struct iscsi_scsi_rsp *)&cmd->pdu[0];
- struct kvec *iov;
u32 padding = 0, tx_size = 0;
- int iov_count = 0;
+ int niov = 0;
bool inc_stat_sn = (cmd->i_state == ISTATE_SEND_STATUS);

iscsit_build_rsp_pdu(cmd, conn, inc_stat_sn, hdr);

- iov = &cmd->iov_misc[0];
- iov[iov_count].iov_base = cmd->pdu;
- iov[iov_count++].iov_len = ISCSI_HDR_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, cmd->pdu, ISCSI_HDR_LEN);
tx_size += ISCSI_HDR_LEN;

/*
@@ -3242,9 +3209,8 @@ static int iscsit_send_response(struct iscsi_cmd *cmd, struct iscsi_conn *conn)

padding = -(cmd->se_cmd.scsi_sense_length) & 3;
hton24(hdr->dlength, (u32)cmd->se_cmd.scsi_sense_length);
- iov[iov_count].iov_base = cmd->sense_buffer;
- iov[iov_count++].iov_len =
- (cmd->se_cmd.scsi_sense_length + padding);
+ iscsit_map_misc_iovec(cmd, niov++, cmd->sense_buffer,
+ cmd->se_cmd.scsi_sense_length + padding);
tx_size += cmd->se_cmd.scsi_sense_length;

if (padding) {
@@ -3261,8 +3227,7 @@ static int iscsit_send_response(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
(cmd->se_cmd.scsi_sense_length + padding),
0, NULL, (u8 *)&cmd->data_crc);

- iov[iov_count].iov_base = &cmd->data_crc;
- iov[iov_count++].iov_len = ISCSI_CRC_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, &cmd->data_crc, ISCSI_CRC_LEN);
tx_size += ISCSI_CRC_LEN;

pr_debug("Attaching CRC32 DataDigest for"
@@ -3282,13 +3247,13 @@ static int iscsit_send_response(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
iscsit_do_crypto_hash_buf(&conn->conn_tx_hash, cmd->pdu,
ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);

- iov[0].iov_len += ISCSI_CRC_LEN;
+ cmd->iov_misc[0].iov_len += ISCSI_CRC_LEN;
tx_size += ISCSI_CRC_LEN;
pr_debug("Attaching CRC32 HeaderDigest for Response"
" PDU 0x%08x\n", *header_digest);
}

- cmd->iov_misc_count = iov_count;
+ cmd->iov_misc_count = niov;
cmd->tx_size = tx_size;

return 0;
@@ -3562,20 +3527,16 @@ static int iscsit_send_text_rsp(
struct iscsi_conn *conn)
{
struct iscsi_text_rsp *hdr = (struct iscsi_text_rsp *)cmd->pdu;
- struct kvec *iov;
u32 tx_size = 0;
- int text_length, iov_count = 0, rc;
+ int text_length, niov = 0, rc;

rc = iscsit_build_text_rsp(cmd, conn, hdr, ISCSI_TCP);
if (rc < 0)
return rc;

text_length = rc;
- iov = &cmd->iov_misc[0];
- iov[iov_count].iov_base = cmd->pdu;
- iov[iov_count++].iov_len = ISCSI_HDR_LEN;
- iov[iov_count].iov_base = cmd->buf_ptr;
- iov[iov_count++].iov_len = text_length;
+ iscsit_map_misc_iovec(cmd, niov++, cmd->pdu, ISCSI_HDR_LEN);
+ iscsit_map_misc_iovec(cmd, niov++, cmd->buf_ptr, text_length);

tx_size += (ISCSI_HDR_LEN + text_length);

@@ -3585,7 +3546,7 @@ static int iscsit_send_text_rsp(
iscsit_do_crypto_hash_buf(&conn->conn_tx_hash, hdr,
ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);

- iov[0].iov_len += ISCSI_CRC_LEN;
+ cmd->iov_misc[0].iov_len += ISCSI_CRC_LEN;
tx_size += ISCSI_CRC_LEN;
pr_debug("Attaching CRC32 HeaderDigest for"
" Text Response PDU 0x%08x\n", *header_digest);
@@ -3596,8 +3557,7 @@ static int iscsit_send_text_rsp(
cmd->buf_ptr, text_length,
0, NULL, (u8 *)&cmd->data_crc);

- iov[iov_count].iov_base = &cmd->data_crc;
- iov[iov_count++].iov_len = ISCSI_CRC_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, &cmd->data_crc, ISCSI_CRC_LEN);
tx_size += ISCSI_CRC_LEN;

pr_debug("Attaching DataDigest for %u bytes of text"
@@ -3605,7 +3565,7 @@ static int iscsit_send_text_rsp(
cmd->data_crc);
}

- cmd->iov_misc_count = iov_count;
+ cmd->iov_misc_count = niov;
cmd->tx_size = tx_size;

return 0;
@@ -3633,16 +3593,12 @@ static int iscsit_send_reject(
struct iscsi_conn *conn)
{
struct iscsi_reject *hdr = (struct iscsi_reject *)&cmd->pdu[0];
- struct kvec *iov;
- u32 iov_count = 0, tx_size;
+ u32 niov = 0, tx_size;

iscsit_build_reject(cmd, conn, hdr);

- iov = &cmd->iov_misc[0];
- iov[iov_count].iov_base = cmd->pdu;
- iov[iov_count++].iov_len = ISCSI_HDR_LEN;
- iov[iov_count].iov_base = cmd->buf_ptr;
- iov[iov_count++].iov_len = ISCSI_HDR_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, cmd->pdu, ISCSI_HDR_LEN);
+ iscsit_map_misc_iovec(cmd, niov++, cmd->buf_ptr, ISCSI_HDR_LEN);

tx_size = (ISCSI_HDR_LEN + ISCSI_HDR_LEN);

@@ -3652,7 +3608,7 @@ static int iscsit_send_reject(
iscsit_do_crypto_hash_buf(&conn->conn_tx_hash, hdr,
ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);

- iov[0].iov_len += ISCSI_CRC_LEN;
+ cmd->iov_misc[0].iov_len += ISCSI_CRC_LEN;
tx_size += ISCSI_CRC_LEN;
pr_debug("Attaching CRC32 HeaderDigest for"
" REJECT PDU 0x%08x\n", *header_digest);
@@ -3662,14 +3618,13 @@ static int iscsit_send_reject(
iscsit_do_crypto_hash_buf(&conn->conn_tx_hash, cmd->buf_ptr,
ISCSI_HDR_LEN, 0, NULL, (u8 *)&cmd->data_crc);

- iov[iov_count].iov_base = &cmd->data_crc;
- iov[iov_count++].iov_len = ISCSI_CRC_LEN;
+ iscsit_map_misc_iovec(cmd, niov++, &cmd->data_crc, ISCSI_CRC_LEN);
tx_size += ISCSI_CRC_LEN;
pr_debug("Attaching CRC32 DataDigest for REJECT"
" PDU 0x%08x\n", cmd->data_crc);
}

- cmd->iov_misc_count = iov_count;
+ cmd->iov_misc_count = niov;
cmd->tx_size = tx_size;

pr_debug("Built Reject PDU StatSN: 0x%08x, Reason: 0x%02x,"

[Sagi Grimberg]

Post by Joern Engel
Found by coverity. l_conn cannot possible be NULL. It can very well
not match any connection, which is the logical equivalent of NULL. At
that point we would happily dereference the pointer and do heck knows
what with the random values we find. A NULL pointer dereference would
have been much better.
---
drivers/target/iscsi/iscsi_target.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 30f4a7d42e32..f9d2b1255856 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4549,7 +4549,7 @@ static void iscsit_logout_post_handler_diffcid(
}
spin_unlock_bh(&sess->conn_lock);
- if (!l_conn)
+ if (&l_conn->conn_list == &sess->sess_conn_list)

I'm not sure I understand how can one get here even hypothetically.
Can you at least put a warn print here (or someone can sched some
light here...)

Post by Joern Engel
return;
if (l_conn->sock)

[Jörn Engel]

Post by Sagi Grimberg

Found by coverity. l_conn cannot possible be NULL. It can very wel=

l

Post by Sagi Grimberg

not match any connection, which is the logical equivalent of NULL. =

At

Post by Sagi Grimberg

that point we would happily dereference the pointer and do heck know=

s

Post by Sagi Grimberg

what with the random values we find. A NULL pointer dereference wou=

ld

Post by Sagi Grimberg

have been much better.
---
drivers/target/iscsi/iscsi_target.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/is=

csi/iscsi_target.c

Post by Sagi Grimberg

index 30f4a7d42e32..f9d2b1255856 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4549,7 +4549,7 @@ static void iscsit_logout_post_handler_diffcid=

(

Post by Sagi Grimberg

}
spin_unlock_bh(&sess->conn_lock);
- if (!l_conn)
+ if (&l_conn->conn_list =3D=3D &sess->sess_conn_list)

=20
I'm not sure I understand how can one get here even hypothetically.
Can you at least put a warn print here (or someone can sched some
light here...)

That makes two of us. I am slightly concerned that some future code
change can make it possible to trigger this condition. Therefore this
patch plus a WARN_ON_ONCE seems preferrable to just removing the code.

It is just a slight preference and if someone else strongly disagrees,
I can live with that as well.

Post by Sagi Grimberg

return;
if (l_conn->sock)

=20

J=C3=B6rn

--
You can't tell where a program is going to spend its time. Bottlenecks
occur in surprising places, so don't try to second guess and put in a
speed hack until you've proven that's where the bottleneck is.
-- Rob Pike