Re: [PATCH] iscsi-target: Fix bug in handling of ExpStatSN ACK during u32 wrap-around

[Roland Dreier]

On Wed, Dec 26, 2012 at 1:33 PM, Ben Hutchings b...@decadent.org.uk wrote:
   if (!(cmd-cmd_flags  ICF_OOO_CMDSN)  !cmd-immediate_cmd 
 -  (cmd-cmd_sn = conn-sess-exp_cmd_sn)) {
 +  iscsi_sna_gte(cmd-stat_sn, conn-sess-exp_cmd_sn)) {
   list_del(cmd-i_conn_node);
   spin_unlock_bh(conn-cmd_lock);
   iscsit_free_cmd(cmd);
 [...]

 This changes cmd-cmd_sn to cmd-stat_sn, but the commit message only
 describes fixes to wrap-around.  Is that another fix or a bug?

Great catch, thanks!  This is indeed a bug introduced by the patch.
I'll send out a fix; I guess you could roll it up into this patch, but
I'm not sure any of this stuff is worth getting into 3.2.

 - R.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iscsi-target: Fix bug in handling of ExpStatSN ACK during u32 wrap-around

[Ben Hutchings]

This patch has just made its way into my queue for 3.2.y:

On Mon, 2012-11-05 at 18:02 -0800, Steve Hodgson wrote:
 This patch fixes a bug in the hanlding of initiator provided ExpStatSN and
 individual iscsi_cmd-stat_sn comparision during iscsi_conn-stat_sn
 wrap-around within iscsit_ack_from_expstatsn() code.
 
 This bug would manifest itself as iscsi_cmd descriptors not being Acked
 by a lower ExpStatSn, causing them to be leaked until an iSCSI connection
 or session reinstatement event occurs to release all commands.
 
 Also fix up two other uses of incorrect CmdSN SNA comparison to use wrapper
 usage from include/scsi/iscsi_proto.h.
[...]
 --- a/drivers/target/iscsi/iscsi_target_erl2.c
 +++ b/drivers/target/iscsi/iscsi_target_erl2.c
 @@ -372,7 +372,7 @@ int iscsit_prepare_cmds_for_realligance(struct iscsi_conn 
 *conn)
* made generic here.
*/
   if (!(cmd-cmd_flags  ICF_OOO_CMDSN)  !cmd-immediate_cmd 
 -  (cmd-cmd_sn = conn-sess-exp_cmd_sn)) {
 +  iscsi_sna_gte(cmd-stat_sn, conn-sess-exp_cmd_sn)) {
   list_del(cmd-i_conn_node);
   spin_unlock_bh(conn-cmd_lock);
   iscsit_free_cmd(cmd);
[...]

This changes cmd-cmd_sn to cmd-stat_sn, but the commit message only
describes fixes to wrap-around.  Is that another fix or a bug?

Ben.

-- 
Ben Hutchings
If God had intended Man to program,
we'd have been born with serial I/O ports.


signature.asc
Description: This is a digitally signed message part