Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 3 · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The test sends its fragment in a way that can never trigger the
bug it is meant to catch, so it passes with or without commit…
- [Medium] test_ipv6_later_frag looks for the regression with `grep -c
'WARNING:.*set_ipv6_addr'`.
- [Medium] test_ipv6_later_frag never checks that the running kernel has
CONFIG_DEBUG_NET, even though the patch adds CONFIG_DEBUG_NET=y to the…
- [Medium] The new test's packet-counter check (ovs_flow_has_packets)
runs `ovs-dpctl.py dump-flows` on a flow whose action is a SET, but…
- [Low] The commit message says the test 'covers the later-fragment case
handled by the upstream fix' but does not name it: commit 3c6a4b11330f…
- [Low] The fragment adds CONFIG_DEBUG_NET=y but not its prerequisite
CONFIG_DEBUG_KERNEL=y.
--
selftests: openvswitch: Test SET on later IPv6 fragments
This adds an openvswitch selftest that sends a crafted noninitial IPv6
fragment through a flow with an IPv6 source-address SET action. The test
checks that the flow counter advances and that dmesg shows no
set_ipv6_addr warning. It also adds CONFIG_DEBUG_NET=y to the openvswitch
config fragment.
> This covers the later-fragment case handled by the upstream fix.
[Severity: Low]
Could this name the fix? It appears to be commit 3c6a4b11330f ("net:
openvswitch: validate transport header presence in set_ipv6_addr").
The commit message also doesn't say that the patch adds CONFIG_DEBUG_NET=y
to tools/testing/selftests/net/openvswitch/config. That change applies to
every OVS selftest built from the fragment. It turns on all
DEBUG_NET_WARN_ON_ONCE() checks and turns off the transport header reset
in __netif_receive_skb_core().
Should that be mentioned too?
> diff --git a/tools/testing/selftests/net/openvswitch/config
> b/tools/testing/selftests/net/openvswitch/config
> index a825e0b5c88e5..b062ebbce0f00 100644
> --- a/tools/testing/selftests/net/openvswitch/config
> +++ b/tools/testing/selftests/net/openvswitch/config
> @@ -3,6 +3,7 @@ CONFIG_INET_DIAG=y
> CONFIG_IP_SCTP=y
> CONFIG_IPV6=y
> CONFIG_NETFILTER=y
> +CONFIG_DEBUG_NET=y
[Severity: Low]
Does this fragment also need CONFIG_DEBUG_KERNEL=y? From net/Kconfig.debug:
config DEBUG_NET
bool "Add generic networking debug"
depends on DEBUG_KERNEL && NET
DEBUG_KERNEL has no default. If this fragment is merged into a base config
that lacks DEBUG_KERNEL, DEBUG_NET is silently dropped and the warning this
test looks for can't fire.
x86_64_defconfig and i386_defconfig already set CONFIG_DEBUG_KERNEL=y, so
the common case isn't affected.
[ ... ]
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index a31f7fb6882dc..b9a4c4bf40672 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
[ ... ]
> @@ -66,6 +67,11 @@ ovs_wait() {
> return 1
> }
>
> +ovs_flow_has_packets() {
> + python3 "$ovs_base/ovs-dpctl.py" dump-flows "$1" |
> + grep -Eq 'packets:[1-9][0-9]*,'
> +}
[Severity: Medium]
Can dump-flows format the SET action that this test installs? The SET
branch of ovsactions.dpstr() in ovs-dpctl.py does this:
field = datum
mask = None
if field[0] == "OVS_ACTION_ATTR_SET_MASKED":
Here datum is the decoded ovskey nla returned by
self.get_attr("OVS_ACTION_ATTR_SET"). So field[0] indexes an nla object
with the integer 0 instead of checking the action name. This branch is not
inside the try/except that covers the other actions.
ovsflow.dpstr() puts the "packets:N," text in the same string as the
actions, and dump-flows prints nothing until that whole string is built.
If indexing the pyroute2 nla with 0 raises KeyError, dump-flows exits with
a traceback and prints nothing. Then ovs_flow_has_packets never matches,
ovs_wait times out, and the test fails even on a fixed kernel.
Whether it raises depends on how pyroute2's nlmsg_base implements
__getitem__. The formatter itself isn't new. However, no existing test
dumps a flow with a SET action, so this test seems to be the first to
depend on it.
> @@ -445,6 +451,43 @@ test_action_set() {
> return 0
> }
>
> +test_ipv6_later_frag() {
> + local t="test_ipv6_later_frag"
> + local warning="WARNING:.*set_ipv6_addr"
[Severity: Medium]
Will this pattern match the warning on every build? __warn() in
kernel/panic.c prints the headline like this:
pr_warn("WARNING: %s:%d at %pS, CPU#%d: %s/%d\n",
file, line, caller,
For the DEBUG_NET_WARN_ON_ONCE() in skb_transport_header(), %s:%d is the
include/linux/skbuff.h location. %pS names the out-of-line function that
contains the WARN. The [inline] frames in the syzbot trace come from
offline symbolization and don't appear in raw dmesg.
set_ipv6_addr() is a static helper with two call sites, both in
set_ipv6(). In the syzbot build, set_ipv6() was already inlined into
do_execute_actions(). If the compiler also inlines set_ipv6_addr(), the
headline names do_execute_actions or set_ipv6 instead. The test would then
pass even though the warning fired.
Would it be more reliable to match on the skbuff.h WARN location or on the
skb_transport_header_was_set() condition?
> + local before after
> +
> + if ! dmesg >/dev/null 2>&1; then
> + info "dmesg unavailable - cannot check for kernel warning"
> + return $ksft_skip
> + fi
[Severity: Medium]
Should the test also skip when the running kernel doesn't have
CONFIG_DEBUG_NET? Without it, include/net/net_debug.h has:
#define DEBUG_NET_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
and __netif_receive_skb_core() also resets an unset transport header:
#if !defined(CONFIG_DEBUG_NET)
if (!skb_transport_header_was_set(skb))
skb_reset_transport_header(skb);
#endif
The test only skips when dmesg can't be read or when a matching warning is
already present. On distro or CI kernels not built from the config
fragment, it returns 0 once the flow counter advances instead of returning
$ksft_skip.
[ ... ]
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-ipv6-frag.py
> b/tools/testing/selftests/net/openvswitch/ovs-ipv6-frag.py
> new file mode 100644
> index 0000000000000..869f9d6503ef4
> --- /dev/null
> +++ b/tools/testing/selftests/net/openvswitch/ovs-ipv6-frag.py
[ ... ]
> + packet = ethernet + ipv6 + fragment + payload
> +
> + with socket.socket(
> + socket.AF_PACKET, socket.SOCK_RAW, socket.htons(0x0003)
> + ) as sock:
> + sock.bind((interface, 0))
> + sent = sock.send(packet)
[Severity: High]
Can a fragment sent this way trigger the warning at all? It looks like the
test passes both with and without commit 3c6a4b11330f.
The socket is bound with ETH_P_ALL, so packet_snd() leaves skb->protocol at
0. Then this happens:
packet_snd()
packet_parse_headers()
skb->protocol = dev_parse_header_protocol(skb); /* ETH_P_IPV6 */
skb_probe_transport_header()
__skb_flow_dissect()
/* NEXTHDR_FRAGMENT, non-first fragment */
fdret = FLOW_DISSECT_RET_OUT_GOOD;
skb_set_transport_header(skb, keys.control.thoff);
Nothing on the way from veth to OVS clears it. __dev_forward_skb2(),
skb_scrub_packet() and __netif_receive_skb_core() all leave a set
transport header in place.
parse_ipv6hdr() in net/openvswitch/flow.c also returns early for a later
fragment without touching it:
if (frag_off) {
key->ip.frag = OVS_FRAG_TYPE_LATER;
key->ip.proto = NEXTHDR_FRAGMENT;
return 0;
}
So on an unfixed kernel, set_ipv6()->set_ipv6_addr()->
update_ipv6_checksum() calls skb_transport_offset() while the transport
header is already set. The check in skb_transport_header() doesn't fire:
DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
As a result, after equals before and test_ipv6_later_frag() returns 0.
The syzbot trace quoted in 3c6a4b11330f goes through
ovs_packet_cmd_execute(). There the skb is built from netlink with no
transport header. Does the test need to inject the packet through
OVS_PACKET_CMD_EXECUTE to cover this regression?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009090629.1694991-1-sahaj123.sc%40gmail.com
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
