[PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Amit Arora]

The scsi_scan_host_selected() should return -EINVAL when the id is equal
to the max_id. Currently it uses ">" when comparing with max_id, and
hence leaves the border case when "id==max_id".
The channel and lun have values valid from 0 up to,
and including, max_channel or max_lun. But, the valid values for id
range from 0 to max_id-1. This patch fixes the problem.

Regards,
Amit Arora

Signed-off-by: Amit Arora <aarora@in.ibm.com>
---
diff -Nuarp linux-2.6.17-rc4.orig/drivers/scsi/scsi_scan.c
linux-2.6.17-rc4/drivers/scsi/scsi_scan.c
--- linux-2.6.17-rc4.orig/drivers/scsi/scsi_scan.c      2006-05-18
20:25:32.000000000 -0600
+++ linux-2.6.17-rc4/drivers/scsi/scsi_scan.c   2006-05-18
20:28:09.000000000 -0600
@@ -1473,7 +1473,7 @@ int scsi_scan_host_selected(struct Scsi_
                __FUNCTION__, channel, id, lun));
  
        if (((channel != SCAN_WILD_CARD) && (channel >
shost->max_channel)) ||
-           ((id != SCAN_WILD_CARD) && (id > shost->max_id)) ||
+           ((id != SCAN_WILD_CARD) && (id >= shost->max_id)) ||
            ((lun != SCAN_WILD_CARD) && (lun > shost->max_lun)))
                return -EINVAL;

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Matthew Wilcox]

On Fri, May 19, 2006 at 04:14:50PM -0700, Amit Arora wrote:
> The scsi_scan_host_selected() should return -EINVAL when the id is equal
> to the max_id. Currently it uses ">" when comparing with max_id, and
> hence leaves the border case when "id==max_id".
> The channel and lun have values valid from 0 up to,
> and including, max_channel or max_lun. But, the valid values for id
> range from 0 to max_id-1. This patch fixes the problem.

You're right, but the patch is wrong.  It's not acceptable to have
different meanings for variables with such similar names.  Either it
needs to be renamed to id_count or we need to fix all the other users of
max_id.

BTW, I think we have another problem with max_lun:

	max_dev_lun = min(max_scsi_luns, shost->max_lun);
	...
	for (lun = 1; lun < max_dev_lun; ++lun)

surely that should be <= ?

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Amit Arora]

On Fri, 2006-05-19 at 21:21, Matthew Wilcox wrote:
> On Fri, May 19, 2006 at 04:14:50PM -0700, Amit Arora wrote:
> > The scsi_scan_host_selected() should return -EINVAL when the id is equal
> > to the max_id. Currently it uses ">" when comparing with max_id, and
> > hence leaves the border case when "id==max_id".
> > The channel and lun have values valid from 0 up to,
> > and including, max_channel or max_lun. But, the valid values for id
> > range from 0 to max_id-1. This patch fixes the problem.
> 
> You're right, but the patch is wrong.  It's not acceptable to have
> different meanings for variables with such similar names.  Either it
> needs to be renamed to id_count or we need to fix all the other users of
> max_id.

I agree that similar sounding variable names should not have different
meanings, but having this slight inconsistency for the time being (till
we have a new naming convention for these variables) is better than
having a bug in the code which brings down the whole system ! Obviously
the current code is wrong and is thus resulting in a system crash
(actually a BUG_ON) when following command is issued on a system with an
Adaptec SCSI controller (aic7xxx/aic79xx driver) :
"echo 0 16 0 > /sys/class/scsi_host/host0/scan"
It might behave similarly on other drivers as well.

Thus, I still think applying the patch might be a good idea in the
immediate future. Moreover, this patch doesn't change the current
definition of any of these variables and results in a behavior which is
currently expected. So, till the time we figure out what should be done
in the long run to remove any confusion over the definition of these
variables - why not apply the patch ?

Sorry, being a novice in this area, I did not understand what you meant
by "other users of max_id". I think all the drivers currently have it as
"maximum value of id possible" + 1. Please correct me if I am wrong.

> 
> BTW, I think we have another problem with max_lun:
> 
> 	max_dev_lun = min(max_scsi_luns, shost->max_lun);
> 	...
> 	for (lun = 1; lun < max_dev_lun; ++lun)
> 
> surely that should be <= ?

Yes, looks wrong to me.


Thanks for your reply!
Regards,
Amit Arora

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Luben Tuikov]

