[PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[Alan Stern]

This patch (as1196) adds a new scsi_device flag to tell sd.c that it
should verify the results of READ CAPACITY by trying the read the last
sector.  This is necessary because a large percentage of USB
mass-storage devices -- too many for a blacklist -- have a bug whereby
they return the total number of sectors rather than the index of the
last sector.

The new sd_read_last_sector() routine carries out the test, using a
very short timeout and a small number of retries.  Any working device
for which the check_capacity flag is set should be able to pass the
test easily.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

James & Greg:

Although part of this patch touches usb-storage, the majority of 
it affects SCSI files.  Is it okay if the whole thing goes in via 
James's tree?

Alan Stern



Index: usb-2.6/include/scsi/scsi_device.h
===================================================================
--- usb-2.6.orig/include/scsi/scsi_device.h
+++ usb-2.6/include/scsi/scsi_device.h
@@ -142,6 +142,7 @@ struct scsi_device {
 	unsigned select_no_atn:1;
 	unsigned fix_capacity:1;	/* READ_CAPACITY is too high by 1 */
 	unsigned guess_capacity:1;	/* READ_CAPACITY might be too high by 1 */
+	unsigned check_capacity:1;	/* Verify READ_CAPACITY result */
 	unsigned retry_hwerror:1;	/* Retry HARDWARE_ERROR */
 	unsigned last_sector_bug:1;	/* do not use multisector accesses on
 					   SD_LAST_BUGGY_SECTORS */
Index: usb-2.6/drivers/scsi/sd.c
===================================================================
--- usb-2.6.orig/drivers/scsi/sd.c
+++ usb-2.6/drivers/scsi/sd.c
@@ -1426,6 +1426,89 @@ static int sd_try_rc16_first(struct scsi
 	return 0;
 }
 
+#if defined(CONFIG_USB_STORAGE) || defined(CONFIG_USB_STORAGE_MODULE)
+/*
+ * Test disk capacity by trying to read the last sector
+ */
+static int sd_read_last_sector(struct scsi_disk *sdkp, unsigned char *buffer,
+		int sector_size)
+{
+	struct scsi_device *sdp = sdkp->device;
+	u32 last;
+	int result, resid;
+	int rc = 0;
+	unsigned char cmd[10];
+
+	/* If the capacity is unknown, we can't test anything */
+	if (sdkp->capacity <= 0)
+		return rc;
+
+	/* One test should be enough (unless it's inconclusive) */
+	sdp->check_capacity = 0;
+
+	/* If the device doesn't use READ(10), assume we're okay */
+	if (!sdp->use_10_for_rw)
+		return rc;
+
+	/* If the capacity is too big for READ(10), assume we're okay */
+	if (sdkp->capacity > 0xffffffff)
+		return rc;
+
+	/* If the sector size is too big, assume we're okay */
+	if (sector_size > SD_BUF_SIZE)
+		return rc;
+	if (sector_size == 0)
+		sector_size = 512;
+
+	if (!sdkp->openers++ && sdp->removable)
+		scsi_set_medium_removal(sdp, SCSI_REMOVAL_PREVENT);
+
+	/* Start by trying to read the first sector, just to warm up */
+	memset(cmd, 0, 10);
+	cmd[0] = READ_10;
+	cmd[8] = 1;
+	result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
+			buffer, sector_size, NULL,
+			SD_TIMEOUT, SD_MAX_RETRIES, &resid);
+	if (result || resid) {
+		sdp->check_capacity = 1;	/* Try again later */
+		goto done;
+	}
+
+	/* Now try to read the last sector */
+	last = sdkp->capacity - 1;
+	cmd[2] = (last >> 24);
+	cmd[3] = (last >> 16);
+	cmd[4] = (last >> 8);
+	cmd[5] = last;
+
+	/* Allow only 1 retry, with a short timeout (some devices
+	 * violate protocol, forcing the command to be aborted).
+	 */
+	result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE,
+			buffer, sector_size, NULL, HZ, 1, &resid);
+	if (result || resid) {
+		sdp->fix_capacity = 1;		/* For next time */
+		rc = -EIO;
+	}
+
+ done:
+	if (!--sdkp->openers && sdp->removable) {
+		if (scsi_block_when_processing_errors(sdp))
+			scsi_set_medium_removal(sdp, SCSI_REMOVAL_ALLOW);
+	}
+	return rc;
+}
+
+#else
+static inline int sd_read_last_sector(struct scsi_disk *sdkp,
+		unsigned char *buffer, int sector_size)
+{
+	return 0;
+}
+
+#endif /* defined(CONFIG_USB_STORAGE) || ... */
+
 /*
  * read disk capacity
  */
