[PATCH rc8-mm1] hotfix libata-scsi corruption

[PATCH rc8-mm1] hotfix libata-scsi corruption

[Hugh Dickins]

2.6.24-rc8-mm1 is corrupting.  smartd does some sg_ioctl into its stack,
and depending on how its stack randomization worked out, this is liable
to end up writing into the adjacent physical page too.  If you're lucky
you have highmem, and ioread16_rep oopses on the virtual address beyond
what ata_pio_sector's kmap_atomic set up; when you're unlucky, it'll go
ahead and corrupt the next page more obscurely.

Bisect led to scsi-misc-2.6.git's 465ff3185e0cb76d46137335a4d21d0d9d3ac8a2
[SCSI] relax scsi dma alignment, and the patch below is good for a hot-fix.
Though I doubt it'll be the final fix, since it appears to undo the whole
point of blk_queue_update_dma_alignment: perhaps libata is at fault to be
going through sectory code for unsectory I/O - I wouldn't know.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 drivers/ata/libata-scsi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 2.6.24-rc8-mm1/drivers/ata/libata-scsi.c	2008-01-17 16:49:47.000000000 +0000
+++ linux/drivers/ata/libata-scsi.c	2008-01-22 15:45:40.000000000 +0000
@@ -826,7 +826,7 @@ static void ata_scsi_sdev_config(struct 
 	sdev->max_device_blocked = 1;
 
 	/* set the min alignment */
-	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
+	blk_queue_update_dma_alignment(sdev->request_queue, ATA_SECT_SIZE - 1);
 }
 
 static void ata_scsi_dev_config(struct scsi_device *sdev,

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[James Bottomley]

Added linux-scsi to the cc.

On Tue, 2008-01-22 at 17:11 +0000, Hugh Dickins wrote:
> 2.6.24-rc8-mm1 is corrupting.  smartd does some sg_ioctl into its stack,
> and depending on how its stack randomization worked out, this is liable
> to end up writing into the adjacent physical page too.  If you're lucky
> you have highmem, and ioread16_rep oopses on the virtual address beyond
> what ata_pio_sector's kmap_atomic set up; when you're unlucky, it'll go
> ahead and corrupt the next page more obscurely.
> 
> Bisect led to scsi-misc-2.6.git's 465ff3185e0cb76d46137335a4d21d0d9d3ac8a2
> [SCSI] relax scsi dma alignment, and the patch below is good for a hot-fix.
> Though I doubt it'll be the final fix, since it appears to undo the whole
> point of blk_queue_update_dma_alignment: perhaps libata is at fault to be
> going through sectory code for unsectory I/O - I wouldn't know.
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> ---
> 
>  drivers/ata/libata-scsi.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- 2.6.24-rc8-mm1/drivers/ata/libata-scsi.c	2008-01-17 16:49:47.000000000 +0000
> +++ linux/drivers/ata/libata-scsi.c	2008-01-22 15:45:40.000000000 +0000
> @@ -826,7 +826,7 @@ static void ata_scsi_sdev_config(struct 
>  	sdev->max_device_blocked = 1;
>  
>  	/* set the min alignment */
> -	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
> +	blk_queue_update_dma_alignment(sdev->request_queue, ATA_SECT_SIZE - 1);
>  }
>  
>  static void ata_scsi_dev_config(struct scsi_device *sdev,

Unfortunately, that's likely not the entire hot fix ... the implication
is that we have some mapping error in the way we do direct SG_IO.

What the fix you propose does is make it far more likely that block will
copy, perform I/O then uncopy (almost certain, since most smartd data
transfers are well under ATA_SECT_SIZE, which is 512).  However,
implicating a generic path like this implies that we would get the same
problem for SCSI commands as well, so the correct hot fix is below.

However, I'd like to see if we can track the problem through the SG_IO
direct path ... how many adjacent page bytes are corrupt?  Just a few or
a large number (I'm wondering if it's an off by one or off by alignment
type bug)?

James

---
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 4cf902e..13376e9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1673,8 +1673,11 @@ struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
 	 * set a reasonable default alignment on word boundaries: the
 	 * host and device may alter it using
 	 * blk_queue_update_dma_alignment() later.
+	 *
+	 * FIXME: Corruption showing up in SG_IO leads this to be
+	 * raised to 511 again.
 	 */
-	blk_queue_dma_alignment(q, 0x03);
+	blk_queue_dma_alignment(q, 511);
 
 	return q;
 }

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[Hugh Dickins]

On Tue, 22 Jan 2008, James Bottomley wrote:
> > --- 2.6.24-rc8-mm1/drivers/ata/libata-scsi.c	2008-01-17 16:49:47.000000000 +0000
> > +++ linux/drivers/ata/libata-scsi.c	2008-01-22 15:45:40.000000000 +0000
> > @@ -826,7 +826,7 @@ static void ata_scsi_sdev_config(struct 
> >  	sdev->max_device_blocked = 1;
> >  
> >  	/* set the min alignment */
> > -	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
> > +	blk_queue_update_dma_alignment(sdev->request_queue, ATA_SECT_SIZE - 1);
> >  }
> >  
> >  static void ata_scsi_dev_config(struct scsi_device *sdev,
> 
> Unfortunately, that's likely not the entire hot fix ... the implication
> is that we have some mapping error in the way we do direct SG_IO.

Quite possibly, I'm not sure.

> What the fix you propose does is make it far more likely that block will
> copy, perform I/O then uncopy (almost certain, since most smartd data
> transfers are well under ATA_SECT_SIZE, which is 512).  However,
> implicating a generic path like this implies that we would get the same
> problem for SCSI commands as well, so the correct hot fix is below.

I've not noticed any problems from the normal activity of the system,
only from smartd's sg_ioctl.  My impression was that it's a libata
issue, because it's going through ata_pio_sector, which does 

	ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);

referring to sect_size, without considering the possibility of any smaller
I/O size.  (Me, I don't even know why it's going PIO rather than DMA:
I'm assuming smartd does things that way, but there's no limit to my
ignorance here.)

> However, I'd like to see if we can track the problem through the SG_IO
> direct path ... how many adjacent page bytes are corrupt?  Just a few or
> a large number (I'm wondering if it's an off by one or off by alignment
> type bug)?

I've assumed it's just the one next page: because ata_pio_sector is
doing a data_xfer of sect_size ATA_SECT_SIZE 512 to an offset above
0xe00 in the smartd stack page.  The time I actually saw corruption
rather than an oops at startup, it was in a tmpfs swap vector page
running 64-bit kernel, and I didn't examine any further pages (just
checked the page before and matched it up to smartd's stack, already
suspecting that).

