Re: [uClinux-dev] [PATCH 5/7] NOMMU: Avoiding duplicate icache flushes of shared maps

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: Text/Plain, Size: 3957 bytes --]

On Monday 14 December 2009 19:41:43 Jamie Lokier wrote:
> David Howells wrote:
> > From: Mike Frysinger <vapier.adi@gmail.com>
> >
> > When working with FDPIC, there are many shared mappings of read-only code
> > regions between applications (the C library, applet packages like
> > busybox, etc.), but the current do_mmap_pgoff() function will issue an
> > icache flush whenever a VMA is added to an MM instead of only doing it
> > when the map is initially created.
> >
> > @@ -1354,10 +1355,14 @@ unsigned long do_mmap_pgoff(struct file *file,
> >  share:
> >  	add_vma_to_mm(current->mm, vma);
> >
> > -	up_write(&nommu_region_sem);
> > +	/* we flush the region from the icache only when the first executable
> > +	 * mapping of it is made  */
> > +	if (vma->vm_flags & VM_EXEC && !region->vm_icache_flushed) {
> > +		flush_icache_range(region->vm_start, region->vm_end);
> > +		region->vm_icache_flushed = true;
> > +	}
> >
> > -	if (prot & PROT_EXEC)
> > -		flush_icache_range(result, result + len);
> > +	up_write(&nommu_region_sem);
> >
> >  	kleave(" = %lx", result);
> >  	return result;
> 
> This looks like it won't work in the following sequence:
> 
>     process A maps MAP_SHARED, PROT_READ|PROT_EXEC (flushes icache)
>     process B maps MAP_SHARED, PROT_READ|PROT_WRITE
>         and proceeds to modify the data
>     process C maps MAP_SHARED, PROT_READ|PROT_EXEC (no icache flush)
> 
> On a possibly related note:
> 
> What about icache flushes in these cases:

David will have to respond here, but a test on my side shows that a mmap() 
request on an existing r-xs mapping does not grant write access.  you get back 
a r-xs mapping.

> When using mprotect() PROT_READ|PROT_WRITE -> PROT_READ|PROT_EXEC,
> e.g. as an FDPIC implementation may do when updating PLT entries.

when would that happen ?  PLT entries arent updated inline in the .text 
section (if they were, you'd have TEXTRELs and the .text wouldnt be shared).  
by definition, the function descriptor is stored in the GOT and that is what 
gets updated by the resolver during lazy relocation.

> And when calling msync(), like this:
> 
>     process A maps MAP_SHARED, PROT_READ|PROT_EXEC (flushes icache)
>     process B maps MAP_SHARED, PROT_READ|PROT_WRITE
>         and proceeds to modify the data
>     process A calls msync()
>         and proceeds to execute the modified contents
> 
> Do you think the mprotect() and msync() calls should flush icache in
> those cases?

from what i can tell, sys_mprotect() and sys_msync() do not currently exist in 
the nommu kernel port, and no one has complained so far ;).

uClibc has simply stubbed out these functions into inlines that always return 
success.  msync() is defined as pushing changes written to a file-backed 
mapping back to disk, and i dont think this gets used under nommu.

> If seen arguments for it, and arguments that the executing process can
> be expected to explicitly flush icache itself in those cases because
> it knows what it is doing.  (Personally I lean towards the kernel
> should be doing it.  IRIX interestingly offers both alternatives, with
> a PROT_EXEC_NOFLUSH).
> 
> But in the first example above, I don't see how process C could be
> expected to know it must flush icache, and process B could just be an
> "optimised with writable mmap" file copy, so it shouldn't have
> responsibility for icache either.

isnt this what the MS_INVALIDATE flag to msync() is for ?

> Or is icache fully flushed on every context switch on all nommu
> architectures anyway, and defined to do so?

ugh, no, this def does not occur, nor should it.  the overhead here would be 
crazy.  on a normal FDPIC boot, the only icache flush called are the initial 
creation of the shared .text maps in the C library and the applications.  and 
with a large busybox, you rarely see another one since the .text is shared.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]