@@ -1479,7 +1562,9 @@ sd_read_capacity(struct scsi_disk *sdkp,
 	 * the capacity.
 	 */
 	if (sdp->fix_capacity ||
-	    (sdp->guess_capacity && (sdkp->capacity & 0x01))) {
+	    (sdp->guess_capacity && (sdkp->capacity & 0x01)) ||
+	    (sdp->check_capacity &&
+			sd_read_last_sector(sdkp, buffer, sector_size) < 0)) {
 		sd_printk(KERN_INFO, sdkp, "Adjusting the sector count "
 				"from its reported value: %llu\n",
 				(unsigned long long) sdkp->capacity);
Index: usb-2.6/drivers/usb/storage/scsiglue.c
===================================================================
--- usb-2.6.orig/drivers/usb/storage/scsiglue.c
+++ usb-2.6/drivers/usb/storage/scsiglue.c
@@ -209,6 +209,14 @@ static int slave_configure(struct scsi_d
 		if (us->fflags & US_FL_CAPACITY_HEURISTICS)
 			sdev->guess_capacity = 1;
 
+		/* To catch devices that have the READ CAPACITY bug and
+		 * aren't on our blacklist, tell the SCSI disk driver to
+		 * test the last sector.  This flag will be ignored if
+		 * either of the previous flags causes the capacity to
+		 * be decremented. */
+		if (!(us->fflags & US_FL_CAPACITY_OK))
+			sdev->check_capacity = 1;
+
 		/* assume SPC3 or latter devices support sense size > 18 */
 		if (sdev->scsi_level > SCSI_SPC_2)
 			us->fflags |= US_FL_SANE_SENSE;
Index: usb-2.6/drivers/usb/storage/transport.c
===================================================================
--- usb-2.6.orig/drivers/usb/storage/transport.c
+++ usb-2.6/drivers/usb/storage/transport.c
@@ -560,6 +560,12 @@ static void last_sector_hacks(struct us_
 	if (sector + 1 != sdkp->capacity)
 		goto done;
 
+	/* Has the capacity already been decremented? */
+	if (sdkp->device->fix_capacity) {
+		us->use_last_sector_hacks = 0;
+		return;
+	}
+
 	if (srb->result == SAM_STAT_GOOD && scsi_get_resid(srb) == 0) {
 
 		/* The command succeeded.  We know this device doesn't

Re: [PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[Matthew Wilcox]

On Wed, Apr 22, 2009 at 10:42:36AM -0400, Alan Stern wrote:
> +	/* If the capacity is unknown, we can't test anything */
> +	if (sdkp->capacity <= 0)
> +		return rc;

sector_t is unsigned, so this should be just == 0.

> +	/* One test should be enough (unless it's inconclusive) */
> +	sdp->check_capacity = 0;
> +
> +	/* If the device doesn't use READ(10), assume we're okay */
> +	if (!sdp->use_10_for_rw)
> +		return rc;
> +
> +	/* If the capacity is too big for READ(10), assume we're okay */
> +	if (sdkp->capacity > 0xffffffff)
> +		return rc;

Won't gcc warn about this when sector_t is 32 bits?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[Greg KH]

On Wed, Apr 22, 2009 at 10:42:36AM -0400, Alan Stern wrote:
> This patch (as1196) adds a new scsi_device flag to tell sd.c that it
> should verify the results of READ CAPACITY by trying the read the last
> sector.  This is necessary because a large percentage of USB
> mass-storage devices -- too many for a blacklist -- have a bug whereby
> they return the total number of sectors rather than the index of the
> last sector.
> 
> The new sd_read_last_sector() routine carries out the test, using a
> very short timeout and a small number of retries.  Any working device
> for which the check_capacity flag is set should be able to pass the
> test easily.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> James & Greg:
> 
> Although part of this patch touches usb-storage, the majority of 
> it affects SCSI files.  Is it okay if the whole thing goes in via 
> James's tree?

No objection from me at all.  Feel free to add a:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
to the patch and push it through James.

thanks,

greg k-h

Re: [PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[Alan Stern]

On Wed, 22 Apr 2009, Matthew Wilcox wrote:

> On Wed, Apr 22, 2009 at 10:42:36AM -0400, Alan Stern wrote:
> > +	/* If the capacity is unknown, we can't test anything */
> > +	if (sdkp->capacity <= 0)
> > +		return rc;
> 
> sector_t is unsigned, so this should be just == 0.

It doesn't make any difference.

> > +	/* One test should be enough (unless it's inconclusive) */
> > +	sdp->check_capacity = 0;
> > +
> > +	/* If the device doesn't use READ(10), assume we're okay */
> > +	if (!sdp->use_10_for_rw)
> > +		return rc;
> > +
> > +	/* If the capacity is too big for READ(10), assume we're okay */
> > +	if (sdkp->capacity > 0xffffffff)
> > +		return rc;
> 
> Won't gcc warn about this when sector_t is 32 bits?

My version of gcc doesn't emit a warning (and I don't have CONFIG_LBD 
defined).  Maybe other versions will.

Alan Stern

Re: [usb-storage] [PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[Andries E. Brouwer]

On Wed, Apr 22, 2009 at 10:42:36AM -0400, Alan Stern wrote:
> This patch (as1196) adds a new scsi_device flag to tell sd.c that it
> should verify the results of READ CAPACITY by trying the read the last
> sector.

Hi Alan,

I can see why you want to do this, but allow me to mutter a bit nevertheless.
Many devices are flaky and get into strange states requiring error-recovery
or reboot when you try an I/O that they do not like.
For this reason, and also for aesthetical reasons, I would prefer never to
do I/O to a device unless user space asks for it.

(So - this would be much more work, but I would prefer a capacity value
that says "it reported this, but we have not checked yet - try an actual
I/O if you really want to know", and leave it at that until the value is
needed. Typically one needs the value (i) to check against it if one
wants to do I/O, or (ii) when user space asks for it because some fdisk type
program is invoked. In case (i) an additional read is superfluous.
In case (ii) user space asked and we try to make sure.)

Comments?

Andries

Re: [usb-storage] [PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[Alan Stern]

On Wed, 22 Apr 2009, Andries E. Brouwer wrote:

> On Wed, Apr 22, 2009 at 10:42:36AM -0400, Alan Stern wrote:
> > This patch (as1196) adds a new scsi_device flag to tell sd.c that it
> > should verify the results of READ CAPACITY by trying the read the last
> > sector.
> 
> Hi Alan,
> 
> I can see why you want to do this, but allow me to mutter a bit nevertheless.
> Many devices are flaky and get into strange states requiring error-recovery
> or reboot when you try an I/O that they do not like.
> For this reason, and also for aesthetical reasons, I would prefer never to
> do I/O to a device unless user space asks for it.

I appreciate your caution -- and your aesthetic sensibilities!

Note that a certain amount of I/O goes on independent of userspace (to
determine the partitioning, if nothing else).  One can make a good case
that this "check the last sector" falls into the same category.

> (So - this would be much more work, but I would prefer a capacity value
> that says "it reported this, but we have not checked yet - try an actual
> I/O if you really want to know", and leave it at that until the value is
> needed. Typically one needs the value (i) to check against it if one
> wants to do I/O, or (ii) when user space asks for it because some fdisk type
> program is invoked. In case (i) an additional read is superfluous.
> In case (ii) user space asked and we try to make sure.)
> 
> Comments?

How would the kernel know when it needed to make sure?  By the time an 
actual I/O request from userspace arrives, it's probably too late -- 
the task has already looked at the capacity value and doesn't know that 
the capacity might change spontaneously.

As a more theoretical point, consider what would happen if the last 
partition included the "last" sector.  If the capacity got changed 
after the partitions were determined, we could run into trouble.  
Better to adjust the capacity right away, before a bogus value can get 
used.

My thought (expressed in an email some months ago, so it's not
surprising that people may have forgotten) was that this "check the
last sector" test would not be applied if we knew beforehand that it
was unnecessary or the device couldn't support it.  If either of the
CAPACITY_OK or FIX_CAPACITY flags is set then the last-sector check
won't be performed.

Thus, when we run across a device that dies during the last-sector
check, we'll have to add an unusual_devs entry for it.  But since
that's basically the same position we're in now, I don't see it as much
of a loss.

Alan Stern

Re: [usb-storage] [PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[Andries E. Brouwer]

On Wed, Apr 22, 2009 at 04:47:03PM -0400, Alan Stern wrote:
> On Wed, 22 Apr 2009, Andries E. Brouwer wrote:
> 
> > On Wed, Apr 22, 2009 at 10:42:36AM -0400, Alan Stern wrote:
> > > This patch (as1196) adds a new scsi_device flag to tell sd.c that it
> > > should verify the results of READ CAPACITY by trying the read the last
> > > sector.
> > 
> > Hi Alan,
> > 
> > I can see why you want to do this, but allow me to mutter a bit nevertheless.
> > Many devices are flaky and get into strange states requiring error-recovery
> > or reboot when you try an I/O that they do not like.
> > For this reason, and also for aesthetical reasons, I would prefer never to
> > do I/O to a device unless user space asks for it.
> 
> I appreciate your caution -- and your aesthetic sensibilities!
> 
> Note that a certain amount of I/O goes on independent of userspace (to
> determine the partitioning, if nothing else).  One can make a good case
> that this "check the last sector" falls into the same category.

Yes - I have often muttered about that as well. It has caused problems.
One should not try to read a partition table when confronted with a
random block device with random data. Only when user space asks.

There have been times when the random device was a magtape, and reading
the last tape block would take an hour or so. Timing is something to
worry about. When I started using computers boot time was much less
than 0.1 second, not noticeable. Today lots of nonsensical things happen
and together cause a minute delay, even though computers are ten thousand
times faster.
Also the error recovery of a SCSI device takes seconds, and only because
I removed code that did make it minutes (with many retries on all levels).

Police needs systems that do not tamper with their data.
If they take or copy a disk for forensic purposes the disk or copy
should remain pristine, precisely as it was found, and not one bit
should be changed. Especially time stamps can be important in
investigations. But if one mounts ext3 read-only, Linux will (or
perhaps would, I have not checked recently) still write to the disk
as a result of replaying the journal.

Some disks are used with a whole-disk filesystem, without partition table.

A good system does no I/O at all, unless there is a request.

> > (So - this would be much more work, but I would prefer a capacity value
> > that says "it reported this, but we have not checked yet - try an actual
> > I/O if you really want to know", and leave it at that until the value is
> > needed. Typically one needs the value (i) to check against it if one
> > wants to do I/O, or (ii) when user space asks for it because some fdisk type
> > program is invoked. In case (i) an additional read is superfluous.
> > In case (ii) user space asked and we try to make sure.)
> > 
> > Comments?
> 
> How would the kernel know when it needed to make sure?  By the time an 
> actual I/O request from userspace arrives, it's probably too late -- 
> the task has already looked at the capacity value and doesn't know that 
> the capacity might change spontaneously.

Consider the situation of the old days: a magtape. We used magtape
as a block device, but asking for the capacity was a very expensive
question - it would involve spooling to the end, and hoping that there
was a good tape mark. (Otherwise operator intervention would be needed.)
It is not difficult for software to cope with the situation of unknown
capacity. It is just a bit silly to write the corresponding code today.
There is not enough motivation.

So, I do not object to your code, I just hope that you have a little bit
of a bad conscience - the hope to do things right at some unspecified
moment in the future.

> As a more theoretical point, consider what would happen if the last 
> partition included the "last" sector.  If the capacity got changed 
> after the partitions were determined, we could run into trouble.  
> Better to adjust the capacity right away, before a bogus value can get 
> used.

I disagree. Indeed, consider the situation that some file includes
the last sector. Then probably that last sector really exists,
and probing is superfluous, a waste of time, possibly needless wear of
the device. Consider the situation that a partition includes the last
sector. Then the entity that was responsible for the partitioning
determined that this last sector is there, and we need not check again.

You see - really the only ordinary software that would be interested
is fdisk type software. It does a get_capacity ioctl, and this ioctl
should cause the kernel to really find out about the size.

(Yes, I know, there are a few other cases, e.g. for RAIDs.)

And, something I have seen several times: for forensic use one
sometimes copies the start of a device, just because one's own
disk is smaller than the suspect's disk. Linux should be able to
handle the situation of a partition table that describes a
partition that extends beyond the end of the disk. All should be
fine, except of course that actual I/O to nonexistent sectors causes
an I/O error.

Andries

Re: [usb-storage] [PATCH] SCSI: add check_capacity flag and sd_read_last_sector()

[Alan Stern]

On Thu, 23 Apr 2009, Andries E. Brouwer wrote:

> > Note that a certain amount of I/O goes on independent of userspace (to
> > determine the partitioning, if nothing else).  One can make a good case
> > that this "check the last sector" falls into the same category.
> 
> Yes - I have often muttered about that as well. It has caused problems.
> One should not try to read a partition table when confronted with a
> random block device with random data. Only when user space asks.
> 
> There have been times when the random device was a magtape, and reading
> the last tape block would take an hour or so. Timing is something to
> worry about. When I started using computers boot time was much less
> than 0.1 second, not noticeable. Today lots of nonsensical things happen
> and together cause a minute delay, even though computers are ten thousand
> times faster.
> Also the error recovery of a SCSI device takes seconds, and only because
> I removed code that did make it minutes (with many retries on all levels).
> 
> Police needs systems that do not tamper with their data.
> If they take or copy a disk for forensic purposes the disk or copy
> should remain pristine, precisely as it was found, and not one bit
> should be changed. Especially time stamps can be important in
> investigations. But if one mounts ext3 read-only, Linux will (or
> perhaps would, I have not checked recently) still write to the disk
> as a result of replaying the journal.
> 
> Some disks are used with a whole-disk filesystem, without partition table.
> 
> A good system does no I/O at all, unless there is a request.
> 
> > > (So - this would be much more work, but I would prefer a capacity value
> > > that says "it reported this, but we have not checked yet - try an actual
> > > I/O if you really want to know", and leave it at that until the value is
> > > needed. Typically one needs the value (i) to check against it if one
> > > wants to do I/O, or (ii) when user space asks for it because some fdisk type
> > > program is invoked. In case (i) an additional read is superfluous.
> > > In case (ii) user space asked and we try to make sure.)
> > > 
> > > Comments?
> > 
> > How would the kernel know when it needed to make sure?  By the time an 
> > actual I/O request from userspace arrives, it's probably too late -- 
> > the task has already looked at the capacity value and doesn't know that 
> > the capacity might change spontaneously.
> 
> Consider the situation of the old days: a magtape. We used magtape
> as a block device, but asking for the capacity was a very expensive
> question - it would involve spooling to the end, and hoping that there
> was a good tape mark. (Otherwise operator intervention would be needed.)
> It is not difficult for software to cope with the situation of unknown
> capacity. It is just a bit silly to write the corresponding code today.
> There is not enough motivation.
> 
> So, I do not object to your code, I just hope that you have a little bit
> of a bad conscience - the hope to do things right at some unspecified
> moment in the future.
> 
> > As a more theoretical point, consider what would happen if the last 
> > partition included the "last" sector.  If the capacity got changed 
> > after the partitions were determined, we could run into trouble.  
> > Better to adjust the capacity right away, before a bogus value can get 
> > used.
> 
> I disagree. Indeed, consider the situation that some file includes
> the last sector. Then probably that last sector really exists,
> and probing is superfluous, a waste of time, possibly needless wear of
> the device. Consider the situation that a partition includes the last
> sector. Then the entity that was responsible for the partitioning
> determined that this last sector is there, and we need not check again.
> 
> You see - really the only ordinary software that would be interested
> is fdisk type software. It does a get_capacity ioctl, and this ioctl
> should cause the kernel to really find out about the size.
> 
> (Yes, I know, there are a few other cases, e.g. for RAIDs.)
> 
> And, something I have seen several times: for forensic use one
> sometimes copies the start of a device, just because one's own
> disk is smaller than the suspect's disk. Linux should be able to
> handle the situation of a partition table that describes a
> partition that extends beyond the end of the disk. All should be
> fine, except of course that actual I/O to nonexistent sectors causes
> an I/O error.

I agree with quite a lot (though not all) of what you say.  However
there's no reason to argue/discuss it further; no doubt you are already
aware of all the important points on both sides of the issue.

I _do_ have a somewhat guilty conscience over introducing an extra 
unsolicited read (actually a pair of reads, since the patch reads 
sector 0 before trying to read the last sector).  In the end, the only 
justification is practicality -- the alternative was less workable.

Alan Stern