[PATCH 05/16] target: simplify return statement

[Joern Engel]

The return statement cannot be reached without either recovery or dump
being set to 1. Therefore the condition always evaluates to true and
recovery and dump are useless variables.

Found by Coverity.

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

diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c
index 0d1e6ee3e992..a0ae5fc0ad75 100644
--- a/drivers/target/iscsi/iscsi_target_erl0.c
+++ b/drivers/target/iscsi/iscsi_target_erl0.c
@@ -345,7 +345,6 @@ static int iscsit_dataout_check_datasn(
struct iscsi_cmd *cmd,
unsigned char *buf)
{
- int dump = 0, recovery = 0;
u32 data_sn = 0;
struct iscsi_conn *conn = cmd->conn;
struct iscsi_data *hdr = (struct iscsi_data *) buf;
@@ -370,13 +369,11 @@ static int iscsit_dataout_check_datasn(
pr_err("Command ITT: 0x%08x, received DataSN: 0x%08x"
" higher than expected 0x%08x.\n", cmd->init_task_tag,
be32_to_cpu(hdr->datasn), data_sn);
- recovery = 1;
goto recover;
} else if (be32_to_cpu(hdr->datasn) < data_sn) {
pr_err("Command ITT: 0x%08x, received DataSN: 0x%08x"
" lower than expected 0x%08x, discarding payload.\n",
cmd->init_task_tag, be32_to_cpu(hdr->datasn), data_sn);
- dump = 1;
goto dump;
}

@@ -392,8 +389,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(

[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;

[Nicholas A. Bellinger]

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;

So yeah, in earlier code (v3.10) ENODEV was supposed to signal that
iser-target was shutting down, and that the NP thread was supposed to
bail out. In the same v3.10 code, there was another check at the out:
label to determine if the return value should be zero to signal a break
from the loop in iscsi_target_login_thread().

However, the ISCSI_NP_THREAD_SHUTDOWN check at the start of
__iscsi_target_login_thread() has the same effect as the original code
at the out: label, so I think it's safe to go ahead and drop the check
all-together.

Applying the following patch to target-pending/for-next.

--nab

commit 1d30686da4a40029cb48eab28442896b58aeceef
Author: Nicholas Bellinger <***@linux-iscsi.org>
Date: Wed Sep 17 13:17:55 2014 -0700

iscsi-target: Drop duplicate __iscsi_target_login_thread check

This patch drops the now duplicate + unnecessary check for -ENODEV from
iscsi_transport->iscsit_accept_np() for jumping to out:, or immediately
returning 1 in __iscsi_target_login_thread() code.

Since commit 81a9c5e72b the jump to out: and returning 1 have the same
effect, and end up hitting the ISCSI_NP_THREAD_SHUTDOWN check regardless
at the top of __iscsi_target_login_thread() during next loop iteration.

So that said, it's safe to go ahead and remove this duplicate check.

Reported-by: Joern Engel <***@logfs.org>
Signed-off-by: Nicholas Bellinger <***@linux-iscsi.org>

diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_lo
index b1ae5cb..02d5ccd 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1267,8 +1267,6 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
iscsit_put_transport(conn->conn_transport);
kfree(conn);
conn = NULL;
- if (ret == -ENODEV)
- goto out;
/* Get another socket */
return 1;
}

[Jörn Engel]

Post by Nicholas A. Bellinger
=20
So yeah, in earlier code (v3.10) ENODEV was supposed to signal that
iser-target was shutting down, and that the NP thread was supposed to
bail out. In the same v3.10 code, there was another check at the out=
label to determine if the return value should be zero to signal a bre=

ak

Post by Nicholas A. Bellinger
from the loop in iscsi_target_login_thread().=20
=20
However, the ISCSI_NP_THREAD_SHUTDOWN check at the start of
__iscsi_target_login_thread() has the same effect as the original cod=

e

Post by Nicholas A. Bellinger
at the out: label, so I think it's safe to go ahead and drop the chec=

k

Post by Nicholas A. Bellinger
all-together.
=20
Applying the following patch to target-pending/for-next.

Looks good to me.

Post by Nicholas A. Bellinger
commit 1d30686da4a40029cb48eab28442896b58aeceef
Date: Wed Sep 17 13:17:55 2014 -0700
=20
iscsi-target: Drop duplicate __iscsi_target_login_thread check
=20
This patch drops the now duplicate + unnecessary check for -ENODE=

V from

Post by Nicholas A. Bellinger
iscsi_transport->iscsit_accept_np() for jumping to out:, or immed=

iately

Post by Nicholas A. Bellinger
returning 1 in __iscsi_target_login_thread() code.
=20
Since commit 81a9c5e72b the jump to out: and returning 1 have the=

same

Post by Nicholas A. Bellinger
effect, and end up hitting the ISCSI_NP_THREAD_SHUTDOWN check reg=

ardless

Post by Nicholas A. Bellinger
at the top of __iscsi_target_login_thread() during next loop iter=

ation.

Post by Nicholas A. Bellinger
=20
So that said, it's safe to go ahead and remove this duplicate che=

ck.

Post by Nicholas A. Bellinger
=20
=20
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/targ=

et/iscsi/iscsi_target_lo

Post by Nicholas A. Bellinger
index b1ae5cb..02d5ccd 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1267,8 +1267,6 @@ static int __iscsi_target_login_thread(struct i=

scsi_np *np)

Post by Nicholas A. Bellinger
iscsit_put_transport(conn->conn_transport);
kfree(conn);
conn =3D NULL;
- if (ret =3D=3D -ENODEV)
- goto out;
/* Get another socket */
return 1;
}
=20

J=C3=B6rn

--
Tough times don't last, but tough people do.
-- Nigerian proverb

[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;

[Nicholas A. Bellinger]

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;

Applied to target-pending/for-next.

[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 05932daaf79c..e7d23990264b 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,"

[Nicholas A. Bellinger]

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

I don't see how an vector overflow is possible here..?

The three callers of iscsit_map_iovec() are each using validated lengths
based upon the original CDB data-length, and iscsit_allocate_iovecs() is
doing iovec allocation based upon same CDB data-length, plus an extra
ISCSI_IOV_DATA_BUFFER of room..

Post by Joern Engel
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.

I like the overall cleanup, but the bit about an off-by-one error in
iscsit_send_datain() is incorrect.

iscsit_send_datain()'s call to iscsit_map_iovec() has an index of 1
because the first iovec element is used by the outgoing
ISCSI_OP_SCSI_DATA_IN header, and the actual payload containing pages
that need to be kmapped starts in the subsequent vector. This was
originally done to dispatch the PDU + payload with a single call to
->sendpage().

cmd->kmapped_nents is then used by iscsit_unmap_iovec() for kunmaping
of the outgoing payload, but not the outgoing PDU header. That said, I
don't see how iscsit_unmap_iovec() is leaking kunmap() for the last
element if cmd->kmapped_nents matches what was kmap()'ed earlier in
iscsit_map_iovec().

Post by Joern Engel
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.

There at about 1/2 dozen vendors shipping on 32-bit ARM. Go visit your
local Frys + Best-Buy and see. ;)

Regardless, I do like the patch as a cleanup, but the commit message is
wrong and AFAICT it doesn't actually fix a bug.

Care to give a different commit message + reasoning for this cleanup..?

--nab

[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 */

[Nicholas A. Bellinger]

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 */

Applied to target-pending/for-next.

[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);

[Nicholas A. Bellinger]

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);

Applied to target-pending/master, with a CC' to stable for v3.1.y.

Thanks Joern!

--nab

[Joern Engel]

Found by coverity.

Signed-off-by: Joern Engel <***@logfs.org>
---
drivers/target/target_core_file.c | 4 +++-
drivers/target/target_core_pscsi.c | 16 ++++++++++++----
2 files changed, 15 insertions(+), 5 deletions(-)

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;
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);

[Nicholas A. Bellinger]

Post by Joern Engel
Found by coverity.
---
drivers/target/target_core_file.c | 4 +++-
drivers/target/target_core_pscsi.c | 16 ++++++++++++----
2 files changed, 15 insertions(+), 5 deletions(-)

Applied to target-pending/for-next.

[Joern Engel]

Each case of match_strdup could leak memory if the same argument was
present before. I am not too concerned, as it would require a
non-sensical combination like "target_lun=foo target_lun=bar", done
with root privileges and even then leak just a few bytes per instance.

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;

[Nicholas A. Bellinger]

Post by Joern Engel
Each case of match_strdup could leak memory if the same argument was
present before. I am not too concerned, as it would require a
non-sensical combination like "target_lun=foo target_lun=bar", done
with root privileges and even then leak just a few bytes per instance.
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(-)

Applied to target-pending/for-next.

[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..dcaebe712259 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 (WARN_ON_ONCE(&l_conn->conn_list == &sess->sess_conn_list))
return;

if (l_conn->sock)

[Nicholas A. Bellinger]

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..dcaebe712259 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 (WARN_ON_ONCE(&l_conn->conn_list == &sess->sess_conn_list))
return;
if (l_conn->sock)

This doesn't look quite right, as it depends upon the ordering of the
connection within sess_conn_list to ascertain when no matching
connection ID has been found.

How about the following instead..?

--nab

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 30f4a7d..b19e432 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4536,6 +4536,7 @@ static void iscsit_logout_post_handler_diffcid(
{
struct iscsi_conn *l_conn;
struct iscsi_session *sess = conn->sess;
+ bool conn_found = false;

if (!sess)
return;
@@ -4544,12 +4545,13 @@ static void iscsit_logout_post_handler_diffcid(
list_for_each_entry(l_conn, &sess->sess_conn_list, conn_list) {
if (l_conn->cid == cid) {
iscsit_inc_conn_usage_count(l_conn);
+ conn_found = true;
break;
}
}
spin_unlock_bh(&sess->conn_lock);

- if (!l_conn)
+ if (!conn_found)
return;

if (l_conn->sock)

[Joern Engel]

Found by coverity.

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

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 7de9f0475d05..376d1dfb2fdf 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -320,7 +320,7 @@ static struct config_group *target_fabric_make_mappedlun(
struct se_node_acl, acl_group);
struct se_portal_group *se_tpg = se_nacl->se_tpg;
struct target_fabric_configfs *tf = se_tpg->se_tpg_wwn->wwn_tf;
- struct se_lun_acl *lacl;
+ struct se_lun_acl *lacl = NULL;
struct config_item *acl_ci;
struct config_group *lacl_cg = NULL, *ml_stat_grp = NULL;
char *buf;
@@ -404,6 +404,7 @@ static struct config_group *target_fabric_make_mappedlun(
kfree(buf);
return &lacl->se_lun_group;
out:
+ kfree(lacl);
if (lacl_cg)
kfree(lacl_cg->default_groups);
kfree(buf);

[Nicholas A. Bellinger]

Post by Joern Engel
Found by coverity.
---
drivers/target/target_core_fabric_configfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 7de9f0475d05..376d1dfb2fdf 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -320,7 +320,7 @@ static struct config_group *target_fabric_make_mappedlun(
struct se_node_acl, acl_group);
struct se_portal_group *se_tpg = se_nacl->se_tpg;
struct target_fabric_configfs *tf = se_tpg->se_tpg_wwn->wwn_tf;
- struct se_lun_acl *lacl;
+ struct se_lun_acl *lacl = NULL;
struct config_item *acl_ci;
struct config_group *lacl_cg = NULL, *ml_stat_grp = NULL;
char *buf;
@@ -404,6 +404,7 @@ static struct config_group *target_fabric_make_mappedlun(
kfree(buf);
return &lacl->se_lun_group;
+ kfree(lacl);
if (lacl_cg)
kfree(lacl_cg->default_groups);
kfree(buf);

This is not right, due to lacl_cg memory reference after first releasing
lacl.

Just to be safe, fixing up the ordering and squashing into the original
below.

--nab

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric
index 7228a18..0638a67 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -320,7 +320,7 @@ static struct config_group *target_fabric_make_mappedlun(
struct se_node_acl, acl_group);
struct se_portal_group *se_tpg = se_nacl->se_tpg;
struct target_fabric_configfs *tf = se_tpg->se_tpg_wwn->wwn_tf;
- struct se_lun_acl *lacl;
+ struct se_lun_acl *lacl = NULL;
struct config_item *acl_ci;
struct config_group *lacl_cg = NULL, *ml_stat_grp = NULL;
char *buf;
@@ -406,6 +406,7 @@ static struct config_group *target_fabric_make_mappedlun(
out:
if (lacl_cg)
kfree(lacl_cg->default_groups);
+ kfree(lacl);
kfree(buf);
return ERR_PTR(ret);

[Jörn Engel]

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

/target/target_core_fabric_configfs.c

Post by Joern Engel
index 7de9f0475d05..376d1dfb2fdf 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -320,7 +320,7 @@ static struct config_group *target_fabric_make_=

mappedlun(

Post by Joern Engel
struct se_node_acl, acl_group);
struct se_portal_group *se_tpg =3D se_nacl->se_tpg;
struct target_fabric_configfs *tf =3D se_tpg->se_tpg_wwn->wwn_tf;
- struct se_lun_acl *lacl;
+ struct se_lun_acl *lacl =3D NULL;
struct config_item *acl_ci;
struct config_group *lacl_cg =3D NULL, *ml_stat_grp =3D NULL;
char *buf;
@@ -404,6 +404,7 @@ static struct config_group *target_fabric_make_=

mappedlun(

Post by Joern Engel
kfree(buf);
return &lacl->se_lun_group;
+ kfree(lacl);
if (lacl_cg)
kfree(lacl_cg->default_groups);
kfree(buf);

=20
This is not right, due to lacl_cg memory reference after first releas=

ing

lacl.
=20
Just to be safe, fixing up the ordering and squashing into the origin=

al

below.

Indeed! Thanks for catching it!

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/t=

arget/target_core_fabric

index 7228a18..0638a67 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -320,7 +320,7 @@ static struct config_group *target_fabric_make_ma=

ppedlun(

struct se_node_acl, acl_group);
struct se_portal_group *se_tpg =3D se_nacl->se_tpg;
struct target_fabric_configfs *tf =3D se_tpg->se_tpg_wwn->wwn=

_tf;

- struct se_lun_acl *lacl;
+ struct se_lun_acl *lacl =3D NULL;
struct config_item *acl_ci;
struct config_group *lacl_cg =3D NULL, *ml_stat_grp =3D NULL;
char *buf;
@@ -406,6 +406,7 @@ static struct config_group *target_fabric_make_ma=

ppedlun(

if (lacl_cg)
kfree(lacl_cg->default_groups);
+ kfree(lacl);
kfree(buf);
return ERR_PTR(ret);
=20
=20

J=C3=B6rn

--
Data expands to fill the space available for storage.
-- Parkinson's Law

[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 376d1dfb2fdf..e23a2c82a1b0 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -911,16 +911,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(

[Nicholas A. Bellinger]

Post by 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.
---
drivers/target/target_core_fabric_configfs.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)

Applied to target-pending/for-next.

[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 dcaebe712259..05932daaf79c 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:

[Nicholas A. Bellinger]

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 dcaebe712259..05932daaf79c 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;

This patch is unnecessary (and introduces a regression) because *cmd is
only allocated and passed into iscsit_handle_nop_out() when TTT ==
0xFFFFFFFF, eg: *cmd is valid only when the incoming NOP-OUT generates a
NOP-IN response, but not when the incoming NOP-OUT is a response to a
locally generated NOP-IN.

That said, iscsit_handle_nop_out() already expects *cmd to be NULL when
TTT != 0xFFFFFFFF, and locates the associated cmd descriptor from the
connection list before stopping the nopin response timer, and releasing
the associated resources.

--nab

[Jörn Engel]

Found by coverity. It appears as if the initiator can cause a kern=

el

NULL pointer dereference at will. Some might consider such behavio=

ur

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 dereferenc=

e

NULL.
=20
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.
=20
---
drivers/target/iscsi/iscsi_target.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
=20
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/i=

scsi/iscsi_target.c

index dcaebe712259..05932daaf79c 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 iscs=

i_conn *conn, unsigned char *buf)

cmd =3D NULL;
if (hdr->ttt =3D=3D cpu_to_be32(0xFFFFFFFF)) {
cmd =3D iscsit_allocate_cmd(conn, TASK_INTERRUPTIBLE);
- if (!cmd)
- goto reject;
}
+ if (!cmd)
+ goto reject;
ret =3D iscsit_handle_nop_out(conn, cmd, buf);
break;

=20
This patch is unnecessary (and introduces a regression) because *cmd =

is

only allocated and passed into iscsit_handle_nop_out() when TTT =3D=3D
0xFFFFFFFF, eg: *cmd is valid only when the incoming NOP-OUT generate=

s a

NOP-IN response, but not when the incoming NOP-OUT is a response to a
locally generated NOP-IN.
=20
That said, iscsit_handle_nop_out() already expects *cmd to be NULL wh=

en

TTT !=3D 0xFFFFFFFF, and locates the associated cmd descriptor from t=

he

connection list before stopping the nopin response timer, and releasi=

ng

the associated resources.

You are right. Not sure if coverity could be smarter here - it would
have to assume that hdr->ttt doesn't get changed by some other thread,
which seems unreasonable as a general rule. Anyway, drop this patch.

J=C3=B6rn

--
Money can buy bandwidth, but latency is forever.
-- John R. Mashey

[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 */

[Nicholas A. Bellinger]

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 */

Applied to target-pending/for-next.

[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;
}

[Nicholas A. Bellinger]

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(-)

Applied to target-pending/for-next.

[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);

[Nicholas A. Bellinger]

Hi Joern,

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(-)

Thanks, applied to target-pending/for-next.

--nab

[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 "

[Nicholas A. Bellinger]

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.

Applied to target-pending/for-next.

[Jörn Engel]

=20
Handled comments from Sagi Grimberg. Thanks for the review, Sagi!
=20
target: remove unused debug code
target: fix memory corruption
target: avoid NULL pointer dereference
target: remove always-true conditions
target: simplify return statement
target: fix condition
target: remove unnecessary check
target: avoid NULL pointer dereference
target: avoid buffer overflow
target: plug memory leak
target: plug memory leak
target: simplify target_fabric_make_lun
target: fix pr_out length parsing
target: handle match_int errors
target: fix unused shift
target: check for vector overflows
=20
drivers/target/iscsi/iscsi_target.c | 185 +++++++++------=

----------

drivers/target/iscsi/iscsi_target_configfs.c | 10 +-
drivers/target/iscsi/iscsi_target_erl0.c | 6 +-
drivers/target/iscsi/iscsi_target_login.c | 8 +-
drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
drivers/target/iscsi/iscsi_target_util.c | 5 +-
drivers/target/target_core_configfs.c | 22 ++-
drivers/target/target_core_fabric_configfs.c | 11 +-
drivers/target/target_core_fabric_lib.c | 6 +-
drivers/target/target_core_file.c | 4 +-
drivers/target/target_core_pr.c | 2 +-
drivers/target/target_core_pscsi.c | 16 ++-
12 files changed, 111 insertions(+), 166 deletions(-)

Ping.

J=C3=B6rn

--
Tough times don't last, but tough people do.
-- Nigerian proverb

[Andy Grover]

Handled comments from Sagi Grimberg. Thanks for the review, Sagi!
target: remove unused debug code
target: fix memory corruption
target: avoid NULL pointer dereference
target: remove always-true conditions
target: simplify return statement
target: fix condition
target: remove unnecessary check
target: avoid NULL pointer dereference
target: avoid buffer overflow
target: plug memory leak
target: plug memory leak
target: simplify target_fabric_make_lun
target: fix pr_out length parsing
target: handle match_int errors
target: fix unused shift
target: check for vector overflows
drivers/target/iscsi/iscsi_target.c | 185 +++++++++----=

------------

drivers/target/iscsi/iscsi_target_configfs.c | 10 +-
drivers/target/iscsi/iscsi_target_erl0.c | 6 +-
drivers/target/iscsi/iscsi_target_login.c | 8 +-
drivers/target/iscsi/iscsi_target_parameters.c | 2 +-
drivers/target/iscsi/iscsi_target_util.c | 5 +-
drivers/target/target_core_configfs.c | 22 ++-
drivers/target/target_core_fabric_configfs.c | 11 +-
drivers/target/target_core_fabric_lib.c | 6 +-
drivers/target/target_core_file.c | 4 +-
drivers/target/target_core_pr.c | 2 +-
drivers/target/target_core_pscsi.c | 16 ++-
12 files changed, 111 insertions(+), 166 deletions(-)

Ping.

=46YI patch 12 doesn't apply to target-pending/for-next because of:

45f6c5b target/configfs: Remove unnecessary null test

but it's easy to fix up.

HTH -- Andy

[Nicholas A. Bellinger]

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 and
recovery and dump are useless variables.
Found by Coverity.
---
drivers/target/iscsi/iscsi_target_erl0.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)

Applied to target-pending/for-next.