Replace the config lock with a refcounted configuration snapshot - #631
Open
Roytak wants to merge 8 commits into
Open
Replace the config lock with a refcounted configuration snapshot#631Roytak wants to merge 8 commits into
Roytak wants to merge 8 commits into
Conversation
A listening socket file descriptor is runtime state, not configuration, but it used to live in struct nc_bind inside struct nc_server_config. The socket reconcile therefore had to write into the currently published configuration to stop nc_server_config_free() from closing a descriptor that the new generation reuses. Move the descriptors into a registry in server_opts, keyed by the endpoint name plus the resolved address and port, guarded by a new binds_lock. struct nc_bind keeps address and port, which are actual configuration. The reconcile now only opens sockets for binds with no registry entry and closes entries the new configuration no longer contains, so nothing is written to either configuration generation and the PHASE 1 / PHASE 2 / rollback socket bookkeeping is gone. A removed endpoint stops listening immediately. nc_server_accept_binds() copies the registry into local poll arrays, releases binds_lock and only then polls, so the lock is never held across the poll. The accepted connection is mapped back to an endpoint by name. nc_server_config_free() no longer closes sockets or unlinks UNIX socket files; both moved to the registry removal path, which also lets nc_server_unix_get_socket_path() become static. The client side Call Home binds keep their own descriptors, which moved to the parallel ch_binds_aux array.
config_lock doubled as the lock for the server_opts fields that are set through the API and are not part of struct nc_server_config. Their setters took it in WRITE mode, while their readers were only protected because the transport handshake happened to hold it in READ mode. Give them a lock of their own, opts_lock, so that the handshake can stop holding config_lock without leaving those readers unsynchronised. The fields are ch_dispatch_data, interactive_auth_clb and its data, pam_config_name, authkey_path_fmt, ssh_protocol_string, user_verify_clb, unix_socket_dir and unix_paths. opts_lock is a leaf lock, only config_lock may be held while acquiring it. It is also held on the authentication path, so it is never held across anything slow: the SSH protocol string and the PAM service name are copied out and pam_start() runs unlocked, and the interactive authentication and TLS verify callbacks are read together with their data pointer, then called after unlocking. Also free unix_socket_dir in nc_server_destroy(), it was leaked.
server_opts.config was held in READ mode across complete transport handshakes - key exchange plus authentication, bounded only by auth-timeout and unbounded when it is configured 0 - so a stalled handshake blocked every configuration update. It becomes a pointer to a refcounted, immutable struct nc_server_config. A handshake path takes a reference under the READ lock, releases the lock and works from the pinned pointer; an applier builds the next generation off-line, swaps the pointer under the WRITE lock and drops the old generation's reference, which is freed by its last reader. The lock is now held for a pointer read plus a refcount change, never across network I/O. The refcount is decremented with acq_rel so the reads of a generation are ordered before the free() of whoever drops the last reference; the increment stays relaxed, the READ lock orders it. The transport options are resolved lazily during the handshake, so the resolvers and the helpers between them and the handshake entry points take an explicit configuration parameter. The pinned generation lives in exactly one place, session->opts.server.config, a borrowed pointer set only by nc_accept() and nc_connect_ch_endpt() and cleared by them once the handshake is over. struct nc_ch_client.thread is mutable runtime state that used to be copied across generations, so the Call Home thread handles move to a registry in server_opts, superseding the redispatch fix. Stopping a client looks the thread up there, unlinks it, and only then joins it, so the whole unlock/join/relock deadlock avoidance and its leak path are gone. A thread whose client is not in the published generation now waits for it instead of exiting, since the socket and thread reconciles run before the swap. server_opts gains an atomic mirror of idle_timeout, stored at swap time. That fixes the two unlocked reads in nc_send_hello_io() and nc_server_recv_hello_io() and removes the last configuration access from nc_ps_poll_session_io(), so nc_ps_poll_sess() no longer takes the configuration lock on every poll iteration of every session. Also fixes nc_server_notif_cert_exp_dates_get(), which aliased the keystore and the truststore in its declaration list, before taking the lock - harmless with an embedded struct, a use-after-free with a pointer. test_config_update_during_auth now asserts the elapsed time, which is what the change is actually about, and four new tests cover the immediate close of a removed endpoint, the API setters during a handshake, a Call Home thread surviving a swap, and concurrent applies racing an accept.
The thread was appended to the registry only after pthread_create(), so a concurrent config apply could miss it and dispatch a second thread for the same client. Register and create the thread atomically under ch_threads_lock and reject a client that already has one, which restores the exclusivity the config write lock used to provide. A thread that terminated for any other reason than being told to left its registry entry behind, so every later apply considered the client running and never dispatched it again. Define the registry entry as the ownership token of the thread argument - whoever removes it joins the thread and frees the argument - and have an abnormally exiting thread unregister, detach and free itself. The thread also gave up when the configuration could not be acquired, which is a transient lock timeout; retry a few times instead. And do not log through the session in the thread cleanup, at that point it belongs to the user and may already have been freed. Finally, keep the rollback of the dispatch reconcile from walking a NULL entry: LY_ARRAY_NEW_GOTO() counts the new element in before the strdup() of the client name, so an allocation failure left a NULL in the array that the rollback then passed to dispatch_stop().
A connection accepted on a socket whose endpoint the pinned config does not have was accepted and then dropped. That also happened for a plain endpoint rename, which changes nothing about the socket. Resolve the endpoint of every bind while the poll set is built and skip the ones the pinned configuration does not know, so the pending connections stay in the listen backlog for a call with a newer configuration instead of being reset. The reuse pass also wrote the new endpoint name into the live registry entry right away, so a later failure of the apply left the registry carrying a name the published generation does not have - with the change above that socket would then never be polled again. Stage the rename in the bind description and store it only once nothing can fail, which is what the function documented all along. Since the registry lock is not held while polling, an apply may close a socket meanwhile. Accepting on a closed descriptor is a normal outcome here, so skip it instead of failing the whole nc_accept(). Also move struct nc_bind_desc to the private header and document how it differs from a configured bind and a registry entry.
A failed Call Home dispatch reconcile jumped to cleanup instead of rollback. The listening sockets were already reconciled against the generation being applied at that point, so a deleted endpoint stayed closed while the published configuration still advertised it and newly opened sockets accepted connections with no endpoint to serve them. nc_server_init() left the initial configuration generation allocated when a later init step failed, which both leaked it and made the server look initialized. Release it and clear the initialized flag. The options lock must not be held across anything slow, but the UNIX socket path resolution ran two realpath() calls under it. Copy the base directory and the hidden path mapping out and resolve the path unlocked. Also report a failed strdup() of the system public keys path format as an allocation error instead of "path format not set", and reject a NULL directory in the UNIX socket dir setter and getter.
A published configuration generation is immutable, but the accept and Call Home paths cast the constness away to pass its keepalives to nc_sock_connect() and to hand an endpoint out of nc_server_endpt_get(). All the callees only read, so make the types say so: nc_sock_connect() takes const keepalives (nc_sock_configure_ka() already did) and nc_server_endpt_get() returns a const endpoint. No cast is then left to let a future caller mutate a generation other threads are reading.
Explain why the published configuration pointer needs the rwlock and cannot be a plain atomic pointer: acquiring a generation is a pointer load followed by a refcount increment on what was loaded, and the read lock is what keeps an applier from publishing and freeing the old generation in between the two. Doing that lock-free would need hazard pointers, RCU or a double-width CAS, none of which the compatibility layer provides, and there is nothing to gain either. Also document why the idle timeout is mirrored into an atomic next to the generation, and that a missing generation simply means no module is ignored in the <hello>.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #627. Stacked on #630.
Readers no longer hold a lock while they use the configuration: a generation is refcounted and pinned with
nc_server_config_acquire(), and an apply publishes a new generation under a write lock held only for the pointer swap. An in-flight handshake can therefore no longer delay or lose a configuration update. Runtime state that must survive a change (listening sockets, Call Home thread handles) moves out of the configuration into registries, and the options settable only through the API get their own leaf lock.Note: conflicts mechanically with #629 in
nc_server_tls_get_num_certs().