I don't believe it's an off-by-one at your SCSI end.

Hugh

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[James Bottomley]

On Tue, 2008-01-22 at 18:36 +0000, Hugh Dickins wrote:
> On Tue, 22 Jan 2008, James Bottomley wrote:
> > > --- 2.6.24-rc8-mm1/drivers/ata/libata-scsi.c	2008-01-17 16:49:47.000000000 +0000
> > > +++ linux/drivers/ata/libata-scsi.c	2008-01-22 15:45:40.000000000 +0000
> > > @@ -826,7 +826,7 @@ static void ata_scsi_sdev_config(struct 
> > >  	sdev->max_device_blocked = 1;
> > >  
> > >  	/* set the min alignment */
> > > -	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
> > > +	blk_queue_update_dma_alignment(sdev->request_queue, ATA_SECT_SIZE - 1);
> > >  }
> > >  
> > >  static void ata_scsi_dev_config(struct scsi_device *sdev,
> > 
> > Unfortunately, that's likely not the entire hot fix ... the implication
> > is that we have some mapping error in the way we do direct SG_IO.
> 
> Quite possibly, I'm not sure.
> 
> > What the fix you propose does is make it far more likely that block will
> > copy, perform I/O then uncopy (almost certain, since most smartd data
> > transfers are well under ATA_SECT_SIZE, which is 512).  However,
> > implicating a generic path like this implies that we would get the same
> > problem for SCSI commands as well, so the correct hot fix is below.
> 
> I've not noticed any problems from the normal activity of the system,
> only from smartd's sg_ioctl.  My impression was that it's a libata
> issue, because it's going through ata_pio_sector, which does 
> 
> 	ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);
> 
> referring to sect_size, without considering the possibility of any smaller
> I/O size.  (Me, I don't even know why it's going PIO rather than DMA:
> I'm assuming smartd does things that way, but there's no limit to my
> ignorance here.)

