Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 0 (1) · Medium: 2 · Low: 3
- [Medium] The `run_pair nosample` run (offload features turned off) has
no rendezvous between the two helpers: both are expected to abort…
- [Medium] The payload verification for splice fragments generates
identical data for all fragments, which silently defeats the test's…
- [Low] tls.sh reuses the global, fixed network-namespace names
`nssv`/`nscl` and exactly the same random netdevsim device-ID ranges…
- [Low] Two assertions inspect only one of the two ports/namespaces even
though their messages imply both: (a) the `tls-hw-tx-offload`/`tls-hw-…
- [Low] The "per-port debugfs tls file exists" assertion is dead code: a
few lines earlier the script already does `if [ !
Pre-existing issues:
- [High] The new test is the first in-tree user of setsockopt(SOL_TLS,
TLS_TX_MAX_PAYLOAD_LEN) on a device-offloaded (TLS_HW) socket, and it…
--
selftests: netdevsim: add a kTLS device offload test
Adds tls.sh plus the tls_offload helper under
tools/testing/selftests/drivers/net/netdevsim/, running kTLS over a linked
netdevsim pair with one port per netns. The helper asserts via
/proc/net/tls_stat that both directions landed on the device path, then
exercises bulk transfers, MSG_MORE writes, splice() with TLS_TX_ZEROCOPY_RO
and a minimal record limit, plus the ethtool off/on path.
A few questions about the new test below.
> diff --git a/tools/testing/selftests/drivers/net/netdevsim/tls.sh
> b/tools/testing/selftests/drivers/net/netdevsim/tls.sh
> new file mode 100755
> index 0000000000000..59b7d1c0e5148
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/netdevsim/tls.sh
> @@ -0,0 +1,334 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Exercise netdevsim's emulated kTLS device offload over a linked
> +# netdevsim pair, one port per network namespace.
> +#
> +# shellcheck disable=SC2154 # ksft_skip comes from lib.sh
> +
> +lib_dir=$(dirname "$0")
> +# shellcheck source=./../../../net/lib.sh
> +# shellcheck disable=SC1091
> +source "$lib_dir"/../../../net/lib.sh
> +
> +# Device IDs and the nssv/nscl namespaces are picked the same way as peer.sh.
> +NSIM_DEV_1_ID=$((256 + RANDOM % 256))
> +NSIM_DEV_1_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_1_ID
> +NSIM_DEV_2_ID=$((512 + RANDOM % 256))
> +NSIM_DEV_2_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_2_ID
[ ... ]
> +# Each resource is handed to defer only once it exists, so a run that loses a
> +# race for one of the global names does not tear down the winner's.
> +setup()
> +{
> + set -e
> +
> + echo "$NSIM_DEV_1_ID" > "$NSIM_DEV_SYS_NEW"
> + defer nsim_dev_del "$NSIM_DEV_1_ID"
> + echo "$NSIM_DEV_2_ID" > "$NSIM_DEV_SYS_NEW"
> + defer nsim_dev_del "$NSIM_DEV_2_ID"
[ ... ]
> + ip netns add nssv
> + defer ip netns del nssv
> + ip netns add nscl
> + defer ip netns del nscl
[Severity: Low]
The fixed names nssv and nscl, and the device ID ranges $((256 + RANDOM %
256)) / $((512 + RANDOM % 256)), are exactly the ones peer.sh in the same
directory already uses.
What happens when a stale nssv or nscl namespace is left behind by an
aborted peer.sh or tls.sh run, or when peer.sh runs concurrently? setup()
runs under set -e, so ip netns add nssv (or the new_device write) failing
aborts the script mid-setup with a raw shell error and no PASS, FAIL or
SKIP line at all.
Would setup_ns() from the already sourced tools/testing/selftests/net/lib.sh
be preferable here, since it generates per-run unique namespace names?
[ ... ]
> +run_pair()
> +{
> + local sample="${1:-sample}"
> + local srv_rc cli_rc ready=1
[ ... ]
> + if [ "$sample" = "sample" ]; then
> + # Comfortably longer than the helpers take to get to the
> + # barrier. Timing out here means a sample would be stale, so
> + # fail the run rather than assert on it.
> + slowwait "$READY_TIMEOUT_SEC" both_ready >/dev/null || ready=0
> + if [ "$ready" -eq 1 ]; then
> + CONNS_1=$(dbg_field "$NSIM_DEV_1_TLS" count)
> + CONNS_2=$(dbg_field "$NSIM_DEV_2_TLS" count)
> + : "${CONNS_1:=0}"
> + : "${CONNS_2:=0}"
> + fi
> + fi
> + touch "$SYNCDIR/go"
[ ... ]
> +# The offload has to be advertised, and on by default like the other
> +# netdevsim crypto offloads.
> +for f in tls-hw-tx-offload tls-hw-rx-offload; do
> + [ "$(feature nssv "$NSIM_DEV_1_NAME" "$f")" = "on" ]
> + check "$f advertised and on by default" $?
> +done
[Severity: Low]
This loop only queries feature nssv "$NSIM_DEV_1_NAME", but the message says
the feature is advertised and on by default without qualifying which port.
Should the client port in nscl be checked too, the way set_features() does?
The same one-sided pattern shows up later for the current-context counters:
[ "$(tls_stat nssv TlsCurrTxDevice)" -eq 0 ] && \
[ "$(tls_stat nssv TlsCurrRxDevice)" -eq 0 ]
check "no device contexts left behind" $?
/proc/net/tls_stat is per netns, and enable_ktls() installs TX and RX
contexts on both ends, so would a context left behind on the client port in
nscl pass unnoticed here?
> +
> +[ -e "$NSIM_DEV_1_TLS" ]
> +check "per-port debugfs tls file exists" $?
[Severity: Low]
Is this assertion reachable in the failing state? A few lines earlier the
script already does:
if [ ! -e "$NSIM_DEV_1_TLS" ]; then
echo "SKIP: netdevsim built without TLS device offload
(CONFIG_TLS_DEVICE=n)"
exit "$ksft_skip"
fi
so by the time [ -e "$NSIM_DEV_1_TLS" ] runs it is always true, and the
check only adds to num_pass.
Meanwhile $NSIM_DEV_2_TLS is never checked for existence, although
all_ctx_released() and the tx_packets/rx_packets assertions parse it. If
that file is missing or a field name changes, dbg_field() returns an empty
string and [ "" -eq 0 ] / [ "" -ge 1 ] emit "integer expression expected"
rather than a clean FAIL. Should the existence check cover port 2 instead?
[ ... ]
> +[ "$(tls_stat nssv TlsCurrTxDevice)" -eq 0 ] && \
> + [ "$(tls_stat nssv TlsCurrRxDevice)" -eq 0 ]
> +check "no device contexts left behind" $?
[ ... ]
> +# Turning the features off has to make the offload refuse the connection;
> +# the test binary insists on the device path, so it must now fail.
> +set_features off
> +check "both offload features turned off on both ports" $?
> +
> +run_pair nosample
> +rc=$?
> +[ "$rc" -ne 0 ]
> +check "offload declined once the feature is off" $?
> +
> +# Both directions on both ends have to have landed on the software path.
> +[ "$(tls_stat nssv TlsTxSw)" -ge 1 ] && [ "$(tls_stat nssv TlsRxSw)" -ge 1 ]
> && \
> + [ "$(tls_stat nscl TlsTxSw)" -ge 1 ] && \
> + [ "$(tls_stat nscl TlsRxSw)" -ge 1 ]
> +check "software path used when offload is off" $?
[Severity: Medium]
Can this assertion fail intermittently? In nosample mode run_pair() skips
the slowwait/both_ready barrier entirely, and neither helper reaches
rendezvous() because enable_ktls() exits first:
tools/testing/selftests/drivers/net/netdevsim/tls_offload.c:enable_ktls() {
if (read_tls_stat("TlsTxDevice") != tx_before + 1)
fail("TX did not land on the device path (TlsTxDevice %lu ->
%lu)",
...
}
So whichever end aborts first closes its socket and sends a FIN. If the
peer processes that FIN before it gets to setsockopt(TCP_ULP, "tls"):
net/tls/tls_main.c:tls_init() {
...
if (sk->sk_state != TCP_ESTABLISHED)
return -ENOTCONN;
...
}
the peer dies in die("setsockopt(TCP_ULP, tls)") and never installs software
kTLS, so TlsTxSw/TlsRxSw in that namespace stay at 0 and this check fails.
Note the first run already asserted TlsTxSw == 0 in both namespaces, so
earlier traffic cannot cover for it.
"offload declined once the feature is off" still passes in that case since
it only requires a non-zero exit status, so the failure reason is hidden.
Would a rendezvous (or a retry) before the helpers give up make this run
deterministic?
> diff --git a/tools/testing/selftests/drivers/net/netdevsim/tls_offload.c
> b/tools/testing/selftests/drivers/net/netdevsim/tls_offload.c
> new file mode 100644
> index 0000000000000..0d7c8950ca6ce
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/netdevsim/tls_offload.c
> @@ -0,0 +1,505 @@
[ ... ]
> +/* A record can be up to 16K, so normally one segment carries a piece of a
> + * single record. Shrinking the limit puts a dozen or so whole records in
> + * every segment instead, which is the multi-record path through the driver.
> + */
> +static void send_small_records(int fd, unsigned int seed)
> +{
> + char buf[SMALL_LEN];
> + uint16_t limit;
> +
> + limit = REC_LIM_MIN;
> + if (setsockopt(fd, SOL_TLS, TLS_TX_MAX_PAYLOAD_LEN, &limit,
> + sizeof(limit)))
> + die("setsockopt(TLS_TX_MAX_PAYLOAD_LEN)");
[Severity: High]
This isn't a bug introduced by this patch, but it looks like the first
in-tree caller of TLS_TX_MAX_PAYLOAD_LEN on a socket that the test has just
asserted is device offloaded (tx_conf == TLS_HW), and the kernel handler
looks type confused:
net/tls/tls_main.c:do_tls_setsockopt_tx_payload_len() {
struct tls_sw_context_tx *sw_ctx = tls_sw_ctx_tx(ctx);
...
if (sw_ctx && sw_ctx->open_rec)
return -EBUSY;
...
}
include/net/tls.h:tls_sw_ctx_tx() is an unchecked cast:
return (struct tls_sw_context_tx *)tls_ctx->priv_ctx_tx;
and for an offloaded socket net/tls/tls_device.c:tls_set_device_offload()
stores a struct tls_offload_context_tx there:
ctx->priv_ctx_tx = offload_ctx;
There is no tx_conf == TLS_SW test, so the ->open_rec read lands inside the
offload context's sg_tx_data[MAX_SKB_FRAGS], which tls_push_record() fills
for every offloaded record:
net/tls/tls_device.c:tls_push_record() {
sg_unmark_end(&offload_ctx->sg_tx_data[i]);
sg_set_page(&offload_ctx->sg_tx_data[i], skb_frag_page(frag),
skb_frag_size(frag), skb_frag_off(frag));
...
}
Two things follow, and which one happens depends on the struct layout:
If the aliased bytes are non-zero (page_link/offset/length, e.g. once the
offsets shift with CONFIG_DEBUG_SPINLOCK or LOCKDEP changing the
scatterlist stride), setsockopt returns a spurious -EBUSY. That is fatal
for this helper, since the call above goes straight to
die("setsockopt(TLS_TX_MAX_PAYLOAD_LEN)") after the bulk, MSG_MORE and
splice traffic has already populated sg_tx_data.
If they are zero (default x86_64 layout, where the read lands on
sg_tx_data[n].dma_address, which tls_device.c never writes), the intended
guard never fires, because tls_offload_context_tx->open_record is never
consulted. The limit can then be shrunk while an offloaded record is open,
and the next send hits:
net/tls/tls_device.c:tls_push_data() {
...
copy = min_t(size_t, size, max_open_record_len - record->len);
...
}
with max_open_record_len < record->len, i.e. a u32 underflow that makes copy
== size and lets the record grow past the requested limit and past the 2^14
payload ceiling.
Should the guard check ctx->tx_conf == TLS_SW (and consult
tls_offload_ctx_tx(ctx)->open_record for the TLS_HW case) before the test
starts relying on this option on offloaded sockets?
> +
> + fill_pattern(buf, sizeof(buf), seed);
> + write_all(fd, buf, sizeof(buf));
[ ... ]
> @@ -0,0 +1,505 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * kTLS device offload data path exercise, driven by tls.sh.
> + *
> + * One instance runs as the server and one as the client, each in its own
> + * network namespace, connected back to back by a linked netdevsim pair.
> + * Both ends enable kTLS and rely on netdevsim's emulated TLS offload, so
> + * every record travels through net/tls/tls_device.c rather than the
> + * software path.
> + *
> + * The two processes rendezvous through a shared directory so that neither
> + * side sends before the other has installed its RX offload.
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <arpa/inet.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <netinet/in.h>
> +#include <netinet/tcp.h>
> +#include <stdarg.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/socket.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +#include <linux/tls.h>
> +
> +#ifndef SOL_TLS
> +#define SOL_TLS 282
> +#endif
> +
> +#ifndef TCP_ULP
> +#define TCP_ULP 31
> +#endif
> +
> +#define BULK_LEN (200 * 1024)
> +#define MORE_FRAGS 64
> +#define SPLICE_FRAG_LEN 4096
> +#define SPLICE_FRAGS 8
> +
> +/* TLS_MIN_RECORD_SIZE_LIM and TLS_MAX_PAYLOAD_SIZE, which are not uapi. */
> +#define REC_LIM_MIN 64
> +#define REC_LIM_MAX 16384
> +
> +#define SMALL_RECS 100
> +#define SMALL_LEN (REC_LIM_MIN * SMALL_RECS)
> +
> +#define SYNC_TIMEOUT_MS 20000
> +#define CONNECT_TIMEOUT_MS 20000
> +
> +/* accept() and the transfers have no timeout of their own, and tls.sh waits
> + * on both helpers. Bound the whole run so a stuck peer cannot hang the
> test.
> + */
> +#define RUN_TIMEOUT_SEC 120
> +
> +static const char *role;
> +
> +static void die(const char *what)
> +{
> + fprintf(stderr, "%s: %s: %s\n", role, what, strerror(errno));
> + exit(1);
> +}
> +
> +static void fail(const char *fmt, ...)
> +{
> + va_list ap;
> +
> + fprintf(stderr, "%s: ", role);
> + va_start(ap, fmt);
> + vfprintf(stderr, fmt, ap);
> + va_end(ap);
> + fprintf(stderr, "\n");
> + exit(1);
> +}
> +
> +static void msleep(unsigned int ms)
> +{
> + struct timespec ts = {
> + .tv_sec = ms / 1000,
> + .tv_nsec = (ms % 1000) * 1000000L,
> + };
> +
> + nanosleep(&ts, NULL);
> +}
> +
> +/* /proc/net/tls_stat is per netns, so both ends can check that their own
> + * connection really landed on the device path.
> + */
> +static unsigned long read_tls_stat(const char *name)
> +{
> + char line[256];
> + unsigned long val;
> + FILE *f;
> +
> + f = fopen("/proc/net/tls_stat", "r");
> + if (!f)
> + die("open /proc/net/tls_stat");
> +
> + while (fgets(line, sizeof(line), f)) {
> + char key[64];
> +
> + if (sscanf(line, "%63s %lu", key, &val) != 2)
> + continue;
> + if (!strcmp(key, name)) {
> + fclose(f);
> + return val;
> + }
> + }
> +
> + fclose(f);
> + fail("%s not found in /proc/net/tls_stat", name);
> + return 0;
> +}
> +
> +static void fill_pattern(char *buf, size_t len, unsigned int seed)
> +{
> + size_t i;
> +
> + for (i = 0; i < len; i++)
> + buf[i] = (char)(seed + i * 31 + (i >> 8) * 7);
> +}
> +
> +static void check_pattern(const char *buf, size_t len, unsigned int seed,
> + const char *what)
> +{
> + char *want = malloc(len);
> + size_t i;
> +
> + if (!want)
> + die("malloc");
> +
> + fill_pattern(want, len, seed);
> + for (i = 0; i < len; i++) {
> + if (buf[i] != want[i])
> + fail("%s: payload mismatch at byte %zu: got 0x%02x want
> 0x%02x",
> + what, i, (unsigned char)buf[i],
> + (unsigned char)want[i]);
> + }
> +
> + free(want);
> +}
> +
> +static void write_all(int fd, const char *buf, size_t len)
> +{
> + size_t done = 0;
> +
> + while (done < len) {
> + ssize_t n = send(fd, buf + done, len - done, 0);
> +
> + if (n < 0) {
> + if (errno == EINTR)
> + continue;
> + die("send");
> + }
> + done += n;
> + }
> +}
> +
> +static void read_all(int fd, char *buf, size_t len)
> +{
> + size_t done = 0;
> +
> + while (done < len) {
> + ssize_t n = recv(fd, buf + done, len - done, 0);
> +
> + if (n < 0) {
> + if (errno == EINTR)
> + continue;
> + die("recv");
> + }
> + if (n == 0)
> + fail("peer closed after %zu of %zu bytes", done, len);
> + done += n;
> + }
> +}
> +
> +static void enable_ktls(int fd)
> +{
> + struct tls12_crypto_info_aes_gcm_128 ci = {};
> + unsigned long tx_before, rx_before;
> +
> + tx_before = read_tls_stat("TlsTxDevice");
> + rx_before = read_tls_stat("TlsRxDevice");
> +
> + if (setsockopt(fd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls")))
> + die("setsockopt(TCP_ULP, tls)");
> +
> + ci.info.version = TLS_1_2_VERSION;
> + ci.info.cipher_type = TLS_CIPHER_AES_GCM_128;
> + memset(ci.iv, 'i', sizeof(ci.iv));
> + memset(ci.key, 'k', sizeof(ci.key));
> + memset(ci.salt, 's', sizeof(ci.salt));
> + memset(ci.rec_seq, 0, sizeof(ci.rec_seq));
> +
> + if (setsockopt(fd, SOL_TLS, TLS_TX, &ci, sizeof(ci)))
> + die("setsockopt(TLS_TX)");
> + if (setsockopt(fd, SOL_TLS, TLS_RX, &ci, sizeof(ci)))
> + die("setsockopt(TLS_RX)");
> +
> + /* The whole point of the exercise: refuse to silently fall back to
> + * the software path, otherwise the test would pass without ever
> + * touching tls_device.c.
> + */
> + if (read_tls_stat("TlsTxDevice") != tx_before + 1)
> + fail("TX did not land on the device path (TlsTxDevice %lu ->
> %lu)",
> + tx_before, read_tls_stat("TlsTxDevice"));
> + if (read_tls_stat("TlsRxDevice") != rx_before + 1)
> + fail("RX did not land on the device path (TlsRxDevice %lu ->
> %lu)",
> + rx_before, read_tls_stat("TlsRxDevice"));
> +}
> +
> +static void sync_path(char *out, size_t len, const char *dir, const char
> *who)
> +{
> + if ((size_t)snprintf(out, len, "%s/%s.ready", dir, who) >= len)
> + fail("sync dir path too long");
> +}
> +
> +static void rendezvous(const char *dir, const char *me, const char *peer)
> +{
> + char mine[PATH_MAX], theirs[PATH_MAX];
> + unsigned int waited = 0;
> + int fd;
> +
> + sync_path(mine, sizeof(mine), dir, me);
> + sync_path(theirs, sizeof(theirs), dir, peer);
> +
> + fd = open(mine, O_CREAT | O_WRONLY, 0600);
> + if (fd < 0)
> + die("create sync file");
> + close(fd);
> +
> + while (access(theirs, F_OK)) {
> + if (waited >= SYNC_TIMEOUT_MS)
> + fail("timed out waiting for %s", peer);
> + msleep(20);
> + waited += 20;
> + }
> +}
> +
> +/* Both ends stop here with their offload installed and no data sent yet,
> + * so that the driver state can be inspected from the outside.
> + */
> +static void wait_for_go(const char *dir)
> +{
> + unsigned int waited = 0;
> + char go[PATH_MAX];
> +
> + if ((size_t)snprintf(go, sizeof(go), "%s/go", dir) >= sizeof(go))
> + fail("sync dir path too long");
> +
> + while (access(go, F_OK)) {
> + if (waited >= SYNC_TIMEOUT_MS)
> + fail("timed out waiting for go");
> + msleep(20);
> + waited += 20;
> + }
> +}
> +
> +/* Small writes with MSG_MORE accumulate into one open record before it is
> + * pushed, which is the interesting part of tls_push_data().
> + */
> +static void send_msg_more(int fd, unsigned int seed)
> +{
> + char buf[MORE_FRAGS + 1];
> + int i;
> +
> + fill_pattern(buf, sizeof(buf), seed);
> +
> + for (i = 0; i < MORE_FRAGS; i++) {
> + if (send(fd, buf + i, 1, MSG_MORE) != 1)
> + die("send(MSG_MORE)");
> + }
> + if (send(fd, buf + MORE_FRAGS, 1, 0) != 1)
> + die("send(last)");
> +}
> +
> +/* splice() reaches tls_push_data() with MSG_SPLICE_PAGES once
> + * TLS_TX_ZEROCOPY_RO is enabled, which is a distinct fragment path.
> + */
> +static void send_splice(int fd, unsigned int seed)
> +{
> + char buf[SPLICE_FRAG_LEN];
> + int val = 1;
> + int i;
> +
> + if (setsockopt(fd, SOL_TLS, TLS_TX_ZEROCOPY_RO, &val, sizeof(val)))
> + die("setsockopt(TLS_TX_ZEROCOPY_RO)");
> +
> + for (i = 0; i < SPLICE_FRAGS; i++) {
> + int p[2];
> +
> + fill_pattern(buf, sizeof(buf), seed + i * SPLICE_FRAG_LEN);
> +
> + if (pipe(p))
> + die("pipe");
> + if (write(p[1], buf, sizeof(buf)) != sizeof(buf))
> + die("write to pipe");
> + if (splice(p[0], NULL, fd, NULL, sizeof(buf),
> + i == SPLICE_FRAGS - 1 ? 0 : SPLICE_F_MORE) !=
> + sizeof(buf))
> + die("splice");
> + close(p[0]);
> + close(p[1]);
> + }
> +
> + val = 0;
> + if (setsockopt(fd, SOL_TLS, TLS_TX_ZEROCOPY_RO, &val, sizeof(val)))
> + die("setsockopt(TLS_TX_ZEROCOPY_RO off)");
> +}
> +
> +/* A record can be up to 16K, so normally one segment carries a piece of a
> + * single record. Shrinking the limit puts a dozen or so whole records in
> + * every segment instead, which is the multi-record path through the driver.
> + */
> +static void send_small_records(int fd, unsigned int seed)
> +{
> + char buf[SMALL_LEN];
> + uint16_t limit;
> +
> + limit = REC_LIM_MIN;
> + if (setsockopt(fd, SOL_TLS, TLS_TX_MAX_PAYLOAD_LEN, &limit,
> + sizeof(limit)))
> + die("setsockopt(TLS_TX_MAX_PAYLOAD_LEN)");
> +
> + fill_pattern(buf, sizeof(buf), seed);
> + write_all(fd, buf, sizeof(buf));
> +
> + limit = REC_LIM_MAX;
> + if (setsockopt(fd, SOL_TLS, TLS_TX_MAX_PAYLOAD_LEN, &limit,
> + sizeof(limit)))
> + die("setsockopt(TLS_TX_MAX_PAYLOAD_LEN restore)");
> +}
> +
> +#define SEED_C2S_BULK 0x11
> +#define SEED_S2C_BULK 0x22
> +#define SEED_C2S_MORE 0x33
> +#define SEED_C2S_SPLICE 0x44
> +#define SEED_C2S_SMALL 0x55
> +
> +static void run_client(int fd)
> +{
> + char *buf = malloc(BULK_LEN);
> +
> + if (!buf)
> + die("malloc");
> +
> + fill_pattern(buf, BULK_LEN, SEED_C2S_BULK);
> + write_all(fd, buf, BULK_LEN);
> +
> + read_all(fd, buf, BULK_LEN);
> + check_pattern(buf, BULK_LEN, SEED_S2C_BULK, "server -> client bulk");
> +
> + send_msg_more(fd, SEED_C2S_MORE);
> + send_splice(fd, SEED_C2S_SPLICE);
> + send_small_records(fd, SEED_C2S_SMALL);
> +
> + /* Wait for the server's verdict before tearing anything down. */
> + read_all(fd, buf, 1);
> + if (buf[0] != 'k')
> + fail("server reported a failure");
> +
> + free(buf);
> +}
> +
> +static void run_server(int fd)
> +{
> + size_t splice_len = (size_t)SPLICE_FRAG_LEN * SPLICE_FRAGS;
> + char *buf = malloc(BULK_LEN);
> + char more[MORE_FRAGS + 1];
> + char *sbuf;
> + char ok = 'k';
> + int i;
> +
> + sbuf = malloc(splice_len);
> + if (!buf || !sbuf)
> + die("malloc");
> +
> + read_all(fd, buf, BULK_LEN);
> + check_pattern(buf, BULK_LEN, SEED_C2S_BULK, "client -> server bulk");
> +
> + fill_pattern(buf, BULK_LEN, SEED_S2C_BULK);
> + write_all(fd, buf, BULK_LEN);
> +
> + read_all(fd, more, sizeof(more));
> + check_pattern(more, sizeof(more), SEED_C2S_MORE, "client -> server
> MSG_MORE");
> +
> + read_all(fd, sbuf, splice_len);
> + for (i = 0; i < SPLICE_FRAGS; i++)
> + check_pattern(sbuf + (size_t)i * SPLICE_FRAG_LEN,
> + SPLICE_FRAG_LEN, SEED_C2S_SPLICE +
> + i * SPLICE_FRAG_LEN, "client -> server splice");
> +
> + read_all(fd, buf, SMALL_LEN);
> + check_pattern(buf, SMALL_LEN, SEED_C2S_SMALL,
> + "client -> server small records");
> +
> + write_all(fd, &ok, 1);
> +
> + free(sbuf);
> + free(buf);
> +}
> +
> +static int do_server(const char *ip, int port, const char *syncdir)
> +{
> + struct sockaddr_in sa = {};
> + int lfd, fd, one = 1;
> +
> + lfd = socket(AF_INET, SOCK_STREAM, 0);
> + if (lfd < 0)
> + die("socket");
> + if (setsockopt(lfd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one)))
> + die("SO_REUSEADDR");
> +
> + sa.sin_family = AF_INET;
> + sa.sin_port = htons(port);
> + if (inet_pton(AF_INET, ip, &sa.sin_addr) != 1)
> + fail("bad bind address %s", ip);
> +
> + if (bind(lfd, (struct sockaddr *)&sa, sizeof(sa)))
> + die("bind");
> + if (listen(lfd, 1))
> + die("listen");
> +
> + fd = accept(lfd, NULL, NULL);
> + if (fd < 0)
> + die("accept");
> + close(lfd);
> +
> + enable_ktls(fd);
> + rendezvous(syncdir, "server", "client");
> + wait_for_go(syncdir);
> +
> + run_server(fd);
> +
> + close(fd);
> + return 0;
> +}
> +
> +static int do_client(const char *ip, int port, const char *syncdir)
> +{
> + struct sockaddr_in sa = {};
> + unsigned int waited = 0;
> + int fd;
> +
> + sa.sin_family = AF_INET;
> + sa.sin_port = htons(port);
> + if (inet_pton(AF_INET, ip, &sa.sin_addr) != 1)
> + fail("bad server address %s", ip);
> +
> + for (;;) {
> + fd = socket(AF_INET, SOCK_STREAM, 0);
> + if (fd < 0)
> + die("socket");
> + if (!connect(fd, (struct sockaddr *)&sa, sizeof(sa)))
> + break;
> + close(fd);
> + if (waited >= CONNECT_TIMEOUT_MS)
> + die("connect");
> + msleep(20);
> + waited += 20;
> + }
> +
> + enable_ktls(fd);
> + rendezvous(syncdir, "client", "server");
> + wait_for_go(syncdir);
> +
> + run_client(fd);
> +
> + close(fd);
> + return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int port;
> +
> + if (argc != 5) {
> + fprintf(stderr,
> + "usage: %s server|client <ip> <port> <syncdir>\n",
> + argv[0]);
> + return 2;
> + }
> +
> + role = argv[1];
> + port = atoi(argv[3]);
> +
> + alarm(RUN_TIMEOUT_SEC);
> +
> + if (!strcmp(role, "server"))
> + return do_server(argv[2], port, argv[4]);
> + if (!strcmp(role, "client"))
> + return do_client(argv[2], port, argv[4]);
> +
> + fprintf(stderr, "unknown role %s\n", role);
> + return 2;
> +}
[Severity: Medium]
Does the per-fragment seed here actually produce distinct payloads?
fill_pattern() truncates every byte to 8 bits:
buf[i] = (char)(seed + i * 31 + (i >> 8) * 7);
and SPLICE_FRAG_LEN is 4096, a multiple of 256, so seed + i *
SPLICE_FRAG_LEN only ever differs from seed in bits 8 and above. All
SPLICE_FRAGS fragments therefore get byte-for-byte identical contents,
and the matching loop in run_server()
check_pattern(sbuf + (size_t)i * SPLICE_FRAG_LEN,
SPLICE_FRAG_LEN, SEED_C2S_SPLICE +
i * SPLICE_FRAG_LEN, "client -> server splice");
collapses to the same expected buffer for each i.
That is not a correctness problem for the transfer itself, but it does
remove most of what the splice case is supposed to prove: with every
fragment identical, reordering, duplication or dropping-and-refilling a
fragment inside the TLS_TX_ZEROCOPY_RO / MSG_SPLICE_PAGES path still
compares equal and the test passes. Given that the whole point of this
hunk is to exercise a distinct fragment path in tls_push_data(), it
seems worth having the payload distinguish the fragments.
Would a seed step that survives the truncation be preferable here, e.g.
seed + i rather than seed + i * SPLICE_FRAG_LEN (with the same change on
the server side), or should fill_pattern() fold the seed's high bits into
the byte value so that any seed spacing works? Note the other cases are
unaffected, since SEED_C2S_BULK/SEED_S2C_BULK/SEED_C2S_MORE/SEED_C2S_SMALL
all differ in the low 8 bits.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901154914.11637-1-jiayuan.chen%40linux.dev
