[PATCH v2 6/6] posix/mmap: add mmap hooks for MAP_SHARED shms

Gedare Bloom gedare at rtems.org
Fri Apr 14 08:39:44 UTC 2017


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 <gedare at 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
>


More information about the devel mailing list