--- Amit Arora <aarora@in.ibm.com> wrote:
> On Fri, 2006-05-19 at 21:21, Matthew Wilcox wrote:
> > On Fri, May 19, 2006 at 04:14:50PM -0700, Amit Arora wrote:
> > > The scsi_scan_host_selected() should return -EINVAL when the id is equal
> > > to the max_id. Currently it uses ">" when comparing with max_id, and
> > > hence leaves the border case when "id==max_id".
> > > The channel and lun have values valid from 0 up to,
> > > and including, max_channel or max_lun. But, the valid values for id
> > > range from 0 to max_id-1. This patch fixes the problem.
> > 
> > You're right, but the patch is wrong.  It's not acceptable to have
> > different meanings for variables with such similar names.  Either it
> > needs to be renamed to id_count or we need to fix all the other users of
> > max_id.
> 
> I agree that similar sounding variable names should not have different
> meanings, but having this slight inconsistency for the time being (till
> we have a new naming convention for these variables) is better than
> having a bug in the code which brings down the whole system ! Obviously
> the current code is wrong and is thus resulting in a system crash
> (actually a BUG_ON) when following command is issued on a system with an
> Adaptec SCSI controller (aic7xxx/aic79xx driver) :
> "echo 0 16 0 > /sys/class/scsi_host/host0/scan"
> It might behave similarly on other drivers as well.
> 
> Thus, I still think applying the patch might be a good idea in the
> immediate future. Moreover, this patch doesn't change the current
> definition of any of these variables and results in a behavior which is
> currently expected. So, till the time we figure out what should be done
> in the long run to remove any confusion over the definition of these
> variables - why not apply the patch ?
> 
> Sorry, being a novice in this area, I did not understand what you meant
> by "other users of max_id". I think all the drivers currently have it as
> "maximum value of id possible" + 1. Please correct me if I am wrong.
> 
> > 
> > BTW, I think we have another problem with max_lun:
> > 
> > 	max_dev_lun = min(max_scsi_luns, shost->max_lun);
> > 	...
> > 	for (lun = 1; lun < max_dev_lun; ++lun)
> > 
> > surely that should be <= ?
> 
> Yes, looks wrong to me.

Hi Amit,

Welcome to the club!

You'll find a lot more other "bugs" in the Linux SCSI Core.  Not much
has changed in the past 6 years, nothing in the core at least.

As to your particular gripe: you're right.  But I doubt anything would
get fixed or changed any time soon.  There is very little competence
which also has power to change things in the SCSI part of the Linux.
Take a look at the SCSI mailing list archives for good ol' laughs.

People here think that the concept of a "host" should be escalated to
the block subsystem, people here think that "REQUEST SENSE" clears ACA,
and that calling done() and then turning around to do eh on the
command in question is ok.  People here think that using link
commands has something to do with barriers and atomic transactions.
"SCSI device with Target ports" is called "techno-gibberish" here.

Anyway, I'd suggest fixing such things in your own version/git tree
of the kernel.

As to naming: I always call such things "num_of_xyz" to be absolutely
clear that the enumeration is "0 <= index_xyz < num_of_xyz".

Good luck,
    Luben

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Matthew Wilcox]

On Sat, May 20, 2006 at 08:34:28PM -0700, Luben Tuikov wrote:
> As to your particular gripe: you're right.  But I doubt anything would
> get fixed or changed any time soon.  There is very little competence
> which also has power to change things in the SCSI part of the Linux.
> Take a look at the SCSI mailing list archives for good ol' laughs.

Hey, Luben.  Nice to hear from you again.  Unfortunately, you seem to
have forgotton to attach a patch to solve this particular issue.

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Amit Arora]

On Sat, 2006-05-20 at 20:34, Luben Tuikov wrote:
> Hi Amit,
> 
> Welcome to the club!
> 
> You'll find a lot more other "bugs" in the Linux SCSI Core.  Not much
> has changed in the past 6 years, nothing in the core at least.
> 
> As to your particular gripe: you're right.  But I doubt anything would
> get fixed or changed any time soon.  There is very little competence
> which also has power to change things in the SCSI part of the Linux.
> Take a look at the SCSI mailing list archives for good ol' laughs.
> 
> People here think that the concept of a "host" should be escalated to
> the block subsystem, people here think that "REQUEST SENSE" clears ACA,
> and that calling done() and then turning around to do eh on the
> command in question is ok.  People here think that using link
> commands has something to do with barriers and atomic transactions.
> "SCSI device with Target ports" is called "techno-gibberish" here.
> 
> Anyway, I'd suggest fixing such things in your own version/git tree
> of the kernel.
> 
> As to naming: I always call such things "num_of_xyz" to be absolutely
> clear that the enumeration is "0 <= index_xyz < num_of_xyz".
> 
> Good luck,
>     Luben 

Hi Luben,

Thanks for replying!

I understand the competency part - but then, there are experts like you
who can surely fix such problems. :)
I would appreciate if you can come up with an acceptable patch for this.

