Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[James Bottomley]

On Mon, 2008-06-02 at 15:57 +0900, FUJITA Tomonori wrote:
> Some REQ_TYPE_ATA_PC commands uses the stack buffers for DMA, which
> leads to memory corruption on a non-coherent platform.
> 
> With regard to alignment and padding, ide-cd has the the dma safe
> check for sg requests and REQ_TYPE_ATA_PC. This adds the stack buffer
> check to that check.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Borislav Petkov <petkovbb@gmail.com>
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/ide-cd.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index e3f085c..e12d602 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1195,6 +1195,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
>  		struct request_queue *q = drive->queue;
>  		unsigned int alignment;
>  		unsigned long addr;
> +		unsigned long stack_mask = ~(THREAD_SIZE - 1);
>  
>  		if (rq->bio)
>  			addr = (unsigned long)bio_data(rq->bio);
> @@ -1212,6 +1213,10 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
>  		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
>  		if (addr & alignment || rq->data_len & alignment)
>  			info->dma = 0;
> +
> +		if (!((addr & stack_mask) ^
> +		      ((unsigned long)current->stack & stack_mask)))

That can basically become

if ((addr & stack_mask)  == ((unsigned long)current->stack & stack_mask))

to be a bit clearer, can't it?

I'm also not keen on the use of current->stack.  It looks like this
commit:

commit f7e4217b007d1f73e7e3cf10ba4fea4a608c603f
Author: Roman Zippel <zippel@linux-m68k.org>
Date:   Wed May 9 02:35:17 2007 -0700

    rename thread_info to stack

Introduced a task_stack_page() accessor to get this instead, so perhaps
we should use it (I've cc'd Roman and linux-arch for opinions).

> +			info->dma = 0;
>  	}
>  
>  	/* start sending the command to the drive */

James

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[Borislav Petkov]

On Thu, Jun 05, 2008 at 11:10:48AM -0500, James Bottomley wrote:
> On Mon, 2008-06-02 at 15:57 +0900, FUJITA Tomonori wrote:
> > Some REQ_TYPE_ATA_PC commands uses the stack buffers for DMA, which
> > leads to memory corruption on a non-coherent platform.
> > 
> > With regard to alignment and padding, ide-cd has the the dma safe
> > check for sg requests and REQ_TYPE_ATA_PC. This adds the stack buffer
> > check to that check.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Borislav Petkov <petkovbb@gmail.com>
> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> >  drivers/ide/ide-cd.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> > index e3f085c..e12d602 100644
> > --- a/drivers/ide/ide-cd.c
> > +++ b/drivers/ide/ide-cd.c
> > @@ -1195,6 +1195,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
> >  		struct request_queue *q = drive->queue;
> >  		unsigned int alignment;
> >  		unsigned long addr;
> > +		unsigned long stack_mask = ~(THREAD_SIZE - 1);
> >  
> >  		if (rq->bio)
> >  			addr = (unsigned long)bio_data(rq->bio);
> > @@ -1212,6 +1213,10 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
> >  		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
> >  		if (addr & alignment || rq->data_len & alignment)
> >  			info->dma = 0;
> > +
> > +		if (!((addr & stack_mask) ^
> > +		      ((unsigned long)current->stack & stack_mask)))
> 
> That can basically become
> 
> if ((addr & stack_mask)  == ((unsigned long)current->stack & stack_mask))
> 
> to be a bit clearer, can't it?

yep, yep. Clearer it is :).
> 
> I'm also not keen on the use of current->stack.  It looks like this
> commit:
> 
> commit f7e4217b007d1f73e7e3cf10ba4fea4a608c603f
> Author: Roman Zippel <zippel@linux-m68k.org>
> Date:   Wed May 9 02:35:17 2007 -0700
> 
>     rename thread_info to stack
> 
> Introduced a task_stack_page() accessor to get this instead, so perhaps
> we should use it (I've cc'd Roman and linux-arch for opinions).
> 
> > +			info->dma = 0;
> >  	}
> >  
> >  	/* start sending the command to the drive */
> 
> James
> 

-- 
Regards/Gruß,
    Boris.

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[FUJITA Tomonori]

On Thu, 05 Jun 2008 11:10:48 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Mon, 2008-06-02 at 15:57 +0900, FUJITA Tomonori wrote:
> > Some REQ_TYPE_ATA_PC commands uses the stack buffers for DMA, which
> > leads to memory corruption on a non-coherent platform.
> > 
> > With regard to alignment and padding, ide-cd has the the dma safe
> > check for sg requests and REQ_TYPE_ATA_PC. This adds the stack buffer
> > check to that check.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Borislav Petkov <petkovbb@gmail.com>
> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> >  drivers/ide/ide-cd.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> > index e3f085c..e12d602 100644
> > --- a/drivers/ide/ide-cd.c
> > +++ b/drivers/ide/ide-cd.c
> > @@ -1195,6 +1195,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
> >  		struct request_queue *q = drive->queue;
> >  		unsigned int alignment;
> >  		unsigned long addr;
> > +		unsigned long stack_mask = ~(THREAD_SIZE - 1);
> >  
> >  		if (rq->bio)
> >  			addr = (unsigned long)bio_data(rq->bio);
> > @@ -1212,6 +1213,10 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
> >  		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
> >  		if (addr & alignment || rq->data_len & alignment)
> >  			info->dma = 0;
> > +
> > +		if (!((addr & stack_mask) ^
> > +		      ((unsigned long)current->stack & stack_mask)))
> 
> That can basically become
> 
> if ((addr & stack_mask)  == ((unsigned long)current->stack & stack_mask))
> 
> to be a bit clearer, can't it?

Ok, I'll do next time.


> I'm also not keen on the use of current->stack.  It looks like this
> commit:
> 
> commit f7e4217b007d1f73e7e3cf10ba4fea4a608c603f
> Author: Roman Zippel <zippel@linux-m68k.org>
> Date:   Wed May 9 02:35:17 2007 -0700
> 
>     rename thread_info to stack
> 
> Introduced a task_stack_page() accessor to get this instead, so perhaps
> we should use it (I've cc'd Roman and linux-arch for opinions).

Thanks. Seems that we should use it here (though current->stack look
like work now).

lib/debugobjects.c has the same code (that uses current->stack) to
detect an address on the stack.

Next time, I'll add a helper function to detect an address on the
stack, which is useful for debugobjects.c, blk-map.c and ide-cd.c.

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[Roman Zippel]

Hi,

On Thu, 5 Jun 2008, James Bottomley wrote:

> I'm also not keen on the use of current->stack.  It looks like this
> commit:
> 
> commit f7e4217b007d1f73e7e3cf10ba4fea4a608c603f
> Author: Roman Zippel <zippel@linux-m68k.org>
> Date:   Wed May 9 02:35:17 2007 -0700
> 
>     rename thread_info to stack
> 
> Introduced a task_stack_page() accessor to get this instead, so perhaps
> we should use it (I've cc'd Roman and linux-arch for opinions).

This helper was mainly introduced to help with the transition from direct 
thread_info access. I don't see this field go away or change somehow in 
the near future, so direct access of stack is IMO ok, but we already have 
the helper, so using it to improve readability is fine too.

bye, Roman