linux-kernel - Re: [PATCH] SPARC64: fix iommu sg chaining

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20071018080110O.fujita.tomonori@lab.ntt.co.jp>
Date:	Thu, 18 Oct 2007 08:01:10 +0900
From:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
To:	jens.axboe@...cle.com
Cc:	davem@...emloft.net, linux-kernel@...r.kernel.org,
	linux-scsi@...r.kernel.org, tomof@....org
Subject: Re: [PATCH] SPARC64: fix iommu sg chaining

On Wed, 17 Oct 2007 20:37:58 +0900
FUJITA Tomonori <fujita.tomonori@....ntt.co.jp> wrote:

> On Wed, 17 Oct 2007 13:01:42 +0200
> Jens Axboe <jens.axboe@...cle.com> wrote:
> 
> > On Wed, Oct 17 2007, Jens Axboe wrote:
> > > On Wed, Oct 17 2007, David Miller wrote:
> > > > From: Jens Axboe <jens.axboe@...cle.com>
> > > > Date: Wed, 17 Oct 2007 11:16:29 +0200
> > > > 
> > > > > On Wed, Oct 17 2007, David Miller wrote:
> > > > > > From: Jens Axboe <jens.axboe@...cle.com>
> > > > > > Date: Wed, 17 Oct 2007 10:45:28 +0200
> > > > > > 
> > > > > > > Righto, it's invalid to call sg_next() on the last entry!
> > > > > > 
> > > > > > Unfortunately, that's what the sparc64 code wanted to do, this
> > > > > > transformation in the sparc64 sg chaining patch is not equilavent:
> > > > > > 
> > > > > > -	struct scatterlist *sg_end = sg + nelems;
> > > > > > +	struct scatterlist *sg_end = sg_last(sg, nelems);
> > > > > >  ...
> > > > > > -			while (sg < sg_end &&
> > > > > > +			while (sg != sg_end &&
> > > > > 
> > > > > Auch indeed. That'd probably be better as a
> > > > > 
> > > > >         do {
> > > > >                 ...
> > > > >         } while (sg != sg_end);
> > > > 
> > > > Ok, next bug, introduced by this change:
> > > > 
> > > > commit f565913ef8a8d0cfa46a1faaf8340cc357a46f3a
> > > > Author: Jens Axboe <jens.axboe@...cle.com>
> > > > Date:   Fri Sep 21 10:44:19 2007 +0200
> > > > 
> > > >     block: convert to using sg helpers
> > > >     
> > > >     Convert the main rq mapper (blk_rq_map_sg()) to the sg helper setup.
> > > >     
> > > >     Signed-off-by: Jens Axboe <jens.axboe@...cle.com>
> > > > 
> > > > Specifically this part:
> > > > 
> > > >  new_segment:
> > > > -			memset(&sg[nsegs],0,sizeof(struct scatterlist));
> > > > -			sg[nsegs].page = bvec->bv_page;
> > > > -			sg[nsegs].length = nbytes;
> > > > -			sg[nsegs].offset = bvec->bv_offset;
> > > > +			sg = next_sg;
> > > > +			next_sg = sg_next(sg);
> > > >  
> > > > +			sg->page = bvec->bv_page;
> > > > +			sg->length = nbytes;
> > > > +			sg->offset = bvec->bv_offset;
> > > > 
> > > > You can't remove that memset(), it's there for a reason.  The IOMMU
> > > > layers depended upon the code zero'ing out the whole scatterlist
> > > > struct, there might be more to it than page, length and offset :-)
> > > 
> > > I realize that, and I was pretty worried about this specific change. But
> > > there's only been one piece of fallout because if it until now - well
> > > two, with the sparc64 stuff.
> > > 
> > > The problem is that you cannot zero the entire sg entry, because then
> > > you'd potentially overwrite the chain pointer.
> > > 
> > > I'd propose just adding a
> > > 
> > >         sg_dma_address(sg) = 0;
> > >         sg_dma_len(sg) = 0;
> > > 
> > > there for now, or provide an arch_clear_sg_entry() helper if we need
> > > more killed.
> > 
> > Actually, just clearing AFTER sg_next() would be fine, since we know
> > that is not a link entry. Duh...
> > 
> > diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> > index 9eabac9..1014d34 100644
> > --- a/block/ll_rw_blk.c
> > +++ b/block/ll_rw_blk.c
> > @@ -1352,6 +1352,7 @@ new_segment:
> >  			sg = next_sg;
> >  			next_sg = sg_next(sg);
> >  
> > +			memset(sg, 0, sizeof(*sg));
> >  			sg->page = bvec->bv_page;
> >  			sg->length = nbytes;
> >  			sg->offset = bvec->bv_offset;
> > 
> > -- 
> 
> So now how about removing zero'ing out sglist in scsi-ml?

Really sorry, I must have been stoned.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/