iscsi bug fixes for 3.3

iscsi bug fixes for 3.3

[michaelc]

The follwing patches were made over scsi-misc/scsi-rc and I think
should be ok for 3.3 fixes.

The first one fixes a patch that was added in 3.3, so it should go
in.

The last 2 patches fix bugs that have been in the kernel for a
while, but could cause oopses or hangs. These last patches
could go to stable.

[PATCH 1/3] iscsi: fix setting of pid from netlink skb

[michaelc]

From: Mike Christie <michaelc@cs.wisc.edu>

NETLINK_CREDS's pid now returns 0, so I guess we are supposed to
be using NETLINK_CB. This changed while the patch to export the
pid was getting merged upstream, so it was not noticed until both
the network and iscsi changes were in the same tree.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/scsi_transport_iscsi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index cfd4914..e3e3c7d 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1941,7 +1941,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
 	switch (nlh->nlmsg_type) {
 	case ISCSI_UEVENT_CREATE_SESSION:
 		err = iscsi_if_create_session(priv, ep, ev,
-					      NETLINK_CREDS(skb)->pid,
+					      NETLINK_CB(skb).pid,
 					      ev->u.c_session.initial_cmdsn,
 					      ev->u.c_session.cmds_max,
 					      ev->u.c_session.queue_depth);
@@ -1954,7 +1954,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
 		}
 
 		err = iscsi_if_create_session(priv, ep, ev,
-					NETLINK_CREDS(skb)->pid,
+					NETLINK_CB(skb).pid,
 					ev->u.c_bound_session.initial_cmdsn,
 					ev->u.c_bound_session.cmds_max,
 					ev->u.c_bound_session.queue_depth);
-- 
1.7.6

[PATCH 2/3] libiscsi_tcp: fix max_r2t manipulation

[michaelc]

From: Mike Christie <michaelc@cs.wisc.edu>

Problem description from Xi Wang:
A large max_r2t could lead to integer overflow in subsequent call to
iscsi_tcp_r2tpool_alloc(), allocating a smaller buffer than expected
and leading to out-of-bounds write.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
cc: xi.wang@gmail.com