Actually, I don't think it's a smaller I/O issue.  The SMART protocol
specifically mandates that the transfers for SMART READ DATA and SMART
READ LOG shall be 512 bytes).  However, the pio transfer routine does
seem to be assuming sector alignment as well, which will be where your
problems are coming from.  I think we need to specify sector minimum
alignment for ata (but not atapi, which has its own non sector size pio
routine).  How about the attached?

We have to do this for all ATA devices, because they'll likely all
support SMART, and SMART is defined to be a PIO command.

James

---

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4bb268b..bc5cf6b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -824,9 +824,6 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
 	 * requests.
 	 */
 	sdev->max_device_blocked = 1;
-
-	/* set the min alignment */
-	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
 }
 
 static void ata_scsi_dev_config(struct scsi_device *sdev,
@@ -842,7 +839,14 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
 	if (dev->class == ATA_DEV_ATAPI) {
 		struct request_queue *q = sdev->request_queue;
 		blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
-	}
+
+		/* set the min alignment */
+		blk_queue_update_dma_alignment(sdev->request_queue,
+					       ATA_DMA_PAD_SZ - 1);
+	} else
+		/* ATA devices must be sector aligned */
+		blk_queue_update_dma_alignment(sdev->request_queue,
+					       ATA_SECT_SIZE - 1);
 
 	if (dev->flags & ATA_DFLAG_AN)
 		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);


> > However, I'd like to see if we can track the problem through the SG_IO
> > direct path ... how many adjacent page bytes are corrupt?  Just a few or
> > a large number (I'm wondering if it's an off by one or off by alignment
> > type bug)?
> 
> I've assumed it's just the one next page: because ata_pio_sector is
> doing a data_xfer of sect_size ATA_SECT_SIZE 512 to an offset above
> 0xe00 in the smartd stack page.  The time I actually saw corruption
> rather than an oops at startup, it was in a tmpfs swap vector page
> running 64-bit kernel, and I didn't examine any further pages (just
> checked the page before and matched it up to smartd's stack, already
> suspecting that).
> 
> I don't believe it's an off-by-one at your SCSI end.
> 
> Hugh
> -
> 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 rc8-mm1] hotfix libata-scsi corruption

[Hugh Dickins]

On Tue, 22 Jan 2008, James Bottomley wrote:
> 
> Actually, I don't think it's a smaller I/O issue.  The SMART protocol
> specifically mandates that the transfers for SMART READ DATA and SMART
> READ LOG shall be 512 bytes).  However, the pio transfer routine does
> seem to be assuming sector alignment as well, which will be where your
> problems are coming from.  I think we need to specify sector minimum
> alignment for ata (but not atapi, which has its own non sector size pio
> routine).  How about the attached?
> 
> We have to do this for all ATA devices, because they'll likely all
> support SMART, and SMART is defined to be a PIO command.

Thanks, you've answered several of my uncertainties (why the PIO,
why the sector size).

I've just tried it, and can confirm that your replacement patch
below fixes the issue for me in practice.

What I can't say, you and Jeff and others will judge, is whether that's
actually the right placement for the blk_queue_update_dma_alignment call
(as an outsider, I'm not convinced that the SMART requirement should be
imposing this limitation on the rest).

