Skip to content

snapct: switch to tcp-based ping #8657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ptaffet-jump wants to merge 1 commit into
base: main
Choose a base branch
from
+284 −200
Open

snapct: switch to tcp-based ping #8657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions src/discof/restore/fd_snapct_tile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,9 +351,10 @@ static ulong
rlimit_file_cnt( fd_topo_t const * topo FD_PARAM_UNUSED,
fd_topo_tile_t const * tile ) {
ulong cnt = 1UL + /* stderr */
1UL; /* logfile */
1UL + /* logfile */
1UL;
Comment on lines +354 to +355
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rlimit_file_cnt() now adds an extra bare + 1UL with no comment or corresponding fd usage. This looks like an accidental overcount and makes it harder to audit the fd budget (especially now that FD_SSPING_FD_CNT is significant). Either remove this term or document exactly which file descriptor it is intended to account for.

Suggested change
1UL + /* logfile */
1UL;
1UL; /* logfile */

Copilot uses AI. Check for mistakes.
if( download_enabled( tile ) ) {
cnt += 1UL + /* ssping socket */
cnt += FD_SSPING_FD_CNT + /* ssping socket */
2UL + /* dirfd + full snapshot download temp fd */
tile->snapct.sources.servers_cnt; /* http resolver peer full sockets */
if( tile->snapct.incremental_snapshots ) {
Expand All @@ -375,8 +376,18 @@ populate_allowed_seccomp( fd_topo_t const * topo,
FD_SCRATCH_ALLOC_INIT( l, scratch );
fd_snapct_tile_t * ctx = FD_SCRATCH_ALLOC_APPEND( l, alignof(fd_snapct_tile_t), sizeof(fd_snapct_tile_t) );

int ping_fd = download_enabled( tile ) ? fd_ssping_get_sockfd( ctx->ssping ) : -1;
populate_sock_filter_policy_fd_snapct_tile( out_cnt, out, (uint)fd_log_private_logfile_fd(), (uint)ctx->local_out.dir_fd, (uint)ctx->local_out.full_snapshot_fd, (uint)ctx->local_out.incremental_snapshot_fd, (uint)ping_fd );
int min_ping_fd = INT_MAX;
int max_ping_fd = -1;
int ping_fds[ 256 ];
if( download_enabled( tile ) ) {
ulong fd_cnt = fd_ssping_get_sockfds( ctx->ssping, ping_fds, 256UL );
for( ulong i=0UL; i<fd_cnt; i++ ) {
min_ping_fd = fd_int_min( min_ping_fd, ping_fds[ i ] );
max_ping_fd = fd_int_max( max_ping_fd, ping_fds[ i ] );
}
}

populate_sock_filter_policy_fd_snapct_tile( out_cnt, out, (uint)fd_log_private_logfile_fd(), (uint)ctx->local_out.dir_fd, (uint)ctx->local_out.full_snapshot_fd, (uint)ctx->local_out.incremental_snapshot_fd, (uint)min_ping_fd, (uint)max_ping_fd );
return sock_filter_policy_fd_snapct_tile_instr_cnt;
}

Expand All @@ -385,7 +396,7 @@ populate_allowed_fds( fd_topo_t const * topo,
fd_topo_tile_t const * tile,
ulong out_fds_cnt,
int * out_fds ) {
if( FD_UNLIKELY( out_fds_cnt<6UL ) ) FD_LOG_ERR(( "out_fds_cnt %lu", out_fds_cnt ));
if( FD_UNLIKELY( out_fds_cnt<255UL ) ) FD_LOG_ERR(( "out_fds_cnt %lu", out_fds_cnt ));
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate_allowed_fds() hard-codes a minimum capacity check of 255. This is tightly coupled to the current FD_SSPING_FD_CNT (249) and the current list of always/optionally included fds, and it will silently become wrong if any of those change. Consider expressing this in terms of FD_SSPING_FD_CNT plus the maximum number of non-ssping fds that can be appended in this function (and accounting for the optional logfile fd) so the bound stays correct as the code evolves.

Copilot uses AI. Check for mistakes.

ulong out_cnt = 0;
out_fds[ out_cnt++ ] = 2UL; /* stderr */
Expand All @@ -400,7 +411,7 @@ populate_allowed_fds( fd_topo_t const * topo,
if( FD_LIKELY( -1!=ctx->local_out.dir_fd ) ) out_fds[ out_cnt++ ] = ctx->local_out.dir_fd;
if( FD_LIKELY( -1!=ctx->local_out.full_snapshot_fd ) ) out_fds[ out_cnt++ ] = ctx->local_out.full_snapshot_fd;
if( FD_LIKELY( -1!=ctx->local_out.incremental_snapshot_fd ) ) out_fds[ out_cnt++ ] = ctx->local_out.incremental_snapshot_fd;
if( FD_LIKELY( download_enabled( tile ) ) ) out_fds[ out_cnt++ ] = fd_ssping_get_sockfd( ctx->ssping );
if( FD_LIKELY( download_enabled( tile ) ) ) out_cnt += fd_ssping_get_sockfds( ctx->ssping, out_fds+out_cnt, out_fds_cnt-out_cnt );

return out_cnt;
}
Expand Down
34 changes: 20 additions & 14 deletions src/discof/restore/fd_snapct_tile.seccomppolicy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# logfile_fd: It can be disabled by configuration, but typically tiles
# will open a log file on boot and write all messages there.
uint logfile_fd, uint dir_fd, uint out_full_fd, uint out_inc_fd, uint ping_fd
uint logfile_fd, uint dir_fd, uint out_full_fd, uint out_inc_fd, uint min_ping_fd, uint max_ping_fd

# logging: all log messages are written to a file and/or pipe
#
Expand Down Expand Up @@ -31,13 +31,14 @@ lseek: (or (eq (arg 0) out_full_fd)
# OpenSSL calls read to receive bytes from a socket. We will restrict
# this being called on the stderr, the log file descriptor, the snapshot
# directory, the full and incremental snapshot files, and the ping file
# descriptor.
# descriptors.
read: (not (or (eq (arg 0) 2)
(eq (arg 0) logfile_fd)
(eq (arg 0) dir_fd)
(eq (arg 0) out_full_fd)
(eq (arg 0) out_inc_fd)
(eq (arg 0) ping_fd)))
(and (>= (arg 0) min_ping_fd)
(<= (arg 0) max_ping_fd))))

# logging: 'WARNING' and above fsync the logfile to disk immediately
#
Expand All @@ -51,7 +52,8 @@ socket: (and (eq (arg 0) "AF_INET")
(eq (arg 2) 0))

# snapshot: need to be able to connect to a peer to download a snapshot
# from and for snapshot slot resolution.
# from and for snapshot slot resolution. We also call connect on all
# the ping file descriptors.
#
# arg 0 is the file descriptor of the socket to connect to. We will
# restrict this being called on any of the snapshot file descriptors,
Expand All @@ -60,8 +62,7 @@ connect: (not (or (eq (arg 0) 2)
(eq (arg 0) logfile_fd)
(eq (arg 0) dir_fd)
(eq (arg 0) out_full_fd)
(eq (arg 0) out_inc_fd)
(eq (arg 0) ping_fd)))
(eq (arg 0) out_inc_fd)))

# snapshot: need to close sockets that were opened when calls to connect
# fail.
Expand All @@ -74,28 +75,31 @@ close: (not (or (eq (arg 0) 2)
(eq (arg 0) dir_fd)
(eq (arg 0) out_full_fd)
(eq (arg 0) out_inc_fd)
(eq (arg 0) ping_fd)))
(and (>= (arg 0) min_ping_fd)
(<= (arg 0) max_ping_fd))))

