build problems on architectures where FIXADDR_* stuff is not constant

build problems on architectures where FIXADDR_* stuff is not constant

[Oleg Drokin]

Hello!

   Since there are architectures where FIXADDR_* stuff is not constant (e.g. UML),
   I propose this patch that allows such architectures to build. (now compilation
   dies with complaints about using not constant value as struct initialiser).
   Here is my proposed patch, or may be there is a better way?

   This is against latest 2.5 bk tree.

Bye,
    Oleg

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1094  -> 1.1095 
#	         mm/memory.c	1.123   -> 1.124  
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/05/13	green@angband.namesys.com	1.1095
# memory.c:
#   Well, not everyone have these FIXADDR_* as constants. UML have those as computed value.
#   So we need to assign those not in struct initializer.
# --------------------------------------------
#
diff -Nru a/mm/memory.c b/mm/memory.c
--- a/mm/memory.c	Tue May 13 16:18:28 2003
+++ b/mm/memory.c	Tue May 13 16:18:28 2003
@@ -696,15 +696,15 @@
 				   ones, we can make this be "&init_mm" or
 				   something.  */
 				.vm_mm = NULL,
-				.vm_start = FIXADDR_START,
-				.vm_end = FIXADDR_TOP,
-				.vm_page_prot = PAGE_READONLY,
 				.vm_flags = VM_READ | VM_EXEC,
 			};
 			unsigned long pg = start & PAGE_MASK;
 			pgd_t *pgd;
 			pmd_t *pmd;
 			pte_t *pte;
+			fixmap_vma.vm_start = FIXADDR_START;
+			fixmap_vma.vm_end = FIXADDR_TOP;
+			fixmap_vma.vm_page_prot = PAGE_READONLY;
 			pgd = pgd_offset_k(pg);
 			if (!pgd)
 				return i ? : -EFAULT;

Re: build problems on architectures where FIXADDR_* stuff is not constant

[Andrew Morton]

Oleg Drokin <green@namesys.com> wrote:
>
>    Since there are architectures where FIXADDR_* stuff is not constant (e.g. UML),
> ...
> +			fixmap_vma.vm_start = FIXADDR_START;
> +			fixmap_vma.vm_end = FIXADDR_TOP;
> +			fixmap_vma.vm_page_prot = PAGE_READONLY;
>  			pgd = pgd_offset_k(pg);
>  			if (!pgd)
>  				return i ? : -EFAULT;

That's modifying static storage which other, unrelated processes or CPUs
may be playing with.

The new code in get_user_pages() is rather rude - it's returning a
statically allocated VMA which isn't in the VMA tree - the caller (who
holds mmap_sem()) could reasonably expect that the VMA can be located via
find_vma(), or removed from the tree or whatever.  But it cannot.

I think it needs to be redone.  Either by stuffing a VMA into every
process's mm which describes the fixmap area, or by failing
get_user_pages() if the caller has passed in a non-NULL `vmas' and is
requesting access to the fixmap area.

Probably the latter.  That'll require that access_process_vm() be changed
to not require a vma.  It's only using the vma for cache flushing, but the
flishing in there is borked anyway.  

Re: build problems on architectures where FIXADDR_* stuff is not constant

[Andrew Morton]

Andrew Morton <akpm@digeo.com> wrote:
>
> The new code in get_user_pages() is rather rude - it's returning a
> statically allocated VMA which isn't in the VMA tree - the caller (who
> holds mmap_sem()) could reasonably expect that the VMA can be located via
> find_vma(), or removed from the tree or whatever.  But it cannot.
> 
> I think it needs to be redone.  Either by stuffing a VMA into every
> process's mm which describes the fixmap area, or by failing
> get_user_pages() if the caller has passed in a non-NULL `vmas' and is
> requesting access to the fixmap area.

Or by lazily instantiating the fixmap VMA within get_user_pages().  So if
someone happens to want to access the fixmap, that's when the vma which
describes it gets stuffed into the tree.

That'd require that get_user_pages() be called under down_write(mmap_sem).

Re: build problems on architectures where FIXADDR_* stuff is not constant

[Oleg Drokin]

Hello!

On Tue, May 13, 2003 at 01:46:20PM -0700, Andrew Morton wrote:
> >    Since there are architectures where FIXADDR_* stuff is not constant (e.g. UML),
> > ...
> > +			fixmap_vma.vm_start = FIXADDR_START;
> > +			fixmap_vma.vm_end = FIXADDR_TOP;
> > +			fixmap_vma.vm_page_prot = PAGE_READONLY;
> >  			pgd = pgd_offset_k(pg);
> >  			if (!pgd)
> >  				return i ? : -EFAULT;
> That's modifying static storage which other, unrelated processes or CPUs
> may be playing with.

Ah, stupid me. Missed the "static" thing :(

Bye,
    Oleg

Re: build problems on architectures where FIXADDR_* stuff is not constant

[Ingo Oeser]

Hi Andrew,

I finally got around to handle this issue.

On Tue, May 13, 2003 at 06:12:27PM -0700, Andrew Morton wrote:
> Ingo Oeser <ingo.oeser@informatik.tu-chemnitz.de> wrote:
> >
> > What, if you fold a part of my page_walker code (the special case
> >  for one page actually) into the baseline?
> > 
> >  This will resolve the issue and should remove the vma argument
> >  from get_user_pages().
> > 
> >  It will also speed up that path a little.
> > 
> >  What do you think?
> 
> I'd be interested in seeing a diff.  Especially if it
> digs that crap out of get_user_pages().

Yes, but the change would be quite big:
   - follow_hugetlb_pages() is not needed anymore, since
     follow_page() can handle them and every user of
     get_user_pages() needing to get a vma back is just needing
     exactly one page.

   Required users of a possible get_single_user_page():  
      fs/binfmt_elf.c:elf_core_dump()
      kernel/ptrace.c:access_process_vm()

   Recommended users (because they need only one page):
      kernel/futex.c:__pin_page()
      arch/i386/lib/usercopy.c:_copy_to_user_ll()
 
   - get_user_pages() will loose its last argument. 

     I could also remove the force argument then, since
     drivers/media/video/video-buf.c is the only user and it
     looks bogus to me wrt. this usage.

   - the FIXADDR handling would have its own inline function,
     degenerating into a assignement, if not needed and might be
     duplicated partially, if inlined.

   - the follow_page and faulting would become a own function.

   - all callers of both functions must be updated and it will
     affect the pgcl patches.

So the amount of changes is quite similiar to the inclusion of
the whole new page-walking-api.

If you want pieces, please tell me, which of the changes above
you want.

Regards

Ingo Oeser
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>