Make kern.ipc.shm_allow_removed default to 1.

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Make kern.ipc.shm_allow_removed default to 1.

Edward Napierala
I'd like to commit - well, discuss it first - the following change:

https://reviews.freebsd.org/D3603

This changes the default setting of kern.ipc.shm_allow_removed from 0 to 1.
This removes the need of manually changing this flag for Google Chrome
users. It also improves compatibility with Linux applications running under
Linuxulator compatibility layer, and possibly also helps in porting software
from Linux.

Generally speaking, the flag allows applications to create the shared memory
segment, attach it, remove it, and then continue to use it and to reattach it
later. This means that the kernel will automatically "clean up" after the
application exits.

It could be argued that it's against POSIX. However, SUSv3 says this
about IPC_RMID: "Remove the shared memory identifier specified by shmid from
the system and destroy the shared memory segment and shmid_ds data structure
associated with it." From my reading, we break it in any case by deferring
removal of the segment until it's detached; we won't break it any more
by also deferring removal of the identifier.

This is the behaviour exhibited by Linux since... probably always, and
also by OpenBSD since the following commit:

revision 1.54
date: 2011/10/27 07:56:28; author: robert; state: Exp; lines: +3 -8;
Allow segments to be used even after they were marked for deletion with
the IPC_RMID flag.
This is permitted as an extension beyond the standards and this is similar
to what other operating systems like linux do.

Arguments - on both sides - welcome.  Thanks!

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Make kern.ipc.shm_allow_removed default to 1.

John Baldwin
On Tuesday, September 08, 2015 09:13:10 PM Edward Tomasz NapieraƂa wrote:

> I'd like to commit - well, discuss it first - the following change:
>
> https://reviews.freebsd.org/D3603
>
> This changes the default setting of kern.ipc.shm_allow_removed from 0 to 1.
> This removes the need of manually changing this flag for Google Chrome
> users. It also improves compatibility with Linux applications running under
> Linuxulator compatibility layer, and possibly also helps in porting software
> from Linux.
>
> Generally speaking, the flag allows applications to create the shared memory
> segment, attach it, remove it, and then continue to use it and to reattach it
> later. This means that the kernel will automatically "clean up" after the
> application exits.
>
> It could be argued that it's against POSIX. However, SUSv3 says this
> about IPC_RMID: "Remove the shared memory identifier specified by shmid from
> the system and destroy the shared memory segment and shmid_ds data structure
> associated with it." From my reading, we break it in any case by deferring
> removal of the segment until it's detached; we won't break it any more
> by also deferring removal of the identifier.

It is against POSIX.  When I last raised this with kib@ and alc@, Alan noted
that our current behavior is not really against POSIX as he felt there was
enough wriggle room to permit existing mappings to remain valid.

That said, in the thread kib@ felt it was ok to change the default.  I think
it's probably fine.  It's a hack for the fact that shmget() isn't returning
a real fd the way shm_open() does, so in that sense it is ok.  That is,
in shm_open() you can do this:

        fd = shm_open("/some/path");
        shm_unlink("/some/path");
       
        mmap(..., fd);

        /* later */

        mmap(..., fd);

        /* or pass fd over a UNIX domain socket and map it in another process */

        close(fd);

        /* now mmap won't work */

With FreeBSD's SHM_ANON you can collapse the first two steps down to:

        fd = shm_open(SHM_ANON);

If you think of the return value from shmget() as an fd, then setting this to 1
does make sense in a way.  That is you can have a sequence of:

        id = shmget(...);

        hold = shmat(id, ...);  /* "hold" mapping */

        shmctl(IPC_RMID, id);

        /* later */

        shmat(id, ...);

        /* or pass 'id' as a value across a socket and shmat in another process */

    shmdt(hold);

    /* now shmat may or may not work */

However, note that this does require you to do some things carefully:

  1) You have to keep an existing mapping around via shmat() that serves as
     the equivalent of the open file descriptor to keep the 'id' valid.
     This also means you have to defer the IPC_RMID until after you have
     created this "hold" mapping.

  2) You always have to invoke IPC_RMID.  In particular, even if you use
     shmget() with IPC_PRIVATE you have to explicitly IPC_RMID.

So, I think it is fine to change this (and have asked about doing it myself
in the past).  However, I think shm_open() is definitely a superior API for
this sort of thing with semantics that are clearer and more inline with UNIX.

--
John Baldwin
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Make kern.ipc.shm_allow_removed default to 1.

Konstantin Belousov
On Fri, Sep 11, 2015 at 09:24:12AM -0700, John Baldwin wrote:

