I forgot to add the mmap function calls in the operations table. There
is also a null pointer dereference that I have to track down.
On Thu, Apr 13, 2017 at 4:22 PM, Gedare Bloom <ged...@rtems.org> wrote:
> Updates #2859.
> ---
> cpukit/posix/include/rtems/posix/mmanimpl.h | 2 +-
> cpukit/posix/include/rtems/posix/shm.h | 30 ++++
> cpukit/posix/src/mmap.c | 208
> +++++++++++++++++-----------
> cpukit/posix/src/munmap.c | 17 ++-
> cpukit/posix/src/shmheap.c | 20 +++
> cpukit/posix/src/shmwkspace.c | 17 +++
> 6 files changed, 203 insertions(+), 91 deletions(-)
>
> diff --git a/cpukit/posix/include/rtems/posix/mmanimpl.h
> b/cpukit/posix/include/rtems/posix/mmanimpl.h
> index eff20e2..bb33ac9 100644
> --- a/cpukit/posix/include/rtems/posix/mmanimpl.h
> +++ b/cpukit/posix/include/rtems/posix/mmanimpl.h
> @@ -34,7 +34,7 @@ typedef struct mmap_mappings_s {
> size_t len; /**< The length of memory mapped */
> int flags; /**< The mapping flags */
> rtems_libio_t *iop; /**< The mapped object's file descriptor pointer */
> - bool is_shm; /**< True if mapping a shared memory object */
> + bool is_shared_shm; /**< True if MAP_SHARED of shared memory */
> } mmap_mapping;
>
> extern rtems_chain_control mmap_mappings;
> diff --git a/cpukit/posix/include/rtems/posix/shm.h
> b/cpukit/posix/include/rtems/posix/shm.h
> index dfdd58e..9284b39 100644
> --- a/cpukit/posix/include/rtems/posix/shm.h
> +++ b/cpukit/posix/include/rtems/posix/shm.h
> @@ -91,6 +91,16 @@ struct POSIX_Shm_Object_operations {
> * Returns the number of bytes read (copied) into @a buf.
> */
> int ( *object_read ) ( POSIX_Shm_Object *shm_obj, void *buf, size_t count
> );
> +
> + /**
> + * @brief Maps a shared memory object.
> + *
> + * Establishes a memory mapping between the shared memory object and the
> + * caller.
> + *
> + * Returns the mapped address of the object.
> + */
> + void * ( *object_mmap ) ( POSIX_Shm_Object *shm_obj, size_t len, int prot,
> off_t off);
> };
>
> /**
> @@ -144,6 +154,16 @@ extern int _POSIX_Shm_Object_read_from_workspace(
> );
>
> /**
> + * @brief object_mmap operation for shm objects stored in RTEMS Workspace.
> + */
> +extern void * _POSIX_Shm_Object_mmap_from_workspace(
> + POSIX_Shm_Object *shm_obj,
> + size_t len,
> + int prot,
> + off_t off
> +);
> +
> +/**
> * @brief object_create operation for shm objects stored in C program heap.
> */
> extern int _POSIX_Shm_Object_create_from_heap(
> @@ -173,6 +193,16 @@ extern int _POSIX_Shm_Object_read_from_heap(
> size_t count
> );
>
> +/**
> + * @brief object_mmap operation for shm objects stored in C program heap.
> + */
> +extern void * _POSIX_Shm_Object_mmap_from_heap(
> + POSIX_Shm_Object *shm_obj,
> + size_t len,
> + int prot,
> + off_t off
> +);
> +
> /** @} */
>
> #ifdef __cplusplus
> diff --git a/cpukit/posix/src/mmap.c b/cpukit/posix/src/mmap.c
> index b1d1653..74c8a33 100644
> --- a/cpukit/posix/src/mmap.c
> +++ b/cpukit/posix/src/mmap.c
> @@ -24,7 +24,7 @@
>
> #include "rtems/libio_.h"
>
> -#include <rtems/posix/mmanimpl.h>
> +#include <rtems/posix/mmanimpl.h>
> #include <rtems/posix/shmimpl.h>
>
> #define RTEMS_MUTEX_ATTRIBS \
> @@ -107,13 +107,28 @@ bool mmap_mappings_lock_release(
> return true;
> }
>
> -static void shm_mmap( rtems_libio_t *iop )
> +/* Helper function only gets called for mmap mappings of shared memory
> objects
> + * with the MAP_SHARED flag.
> + */
> +static void *shm_mmap( rtems_libio_t *iop, size_t len, int prot, off_t off)
> {
> POSIX_Shm_Control *shm = iop_to_shm( iop );
> + void *m;
>
> _Objects_Allocator_lock();
> - ++shm->reference_count;
> +
> + m = (*shm->shm_object.ops->object_mmap)( &shm->shm_object, len, prot, off);
> + if ( m != NULL ) {
> + /* Keep a reference in the shared memory to prevent its removal. */
> + ++shm->reference_count;
> +
> + /* Update atime */
> + _POSIX_Shm_Update_atime(shm);
> + }
> +
> _Objects_Allocator_unlock();
> +
> + return m;
> }
>
> void *mmap(
> @@ -124,12 +139,13 @@ void *mmap(
> mmap_mapping *mapping;
> ssize_t r;
> rtems_libio_t *iop;
> -
> - /*
> - * Clear errno.
> - */
> + bool map_fixed;
> +
> + map_fixed = (flags & MAP_FIXED) == MAP_FIXED;
> +
> + /* Clear errno. */
> errno = 0;
> -
> +
> /*
> * Get a stat of the file to get the dev + inode number and to make sure
> the
> * fd is ok. The normal libio calls cannot be used because we need to
> return
> @@ -141,8 +157,7 @@ void *mmap(
> return MAP_FAILED;
> }
>
> - /* Sucessful fstat ensures we have a good file descriptor. Get the
> - * associated iop for later. */
> + /* fstat ensures we have a good file descriptor. Hold on to iop. */
> iop = rtems_libio_iop( fildes );
>
> if ( len == 0 ) {
> @@ -150,14 +165,12 @@ void *mmap(
> return MAP_FAILED;
> }
>
> - /*
> - * Check the type of file we have and make sure it is supported.
> - */
> + /* Check the type of file we have and make sure it is supported. */
> if ( S_ISDIR( sb.st_mode ) || S_ISLNK( sb.st_mode )) {
> errno = ENODEV;
> return MAP_FAILED;
> }
> -
> +
> /*
> * We can provide read, write and execute because the memory in RTEMS does
> * not normally have protections but we cannot hide access to memory.
> @@ -166,7 +179,18 @@ void *mmap(
> errno = ENOTSUP;
> return MAP_FAILED;
> }
> -
> +
> + /* Either MAP_SHARED or MAP_PRIVATE must be defined, but not both */
> + if ( (flags & MAP_SHARED) == MAP_SHARED ) {
> + if ( (flags & MAP_PRIVATE) == MAP_PRIVATE ) {
> + errno = EINVAL;
> + return MAP_FAILED;
> + }
> + } else if ( (flags & MAP_PRIVATE != MAP_PRIVATE) ) {
> + errno = EINVAL;
> + return MAP_FAILED;
> + }
> +
> /*
> * We can not normally provide restriction of write access. Reject any
> * attempt to map without write permission, since we are not able to
> @@ -177,11 +201,9 @@ void *mmap(
> return MAP_FAILED;
> }
>
> - /*
> - * Check to see if the mapping is valid for a regular file.
> - */
> + /* Check to see if the mapping is valid for a regular file. */
> if ( S_ISREG( sb.st_mode )
> - /* FIXME: Should this be using strict inequality comparisons? It would
> + /* FIXME: Should this be using strict inequality (>) comparisons? It would
> * be valid to map a region exactly equal to the st_size, e.g. see below.
> */
> && (( off >= sb.st_size ) || (( off + len ) >= sb.st_size ))) {
> errno = EOVERFLOW;
> @@ -190,29 +212,92 @@ void *mmap(
>
> /*
> * Check to see if the mapping is valid for other file/object types.
> + * Does this satisfy for devices?
> */
> if ( sb.st_size < off + len ) {
> errno = ENXIO;
> return MAP_FAILED;
> }
>
> + /* Do not seek on character devices, pipes, sockets, or memory objects. */
> + if ( S_ISREG( sb.st_mode ) || S_ISBLK( sb.st_mode ) ) {
> + if ( lseek( fildes, off, SEEK_SET ) < 0 ) {
> + return MAP_FAILED;
> + }
> + }
> +
> + /* Create the mapping */
> + mapping = malloc( sizeof( mmap_mapping ));
> + if ( !mapping ) {
> + errno = ENOMEM;
> + return NULL;
> + }
> + memset( mapping, 0, sizeof( mmap_mapping ));
> + mapping->len = len;
> + mapping->flags = flags;
> + mapping->iop = iop;
> +
> + /*
> + * HACK: We should have a better generic way to distinguish between
> + * shm objects and other mmap'd files. We need to know at munmap time
> + * if the mapping is to a shared memory object in order to refcnt shms.
> + * We could do this by providing mmap in the file operations if needed.
> + */
> + if ( (MAP_PRIVATE == (flags & MAP_PRIVATE)) ||
> + S_ISREG( sb.st_mode ) || S_ISBLK( sb.st_mode ) ||
> + S_ISCHR( sb.st_mode ) || S_ISFIFO( sb.st_mode ) ||
> + S_ISSOCK( sb.st_mode ) ) {
> + mapping->is_shared_shm = false;
> + } else {
> + mapping->is_shared_shm = true;
> + }
> +
> /*
> - * Obtain the mmap lock. Sets errno on failure.
> + * MAP_SHARED currently is only supported for shared memory objects.
> */
> + if ( (MAP_SHARED == (flags & MAP_SHARED)) && (mapping->is_shared_shm ==
> false) ) {
> + free( mapping );
> + errno = ENOTSUP;
> + return MAP_FAILED;
> + }
> +
> + if ( map_fixed ) {
> + mapping->addr = addr;
> + } else if ( MAP_PRIVATE == (flags & MAP_PRIVATE) ) {
> + /* private mappings of shared memory do not need special treatment. */
> + mapping->is_shared_shm = false;
> + mapping->addr = malloc( len );
> + if ( !mapping->addr ) {
> + free( mapping );
> + errno = ENOMEM;
> + return MAP_FAILED;
> + }
> + }
> +
> + /* MAP_FIXED is not supported for shared memory objects with MAP_SHARED. */
> + if ( map_fixed && mapping->is_shared_shm ) {
> + free( mapping );
> + errno = ENOTSUP;
> + return MAP_FAILED;
> + }
> +
> + /* Lock access to mmap_mappings. Sets errno on failure. */
> if ( !mmap_mappings_lock_obtain( ) )
> return MAP_FAILED;
>
> - if (( flags & MAP_FIXED ) == MAP_FIXED ) {
> + if ( map_fixed ) {
> rtems_chain_node* node = rtems_chain_first (&mmap_mappings);
> while ( !rtems_chain_is_tail( &mmap_mappings, node )) {
> /*
> * If the map is fixed see if this address is already mapped. At this
> * point in time if there is an overlap in the mappings we return an
> - * error.
> + * error. POSIX allows us to also return successfully by unmapping
> + * the overlapping prior mappings.
> */
> mapping = (mmap_mapping*) node;
> if ( ( addr >= mapping->addr ) &&
> ( addr < ( mapping->addr + mapping->len )) ) {
> + free( mapping );
> mmap_mappings_lock_release( );
> errno = ENXIO;
> return MAP_FAILED;
> @@ -221,74 +306,29 @@ void *mmap(
> }
> }
>
> - mapping = malloc( sizeof( mmap_mapping ));
> - if ( !mapping ) {
> - mmap_mappings_lock_release( );
> - errno = ENOMEM;
> - return NULL;
> - }
> -
> - memset( mapping, 0, sizeof( mmap_mapping ));
> -
> - mapping->len = len;
> - mapping->flags = flags;
> - mapping->iop = iop;
> -
> - if (( flags & MAP_FIXED ) != MAP_FIXED ) {
> - mapping->addr = malloc( len );
> - if ( !mapping->addr ) {
> - mmap_mappings_lock_release( );
> - free( mapping );
> - errno = ENOMEM;
> - return MAP_FAILED;
> - }
> -
> + /* Populate the data */
> + if ( MAP_PRIVATE == (flags & MAP_PRIVATE) ) {
> /*
> - * Do not seek on character devices, pipes, sockets, or memory objects.
> + * Use read() for private mappings. This updates atime as needed.
> + * Changes to the underlying object will NOT be reflected in the mapping.
> + * The underlying object can be removed while the mapping exists.
> */
> - if ( S_ISREG( sb.st_mode ) || S_ISBLK( sb.st_mode ) ) {
> - if ( lseek( fildes, off, SEEK_SET ) < 0 ) {
> - mmap_mappings_lock_release( );
> + r = read( fildes, mapping->addr, len );
> +
> + if ( r != len ) {
> + mmap_mappings_lock_release( );
> + if ( !map_fixed ) {
> free( mapping->addr );
> - free( mapping );
> - return MAP_FAILED;
> }
> + free( mapping );
> + errno = ENXIO;
> + return MAP_FAILED;
> }
> + } else if ( MAP_SHARED == (flags & MAP_SHARED) ) {
> + /* Currently only shm objects can be MAP_SHARED. */
> + mapping->addr = shm_mmap(iop, len, prot, off);
> }
>
> - /* read updates atime satisfying POSIX spec for mmap */
> - r = read( fildes, mapping->addr, len );
> -
> - if ( r != len ) {
> - mmap_mappings_lock_release( );
> - free( mapping->addr );
> - free( mapping );
> - errno = ENXIO;
> - return MAP_FAILED;
> - }
> -
> - /* FIXME: should have a better generic way to handle differences between
> - * shm objects and other mmap'd files. the best way is probably to add an
> - * mmap/munmap handler to the file_operations table, or some other
> reasonable
> - * OOP-like approach to discriminate the 'type' of mapping. */
> - if ( S_ISREG( sb.st_mode ) || S_ISBLK( sb.st_mode ) ||
> - S_ISCHR( sb.st_mode ) || S_ISFIFO( sb.st_mode ) ||
> - S_ISSOCK( sb.st_mode ) ) {
> - mapping->is_shm = false;
> - } else {
> - mapping->is_shm = true;
> - shm_mmap(iop);
> - }
> -
> - /* FIXME:
> - * MAP_SHARED is not supported for regular files, and probably should be?
> - * MAP_PRIVATE is not supported for shared memory objects, and should be?
> - */
> -
> - /* FIXME: for a mapping with MAP_SHARED and PROT_WRITE the mtime/ctime
> - * should be updated when there is a write reference to the mapped region.
> - * How do we make this happen? */
> -
> /* add an extra reference to the file associated with fildes that
> * is not removed by a subsequent close(). This reference shall be removed
> * when there are no more mappings to the file. */
> @@ -297,6 +337,6 @@ void *mmap(
> rtems_chain_append( &mmap_mappings, &mapping->node );
>
> mmap_mappings_lock_release( );
> -
> +
> return mapping->addr;
> }
> diff --git a/cpukit/posix/src/munmap.c b/cpukit/posix/src/munmap.c
> index b315034..588562f 100644
> --- a/cpukit/posix/src/munmap.c
> +++ b/cpukit/posix/src/munmap.c
> @@ -18,12 +18,19 @@
> #include <rtems/posix/mmanimpl.h>
> #include <rtems/posix/shmimpl.h>
>
> +static void shm_munmap( rtems_libio_t *iop )
> +{
> + POSIX_Shm_Control *shm = iop_to_shm( iop );
> +
> + /* decrement mmap's shm reference_count and maybe delete the object */
> + POSIX_Shm_Attempt_delete(shm);
> +}
> +
> int munmap(void *addr, size_t len)
> {
> mmap_mapping *mapping;
> rtems_chain_node *node;
> uint32_t refcnt;
> - POSIX_Shm_Control *shm;
>
> /*
> * Clear errno.
> @@ -51,11 +58,9 @@ int munmap(void *addr, size_t len)
> ( addr < ( mapping->addr + mapping->len )) ) {
> rtems_chain_extract( node );
> /* FIXME: generally need a way to clean-up the backing object, but
> - * currently it only matters for shm objects. */
> - if ( mapping->is_shm == true ) {
> - /* decrement mmap's shm reference_count and maybe delete the object
> */
> - shm = iop_to_shm(mapping->iop);
> - POSIX_Shm_Attempt_delete(shm);
> + * currently it only matters for MAP_SHARED shm objects. */
> + if ( mapping->is_shared_shm == true ) {
> + shm_munmap(mapping->iop);
> }
> refcnt = rtems_libio_decrement_mapping_refcnt(mapping->iop);
> if ( refcnt == 0 ) {
> diff --git a/cpukit/posix/src/shmheap.c b/cpukit/posix/src/shmheap.c
> index 3449c15..47f5b47 100644
> --- a/cpukit/posix/src/shmheap.c
> +++ b/cpukit/posix/src/shmheap.c
> @@ -70,6 +70,7 @@ int _POSIX_Shm_Object_resize_from_heap(
> return err;
> }
>
> +/* This is identical to _POSIX_Shm_Object_read_from_wkspace */
> int _POSIX_Shm_Object_read_from_heap(
> POSIX_Shm_Object *shm_obj,
> void *buf,
> @@ -88,3 +89,22 @@ int _POSIX_Shm_Object_read_from_heap(
> return count;
> }
>
> +/* This is identical to _POSIX_Shm_Object_mmap_from_wkspace */
> +void * _POSIX_Shm_Object_mmap_from_heap(
> + POSIX_Shm_Object *shm_obj,
> + size_t len,
> + int prot,
> + off_t off
> +)
> +{
> + if ( shm_obj == NULL || shm_obj->handle == NULL )
> + return 0;
> +
> + /* This is already checked by mmap. Maybe make it a debug assert? */
> + if ( shm_obj->size < len + off ) {
> + return NULL;
> + }
> +
> + return &(shm_obj->handle[off]);
> +}
> +
> diff --git a/cpukit/posix/src/shmwkspace.c b/cpukit/posix/src/shmwkspace.c
> index 6d6cd21..926c14b 100644
> --- a/cpukit/posix/src/shmwkspace.c
> +++ b/cpukit/posix/src/shmwkspace.c
> @@ -75,4 +75,21 @@ int _POSIX_Shm_Object_read_from_workspace(
> return count;
> }
>
> +void * _POSIX_Shm_Object_mmap_from_wkspace(
> + POSIX_Shm_Object *shm_obj,
> + size_t len,
> + int prot,
> + off_t off
> +)
> +{
> + if ( shm_obj == NULL || shm_obj->handle == NULL )
> + return 0;
> +
> + /* This is already checked by mmap. Maybe make it a debug assert? */
> + if ( shm_obj->size < len + off ) {
> + return NULL;
> + }
> +
> + return &(shm_obj->handle[off]);
> +}
>
> --
> 2.7.4
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
