Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message says "Here we are adding some more error
paths, so we just handle errors by closing the data sock".
--
selftests: drv-net: psp: factor out psp connection setup
This commit moves the PSP connection setup and the assoc uapi calls in
psp.py and psp_responder.c into helpers. Other tests that start with a key
exchange can then reuse that sequence. It also makes run_session() close
the data socket when conn_setup_psp() fails.
The caller of conn_setup_psp() not handling errors was preexisting. Here
we are adding some more error paths, so we just handle errors by closing
the data sock to make the code a bit more robust.
[Severity: Low]
This isn't a bug, but is the "adding some more error paths" part still
accurate for v3?
Before this patch, conn_setup_psp() returned -1 in three cases:
psp_rx_assoc() returning NULL, recv() returning < 0, and psp_tx_assoc()
returning NULL.
After the patch, it fails in the same three cases:
rx_assoc() returns -1 when rsp is NULL
conn_setup_psp() returns -1 when recv() returns < 0
tx_assoc() returns -1 when tsp is NULL
The v3 changelog says the per-version key length table was replaced with
the key length from the kernel's rx-assoc reply. That table lookup looks
like the only new way this function could fail, so the sentence may be
left over from v1/v2.
The only behaviour change is in run_session():
if (conn_setup_psp(ys, opts, data_sock)) {
close(data_sock);
data_sock = -1;
}
Could the commit message give the actual reason for that change? For
example, the existing failures can leave a half set up data socket open.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-psp-v3-0-a8dd38f5a727%40gmail.com