Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 0 · Low: 7
- [Low] `struct psphdr { ... } __packed;` in
tools/testing/selftests/net/lib/gro.c uses the `__packed` macro, but…
- [Low] The `psp_ver_diff` sub-test claims to verify that 'different PSP
version doesn't coalesce', but it is implemented by the same…
- [Low] The file-level comment block of
tools/testing/selftests/net/lib/gro.c enumerates every test case and…
- [Low] Concern (validated as NOT a defect): gro.c gains `--psp-assoc`
and three psp_* sub-tests with no in-tree caller, making the feature…
- [Low] Concern (validated as NOT a defect): psp_encapsulate() hardcodes
IPv6 header length and does not walk extension headers or reject…
- [Low] Concern (validated as NOT a defect): psp_encapsulate() recomputes
the IPv4 header checksum from scratch, repairing the deliberately…
- [Low] Concern (validated as NOT a defect): each parsed PSP association
allocates an EVP_CIPHER_CTX that is never freed, on error paths and at…
--
selftests: net: gro: Add PSP encapsulation and encryption
This adds an optional PSP transport mode to the gro selftest helper: frames
are encapsulated and encrypted in software with OpenSSL before being sent,
and the receiving NIC is expected to decrypt and decapsulate them. Three
PSP-only sub-tests (psp_spi_diff, psp_ver_diff, psp_mixed) and a new
--psp-assoc option are added, with libcrypto detection scoped to the gro
binary in the Makefile.
A few questions below.
> diff --git a/tools/testing/selftests/net/lib/gro.c
> b/tools/testing/selftests/net/lib/gro.c
> index 7a333155de1a..fe4a4c0e17ae 100644
> --- a/tools/testing/selftests/net/lib/gro.c
> +++ b/tools/testing/selftests/net/lib/gro.c
[ ... ]
> @@ -110,6 +119,38 @@
>
> #define EXIT_OVER_COALESCE 42
>
> +/* PSP transport mode encapsulation, as built by the sender:
> + *
> + * [eth][IP][UDP dport=1000][PSP hdr][encrypted L4][ICV]
> + *
> + * Can't use the kernel-only include/net/psp/types.h header, so copy the
> + * encoding here.
> + */
> +struct psphdr {
> + uint8_t nexthdr;
> + uint8_t hdrlen;
> + uint8_t crypt_offset;
> + uint8_t verfl;
> + uint32_t spi; /* big endian */
> + uint64_t iv; /* big endian */
> +} __packed;
[Severity: Low]
Where does __packed come from in this translation unit?
The include path for this binary is only:
CFLAGS += -I../../../../../usr/include/ $(KHDR_INCLUDES)
CFLAGS += -I../../
scripts/headers_install.sh textually rewrites the macro when exporting uapi
headers:
s/(^|[^a-zA-Z0-9])__packed([^a-zA-Z0-9_]|$)/\1__attribute__((packed))\2/g
so the installed headers (including linux/psp.h) never define __packed, and
kselftest.h plus net/lib/ksft.h only pull in libc headers. In tools/ the
macro is defined by tools/include/linux/compiler.h and
tools/include/linux/compiler-gcc.h, neither of which is on this include
path.
Doesn't that make the declaration parse as a file-scope tentative definition
of an object named __packed of type struct psphdr, leaving the struct
unpacked?
The layouts happen to agree today (spi at offset 4, iv at offset 8,
sizeof == 16), so PSP_HDR_LEN and the offsetof() used for the nonce are
still right. But the unpacked struct carries 8-byte alignment while psph
lands at ETH_HLEN + 20 + 8 == 42 inside psp_scratch, i.e. 2 mod 8, so:
psph->spi = htonl(assoc->spi);
psph->iv = htobe64(psp_next_iv++);
are stores the compiler is entitled to treat as aligned. Would using
__attribute__((packed)) directly, or defining __packed locally as the sgx
and bpf selftests do, be preferable here?
> +
> +#define PSP_UDP_PORT 1000
> +#define PSP_UDP_LEN sizeof(struct udphdr)
> +#define PSP_HDR_LEN sizeof(struct psphdr)
[ ... ]
> @@ -1111,6 +1374,39 @@ static void send_changed_pppoe_sid(int fd, struct
> sockaddr_ll *daddr)
> write_packet(fd, buf, pkt_size, daddr);
> }
>
> +/* PSP packets don't coalesce across SPIs or versions. */
> +static void send_psp_assoc_switch(int fd, struct sockaddr_ll *daddr)
> +{
> + static char buf[MAX_HDR_LEN + PAYLOAD_LEN];
> + int pkt_size = total_hdr_len + PAYLOAD_LEN;
> +
> + if (psp_num_assoc < 2)
> + error(1, 0, "%s needs two PSP associations", testname);
> +
> + create_packet(buf, 0, 0, PAYLOAD_LEN, 0);
> + write_packet(fd, buf, pkt_size, daddr);
> +
> + create_packet(buf, PAYLOAD_LEN, 0, PAYLOAD_LEN, 0);
> + psp_idx = 1;
> + write_packet(fd, buf, pkt_size, daddr);
> + psp_idx = 0;
> +}
[Severity: Low]
Does psp_ver_diff actually exercise the version dimension?
Both psp_spi_diff and psp_ver_diff dispatch to send_psp_assoc_switch(),
which only flips psp_idx from association 0 to association 1. Since the two
RX associations always carry distinct device-allocated SPIs, the second
frame differs in SPI as well as version.
The kernel side compares the whole extension:
include/net/psp/functions.h:__psp_skb_coalesce_diff() {
...
diffs |= memcmp(a, b, sizeof(*a));
...
}
over struct psp_skb_ext { __be32 spi; u16 dev_id; u8 generation;
u8 version; }, so the differing SPI alone already forces the flush.
Wouldn't psp_ver_diff then keep passing even if the version field were
dropped from the software or hardware comparison, making it a duplicate of
psp_spi_diff? Since SPIs are allocated by the device and two associations
cannot share one, would it make sense to at least assert
psp_assocs[0].version != psp_assocs[1].version in the sender, or note in a
comment that this case mainly exercises multi-version decap?
> static void bind_packetsocket(int fd)
> {
> struct sockaddr_ll daddr = {};
[ ... ]
> @@ -1559,6 +1855,16 @@ static void gro_sender(void)
> usleep(fin_delay_us);
> write_packet(txfd, fin_pkt, total_hdr_len, &daddr);
>
> + /* PSP sub-tests */
> + } else if (strcmp(testname, "psp_spi_diff") == 0 ||
> + strcmp(testname, "psp_ver_diff") == 0) {
> + send_psp_assoc_switch(txfd, &daddr);
> + usleep(fin_delay_us);
> + write_packet(txfd, fin_pkt, total_hdr_len, &daddr);
[ ... ]
> @@ -1782,6 +2088,22 @@ static void gro_receiver(void)
> printf("different PPPoE session ID doesn't coalesce: ");
> check_recv_pkts(rxfd, correct_payload, 2);
>
> + /* PSP sub-tests */
> + } else if (strcmp(testname, "psp_spi_diff") == 0) {
> + correct_payload[0] = PAYLOAD_LEN;
> + correct_payload[1] = PAYLOAD_LEN;
> + printf("different PSP SPI doesn't coalesce: ");
> + check_recv_pkts(rxfd, correct_payload, 2);
> + } else if (strcmp(testname, "psp_ver_diff") == 0) {
> + correct_payload[0] = PAYLOAD_LEN;
> + correct_payload[1] = PAYLOAD_LEN;
> + printf("different PSP version doesn't coalesce: ");
> + check_recv_pkts(rxfd, correct_payload, 2);
[ ... ]
> @@ -1802,6 +2124,7 @@ static void parse_args(int argc, char **argv)
> { "ip6ip6", no_argument, NULL, 'E' },
> { "pppoev4", no_argument, NULL, 'p' },
> { "pppoev6", no_argument, NULL, 'P' },
> + { "psp-assoc", required_argument, NULL, OPT_PSP_ASSOC },
[Severity: Low]
Should the file-level comment block at the top of gro.c be extended too?
It enumerates every sub-test family (data_*, flags_*, tcp_*, ip_*, large_*,
single, capacity), but psp_spi_diff, psp_ver_diff, psp_mixed and the new
--psp-assoc version,spi,key option are not listed there, and the last patch
of the series does not touch gro.c either.
The requirement that both sides be invoked with the same --psp-assoc
arguments is also undocumented, even though the receiver's expected sizes
depend on it:
static int calc_mss(void)
{
return ASSUMED_MTU - (total_hdr_len - ETH_HLEN) -
(psp_enabled ? PSP_ENCAP_LEN : 0);
}
Could that coupling be spelled out next to the other test descriptions?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908065244.3799142-1-tariqt%40nvidia.com
