RE: [PATCH 2/4] iovmm: fix roundup for next area and end check forthe last area

[Guzman Lugo, Fernando]

> -----Original Message-----
> From: David Cohen [mailto:david.cohen@xxxxxxxxx] 
> Sent: Friday, October 01, 2010 5:57 AM
> To: Guzman Lugo, Fernando
> Cc: Doyu Hiroshi (Nokia-MS/Espoo); Contreras Felipe 
> (Nokia-MS/Helsinki); Palande Ameya (Nokia-MS/Helsinki); 
> linux-kernel@xxxxxxxxxxxxxxx; andy.shevchenko@xxxxxxxxx; 
> linux-omap@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/4] iovmm: fix roundup for next area and 
> end check for the last area
> 
> Hi,
> 
> On Fri, Oct 01, 2010 at 05:08:53AM +0200, ext Fernando Guzman 
> Lugo wrote:
> > As da_end does not belongs to the area the roundup should 
> be done to 
> > da_end and not to da_end + 1.
> > Also the end check for the last area should be ULONG_MAX - 
> start + 1 
> > >= bytes.
> > 
> > Signed-off-by: Fernando Guzman Lugo <x0095840@xxxxxx>
> > ---
> >  arch/arm/plat-omap/iovmm.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/plat-omap/iovmm.c 
> b/arch/arm/plat-omap/iovmm.c 
> > index 24ca9c4..fc6b109 100644
> > --- a/arch/arm/plat-omap/iovmm.c
> > +++ b/arch/arm/plat-omap/iovmm.c
> > @@ -289,19 +289,19 @@ static struct iovm_struct 
> *alloc_iovm_area(struct iommu *obj, u32 da,
> >  	prev_end = 0;
> >  	list_for_each_entry(tmp, &obj->mmap, list) {
> >  
> > -		if (prev_end >= start)
> > +		if (prev_end > start)
> >  			break;
> >  
> >  		if (start + bytes <= tmp->da_start)
> >  			goto found;
> >  
> >  		if (flags & IOVMF_DA_ANON)
> > -			start = roundup(tmp->da_end + 1, alignement);
> > +			start = roundup(tmp->da_end, alignement);
> 
> There's a lack of comment here, but the purpose of 
> tmp->da_end + 1 is to create a gap between iovm areas to 
> force to trigger iommu faults when some access exceeds a 
> valid area. Without this gap, such situation may produce data 
> corruption which is much more difficult to track.

That only works when you are accessing sequencially beyond the
End of the vm_area. However if you are accessing a random address
Which is in the mmu tables you still can corrupt memory which does
Not belong to you. That looks not very effective then why waste
Memory?

Maybe other mechanism should be implemente like in the process
Switching when if the process has DMM virtual memory area and if
So enablig only that area (all other process areas will be
Dissabled and it would get a mmufault in case of access). However
That increase the time of switching between process.

Regards,
Fernando.

> 
> Br,
> 
> David
> 
> >  
> >  		prev_end = tmp->da_end;
> >  	}
> >  
> > -	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > +	if ((start >= prev_end) && (ULONG_MAX - start + 1 >= bytes))
> >  		goto found;
> >  
> >  	dev_dbg(obj->dev, "%s: no space to fit %08x(%x) flags: %08x\n",
> > --
> > 1.6.3.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> > linux-kernel" in the body of a message to majordomo@xxxxxxxxxxxxxxx 
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/