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 the parameterized GSO test gains "two
limits which cut the input into two and three output skbs".
- [Low] The new exact gso_segs checks cannot catch a broken gso_segs
value on the last re-segmented output.
- [Low] In gso_test_func(), the new `KUNIT_ASSERT_LT(test, i,
tcase->nr_segs);` inside the segment-freeing loop…
--
net: net_test: add tests for TCP re-segmentation
This commit adds KUnit coverage for the max_segs parameter of the GSO
engine. It adds three re-segmentation cases to the parameterized
skb_segment() test. It also adds TCP tests that drive __skb_gso_segment()
and validate_xmit_skb_list(), and a test that checks that
netif_skb_features() applies the GSO size limit of the packet's L3
protocol.
> The parameterized GSO test gains a max_segs input and three cases: two
> limits which cut the input into two and three output skbs, and a max_segs
> of one MSS, which must leave the output unchanged.
[Severity: Low]
Is "two limits" accurate here? GSO_TEST_RESEGMENT and
GSO_TEST_RESEGMENT_MULTI both use the same limit:
.max_segs = 2,
That limit is applied to a 3003 byte payload and to a 5003 byte payload.
The only other value tested is max_segs = 1 in GSO_TEST_RESEGMENT_ONE_MSS.
Could the message say that one limit is applied to two inputs?
> diff --git a/net/core/net_test.c b/net/core/net_test.c
> index 9c3a590865d26..6a91db83e5301 100644
> --- a/net/core/net_test.c
> +++ b/net/core/net_test.c
[ ... ]
> @@ -239,6 +297,7 @@ static void gso_test_func(struct kunit *test)
> for (cur = segs, i = 0; cur; cur = next, i++) {
> next = cur->next;
>
> + KUNIT_ASSERT_LT(test, i, tcase->nr_segs);
> KUNIT_ASSERT_EQ(test, cur->len, sizeof(hdr) + tcase->segs[i]);
[Severity: Low]
If skb_segment() returns more outputs than tcase->nr_segs, this assertion
aborts the test through kunit_try_catch_throw(). Does that leak cur and
every later skb on the ->next chain, along with their page frag
references? It would also skip freeing the input skb at free_gso_skb.
The v4 changelog says "free the segments when a bounded case does not
match". gso_test_tcp_resegment() does this: it uses EXPECT checks and
consumes every segment, but gso_test_func() still aborts here.
The other ASSERTs in this loop already abort the same way. This one does
stop the read past the end of tcase->segs[] on the next line.
Would an EXPECT here, with the per-index checks skipped once i reaches
tcase->nr_segs, keep both the bounds protection and the cleanup?
>
> /* segs have skb->data pointing to the mac header */
> @@ -247,6 +306,18 @@ static void gso_test_func(struct kunit *test)
>
> /* header was copied to all segs */
> KUNIT_ASSERT_EQ(test, memcmp(skb_mac_header(cur), hdr,
> sizeof(hdr)), 0);
> + if (tcase->segs_are_gso) {
> + KUNIT_EXPECT_TRUE(test, skb_is_gso(cur));
> + KUNIT_EXPECT_EQ(test, skb_shinfo(cur)->gso_size,
> + GSO_TEST_SIZE);
> + KUNIT_EXPECT_EQ(test, skb_shinfo(cur)->gso_segs,
> + DIV_ROUND_UP(tcase->segs[i],
> + GSO_TEST_SIZE));
[Severity: Low]
Can this check detect a wrong gso_segs on the last output? skb_segment()
first gives every output partial_segs and then fixes up the tail:
net/core/skbuff.c:skb_segment() {
...
for (iter = segs; iter; iter = iter->next) {
skb_shinfo(iter)->gso_size = gso_size;
skb_shinfo(iter)->gso_segs = partial_segs;
...
if (tail->len - doffset <= gso_size)
skb_shinfo(tail)->gso_size = 0;
else if (tail != segs)
skb_shinfo(tail)->gso_segs = DIV_ROUND_UP(tail->len -
doffset, gso_size);
...
}
GSO_TEST_RESEGMENT, GSO_TEST_RESEGMENT_MULTI and gso_test_tcp_resegment()
all use max_segs = 2 and end with a 1003 byte tail. DIV_ROUND_UP(1003,
1000) is 2, the same as partial_segs, so these checks would still pass if
the tail fix-up were dropped.
The one test whose tail count differs is gso_test_tcp_resegment_l3_len().
Its outputs are 65000, 65000, 65000 and 9800 bytes, so the tail should have
gso_segs of 10 rather than 65. That test does not check gso_segs.
The v4 changelog says "check the exact gso_segs of each output". Could one
of the vectors use a tail that gives a different count? For example,
max_segs = 3 with a 4003 byte payload should give gso_segs of 3 and 2.
> + KUNIT_EXPECT_FALSE(test, skb_shinfo(cur)->gso_type &
> + SKB_GSO_PARTIAL);
> + } else if (tcase->max_segs) {
> + KUNIT_EXPECT_FALSE(test, skb_is_gso(cur));
> + }
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930111526.2183107-1-wang.zhan%40smartx.com
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