Hugh

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 4bb268b..bc5cf6b 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -824,9 +824,6 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
>  	 * requests.
>  	 */
>  	sdev->max_device_blocked = 1;
> -
> -	/* set the min alignment */
> -	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
>  }
>  
>  static void ata_scsi_dev_config(struct scsi_device *sdev,
> @@ -842,7 +839,14 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
>  	if (dev->class == ATA_DEV_ATAPI) {
>  		struct request_queue *q = sdev->request_queue;
>  		blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
> -	}
> +
> +		/* set the min alignment */
> +		blk_queue_update_dma_alignment(sdev->request_queue,
> +					       ATA_DMA_PAD_SZ - 1);
> +	} else
> +		/* ATA devices must be sector aligned */
> +		blk_queue_update_dma_alignment(sdev->request_queue,
> +					       ATA_SECT_SIZE - 1);
>  
>  	if (dev->flags & ATA_DFLAG_AN)
>  		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[Jeff Garzik]

James Bottomley wrote:
> Actually, I don't think it's a smaller I/O issue.  The SMART protocol
> specifically mandates that the transfers for SMART READ DATA and SMART
> READ LOG shall be 512 bytes).  However, the pio transfer routine does
> seem to be assuming sector alignment as well, which will be where your
> problems are coming from.  I think we need to specify sector minimum
> alignment for ata (but not atapi, which has its own non sector size pio
> routine).  How about the attached?
> 
> We have to do this for all ATA devices, because they'll likely all
> support SMART, and SMART is defined to be a PIO command.
> 
> James
> 
> ---
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 4bb268b..bc5cf6b 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -824,9 +824,6 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
>  	 * requests.
>  	 */
>  	sdev->max_device_blocked = 1;
> -
> -	/* set the min alignment */
> -	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
>  }
>  
>  static void ata_scsi_dev_config(struct scsi_device *sdev,
> @@ -842,7 +839,14 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
>  	if (dev->class == ATA_DEV_ATAPI) {
>  		struct request_queue *q = sdev->request_queue;
>  		blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
> -	}
> +
> +		/* set the min alignment */
> +		blk_queue_update_dma_alignment(sdev->request_queue,
> +					       ATA_DMA_PAD_SZ - 1);
> +	} else
> +		/* ATA devices must be sector aligned */
> +		blk_queue_update_dma_alignment(sdev->request_queue,
> +					       ATA_SECT_SIZE - 1);
>  
>  	if (dev->flags & ATA_DFLAG_AN)
>  		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);


ACK

Unlike ATAPI, ATA is indeed all 512-byte alignment transfers (_not_ 
sector size, which may or may not be 512 bytes)

Does this apply to libata?  libata + jejb dma alignment patch?

What tree...

	Jeff

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[James Bottomley]

On Tue, 2008-01-22 at 20:20 +0000, Hugh Dickins wrote:
> On Tue, 22 Jan 2008, James Bottomley wrote:
> > 
> > Actually, I don't think it's a smaller I/O issue.  The SMART protocol
> > specifically mandates that the transfers for SMART READ DATA and SMART
> > READ LOG shall be 512 bytes).  However, the pio transfer routine does
> > seem to be assuming sector alignment as well, which will be where your
> > problems are coming from.  I think we need to specify sector minimum
> > alignment for ata (but not atapi, which has its own non sector size pio
> > routine).  How about the attached?
> > 
> > We have to do this for all ATA devices, because they'll likely all
> > support SMART, and SMART is defined to be a PIO command.
> 
> Thanks, you've answered several of my uncertainties (why the PIO,
> why the sector size).
> 
> I've just tried it, and can confirm that your replacement patch
> below fixes the issue for me in practice.

Thanks!

> What I can't say, you and Jeff and others will judge, is whether that's
> actually the right placement for the blk_queue_update_dma_alignment call
> (as an outsider, I'm not convinced that the SMART requirement should be
> imposing this limitation on the rest).

