[PATCH] make devmem map only a single page at end of memory

Michael Abbott michael at araneidae.co.uk
Tue Apr 13 13:30:41 UTC 2010


On Tue, 13 Apr 2010, Michael Abbott wrote:
> On Tue, 13 Apr 2010, Jörgen Pihlflyckt wrote:
> > I noticed that the devmem applet maps two consecutive pages of memory in 
> > order to allow access to a (non-aligned) object that spans two pages. 
> > This is a problem if we are working with the last page of memory because 
> > the next page would wrap around to the first page of memory.
> > 
> > This patch makes devmem map only a single page if it is at the end of 
> > the memory address space.
> 
> Forgive me (and this patch isn't tested), but if you're going to do this, 
> let's do it right:
> 
> diff --git a/miscutils/devmem.c b/miscutils/devmem.c
> index e13dedc..225a9ed 100644
> --- a/miscutils/devmem.c
> +++ b/miscutils/devmem.c
> @@ -14,6 +14,8 @@ int devmem_main(int argc UNUSED_PARAM, char **argv)
>  	uint64_t writeval = writeval; /* for compiler */
>  	off_t target;
>  	unsigned page_size = getpagesize();
> +	off_t page_mask = ~(off_t)(page_size - 1);
> +	size_t map_length;
>  	int fd;
>  	int width = 8 * sizeof(int);
>  
> @@ -57,18 +59,19 @@ int devmem_main(int argc UNUSED_PARAM, char **argv)
>  		bb_show_usage(); /* bb_strtouXX failed */
>  
>  	fd = xopen("/dev/mem", argv[3] ? (O_RDWR | O_SYNC) : (O_RDONLY | O_SYNC));
> +	map_length = ((target + width - 1) & page_mask) - (target & page_mask) + page_size;
>  	map_base = mmap(NULL,
> -			page_size * 2 /* in case value spans page */,
> +			map_length,
>  			argv[3] ? (PROT_READ | PROT_WRITE) : PROT_READ,
>  			MAP_SHARED,
>  			fd,
> -			target & ~(off_t)(page_size - 1));
> +			target & page_mask);
>  	if (map_base == MAP_FAILED)
>  		bb_perror_msg_and_die("mmap");
>  
>  //	printf("Memory mapped at address %p.\n", map_base);
>  
> -	virt_addr = (char*)map_base + (target & (page_size - 1));
> +	virt_addr = (char*)map_base + (target & page_mask);

Ouch.  Please leave this part of the patch out, leave the original line 
alone!

>  
>  	if (!argv[3]) {
>  		switch (width) {
> @@ -119,7 +123,7 @@ int devmem_main(int argc UNUSED_PARAM, char **argv)
>  	}
>  
>  	if (ENABLE_FEATURE_CLEAN_UP) {
> -		if (munmap(map_base, page_size * 2) == -1)
> +		if (munmap(map_base, map_length) == -1)
>  			bb_perror_msg_and_die("munmap");
>  		close(fd);
>  	}
> 
> Two points: you might as well compute the page span properly, if you're 
> going to touch this code, and of course you do need to unmap what you've 
> just mapped!
> 
> *Think* I've got the calculation right, but as I said, completely 
> untested, sorry.

Um.


More information about the busybox mailing list