> On Tuesday, September 08, 2015 09:13:10 PM Edward Tomasz Napiera??a wrote:
> > I'd like to commit - well, discuss it first - the following change:
> >
> > https://reviews.freebsd.org/D3603
> >
> > This changes the default setting of kern.ipc.shm_allow_removed from 0 to 1.
> > This removes the need of manually changing this flag for Google Chrome
> > users. It also improves compatibility with Linux applications running under
> > Linuxulator compatibility layer, and possibly also helps in porting software
> > from Linux.
> >
> > Generally speaking, the flag allows applications to create the shared memory
> > segment, attach it, remove it, and then continue to use it and to reattach it
> > later. This means that the kernel will automatically "clean up" after the
> > application exits.
> >
> > It could be argued that it's against POSIX. However, SUSv3 says this
> > about IPC_RMID: "Remove the shared memory identifier specified by shmid from
> > the system and destroy the shared memory segment and shmid_ds data structure
> > associated with it." From my reading, we break it in any case by deferring
> > removal of the segment until it's detached; we won't break it any more
> > by also deferring removal of the identifier.
>
> It is against POSIX.  When I last raised this with kib@ and alc@, Alan noted
> that our current behavior is not really against POSIX as he felt there was
> enough wriggle room to permit existing mappings to remain valid.
>
> That said, in the thread kib@ felt it was ok to change the default.  I think
> it's probably fine.  It's a hack for the fact that shmget() isn't returning
> a real fd the way shm_open() does, so in that sense it is ok.  That is,
> in shm_open() you can do this:
>
> fd = shm_open("/some/path");
> shm_unlink("/some/path");
>
> mmap(..., fd);
>
> /* later */
>
> mmap(..., fd);
>
> /* or pass fd over a UNIX domain socket and map it in another process */
>
> close(fd);
>
> /* now mmap won't work */
>
> With FreeBSD's SHM_ANON you can collapse the first two steps down to:
>
> fd = shm_open(SHM_ANON);
>
> If you think of the return value from shmget() as an fd, then setting this to 1
> does make sense in a way.  That is you can have a sequence of:
>
> id = shmget(...);
>
> hold = shmat(id, ...);  /* "hold" mapping */
>
> shmctl(IPC_RMID, id);
>
> /* later */
>
> shmat(id, ...);
>
> /* or pass 'id' as a value across a socket and shmat in another process */
>
>     shmdt(hold);
>
>     /* now shmat may or may not work */
>
> However, note that this does require you to do some things carefully:
>
>   1) You have to keep an existing mapping around via shmat() that serves as
>      the equivalent of the open file descriptor to keep the 'id' valid.
>      This also means you have to defer the IPC_RMID until after you have
>      created this "hold" mapping.
>
>   2) You always have to invoke IPC_RMID.  In particular, even if you use
>      shmget() with IPC_PRIVATE you have to explicitly IPC_RMID.
>
> So, I think it is fine to change this (and have asked about doing it myself
> in the past).  However, I think shm_open() is definitely a superior API for
> this sort of thing with semantics that are clearer and more inline with UNIX.

The request to change the defaults written after somewhat long
discussion elsewere, which was started by the fact that chrome requires
the shm_allow_removed = 1 behaviour.

My opinion is that yes, =1 behaviour contradicts the SUSv4tc1 specification:
From shmat() description:
[EINVAL] The value of shmid is not a valid shared memory identifier
From shmctl():
IPC_RMID
    Remove the shared memory identifier specified by shmid from the
    system and destroy the shared memory segment and shmid_ds data structure
    associated with it.

Note the part about IPC_RMID that required it to not only delete the
identifier, but also to 'destroy the shared memory segment'. This was
interpreted as being non-compliant with the setting of =0. Solaris
man page describes out =0 behaviour, not the POSIX destruction of the
segment on IPC_RMID.

IMO we can switch the default, but then we would become even more
non-compliant.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Make kern.ipc.shm_allow_removed default to 1.

John Baldwin
On Friday, September 11, 2015 08:06:12 PM Konstantin Belousov wrote:

