| Message ID | 20230429201822.2605207-4-bugaevc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | The remaining x86_64-gnu patches | expand |
Applied, thanks! Sergey Bugaev, le sam. 29 avril 2023 23:18:19 +0300, a ecrit: > If we're trying to interrupt an interruptible RPC, but the server fails > to respond to our __interrupt_operation () call, we instead destroy the > reply port we were expecting the reply to the RPC on. > > Instead of deallocating the name completely, replace it with a dead > name, so the name won't get reused for some other right, and deallocate > it in _hurd_intr_rpc_mach_msg once we return from the signal handler. > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> > --- > This is not required for x86_64, but probably a good idea anyway. I have > checked that this does not fatally break things (this commit has been > sitting in my tree, getting built along with the other changes, for quite > some time without noticably breaking anything), but I have not run the > full testsuite. Please do that on your end. > > hurd/hurdsig.c | 15 ++++++++++++--- > hurd/intr-msg.c | 1 + > sysdeps/mach/hurd/mig-reply.c | 25 +++++++------------------ > 3 files changed, 20 insertions(+), 21 deletions(-) > > diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c > index b3808f9e..78ea59d9 100644 > --- a/hurd/hurdsig.c > +++ b/hurd/hurdsig.c > @@ -477,9 +477,18 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, int sigthread, > if (reply) > { > /* The interrupt didn't work. > - Destroy the receive right the thread is blocked on. */ > - __mach_port_destroy (__mach_task_self (), *reply); > - *reply = MACH_PORT_NULL; > + Destroy the receive right the thread is blocked on, and > + replace it with a dead name to keep the name from reuse until > + the therad is done with it. To do this atomically, first > + insert a send right, and then destroy the receive right, > + turning the send right into a dead name. */ > + err = __mach_port_insert_right (__mach_task_self (), > + *reply, *reply, > + MACH_MSG_TYPE_MAKE_SEND); > + assert_perror (err); > + err = __mach_port_mod_refs (__mach_task_self (), *reply, > + MACH_PORT_RIGHT_RECEIVE, -1); > + assert_perror (err); > } > > /* The system call return value register now contains > diff --git a/hurd/intr-msg.c b/hurd/intr-msg.c > index 1a086b51..716d87ab 100644 > --- a/hurd/intr-msg.c > +++ b/hurd/intr-msg.c > @@ -305,6 +305,7 @@ _hurd_intr_rpc_mach_msg (mach_msg_header_t *msg, > { > /* Make sure we have a valid reply port. The one we were using > may have been destroyed by interruption. */ > + __mig_dealloc_reply_port (rcv_name); > m->header.msgh_local_port = rcv_name = __mig_get_reply_port (); > m->header.msgh_bits = msgh_bits; > option = user_option; > diff --git a/sysdeps/mach/hurd/mig-reply.c b/sysdeps/mach/hurd/mig-reply.c > index 3fdee80e..7ea001df 100644 > --- a/sysdeps/mach/hurd/mig-reply.c > +++ b/sysdeps/mach/hurd/mig-reply.c > @@ -69,29 +69,18 @@ __mig_dealloc_reply_port (mach_port_t arg) > mach_port_t port = get_reply_port (); > > set_reply_port (MACH_PORT_NULL); /* So the mod_refs RPC won't use it. */ > - > - /* Normally, ARG should be the same as PORT that we store. However, if a > - signal has interrupted the RPC, the stored PORT has been deallocated and > - reset to MACH_PORT_NULL (or possibly MACH_PORT_DEAD). In this case the > - MIG routine still has the old name, which it passes to us here. We must > - not deallocate (or otherwise touch) it, since it may be already allocated > - to another port right. Fortunately MIG itself doesn't do anything with > - the reply port on errors either, other than immediately calling this > - function. > - > - And so: > - 1. Assert that things are sane, i.e. and PORT is either invalid or same > - as ARG. > - 2. Only deallocate the name if our stored PORT still names it. In that > - case we're sure the right has not been deallocated / the name reused. > - */ > - > + assert (port == arg); > if (!MACH_PORT_VALID (port)) > return; > - assert (port == arg); > > err = __mach_port_mod_refs (__mach_task_self (), port, > MACH_PORT_RIGHT_RECEIVE, -1); > + if (err == KERN_INVALID_RIGHT) > + /* It could be that during signal handling, the receive right had been > + replaced with a dead name. */ > + err = __mach_port_mod_refs (__mach_task_self (), port, > + MACH_PORT_RIGHT_DEAD_NAME, -1); > + > assert_perror (err); > } > weak_alias (__mig_dealloc_reply_port, mig_dealloc_reply_port) > -- > 2.40.1 >
diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c index b3808f9e..78ea59d9 100644 --- a/hurd/hurdsig.c +++ b/hurd/hurdsig.c @@ -477,9 +477,18 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, int sigthread, if (reply) { /* The interrupt didn't work. - Destroy the receive right the thread is blocked on. */ - __mach_port_destroy (__mach_task_self (), *reply); - *reply = MACH_PORT_NULL; + Destroy the receive right the thread is blocked on, and + replace it with a dead name to keep the name from reuse until + the therad is done with it. To do this atomically, first + insert a send right, and then destroy the receive right, + turning the send right into a dead name. */ + err = __mach_port_insert_right (__mach_task_self (), + *reply, *reply, + MACH_MSG_TYPE_MAKE_SEND); + assert_perror (err); + err = __mach_port_mod_refs (__mach_task_self (), *reply, + MACH_PORT_RIGHT_RECEIVE, -1); + assert_perror (err); } /* The system call return value register now contains diff --git a/hurd/intr-msg.c b/hurd/intr-msg.c index 1a086b51..716d87ab 100644 --- a/hurd/intr-msg.c +++ b/hurd/intr-msg.c @@ -305,6 +305,7 @@ _hurd_intr_rpc_mach_msg (mach_msg_header_t *msg, { /* Make sure we have a valid reply port. The one we were using may have been destroyed by interruption. */ + __mig_dealloc_reply_port (rcv_name); m->header.msgh_local_port = rcv_name = __mig_get_reply_port (); m->header.msgh_bits = msgh_bits; option = user_option; diff --git a/sysdeps/mach/hurd/mig-reply.c b/sysdeps/mach/hurd/mig-reply.c index 3fdee80e..7ea001df 100644 --- a/sysdeps/mach/hurd/mig-reply.c +++ b/sysdeps/mach/hurd/mig-reply.c @@ -69,29 +69,18 @@ __mig_dealloc_reply_port (mach_port_t arg) mach_port_t port = get_reply_port (); set_reply_port (MACH_PORT_NULL); /* So the mod_refs RPC won't use it. */ - - /* Normally, ARG should be the same as PORT that we store. However, if a - signal has interrupted the RPC, the stored PORT has been deallocated and - reset to MACH_PORT_NULL (or possibly MACH_PORT_DEAD). In this case the - MIG routine still has the old name, which it passes to us here. We must - not deallocate (or otherwise touch) it, since it may be already allocated - to another port right. Fortunately MIG itself doesn't do anything with - the reply port on errors either, other than immediately calling this - function. - - And so: - 1. Assert that things are sane, i.e. and PORT is either invalid or same - as ARG. - 2. Only deallocate the name if our stored PORT still names it. In that - case we're sure the right has not been deallocated / the name reused. - */ - + assert (port == arg); if (!MACH_PORT_VALID (port)) return; - assert (port == arg); err = __mach_port_mod_refs (__mach_task_self (), port, MACH_PORT_RIGHT_RECEIVE, -1); + if (err == KERN_INVALID_RIGHT) + /* It could be that during signal handling, the receive right had been + replaced with a dead name. */ + err = __mach_port_mod_refs (__mach_task_self (), port, + MACH_PORT_RIGHT_DEAD_NAME, -1); + assert_perror (err); } weak_alias (__mig_dealloc_reply_port, mig_dealloc_reply_port)
If we're trying to interrupt an interruptible RPC, but the server fails to respond to our __interrupt_operation () call, we instead destroy the reply port we were expecting the reply to the RPC on. Instead of deallocating the name completely, replace it with a dead name, so the name won't get reused for some other right, and deallocate it in _hurd_intr_rpc_mach_msg once we return from the signal handler. Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> --- This is not required for x86_64, but probably a good idea anyway. I have checked that this does not fatally break things (this commit has been sitting in my tree, getting built along with the other changes, for quite some time without noticably breaking anything), but I have not run the full testsuite. Please do that on your end. hurd/hurdsig.c | 15 ++++++++++++--- hurd/intr-msg.c | 1 + sysdeps/mach/hurd/mig-reply.c | 25 +++++++------------------ 3 files changed, 20 insertions(+), 21 deletions(-)