It's certainly the correct placement.  The slight problem is that the
actual alignment checking is only really done for commands coming down
from the user, not for commands incoming from the kernel.  That leaves
us a potential nasty in IDENTIFY_DEVICE; that's also a PIO only 512 byte
transfer command.

libsas looks to be OK because it specifically kmallocs a 512 byte buffer
which should (for off slab data) be 512 byte aligned.  libata actually
has an issue because the usual location for IDENTIFY_DEVICE data is
inside a struct ata_device, which is highly unlikely to be correctly
aligned.  Fortunately, I think we can only get the bug if we actually
cross a page boundary for non contiguous pages in the identify data,
which a kernel allocation will never do, so libata should be safe as
well.

James

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[Alan Cox]

> However, I'd like to see if we can track the problem through the SG_IO
> direct path ... how many adjacent page bytes are corrupt?  Just a few or
> a large number (I'm wondering if it's an off by one or off by alignment
> type bug)?

Which ATA controller is involved - in theory ATA DMA is byte aligned safe
(or dword anyway) in practice I don't know if we've ever tested the non
512 byte aligned case historically for ATA just ATAPI ?

Alan

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[Hugh Dickins]

On Tue, 22 Jan 2008, Alan Cox wrote:
> > However, I'd like to see if we can track the problem through the SG_IO
> > direct path ... how many adjacent page bytes are corrupt?  Just a few or
> > a large number (I'm wondering if it's an off by one or off by alignment
> > type bug)?

We moved away from that concern ....

> Which ATA controller is involved - in theory ATA DMA is byte aligned safe
> (or dword anyway) in practice I don't know if we've ever tested the non
> 512 byte aligned case historically for ATA just ATAPI ?

.... but if it's still relevant, this was with ata_piix.

Hugh

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[Hugh Dickins]

On Tue, 22 Jan 2008, James Bottomley wrote:
> 
> libsas looks to be OK because it specifically kmallocs a 512 byte buffer
> which should (for off slab data) be 512 byte aligned.

I don't remember the various SLAB and SLOB and SLUB rules offhand:
I'm not sure it's safe to rely on such alignment on all of them ....

> libata actually
> has an issue because the usual location for IDENTIFY_DEVICE data is
> inside a struct ata_device, which is highly unlikely to be correctly
> aligned.  Fortunately, I think we can only get the bug if we actually
> cross a page boundary for non contiguous pages in the identify data,
> which a kernel allocation will never do, so libata should be safe as
> well.

.... but this would trump it: yes, we don't need 512-byte alignment
for this, and it is okay to cross a page boundary, just so long as
the start of the next page really belongs to our buffer not somebody
else's.

There doesn't seem much likelihood of anyone vmalloc'ing the buffer
in which that IDENTIFY_DEVICE gets done.  Though this discussion
does make me wonder whether ata_pio_sector ought to have a BUG_ON
(and yes, a BUG_ON rather than a WARN_ON) against the possibility.

Hugh

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[James Bottomley]