> On Fri, Sep 11, 2015 at 09:24:12AM -0700, John Baldwin wrote:
> > On Tuesday, September 08, 2015 09:13:10 PM Edward Tomasz Napiera??a wrote:
> > > I'd like to commit - well, discuss it first - the following change:
> > >
> > > https://reviews.freebsd.org/D3603
> > >
> > > This changes the default setting of kern.ipc.shm_allow_removed from 0 to 1.
> > > This removes the need of manually changing this flag for Google Chrome
> > > users. It also improves compatibility with Linux applications running under
> > > Linuxulator compatibility layer, and possibly also helps in porting software
> > > from Linux.
> > >
> > > Generally speaking, the flag allows applications to create the shared memory
> > > segment, attach it, remove it, and then continue to use it and to reattach it
> > > later. This means that the kernel will automatically "clean up" after the
> > > application exits.
> > >
> > > It could be argued that it's against POSIX. However, SUSv3 says this
> > > about IPC_RMID: "Remove the shared memory identifier specified by shmid from
> > > the system and destroy the shared memory segment and shmid_ds data structure
> > > associated with it." From my reading, we break it in any case by deferring
> > > removal of the segment until it's detached; we won't break it any more
> > > by also deferring removal of the identifier.
> >
> > It is against POSIX.  When I last raised this with kib@ and alc@, Alan noted
> > that our current behavior is not really against POSIX as he felt there was
> > enough wriggle room to permit existing mappings to remain valid.
> >
> > That said, in the thread kib@ felt it was ok to change the default.  I think
> > it's probably fine.  It's a hack for the fact that shmget() isn't returning
> > a real fd the way shm_open() does, so in that sense it is ok.  That is,
> > in shm_open() you can do this:
> >
> > fd = shm_open("/some/path");
> > shm_unlink("/some/path");
> >
> > mmap(..., fd);
> >
> > /* later */
> >
> > mmap(..., fd);
> >
> > /* or pass fd over a UNIX domain socket and map it in another process */
> >
> > close(fd);
> >
> > /* now mmap won't work */
> >
> > With FreeBSD's SHM_ANON you can collapse the first two steps down to:
> >
> > fd = shm_open(SHM_ANON);
> >
> > If you think of the return value from shmget() as an fd, then setting this to 1
> > does make sense in a way.  That is you can have a sequence of:
> >
> > id = shmget(...);
> >
> > hold = shmat(id, ...);  /* "hold" mapping */
> >
> > shmctl(IPC_RMID, id);
> >
> > /* later */
> >
> > shmat(id, ...);
> >
> > /* or pass 'id' as a value across a socket and shmat in another process */
> >
> >     shmdt(hold);
> >
> >     /* now shmat may or may not work */
> >
> > However, note that this does require you to do some things carefully:
> >
> >   1) You have to keep an existing mapping around via shmat() that serves as
> >      the equivalent of the open file descriptor to keep the 'id' valid.
> >      This also means you have to defer the IPC_RMID until after you have
> >      created this "hold" mapping.
> >
> >   2) You always have to invoke IPC_RMID.  In particular, even if you use
> >      shmget() with IPC_PRIVATE you have to explicitly IPC_RMID.
> >
> > So, I think it is fine to change this (and have asked about doing it myself
> > in the past).  However, I think shm_open() is definitely a superior API for
> > this sort of thing with semantics that are clearer and more inline with UNIX.
>
> The request to change the defaults written after somewhat long
> discussion elsewere, which was started by the fact that chrome requires
> the shm_allow_removed = 1 behaviour.
>
> My opinion is that yes, =1 behaviour contradicts the SUSv4tc1 specification:
> From shmat() description:
> [EINVAL] The value of shmid is not a valid shared memory identifier
> From shmctl():
> IPC_RMID
>     Remove the shared memory identifier specified by shmid from the
>     system and destroy the shared memory segment and shmid_ds data structure
>     associated with it.
>
> Note the part about IPC_RMID that required it to not only delete the
> identifier, but also to 'destroy the shared memory segment'. This was
> interpreted as being non-compliant with the setting of =0. Solaris
> man page describes out =0 behaviour, not the POSIX destruction of the
> segment on IPC_RMID.
>
> IMO we can switch the default, but then we would become even more
> non-compliant.

Regardless of the switch I think we should add an IMPLEMENTATION NOTE or the
like to shmctl(2) to describe this sysctl and explain how it affects the
behavior.  We should also probably have a STANDARDS section in that manpage
that details our deviations from the spec.  In an earlier thread, Alan stated
that he did not know of any UNIX that invalidated the memory segment when
IPC_RMID was issued.  All of them leave existing mappings in tact.

--
John Baldwin
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Make kern.ipc.shm_allow_removed default to 1.

Warner Losh
I'll note in the early days of the project we'd often make the
decision to not be POSIX by default when POSIX was stupid.
I think the same precedent applies here. Be more useful
by default, but document how to get the pedantic standard's
conforming behavior for those that need it.

Warner

On Fri, Sep 11, 2015 at 11:35 AM, John Baldwin <[hidden email]> wrote:

