linux-kernel - Re: [PATCH] Fix virt_to

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090702104027.GB14804@linux-mips.org>
Date:	Thu, 2 Jul 2009 11:40:27 +0100
From:	Ralf Baechle <ralf@...ux-mips.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Kevin Cernekee <cernekee@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fix virt_to_phys() warnings

On Wed, Jul 01, 2009 at 10:46:27PM -0700, Andrew Morton wrote:

> > mm/page_alloc.c: In function 'alloc_pages_exact':
> > mm/page_alloc.c:1986: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
> > 
> > drivers/usb/mon/mon_bin.c: In function 'mon_alloc_buff':
> > drivers/usb/mon/mon_bin.c:1264: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
> > 
> > Signed-off-by: Kevin Cernekee <cernekee@...il.com>
> > ---
> > diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> > index f8d9045..0f7a30b 100644
> > --- a/drivers/usb/mon/mon_bin.c
> > +++ b/drivers/usb/mon/mon_bin.c
> > @@ -1261,7 +1261,7 @@ static int mon_alloc_buff(struct mon_pgmap *map, int npages)
> >  			return -ENOMEM;
> >  		}
> >  		map[n].ptr = (unsigned char *) vaddr;
> > -		map[n].pg = virt_to_page(vaddr);
> > +		map[n].pg = virt_to_page((void *) vaddr);
> >  	}
> >  	return 0;
> >  }
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 5d714f8..f6180db 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1983,7 +1983,7 @@ void *alloc_pages_exact(size_t size, gfp_t gfp_mask)
> >  		unsigned long alloc_end = addr + (PAGE_SIZE << order);
> >  		unsigned long used = addr + PAGE_ALIGN(size);
> >  
> > -		split_page(virt_to_page(addr), order);
> > +		split_page(virt_to_page((void *)addr), order);
> >  		while (used < alloc_end) {
> >  			free_page(used);
> >  			used += PAGE_SIZE;
> 
> The virt_to_foo() functions were written back in the Linux dark ages and
> they're horrid.  A byzantine mixture of macros and typecasts which make
> it really hard to work out what type their argument actually should be.

No disagreement there.

> virt_to_page() takes an argument of type `unsigned long'.  (except for
> the include/asm-generic/page.h version which takes any damn type at
> all).

You meant it takes a pointer argument, for example:

mm/page_alloc.c:
	__free_pages(virt_to_page((void *)addr), order);
mm/slob.c:
static inline struct slob_page *slob_page(const void *addr)
{
        return (struct slob_page *)virt_to_page(addr);
}


> virt_to_phys() takes an argument of `const void *' (weird, huh?)

volatile void * seems to be the most common type.  MIPS is the only arch
that accepts volatile const void *; Alpha uses a plain void *.

I don't recall why MIPS uses volatile const - probably to silence warnings
about discarding qualifiers from pointer target type.

> But arch/mips/include/asm/page.h does
> 
> #define virt_to_page(kaddr)        pfn_to_page(PFN_DOWN(virt_to_phys(kaddr)))
> 
> thereby passing an `unsigned long' where a void* was expected.
> 
> 
> So perhaps something along these lines:
> 
> 
> --- a/arch/mips/include/asm/page.h~a
> +++ a/arch/mips/include/asm/page.h
> @@ -184,7 +184,11 @@ typedef struct { unsigned long pgprot; }
>  
>  #endif
>  
> -#define virt_to_page(kaddr)	pfn_to_page(PFN_DOWN(virt_to_phys(kaddr)))
> +static inline struct page *virt_to_page(unsigned long kaddr)
> +{
> +	return pfn_to_page(PFN_DOWN(virt_to_phys((void *)kaddr)));
> +}
> +

Doesn't compile because pfn_to_page is defined in <asm-generic/memory_model.h>
which will only be included further down and moving the order of the order
of things around to get this to build is getting ugly, so I gave up on it.

Anyway, since virt_to_page and virt_to_phys (imho :-) take the same
argument times I'm in favor of Kevin's patch.

  Ralf
--
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/