On Tue, 2008-01-22 at 15:32 -0500, Jeff Garzik wrote:
> James Bottomley wrote:
> > Actually, I don't think it's a smaller I/O issue.  The SMART protocol
> > specifically mandates that the transfers for SMART READ DATA and SMART
> > READ LOG shall be 512 bytes).  However, the pio transfer routine does
> > seem to be assuming sector alignment as well, which will be where your
> > problems are coming from.  I think we need to specify sector minimum
> > alignment for ata (but not atapi, which has its own non sector size pio
> > routine).  How about the attached?
> > 
> > We have to do this for all ATA devices, because they'll likely all
> > support SMART, and SMART is defined to be a PIO command.
> > 
> > James
> > 
> > ---
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 4bb268b..bc5cf6b 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -824,9 +824,6 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
> >  	 * requests.
> >  	 */
> >  	sdev->max_device_blocked = 1;
> > -
> > -	/* set the min alignment */
> > -	blk_queue_update_dma_alignment(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
> >  }
> >  
> >  static void ata_scsi_dev_config(struct scsi_device *sdev,
> > @@ -842,7 +839,14 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
> >  	if (dev->class == ATA_DEV_ATAPI) {
> >  		struct request_queue *q = sdev->request_queue;
> >  		blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
> > -	}
> > +
> > +		/* set the min alignment */
> > +		blk_queue_update_dma_alignment(sdev->request_queue,
> > +					       ATA_DMA_PAD_SZ - 1);
> > +	} else
> > +		/* ATA devices must be sector aligned */
> > +		blk_queue_update_dma_alignment(sdev->request_queue,
> > +					       ATA_SECT_SIZE - 1);
> >  
> >  	if (dev->flags & ATA_DFLAG_AN)
> >  		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);
> 
> 
> ACK
> 
> Unlike ATAPI, ATA is indeed all 512-byte alignment transfers (_not_ 
> sector size, which may or may not be 512 bytes)
> 
> Does this apply to libata?  libata + jejb dma alignment patch?
> 
> What tree...

It's scsi-misc-2.6; the blk_queue_update_dma_alignment() API doesn't
exist in mainline (yet).

James

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[James Bottomley]

On Tue, 2008-01-22 at 22:59 +0000, Hugh Dickins wrote:
> On Tue, 22 Jan 2008, James Bottomley wrote:
> > 
> > libsas looks to be OK because it specifically kmallocs a 512 byte buffer
> > which should (for off slab data) be 512 byte aligned.
> 
> I don't remember the various SLAB and SLOB and SLUB rules offhand:
> I'm not sure it's safe to rely on such alignment on all of them ....
> 
> > libata actually
> > has an issue because the usual location for IDENTIFY_DEVICE data is
> > inside a struct ata_device, which is highly unlikely to be correctly
> > aligned.  Fortunately, I think we can only get the bug if we actually
> > cross a page boundary for non contiguous pages in the identify data,
> > which a kernel allocation will never do, so libata should be safe as
> > well.
> 
> .... but this would trump it: yes, we don't need 512-byte alignment
> for this, and it is okay to cross a page boundary, just so long as
> the start of the next page really belongs to our buffer not somebody
> else's.
> 
> There doesn't seem much likelihood of anyone vmalloc'ing the buffer
> in which that IDENTIFY_DEVICE gets done.  Though this discussion
> does make me wonder whether ata_pio_sector ought to have a BUG_ON
> (and yes, a BUG_ON rather than a WARN_ON) against the possibility.

Oh ... never say never.  One of the things the shift we did in SCSI to
the block submission api and elimination of the single map path actually
enables you to send kernel vmalloc areas straight into the storage APIs
(which was previously absolutely forbidden) ... fortunately no-one has
yet.

However, I think you're right we could do with a check in the PIO path
to make sure each sector is physically contiguous ... unfortunately, an
easy alignment check would trip on the IDENTIFY and other kernel
commands, sigh.

James

Re: [PATCH rc8-mm1] hotfix libata-scsi corruption

[Matt Mackall]

On Tue, 2008-01-22 at 22:59 +0000, Hugh Dickins wrote:
> On Tue, 22 Jan 2008, James Bottomley wrote:
> > 
> > libsas looks to be OK because it specifically kmallocs a 512 byte buffer
> > which should (for off slab data) be 512 byte aligned.
> 
> I don't remember the various SLAB and SLOB and SLUB rules offhand:
> I'm not sure it's safe to rely on such alignment on all of them ....

It doesn't work that way with SLOB kmalloc (nor did it in pre-slabified
kmalloc). One shouldn't be surprised if a SLAB/SLUB debugging feature
breaks that alignment either.

-- 
Mathematics is the supreme nostalgia of our time.