---
 drivers/scsi/cxgbi/libcxgbi.c |   13 ++-----------
 drivers/scsi/iscsi_tcp.c      |   13 +------------
 drivers/scsi/libiscsi.c       |    2 +-
 drivers/scsi/libiscsi_tcp.c   |   18 ++++++++++++++++++
 include/scsi/libiscsi.h       |    2 +-
 include/scsi/libiscsi_tcp.h   |    2 +-
 6 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index d3ff9cd..e6e6aa9 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -2148,11 +2148,10 @@ int cxgbi_set_conn_param(struct iscsi_cls_conn *cls_conn,
 			enum iscsi_param param, char *buf, int buflen)
 {
 	struct iscsi_conn *conn = cls_conn->dd_data;
-	struct iscsi_session *session = conn->session;
 	struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
 	struct cxgbi_conn *cconn = tcp_conn->dd_data;
 	struct cxgbi_sock *csk = cconn->cep->csk;
-	int value, err = 0;
+	int err;
 
 	log_debug(1 << CXGBI_DBG_ISCSI,
 		"cls_conn 0x%p, param %d, buf(%d) %s.\n",
@@ -2174,15 +2173,7 @@ int cxgbi_set_conn_param(struct iscsi_cls_conn *cls_conn,
 							conn->datadgst_en, 0);
 		break;
 	case ISCSI_PARAM_MAX_R2T:
-		sscanf(buf, "%d", &value);
-		if (value <= 0 || !is_power_of_2(value))
-			return -EINVAL;
-		if (session->max_r2t == value)
-			break;
-		iscsi_tcp_r2tpool_free(session);
-		err = iscsi_set_param(cls_conn, param, buf, buflen);
-		if (!err && iscsi_tcp_r2tpool_alloc(session))
-			return -ENOMEM;
+		return iscsi_tcp_set_max_r2t(conn, buf);
 	case ISCSI_PARAM_MAX_RECV_DLENGTH:
 		err = iscsi_set_param(cls_conn, param, buf, buflen);
 		if (!err)
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index db47158..453a740 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -684,10 +684,8 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
 				       int buflen)
 {
 	struct iscsi_conn *conn = cls_conn->dd_data;
-	struct iscsi_session *session = conn->session;
 	struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
 	struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
-	int value;
 
 	switch(param) {
 	case ISCSI_PARAM_HDRDGST_EN:
@@ -699,16 +697,7 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
 			sock_no_sendpage : tcp_sw_conn->sock->ops->sendpage;
 		break;
 	case ISCSI_PARAM_MAX_R2T:
-		sscanf(buf, "%d", &value);
-		if (value <= 0 || !is_power_of_2(value))
-			return -EINVAL;
-		if (session->max_r2t == value)
-			break;
-		iscsi_tcp_r2tpool_free(session);
-		iscsi_set_param(cls_conn, param, buf, buflen);
-		if (iscsi_tcp_r2tpool_alloc(session))
-			return -ENOMEM;
-		break;
+		return iscsi_tcp_set_max_r2t(conn, buf);
 	default:
 		return iscsi_set_param(cls_conn, param, buf, buflen);
 	}
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 143bbe4..911a4a9 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -3200,7 +3200,7 @@ int iscsi_set_param(struct iscsi_cls_conn *cls_conn,
 		sscanf(buf, "%d", &session->initial_r2t_en);
 		break;
 	case ISCSI_PARAM_MAX_R2T:
-		sscanf(buf, "%d", &session->max_r2t);
+		sscanf(buf, "%hu", &session->max_r2t);
 		break;
 	case ISCSI_PARAM_IMM_DATA_EN:
 		sscanf(buf, "%d", &session->imm_data_en);
diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index 5715a3d..24f812d 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -1170,6 +1170,24 @@ void iscsi_tcp_r2tpool_free(struct iscsi_session *session)
 }
 EXPORT_SYMBOL_GPL(iscsi_tcp_r2tpool_free);
 
+int iscsi_tcp_set_max_r2t(struct iscsi_conn *conn, char*buf)
+{
+	struct iscsi_session *session = conn->session;
+	unsigned short r2ts = 0;
+
+	sscanf(buf, "%hu", &r2ts);
+	if (session->max_r2t == r2ts)
+		return 0;
+
+	if (!r2ts || !is_power_of_2(r2ts))
+		return -EINVAL;
+
+	session->max_r2t = r2ts;
+	iscsi_tcp_r2tpool_free(session);
+	return iscsi_tcp_r2tpool_alloc(session);
+}
+EXPORT_SYMBOL_GPL(iscsi_tcp_set_max_r2t);
+
 void iscsi_tcp_conn_get_stats(struct iscsi_cls_conn *cls_conn,
 			      struct iscsi_stats *stats)
 {
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index cedcff3..a84c1ad 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -268,7 +268,7 @@ struct iscsi_session {
 	int			lu_reset_timeout;
 	int			tgt_reset_timeout;
 	int			initial_r2t_en;
-	unsigned		max_r2t;
+	unsigned short		max_r2t;
 	int			imm_data_en;
 	unsigned		first_burst;
 	unsigned		max_burst;
diff --git a/include/scsi/libiscsi_tcp.h b/include/scsi/libiscsi_tcp.h
index ac0cc1d..215469a 100644
--- a/include/scsi/libiscsi_tcp.h
+++ b/include/scsi/libiscsi_tcp.h
@@ -128,7 +128,7 @@ extern void iscsi_tcp_conn_teardown(struct iscsi_cls_conn *cls_conn);
 /* misc helpers */
 extern int iscsi_tcp_r2tpool_alloc(struct iscsi_session *session);
 extern void iscsi_tcp_r2tpool_free(struct iscsi_session *session);
-
+extern int iscsi_tcp_set_max_r2t(struct iscsi_conn *conn, char *buf);
 extern void iscsi_tcp_conn_get_stats(struct iscsi_cls_conn *cls_conn,
 				     struct iscsi_stats *stats);
 #endif /* LIBISCSI_TCP_H */
-- 
1.7.6

[PATCH 3/3] libiscsi: fix cmd timeout/completion race

[michaelc]

From: Mike Christie <michaelc@cs.wisc.edu>

If the driver/lib has called scsi_done and cleaned up internally but
scsi layer has not yet called blk_mark_rq_complete when the command
times out we hit a problem if the timeout code calls blk_mark_rq_complete first.
When the time out code calls into the driver we were returning
BLK_EH_RESET_TIMER and that causes the timeout code to just call
us again later.

We need to be calling BLK_EH_HANDLED so the timeout code can complete
the completion process because it had called blk_mark_rq_complete
on the command and now owns its processing.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/libiscsi.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 911a4a9..b189227 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1909,6 +1909,16 @@ static enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 	ISCSI_DBG_EH(session, "scsi cmd %p timedout\n", sc);
 
 	spin_lock(&session->lock);
+	task = (struct iscsi_task *)sc->SCp.ptr;
+	if (!task) {
+		/*
+		 * Raced with completion. Blk layer has taken ownership
+		 * so let timeout code complete it now.
+		 */
+		rc = BLK_EH_HANDLED;
+		goto done;
+	}
+
 	if (session->state != ISCSI_STATE_LOGGED_IN) {
 		/*
 		 * We are probably in the middle of iscsi recovery so let
@@ -1925,16 +1935,6 @@ static enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
 		goto done;
 	}
 
-	task = (struct iscsi_task *)sc->SCp.ptr;
-	if (!task) {
-		/*
-		 * Raced with completion. Just reset timer, and let it
-		 * complete normally
-		 */
-		rc = BLK_EH_RESET_TIMER;
-		goto done;
-	}
-
 	/*
 	 * If we have sent (at least queued to the network layer) a pdu or
 	 * recvd one for the task since the last timeout ask for
-- 
1.7.6

Re: [PATCH 1/3] iscsi: fix setting of pid from netlink skb

[Bart Van Assche]

On Fri, Jan 27, 2012 at 4:13 AM, <michaelc@cs.wisc.edu> wrote:
> NETLINK_CREDS's pid now returns 0, so I guess we are supposed to
> be using NETLINK_CB. This changed while the patch to export the
> pid was getting merged upstream, so it was not noticed until both
> the network and iscsi changes were in the same tree.
>
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

If my analysis is correct this patch has not only to be applied on the
master branch but also on kernel 3.2. Shouldn't "Cc:
stable@vger.kernel.org #3.2" be added here ? See also
http://www.spinics.net/lists/netdev/msg185565.html.

Bart.

Re: [PATCH 1/3] iscsi: fix setting of pid from netlink skb

[Mike Christie]

On 01/27/2012 01:04 AM, Bart Van Assche wrote:
> On Fri, Jan 27, 2012 at 4:13 AM, <michaelc@cs.wisc.edu> wrote:
>> NETLINK_CREDS's pid now returns 0, so I guess we are supposed to
>> be using NETLINK_CB. This changed while the patch to export the
>> pid was getting merged upstream, so it was not noticed until both
>> the network and iscsi changes were in the same tree.
>>
>> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> 
> If my analysis is correct this patch has not only to be applied on the
> master branch but also on kernel 3.2. Shouldn't "Cc:
> stable@vger.kernel.org #3.2" be added here ? See also
> http://www.spinics.net/lists/netdev/msg185565.html.
> 

The iscsi code that uses the pid just got merged in the 3.3 feature
window, so no need.

Also for stable, I think we normally just tell James it should go in
stable and then he does some magic. In the past, when he knows it should
go there, when the patch gets merged and sent upstream the stable people
were notified automagically.

Re: [PATCH 1/3] iscsi: fix setting of pid from netlink skb

[James Bottomley]

On Fri, 2012-01-27 at 01:37 -0600, Mike Christie wrote:
> On 01/27/2012 01:04 AM, Bart Van Assche wrote:
> > On Fri, Jan 27, 2012 at 4:13 AM, <michaelc@cs.wisc.edu> wrote:
> >> NETLINK_CREDS's pid now returns 0, so I guess we are supposed to
> >> be using NETLINK_CB. This changed while the patch to export the
> >> pid was getting merged upstream, so it was not noticed until both
> >> the network and iscsi changes were in the same tree.
> >>
> >> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> > 
> > If my analysis is correct this patch has not only to be applied on the
> > master branch but also on kernel 3.2. Shouldn't "Cc:
> > stable@vger.kernel.org #3.2" be added here ? See also
> > http://www.spinics.net/lists/netdev/msg185565.html.
> > 
> 
> The iscsi code that uses the pid just got merged in the 3.3 feature
> window, so no need.
> 
> Also for stable, I think we normally just tell James it should go in
> stable and then he does some magic. In the past, when he knows it should
> go there, when the patch gets merged and sent upstream the stable people
> were notified automagically.

That's the

cc: stable@kernel.org

tag in the commit ... but don't necessarily rely on me to add this.  I
do add it in a few cases, but I do need people to indicate should go to
stable by adding the cc: in their commit, otherwise I have to guess.

James