Currently the only reliable way for server-side devices to become aware of canceled asyncs is by checking for STATUS_PENDING in their reselect_async callback. Named pipes and sockets both use this to free extra data (struct pipe_message and struct accept_req respectively).
This is a bit architecturally unpleasant, and hard to follow, and caused me some grief while trying to implement socket ioctls. After some discussion with Jacek I decided to try to restructure things a bit, so that the socket provides a callback which will be called as soon as we know the async is complete.
This requires some other changes, though:
* Not only is it possible for an async to outlive its object, but this even happens normally (consider e.g. STATUS_HANDLES_CLOSED)—even after the object is destroyed, a thread apc still holds a reference to the async. In case the completion callback needs to access object information (as with accept_req, to remove that structure from a per-socket list), we need to ensure that the object is not freed before then. The obvious way to do this is to take a reference to the object, i.e. patch 0003.
* However, this causes a circular reference, since we're effectively also taking a reference to the async on behalf of the object in alloc_accept_req() [and even if that were removed, we'd still be holding a reference to the async on behalf of the object via the async queue. We can't remove all such references as they are the only things keeping a pending async alive.] In order to break this reference loop, we make use of the close_handle callback to terminate all asyncs as soon as the last user-space handle is closed, rather than when the object is destroyed, i.e. patch 0002. Note that this doesn't mean they'll immediately be removed from the list, but they will be as soon as the apc object is done with them.
[As an aside, this also seems to match the way Windows drivers work: according to [1] drivers should cancel pending asyncs in IRP_MJ_CLEANUP, and according to [2] when IRP_MJ_CLOSE is subsequently sent "all outstanding I/O requests have been completed or canceled."]
* Note that we can't easily avoid taking a reference to the socket object from the async. The earliest possible place we can call the async callback is in async_set_result(), once we're sure that the async is done and its results can't be altered. We could call it in async_terminate(), and make sure that async_terminate() is always called on object destruction, but there is a slight snag in that the client is technically allowed to restart any async. That won't actually happen in practice, for any of the asyncs I've seen or have planned, but it's an awful lot of implicit assumptions to remember.
As an added complication, there is an awful socket ioctl, viz. the WSAPoll() ioctl, which can outlive the object it's called on. Neither of the applications I've seen that use the ioctl depend on that behaviour, but this restructuring does actually help make it possible to implement without reference leaks.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/irp-mj-clea... [2] https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/irp-mj-clos...
Zebediah Figura (4): server: Hold a reference to the iosb in the accept_req structure. server: Terminate accept asyncs when the last socket handle is closed. server: Hold a reference to both sockets in the accept_req structure. server: Use a callback to free the accept_req structure.
server/async.c | 15 ++++++++ server/file.h | 4 +++ server/sock.c | 98 ++++++++++++++++++++++++++------------------------ 3 files changed, 71 insertions(+), 46 deletions(-)
For convenience. Mirrors struct pipe_message.
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- server/sock.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/server/sock.c b/server/sock.c index 5281c65e8b0..4ec21fc73b3 100644 --- a/server/sock.c +++ b/server/sock.c @@ -126,6 +126,7 @@ struct accept_req { struct list entry; struct async *async; + struct iosb *iosb; struct sock *acceptsock; int accepted; unsigned int recv_len, local_len; @@ -452,11 +453,13 @@ static void free_accept_req( struct accept_req *req ) list_remove( &req->entry ); if (req->acceptsock) req->acceptsock->accept_recv_req = NULL; release_object( req->async ); + release_object( req->iosb ); free( req ); }
-static void fill_accept_output( struct accept_req *req, struct iosb *iosb ) +static void fill_accept_output( struct accept_req *req ) { + struct iosb *iosb = req->iosb; union unix_sockaddr unix_addr; struct WS_sockaddr *win_addr; unsigned int remote_len; @@ -527,20 +530,17 @@ static void complete_async_accept( struct sock *sock, struct accept_req *req ) { struct sock *acceptsock = req->acceptsock; struct async *async = req->async; - struct iosb *iosb;
if (debug_level) fprintf( stderr, "completing accept request for socket %p\n", sock );
if (acceptsock) { if (!accept_into_socket( sock, acceptsock )) return; - - iosb = async_get_iosb( async ); - fill_accept_output( req, iosb ); - release_object( iosb ); + fill_accept_output( req ); } else { + struct iosb *iosb = req->iosb; obj_handle_t handle;
if (!(acceptsock = accept_socket( sock ))) return; @@ -550,32 +550,22 @@ static void complete_async_accept( struct sock *sock, struct accept_req *req ) release_object( acceptsock ); if (!handle) return;
- iosb = async_get_iosb( async ); - if (!(iosb->out_data = malloc( sizeof(handle) ))) - { - release_object( iosb ); - return; - } + if (!(iosb->out_data = malloc( sizeof(handle) ))) return; + iosb->status = STATUS_SUCCESS; iosb->out_size = sizeof(handle); memcpy( iosb->out_data, &handle, sizeof(handle) ); - release_object( iosb ); set_error( STATUS_ALERTED ); } }
static void complete_async_accept_recv( struct accept_req *req ) { - struct async *async = req->async; - struct iosb *iosb; - if (debug_level) fprintf( stderr, "completing accept recv request for socket %p\n", req->acceptsock );
assert( req->recv_len );
- iosb = async_get_iosb( async ); - fill_accept_output( req, iosb ); - release_object( iosb ); + fill_accept_output( req ); }
static int sock_dispatch_asyncs( struct sock *sock, int event, int error ) @@ -880,10 +870,8 @@ static void sock_reselect_async( struct fd *fd, struct async_queue *queue )
LIST_FOR_EACH_ENTRY_SAFE( req, next, &sock->accept_list, struct accept_req, entry ) { - struct iosb *iosb = async_get_iosb( req->async ); - if (iosb->status != STATUS_PENDING) + if (req->iosb->status != STATUS_PENDING) free_accept_req( req ); - release_object( iosb ); }
/* ignore reselect on ifchange queue */ @@ -1370,6 +1358,7 @@ static struct accept_req *alloc_accept_req( struct sock *acceptsock, struct asyn if (req) { req->async = (struct async *)grab_object( async ); + req->iosb = async_get_iosb( async ); req->acceptsock = acceptsock; req->accepted = 0; req->recv_len = 0;
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- server/sock.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/server/sock.c b/server/sock.c index 4ec21fc73b3..6523418f779 100644 --- a/server/sock.c +++ b/server/sock.c @@ -165,6 +165,7 @@ struct sock static void sock_dump( struct object *obj, int verbose ); static int sock_signaled( struct object *obj, struct wait_queue_entry *entry ); static struct fd *sock_get_fd( struct object *obj ); +static int sock_close_handle( struct object *obj, struct process *process, obj_handle_t handle ); static void sock_destroy( struct object *obj ); static struct object *sock_get_ifchange( struct sock *sock ); static void sock_release_ifchange( struct sock *sock ); @@ -201,7 +202,7 @@ static const struct object_ops sock_ops = NULL, /* unlink_name */ no_open_file, /* open_file */ no_kernel_obj_list, /* get_kernel_obj_list */ - fd_close_handle, /* close_handle */ + sock_close_handle, /* close_handle */ sock_destroy /* destroy */ };
@@ -885,11 +886,27 @@ static struct fd *sock_get_fd( struct object *obj ) return (struct fd *)grab_object( sock->fd ); }
-static void sock_destroy( struct object *obj ) +static int sock_close_handle( struct object *obj, struct process *process, obj_handle_t handle ) { struct sock *sock = (struct sock *)obj; struct accept_req *req, *next;
+ if (sock->obj.handle_count == 1) /* last handle */ + { + if (sock->accept_recv_req) + async_terminate( sock->accept_recv_req->async, STATUS_CANCELLED ); + + LIST_FOR_EACH_ENTRY_SAFE( req, next, &sock->accept_list, struct accept_req, entry ) + async_terminate( req->async, STATUS_CANCELLED ); + } + + return fd_close_handle( obj, process, handle ); +} + +static void sock_destroy( struct object *obj ) +{ + struct sock *sock = (struct sock *)obj; + assert( obj->ops == &sock_ops );
/* FIXME: special socket shutdown stuff? */ @@ -897,12 +914,6 @@ static void sock_destroy( struct object *obj ) if ( sock->deferred ) release_object( sock->deferred );
- if (sock->accept_recv_req) - async_terminate( sock->accept_recv_req->async, STATUS_CANCELLED ); - - LIST_FOR_EACH_ENTRY_SAFE( req, next, &sock->accept_list, struct accept_req, entry ) - async_terminate( req->async, STATUS_CANCELLED ); - async_wake_up( &sock->ifchange_q, STATUS_CANCELLED ); sock_release_ifchange( sock ); free_async_queue( &sock->read_q );
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- server/sock.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/server/sock.c b/server/sock.c index 6523418f779..78b1578113b 100644 --- a/server/sock.c +++ b/server/sock.c @@ -127,7 +127,7 @@ struct accept_req struct list entry; struct async *async; struct iosb *iosb; - struct sock *acceptsock; + struct sock *sock, *acceptsock; int accepted; unsigned int recv_len, local_len; }; @@ -452,9 +452,14 @@ static inline int sock_error( struct fd *fd ) static void free_accept_req( struct accept_req *req ) { list_remove( &req->entry ); - if (req->acceptsock) req->acceptsock->accept_recv_req = NULL; + if (req->acceptsock) + { + req->acceptsock->accept_recv_req = NULL; + release_object( req->acceptsock ); + } release_object( req->async ); release_object( req->iosb ); + release_object( req->sock ); free( req ); }
@@ -1361,7 +1366,7 @@ static int sock_get_ntstatus( int err ) } }
-static struct accept_req *alloc_accept_req( struct sock *acceptsock, struct async *async, +static struct accept_req *alloc_accept_req( struct sock *sock, struct sock *acceptsock, struct async *async, const struct afd_accept_into_params *params ) { struct accept_req *req = mem_alloc( sizeof(*req) ); @@ -1370,7 +1375,9 @@ static struct accept_req *alloc_accept_req( struct sock *acceptsock, struct asyn { req->async = (struct async *)grab_object( async ); req->iosb = async_get_iosb( async ); + req->sock = (struct sock *)grab_object( sock ); req->acceptsock = acceptsock; + if (acceptsock) grab_object( acceptsock ); req->accepted = 0; req->recv_len = 0; req->local_len = 0; @@ -1424,7 +1431,7 @@ static int sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) if (sock->state & FD_WINE_NONBLOCKING) return 0; if (get_error() != (0xc0010000 | WSAEWOULDBLOCK)) return 0;
- if (!(req = alloc_accept_req( NULL, async, NULL ))) return 0; + if (!(req = alloc_accept_req( sock, NULL, async, NULL ))) return 0; list_add_tail( &sock->accept_list, &req->entry );
queue_async( &sock->accept_q, async ); @@ -1473,7 +1480,7 @@ static int sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) return 0; }
- if (!(req = alloc_accept_req( acceptsock, async, params ))) + if (!(req = alloc_accept_req( sock, acceptsock, async, params ))) { release_object( acceptsock ); return 0;
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- server/async.c | 15 +++++++++++++++ server/file.h | 4 ++++ server/sock.c | 23 +++++++++++------------ 3 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/server/async.c b/server/async.c index 6da61fd502c..1ac5117edb9 100644 --- a/server/async.c +++ b/server/async.c @@ -55,6 +55,8 @@ struct async struct completion *completion; /* completion associated with fd */ apc_param_t comp_key; /* completion key associated with fd */ unsigned int comp_flags; /* completion flags */ + async_completion_callback completion_callback; /* callback to be called on completion */ + void *completion_callback_private; /* argument to completion_callback */ };
static void async_dump( struct object *obj, int verbose ); @@ -247,6 +249,8 @@ struct async *create_async( struct fd *fd, struct thread *thread, const async_da async->direct_result = 0; async->completion = fd_get_completion( fd, &async->comp_key ); async->comp_flags = 0; + async->completion_callback = NULL; + async->completion_callback_private = NULL;
if (iosb) async->iosb = (struct iosb *)grab_object( iosb ); else async->iosb = NULL; @@ -362,6 +366,13 @@ void async_set_timeout( struct async *async, timeout_t timeout, unsigned int sta async->timeout_status = status; }
+/* set a callback to be notified when the async is completed */ +void async_set_completion_callback( struct async *async, async_completion_callback func, void *private ) +{ + async->completion_callback = func; + async->completion_callback_private = private; +} + static void add_async_completion( struct async *async, apc_param_t cvalue, unsigned int status, apc_param_t information ) { @@ -420,6 +431,10 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota async->signaled = 1; wake_up( &async->obj, 0 ); } + + if (async->completion_callback) + async->completion_callback( async->completion_callback_private ); + async->completion_callback = NULL; } }
diff --git a/server/file.h b/server/file.h index 686bae084c5..87b500f0b5f 100644 --- a/server/file.h +++ b/server/file.h @@ -209,6 +209,9 @@ extern int is_serial_fd( struct fd *fd ); extern struct object *create_serial( struct fd *fd );
/* async I/O functions */ + +typedef void (*async_completion_callback)( void *private ); + extern void free_async_queue( struct async_queue *queue ); extern struct async *create_async( struct fd *fd, struct thread *thread, const async_data_t *data, struct iosb *iosb ); extern struct async *create_request_async( struct fd *fd, unsigned int comp_flags, const async_data_t *data ); @@ -216,6 +219,7 @@ extern obj_handle_t async_handoff( struct async *async, int success, data_size_t extern void queue_async( struct async_queue *queue, struct async *async ); extern void async_set_timeout( struct async *async, timeout_t timeout, unsigned int status ); extern void async_set_result( struct object *obj, unsigned int status, apc_param_t total ); +extern void async_set_completion_callback( struct async *async, async_completion_callback func, void *private ); extern void set_async_pending( struct async *async, int signal ); extern int async_waiting( struct async_queue *queue ); extern void async_terminate( struct async *async, unsigned int status ); diff --git a/server/sock.c b/server/sock.c index 78b1578113b..3aed5494612 100644 --- a/server/sock.c +++ b/server/sock.c @@ -449,8 +449,9 @@ static inline int sock_error( struct fd *fd ) return optval; }
-static void free_accept_req( struct accept_req *req ) +static void free_accept_req( void *private ) { + struct accept_req *req = private; list_remove( &req->entry ); if (req->acceptsock) { @@ -582,7 +583,7 @@ static int sock_dispatch_asyncs( struct sock *sock, int event, int error )
LIST_FOR_EACH_ENTRY( req, &sock->accept_list, struct accept_req, entry ) { - if (!req->accepted) + if (req->iosb->status == STATUS_PENDING && !req->accepted) { complete_async_accept( sock, req ); if (get_error() != STATUS_PENDING) @@ -591,7 +592,7 @@ static int sock_dispatch_asyncs( struct sock *sock, int event, int error ) } }
- if (sock->accept_recv_req) + if (sock->accept_recv_req && sock->accept_recv_req->iosb->status == STATUS_PENDING) { complete_async_accept_recv( sock->accept_recv_req ); if (get_error() != STATUS_PENDING) @@ -626,9 +627,12 @@ static int sock_dispatch_asyncs( struct sock *sock, int event, int error ) async_wake_up( &sock->write_q, status );
LIST_FOR_EACH_ENTRY_SAFE( req, next, &sock->accept_list, struct accept_req, entry ) - async_terminate( req->async, status ); + { + if (req->iosb->status == STATUS_PENDING) + async_terminate( req->async, status ); + }
- if (sock->accept_recv_req) + if (sock->accept_recv_req && sock->accept_recv_req->iosb->status == STATUS_PENDING) async_terminate( sock->accept_recv_req->async, status ); }
@@ -872,13 +876,6 @@ static void sock_queue_async( struct fd *fd, struct async *async, int type, int static void sock_reselect_async( struct fd *fd, struct async_queue *queue ) { struct sock *sock = get_fd_user( fd ); - struct accept_req *req, *next; - - LIST_FOR_EACH_ENTRY_SAFE( req, next, &sock->accept_list, struct accept_req, entry ) - { - if (req->iosb->status != STATUS_PENDING) - free_accept_req( req ); - }
/* ignore reselect on ifchange queue */ if (&sock->ifchange_q != queue) @@ -1434,6 +1431,7 @@ static int sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) if (!(req = alloc_accept_req( sock, NULL, async, NULL ))) return 0; list_add_tail( &sock->accept_list, &req->entry );
+ async_set_completion_callback( async, free_accept_req, req ); queue_async( &sock->accept_q, async ); sock_reselect( sock ); set_error( STATUS_PENDING ); @@ -1490,6 +1488,7 @@ static int sock_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) release_object( acceptsock );
acceptsock->wparam = params->accept_handle; + async_set_completion_callback( async, free_accept_req, req ); queue_async( &sock->accept_q, async ); sock_reselect( sock ); set_error( STATUS_PENDING );
-
Zebediah Figura