Thanks again!
Regards,
Amit Arora

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[James Bottomley]

On Sat, 2006-05-20 at 12:41 -0700, Amit Arora wrote:
> Thus, I still think applying the patch might be a good idea in the
> immediate future. Moreover, this patch doesn't change the current
> definition of any of these variables and results in a behavior which
> is
> currently expected. So, till the time we figure out what should be
> done
> in the long run to remove any confusion over the definition of these
> variables - why not apply the patch ?

Actually, we've got another cockup here with drivers: some have set this
to 8 or 16 and others to 7 or 15.  If we apply this without auditing
them, for those who set it to 7 or 15, the last target will end up
inaccessible.

James

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Matthew Wilcox]

On Mon, May 22, 2006 at 07:54:35PM -0500, James Bottomley wrote:
> Actually, we've got another cockup here with drivers: some have set this
> to 8 or 16 and others to 7 or 15.  If we apply this without auditing
> them, for those who set it to 7 or 15, the last target will end up
> inaccessible.

So as scsi maintainer, what's your preference for the 'right way' to fix
this?  Clearly a whole-scale driver audit is needed, so my preference is
to rename the variable (how about id_limit?) and then do a sweep
checking that everybody's using it correctly.

Then we need to do a similar check for max_lun and max_channel.  As far
as conventions go, I think we should use 8 and 16; it's just so much
more natural to write 'for (id = 0; id < id_limit; id++)'.

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Hannes Reinecke]

Matthew Wilcox wrote:
> On Mon, May 22, 2006 at 07:54:35PM -0500, James Bottomley wrote:
>> Actually, we've got another cockup here with drivers: some have set this
>> to 8 or 16 and others to 7 or 15.  If we apply this without auditing
>> them, for those who set it to 7 or 15, the last target will end up
>> inaccessible.
> 
> So as scsi maintainer, what's your preference for the 'right way' to fix
> this?  Clearly a whole-scale driver audit is needed, so my preference is
> to rename the variable (how about id_limit?) and then do a sweep
> checking that everybody's using it correctly.
> 
Ah. I see a pattern emerging.

ncr53c7xx.c has this:

#ifdef LINUX_1_2
	|| cmd->device->id > 7
#else
	|| cmd->device->id > host->max_id
#endif

So appearently in the good old days max_id was indeed defined as the
highest available target number. Whereas most 'modern' drivers define
this c-style-wise as the first non-available number.

So I would go for the latter approach and audit the drivers.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

Hannes Reinecke wrote:
> Matthew Wilcox wrote:
>> On Mon, May 22, 2006 at 07:54:35PM -0500, James Bottomley wrote:
>>> Actually, we've got another cockup here with drivers: some have set this
>>> to 8 or 16 and others to 7 or 15.  If we apply this without auditing
>>> them, for those who set it to 7 or 15, the last target will end up
>>> inaccessible.
>> So as scsi maintainer, what's your preference for the 'right way' to fix
>> this?  Clearly a whole-scale driver audit is needed, so my preference is
>> to rename the variable (how about id_limit?) and then do a sweep
>> checking that everybody's using it correctly.
>>
> Ah. I see a pattern emerging.
> 
> ncr53c7xx.c has this:
> 
> #ifdef LINUX_1_2
> 	|| cmd->device->id > 7
> #else
> 	|| cmd->device->id > host->max_id
> #endif
> 
> So appearently in the good old days max_id was indeed defined as the
> highest available target number. Whereas most 'modern' drivers define
> this c-style-wise as the first non-available number.
> 
> So I would go for the latter approach and audit the drivers.
> 
Wasn't too bad actually. Only some very old drivers did it wrong.
I don't think we'd need to redo everything by renaming variables.
Some fixing will be sufficient (see attached patch).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

