On Thu, 2014-01-23 at 15:14 +0530, Arshad Hussain wrote:
> Hi Nicholas,
>
> On 1/23/2014 12:37 AM, Nicholas A. Bellinger wrote:
> > Hi Arshad,
> >
> > On Wed, 2014-01-22 at 11:10 +0530, Arshad Hussain wrote:
> >> On 1/16/2014 3:43 PM, Arshad Hussain wrote:
<SNIP>
> >> Hi,
> >>
> >> I Regret the slow turnaround.
> >>
> >> What I got is, if I do not convert from host to network byte order
> >> and assign BegRun and RunLength values greater than 0. ( of course 0 is
> >> same for both endian ) it crashes the machine.
> >>
> >> This was of course error on my side. However should this have crashed
> >> the target.? Shoud not this have returned an error and stopped.
> >> Your comments please.
> >>
> > Looking at the code in iscsi_target_erl1.c:iscsit_handle_r2t_snack(), it
> > is checking the validity of received BegRun + RunLength values, in that
> > they don't exceed what R2TSN has already been put on the wire:
> >
> > if (runlength) {
> > if ((begrun + runlength) > cmd->r2t_sn) {
> > pr_err("Command ITT: 0x%08x received R2T SNACK"
> > " with BegRun: 0x%08x, RunLength: 0x%08x, exceeds"
> > " current R2TSN: 0x%08x, protocol error.\n",
> > cmd->init_task_tag, begrun, runlength, cmd->r2t_sn);
> > return iscsit_reject_cmd(cmd,
> > ISCSI_REASON_BOOKMARK_INVALID, buf);
> > }
> > last_r2tsn = (begrun + runlength);
> > } else
> > last_r2tsn = cmd->r2t_sn;
> >
> > Can you confirm your bug is with R2T SNACKs, and not a different type of
> > SNACK..?
> Yes I confirm it is with R2T SNACK. My two cents. Without htonl()
> conversion the
> value 0x1 (intel) is getting converted into large value 0x10000000 on
> assignment.
> Which in turn is getting assigned to last_r2tsn. And further down your
> code...
>
> while (begrun < last_r2tsn) {
>
> ... // it may be looping in uncontrolled manner and failing.
> begrun++
> }
>
>
So the above check in iscsit_handle_r2t_snack():
if (runlength) {
if ((begrun + runlength) > cmd->r2t_sn) {
<SEND REJECT>
}
}
is what's supposed to be protecting against the reception of a bogus
runlength exceeding the last R2TSN sent out by the command.
How about testing with the following quick debug patch to see why
runlength=0x10000000 is not hitting the above sanity check..?
Thanks,
--nab
diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c
index e048d64..b5b0044 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.c
+++ b/drivers/target/iscsi/iscsi_target_erl1.c
@@ -164,6 +164,9 @@ static int iscsit_handle_r2t_snack(
ISCSI_REASON_PROTOCOL_ERROR, buf);
}
+ printk("Debug #1 begrun: %u runlength: %u cmd->r2t_sn: %u\n",
+ begrun, runlength, cmd->r2t_sn);
+
if (runlength) {
if ((begrun + runlength) > cmd->r2t_sn) {
pr_err("Command ITT: 0x%08x received R2T SNACK"
@@ -177,6 +180,14 @@ static int iscsit_handle_r2t_snack(
} else
last_r2tsn = cmd->r2t_sn;
+ printk("Debug #2 last_r2tsn: %u\n", last_r2tsn);
+
+ if (last_r2tsn > 0x0000ffff) {
+ printk("Ignoring last_r2tsn that's too large: %u\n", last_r2tsn);
+ dump_stack();
+ return 0;
+ }
+
while (begrun < last_r2tsn) {
r2t = iscsit_get_holder_for_r2tsn(cmd, begrun);
if (!r2t)
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