# snapshot: we need to be able to poll existing connections that we want
# to download snapshots and resolve snapshots from
ppoll

# snapshot: we need to be able to poll the ping connections to see if
# any peers have responded
poll

# snapshot: send HTTP requests to endpoints for snapshot downloading.
# OpenSSL also uses sendto to write to the socket during the lifetime
# of an HTTPS connection.
#
# arg 0 is the file descriptor that we would like to send a request to.
# We will restrict this being called on any of the snapshot file
# descriptors, STDERR or the logfile.
# descriptors, ping file descriptors, STDERR or the logfile.
sendto: (not (or (eq (arg 0) 2)
(eq (arg 0) logfile_fd)
(eq (arg 0) dir_fd)
(eq (arg 0) out_full_fd)
(eq (arg 0) out_inc_fd)
(eq (arg 0) ping_fd)))

sendmmsg: (and (eq (arg 0) ping_fd)
(eq (arg 3) 0))
(and (>= (arg 0) min_ping_fd)
(<= (arg 0) max_ping_fd))))

# snapshot: we need to be able to receive responses for any requests
# that were sent out for snapshot download or slot resolution.
Expand All @@ -115,13 +119,15 @@ recvfrom: (not (or (eq (arg 0) 2)
#
# arg 0 is the file descriptor that corresponds to the socket that we
# want to connect to. We know that we don't want to connect to the
# file descriptors used for logging, stderr, or any of the snapshot fds.
# file descriptors used for logging, stderr, ping or any of the snapshot
# fds. The ping file descriptors set this option in privileged_init.
setsockopt: (and (not (or (eq (arg 0) 2)
(eq (arg 0) logfile_fd)
(eq (arg 0) dir_fd)
(eq (arg 0) out_full_fd)
(eq (arg 0) out_inc_fd)
(eq (arg 0) ping_fd)))
(and (>= (arg 0) min_ping_fd)
(<= (arg 0) max_ping_fd))))
(eq (arg 1) IPPROTO_TCP)
(eq (arg 2) TCP_NODELAY))

Expand Down
Loading
Loading