[PATCH] advansys: fix overrun_buf aligned bug

[PATCH] advansys: fix overrun_buf aligned bug

[FUJITA Tomonori]

struct asc_dvc_var needs overrun buffer to be placed on an 8 byte
boundary. advansys defines struct asc_dvc_var:

struct asc_dvc_var {
	...
	uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);

The problem is that struct asc_dvc_var is placed on
shost->hostdata. So if the hostdata is not on an 8 byte boundary, the
advansys crashes. The hostdata is placed on a sizeof(unsigned long)
boundary so the 8 byte boundary is not garanteed with x86_32.

With 2.6.23 and 2.6.24, the hostdata is on an 8 byte boundary by
chance, but with the current git, it's not.

This patch removes overrun_buf static array and use kzalloc.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/advansys.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index ccef891..3c2d688 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -566,7 +566,7 @@ typedef struct asc_dvc_var {
 	ASC_SCSI_BIT_ID_TYPE unit_not_ready;
 	ASC_SCSI_BIT_ID_TYPE queue_full_or_busy;
 	ASC_SCSI_BIT_ID_TYPE start_motor;
-	uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);
+	uchar *overrun_buf;
 	dma_addr_t overrun_dma;
 	uchar scsi_reset_wait;
 	uchar chip_no;
@@ -13833,6 +13833,12 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 	 */
 	if (ASC_NARROW_BOARD(boardp)) {
 		ASC_DBG(2, "AscInitAsc1000Driver()\n");
+
+		asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
+		if (!asc_dvc_varp->overrun_buf) {
+			ret = -ENOMEM;
+			goto err_free_wide_mem;
+		}
 		warn_code = AscInitAsc1000Driver(asc_dvc_varp);
 
 		if (warn_code || asc_dvc_varp->err_code) {
@@ -13840,8 +13846,10 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 					"warn 0x%x, error 0x%x\n",
 					asc_dvc_varp->init_state, warn_code,
 					asc_dvc_varp->err_code);
-			if (asc_dvc_varp->err_code)
+			if (asc_dvc_varp->err_code) {
 				ret = -ENODEV;
+				kfree(asc_dvc_varp->overrun_buf);
+			}
 		}
 	} else {
 		if (advansys_wide_init_chip(shost))
@@ -13894,6 +13902,7 @@ static int advansys_release(struct Scsi_Host *shost)
 		dma_unmap_single(board->dev,
 					board->dvc_var.asc_dvc_var.overrun_dma,
 					ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+		kfree(board->dvc_var.asc_dvc_var.overrun_buf);
 	} else {
 		iounmap(board->ioremap_addr);
 		advansys_wide_free_mem(board);
-- 
1.5.3.7

Re: [PATCH] advansys: fix overrun_buf aligned bug

[James Bottomley]

On Fri, 2008-02-08 at 09:50 +0900, FUJITA Tomonori wrote:
> struct asc_dvc_var needs overrun buffer to be placed on an 8 byte
> boundary. advansys defines struct asc_dvc_var:
> 
> struct asc_dvc_var {
> 	...
> 	uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);
> 
> The problem is that struct asc_dvc_var is placed on
> shost->hostdata. So if the hostdata is not on an 8 byte boundary, the
> advansys crashes. The hostdata is placed on a sizeof(unsigned long)
> boundary so the 8 byte boundary is not garanteed with x86_32.
> 
> With 2.6.23 and 2.6.24, the hostdata is on an 8 byte boundary by
> chance, but with the current git, it's not.
> 
> This patch removes overrun_buf static array and use kzalloc.

It's a bit of a waste of a kmallocs.  The usual way of fixing this type
of cockup is to float the structure until it becomes aligned, but I
suppose that involves changing all calls to shost_priv in the driver ...

James

Re: [PATCH] advansys: fix overrun_buf aligned bug

[FUJITA Tomonori]

On Thu, 07 Feb 2008 19:01:55 -0600
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> 
> On Fri, 2008-02-08 at 09:50 +0900, FUJITA Tomonori wrote:
> > struct asc_dvc_var needs overrun buffer to be placed on an 8 byte
> > boundary. advansys defines struct asc_dvc_var:
> > 
> > struct asc_dvc_var {
> > 	...
> > 	uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);
> > 
> > The problem is that struct asc_dvc_var is placed on
> > shost->hostdata. So if the hostdata is not on an 8 byte boundary, the
> > advansys crashes. The hostdata is placed on a sizeof(unsigned long)
> > boundary so the 8 byte boundary is not garanteed with x86_32.
> > 
> > With 2.6.23 and 2.6.24, the hostdata is on an 8 byte boundary by
> > chance, but with the current git, it's not.
> > 
> > This patch removes overrun_buf static array and use kzalloc.
> 
> It's a bit of a waste of a kmallocs.  The usual way of fixing this type
> of cockup is to float the structure until it becomes aligned, but I
> suppose that involves changing all calls to shost_priv in the driver ...

Yeah, agreed. It's better but I'm not familiar with the driver so I
use kmalloc. It's not so bad as a short-term solution, I think.

Any chance to push it to final SCSI updates for 2.6.24 merge window?
Though we can push it any time since it's a bug fix.

Anyway, I'm fine with dropping it if Matthew will fix the driver in a
better way. I'm happy unless people blame my IOMMU or sense buffer
patch for this bug. :)

Re: [PATCH] advansys: fix overrun_buf aligned bug

[James Bottomley]

On Fri, 2008-02-08 at 10:16 +0900, FUJITA Tomonori wrote:
> On Thu, 07 Feb 2008 19:01:55 -0600
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > 
> > On Fri, 2008-02-08 at 09:50 +0900, FUJITA Tomonori wrote:
> > > struct asc_dvc_var needs overrun buffer to be placed on an 8 byte
> > > boundary. advansys defines struct asc_dvc_var:
> > > 
> > > struct asc_dvc_var {
> > > 	...
> > > 	uchar overrun_buf[ASC_OVERRUN_BSIZE] __aligned(8);
> > > 
> > > The problem is that struct asc_dvc_var is placed on
> > > shost->hostdata. So if the hostdata is not on an 8 byte boundary, the
> > > advansys crashes. The hostdata is placed on a sizeof(unsigned long)
> > > boundary so the 8 byte boundary is not garanteed with x86_32.
> > > 
> > > With 2.6.23 and 2.6.24, the hostdata is on an 8 byte boundary by
> > > chance, but with the current git, it's not.
> > > 
> > > This patch removes overrun_buf static array and use kzalloc.
> > 
> > It's a bit of a waste of a kmallocs.  The usual way of fixing this type
> > of cockup is to float the structure until it becomes aligned, but I
> > suppose that involves changing all calls to shost_priv in the driver ...
> 
> Yeah, agreed. It's better but I'm not familiar with the driver so I
> use kmalloc. It's not so bad as a short-term solution, I think.
> 
> Any chance to push it to final SCSI updates for 2.6.24 merge window?
> Though we can push it any time since it's a bug fix.
> 
> Anyway, I'm fine with dropping it if Matthew will fix the driver in a
> better way. I'm happy unless people blame my IOMMU or sense buffer
> patch for this bug. :)

Sure, will do ...  I think it's an OK interim fix.

James