> On Friday, September 11, 2015 08:06:12 PM Konstantin Belousov wrote:
> > On Fri, Sep 11, 2015 at 09:24:12AM -0700, John Baldwin wrote:
> > > On Tuesday, September 08, 2015 09:13:10 PM Edward Tomasz Napiera??a
> wrote:
> > > > I'd like to commit - well, discuss it first - the following change:
> > > >
> > > > https://reviews.freebsd.org/D3603
> > > >
> > > > This changes the default setting of kern.ipc.shm_allow_removed from
> 0 to 1.
> > > > This removes the need of manually changing this flag for Google
> Chrome
> > > > users. It also improves compatibility with Linux applications
> running under
> > > > Linuxulator compatibility layer, and possibly also helps in porting
> software
> > > > from Linux.
> > > >
> > > > Generally speaking, the flag allows applications to create the
> shared memory
> > > > segment, attach it, remove it, and then continue to use it and to
> reattach it
> > > > later. This means that the kernel will automatically "clean up"
> after the
> > > > application exits.
> > > >
> > > > It could be argued that it's against POSIX. However, SUSv3 says this
> > > > about IPC_RMID: "Remove the shared memory identifier specified by
> shmid from
> > > > the system and destroy the shared memory segment and shmid_ds data
> structure
> > > > associated with it." From my reading, we break it in any case by
> deferring
> > > > removal of the segment until it's detached; we won't break it any
> more
> > > > by also deferring removal of the identifier.
> > >
> > > It is against POSIX.  When I last raised this with kib@ and alc@,
> Alan noted
> > > that our current behavior is not really against POSIX as he felt there
> was
> > > enough wriggle room to permit existing mappings to remain valid.
> > >
> > > That said, in the thread kib@ felt it was ok to change the default.
> I think
> > > it's probably fine.  It's a hack for the fact that shmget() isn't
> returning
> > > a real fd the way shm_open() does, so in that sense it is ok.  That is,
> > > in shm_open() you can do this:
> > >
> > >     fd = shm_open("/some/path");
> > >     shm_unlink("/some/path");
> > >
> > >     mmap(..., fd);
> > >
> > >     /* later */
> > >
> > >     mmap(..., fd);
> > >
> > >     /* or pass fd over a UNIX domain socket and map it in another
> process */
> > >
> > >     close(fd);
> > >
> > >     /* now mmap won't work */
> > >
> > > With FreeBSD's SHM_ANON you can collapse the first two steps down to:
> > >
> > >     fd = shm_open(SHM_ANON);
> > >
> > > If you think of the return value from shmget() as an fd, then setting
> this to 1
> > > does make sense in a way.  That is you can have a sequence of:
> > >
> > >     id = shmget(...);
> > >
> > >     hold = shmat(id, ...);  /* "hold" mapping */
> > >
> > >     shmctl(IPC_RMID, id);
> > >
> > >     /* later */
> > >
> > >     shmat(id, ...);
> > >
> > >     /* or pass 'id' as a value across a socket and shmat in another
> process */
> > >
> > >     shmdt(hold);
> > >
> > >     /* now shmat may or may not work */
> > >
> > > However, note that this does require you to do some things carefully:
> > >
> > >   1) You have to keep an existing mapping around via shmat() that
> serves as
> > >      the equivalent of the open file descriptor to keep the 'id' valid.
> > >      This also means you have to defer the IPC_RMID until after you
> have
> > >      created this "hold" mapping.
> > >
> > >   2) You always have to invoke IPC_RMID.  In particular, even if you
> use
> > >      shmget() with IPC_PRIVATE you have to explicitly IPC_RMID.
> > >
> > > So, I think it is fine to change this (and have asked about doing it
> myself
> > > in the past).  However, I think shm_open() is definitely a superior
> API for
> > > this sort of thing with semantics that are clearer and more inline
> with UNIX.
> >
> > The request to change the defaults written after somewhat long
> > discussion elsewere, which was started by the fact that chrome requires
> > the shm_allow_removed = 1 behaviour.
> >
> > My opinion is that yes, =1 behaviour contradicts the SUSv4tc1
> specification:
> > From shmat() description:
> > [EINVAL] The value of shmid is not a valid shared memory identifier
> > From shmctl():
> > IPC_RMID
> >     Remove the shared memory identifier specified by shmid from the
> >     system and destroy the shared memory segment and shmid_ds data
> structure
> >     associated with it.
> >
> > Note the part about IPC_RMID that required it to not only delete the
> > identifier, but also to 'destroy the shared memory segment'. This was
> > interpreted as being non-compliant with the setting of =0. Solaris
> > man page describes out =0 behaviour, not the POSIX destruction of the
> > segment on IPC_RMID.
> >
> > IMO we can switch the default, but then we would become even more
> > non-compliant.
>
> Regardless of the switch I think we should add an IMPLEMENTATION NOTE or
> the
> like to shmctl(2) to describe this sysctl and explain how it affects the
> behavior.  We should also probably have a STANDARDS section in that manpage
> that details our deviations from the spec.  In an earlier thread, Alan
> stated
> that he did not know of any UNIX that invalidated the memory segment when
> IPC_RMID was issued.  All of them leave existing mappings in tact.
>
> --
> John Baldwin
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "[hidden email]"
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"