[-- Attachment #2: scsi-scan-fix-max_id --]
[-- Type: text/plain, Size: 3220 bytes --]

diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c
index 6a0f950..b767918 100644
--- a/drivers/scsi/53c700.c
+++ b/drivers/scsi/53c700.c
@@ -376,7 +376,7 @@ NCR_700_detect(struct scsi_host_template
 	dma_sync_single_for_device(hostdata->dev, pScript, sizeof(SCRIPT), DMA_TO_DEVICE);
 	hostdata->state = NCR_700_HOST_FREE;
 	hostdata->cmd = NULL;
-	host->max_id = 7;
+	host->max_id = 8;
 	host->max_lun = NCR_700_MAX_LUNS;
 	BUG_ON(NCR_700_transport_template == NULL);
 	host->transportt = NCR_700_transport_template;
diff --git a/drivers/scsi/53c7xx.c b/drivers/scsi/53c7xx.c
index 7894b8e..cfc9912 100644
--- a/drivers/scsi/53c7xx.c
+++ b/drivers/scsi/53c7xx.c
@@ -709,7 +709,7 @@ request_synchronous (int host, int targe
 	printk (KERN_ALERT "target %d is host ID\n", target);
 	return -1;
     } 
-    else if (target > h->max_id) {
+    else if (target >= h->max_id) {
 	printk (KERN_ALERT "target %d exceeds maximum of %d\n", target,
 	    h->max_id);
 	return -1;
@@ -3622,7 +3622,7 @@ #endif
 #ifdef LINUX_1_2
 	|| cmd->device->id > 7
 #else
-	|| cmd->device->id > host->max_id
+	|| cmd->device->id >= host->max_id
 #endif
 	|| cmd->device->id == host->this_id
 	|| hostdata->state == STATE_DISABLED) {
diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c b/drivers/scsi/aic7xxx/aic79xx_core.c
index 08771f6..e14244a 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -9396,8 +9396,8 @@ ahd_find_tmode_devs(struct ahd_softc *ah
 	} else {
 		u_int max_id;
 
-		max_id = (ahd->features & AHD_WIDE) ? 15 : 7;
-		if (ccb->ccb_h.target_id > max_id)
+		max_id = (ahd->features & AHD_WIDE) ? 16 : 8;
+		if (ccb->ccb_h.target_id >= max_id)
 			return (CAM_TID_INVALID);
 
 		if (ccb->ccb_h.target_lun >= AHD_NUM_LUNS)
diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c b/drivers/scsi/aic7xxx/aic7xxx_core.c
index d375669..50a3dd0 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_core.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_core.c
@@ -6774,8 +6774,8 @@ ahc_find_tmode_devs(struct ahc_softc *ah
 	} else {
 		u_int max_id;
 
-		max_id = (ahc->features & AHC_WIDE) ? 15 : 7;
-		if (ccb->ccb_h.target_id > max_id)
+		max_id = (ahc->features & AHC_WIDE) ? 16 : 8;
+		if (ccb->ccb_h.target_id >= max_id)
 			return (CAM_TID_INVALID);
 
 		if (ccb->ccb_h.target_lun >= AHC_NUM_LUNS)
diff --git a/drivers/scsi/atp870u.c b/drivers/scsi/atp870u.c
index a198d86..58d7e34 100644
--- a/drivers/scsi/atp870u.c
+++ b/drivers/scsi/atp870u.c
@@ -3047,7 +3047,7 @@ #endif		
 		if (atp_dev.chip_ver == 4)
 			shpnt->max_id = 16;
 		else		
-			shpnt->max_id = 7;
+			shpnt->max_id = 8;
 		shpnt->this_id = host_id;
 		shpnt->unique_id = base_io;
 		shpnt->io_port = base_io;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f85d910..fd97d07 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1503,7 +1503,7 @@ int scsi_scan_host_selected(struct Scsi_
 		__FUNCTION__, channel, id, lun));
 
 	if (((channel != SCAN_WILD_CARD) && (channel > shost->max_channel)) ||
-	    ((id != SCAN_WILD_CARD) && (id > shost->max_id)) ||
+	    ((id != SCAN_WILD_CARD) && (id >= shost->max_id)) ||
 	    ((lun != SCAN_WILD_CARD) && (lun > shost->max_lun)))
 		return -EINVAL;
 

Re: [PATCH] scsi: Return -EINVAL when "id == max_id" in scsi_scan_host_selected()

[Luben Tuikov]

--- Matthew Wilcox <matthew@wil.cx> wrote:
> So as scsi maintainer, what's your preference for the 'right way' to fix
> this?  Clearly a whole-scale driver audit is needed, so my preference is
> to rename the variable (how about id_limit?) and then do a sweep
> checking that everybody's using it correctly.
> 
> Then we need to do a similar check for max_lun and max_channel.  As far
> as conventions go, I think we should use 8 and 16; it's just so much
> more natural to write 'for (id = 0; id < id_limit; id++)'.

Think about it: "id_limit".  What if I set "id_limit" equal 0?
Does this mean that the last id is 0 or does it mean that are simply no
ids.

The literature calls such things "number of" ("num_of_xyz") or
"maxiumum number of" ("max_num_xyz").

If you called it "num_of_ids" or "max_num_ids" then there is no
ambiguity that what it talks about is the (maximum) _number_ (as in count)
of the ids.  An index can never be >= the count, else BUG.

     Luben
P.S. Then after that patch, seeing that there is no possible enumeration
to ids other tha in an SPI layer, another patch should possibly get rid of it.