ivshmem: allow the sharing of hugepages

According to shm_open specifications:

 A shared memory object should be identified by a name of the form /somename;
 that is, a null-terminated string of up to NAME_MAX (i.e., 255) characters
 consisting of an initial slash, followed by one or more characters, none of
 which are slashes.

This patch permits to share memory areas that do not specifically belong to
/dev/shmem.

A use case for this patch is sharing huge pages available through a
hugetlbfs mountpoint.

Signed-off-by: Damien Millescamps <damien.millescamps@6wind.com>
---
 hw/misc/ivshmem.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

12.09.2013 22:23, Damien Millescamps wrote:
> According to shm_open specifications:
>
>   A shared memory object should be identified by a name of the form /somename;
>   that is, a null-terminated string of up to NAME_MAX (i.e., 255) characters
>   consisting of an initial slash, followed by one or more characters, none of
>   which are slashes.
>
> This patch permits to share memory areas that do not specifically belong to
> /dev/shmem.
>
> A use case for this patch is sharing huge pages available through a
> hugetlbfs mountpoint.
>
> Signed-off-by: Damien Millescamps <damien.millescamps@6wind.com>
> ---
>   hw/misc/ivshmem.c |   16 +++++++++++++++-
>   1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 2838866..9020bb2 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -751,9 +751,23 @@ static int pci_ivshmem_init(PCIDevice *dev)
>
>           IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
>
> +        /*
> +         * A shared memory object should be identified by a name of the form /somename;
> +         * that is, a null-terminated string of up to NAME_MAX (i.e., 255) characters
> +         * consisting of an initial slash, followed by one or more characters, none of
> +         * which are slashes.

It'd be nice to give a hint about where this definition comes from.

And once you look at shm_open(3), you'll notice that this paragraph
is prefixed with "For portable use", and reads:

  For portable use, a shared memory object should be identified by a name of the
  form  /somename; that is, a null-terminated string of up to NAME_MAX (i.e., 255)
  characters consisting of an initial slash, followed by one or more characters,
  none of which are slashes.

That to say, this is not a _definition_ of a shared memory object, it is just
a suggested name syntax, suggested purely for portability.  In other words,
there may be other acceptable syntaxes for it.

So as the result, I'm not sure this approach is valid.  Maybe we should
always try shared first and create-new second?  I dunno.

Note that whole thing - using shared memory object like this - may lead
to surprizes at least, -- users who previously expected one behavour now
see different behavour.  Most likely the old behavour wasn't correct.

At least this should be documented somewhere in user-visible part of
ivshmem, so users will have an ides when objects will be shared and
when truncated.

> +         */
> +        if (s->shmobj && s->shmobj[0] == '/' && strstr(&s->shmobj[1], "/")) {

Here you're testing for s->shmobj, but before, the code were referencing
it directly as an argument for shm_open().  Can it be NULL in this place?

It is a somewhat minor nitpick, but it'd be not nice to spread such tests
(for NULLness) where the object can't be NULL and to confuse readers.

> +            /* This can't be a shared memory object. */
> +            fd = open(s->shmobj, O_RDWR);
> +            if (fd < 0) {
> +                perror("ivshmem - open");
> +                exit(-1);
> +            }
> +        }
>           /* try opening with O_EXCL and if it succeeds zero the memory
>            * by truncating to 0 */
> -        if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL,
> +        else if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL,
>                           S_IRWXU|S_IRWXG|S_IRWXO)) > 0) {
>              /* truncate file to length PCI device's memory */
>               if (ftruncate(fd, s->ivshmem_size) != 0) {
>

Thanks,

/mjt

On 09/14/2013 11:36 AM, Michael Tokarev wrote:
> That to say, this is not a _definition_ of a shared memory object, it
> is just
> a suggested name syntax, suggested purely for portability.  In other
> words,
> there may be other acceptable syntaxes for it.

You are right, the definition can be found in shm_open(P), and reads:
 
  If name does not begin with the slash character, the effect is
implementation-defined.
  The interpretation of slash characters other  than  the leading slash
character in name
  is implementation-defined.

The "implementation-defined" found in glibc is as follow:

The leading '/', if present, is removed. and '/dev/shm/' is prepended to
the resulting name before calling open().
(found in sysdeps/posix/shm_open.c around line 57 depending on the version).

Note that a workaround in my case is to give a name in the form:
/../../path/to/file" ...

> So as the result, I'm not sure this approach is valid.  Maybe we should
> always try shared first and create-new second?  I dunno.

This is probably a better approach, yes. cf next paragraph.

> Note that whole thing - using shared memory object like this - may lead
> to surprizes at least, -- users who previously expected one behavour now
> see different behavour.  Most likely the old behavour wasn't correct.

By first trying to open with shm_open, and only when it fails with open,
the behavior should stay the same. Because according to glibc
implementation, if "folder" exists in /dev/shm, a name like
"/folder/shared_mem" should work, but will trigger a false positive with
the checks I added.
Note that I am not sure anyone uses this syntax, but still, this is a
false positive. I'll change that.

> At least this should be documented somewhere in user-visible part of
> ivshmem, so users will have an ides when objects will be shared and
> when truncated.

I will add a paragraph in docs/specs/ivshmem_device_spec.txt for that.
Thanks for mentioning it.

> It is a somewhat minor nitpick, but it'd be not nice to spread such tests
> (for NULLness) where the object can't be NULL and to confuse readers.

agreed.

> Thanks,

Thanks for your review, that was helpful. I'll send a reworked patch,
probably on Monday.