[net] net: gso_test: support CONFIG_MAX_SKB_FRAGS up to 45

Commit Message

Willem de Bruijn Nov. 10, 2023, 3:36 p.m. UTC

From: Willem de Bruijn <[e-mail]>

The test allocs a single page to hold all the frag_list skbs. This
is insufficient on kernels with CONFIG_MAX_SKB_FRAGS=45, due to the
increased skb_shared_info frags[] array length.

        gso_test_func: ASSERTION FAILED at net/core/gso_test.c:210
        Expected alloc_size <= ((1UL) << 12), but
            alloc_size == 5075 (0x13d3)
            ((1UL) << 12) == 4096 (0x1000)

Simplify the logic. Just allocate a page for each frag_list skb.

Fixes: 4688ecb1385f ("net: expand skb_segment unit test with frag_list coverage")
Signed-off-by: Willem de Bruijn <[e-mail]>
---
 net/core/gso_test.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

Simon Horman Nov. 12, 2023, 10:14 a.m. UTC | #1

On Fri, Nov 10, 2023 at 10:36:00AM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <[e-mail]>
> 
> The test allocs a single page to hold all the frag_list skbs. This
> is insufficient on kernels with CONFIG_MAX_SKB_FRAGS=45, due to the
> increased skb_shared_info frags[] array length.
> 
>         gso_test_func: ASSERTION FAILED at net/core/gso_test.c:210
>         Expected alloc_size <= ((1UL) << 12), but
>             alloc_size == 5075 (0x13d3)
>             ((1UL) << 12) == 4096 (0x1000)
> 
> Simplify the logic. Just allocate a page for each frag_list skb.
> 
> Fixes: 4688ecb1385f ("net: expand skb_segment unit test with frag_list coverage")
> Signed-off-by: Willem de Bruijn <[e-mail]>

Thanks Willem,

I agree that the logic does as described,
that it should resolve the flagged problem,
and that as a bonus it is a nice simplification.

Reviewed-by: Simon Horman <[e-mail]>

Willem de Bruijn Nov. 12, 2023, 2:23 p.m. UTC | #2

On Sun, Nov 12, 2023 at 5:14 AM Simon Horman <[e-mail]> wrote:
>
> On Fri, Nov 10, 2023 at 10:36:00AM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <[e-mail]>
> >
> > The test allocs a single page to hold all the frag_list skbs. This
> > is insufficient on kernels with CONFIG_MAX_SKB_FRAGS=45, due to the
> > increased skb_shared_info frags[] array length.
> >
> >         gso_test_func: ASSERTION FAILED at net/core/gso_test.c:210
> >         Expected alloc_size <= ((1UL) << 12), but
> >             alloc_size == 5075 (0x13d3)
> >             ((1UL) << 12) == 4096 (0x1000)
> >
> > Simplify the logic. Just allocate a page for each frag_list skb.
> >
> > Fixes: 4688ecb1385f ("net: expand skb_segment unit test with frag_list coverage")
> > Signed-off-by: Willem de Bruijn <[e-mail]>
>
> Thanks Willem,
>
> I agree that the logic does as described,
> that it should resolve the flagged problem,
> and that as a bonus it is a nice simplification.
>
> Reviewed-by: Simon Horman <[e-mail]>

Thanks for the review Simon.

One thing I did not mention in the commit message and should be clear
is that these kunit tests lack cleanup code on error paths. If an
assertion fails, previously allocated memory is leaked.

This seems to be common procedure, and keeps the tests simple.

It takes superuser privileges to insmod tests, and they are not loaded
in a production environment. Which I assume is the basis for finding
this acceptable. They're usually run in a UML process -- if not
necessarily: I discovered this issue when running on a real system
with a config I had not anticipated.

Long story short, leaks on error are not an oversight, and common in
tests. If anyone thinks this is wrong, I can certainly revisit.

[e-mail] Nov. 13, 2023, 11:10 a.m. UTC | #3

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <[e-mail]>:

On Fri, 10 Nov 2023 10:36:00 -0500 you wrote:
> From: Willem de Bruijn <[e-mail]>
> 
> The test allocs a single page to hold all the frag_list skbs. This
> is insufficient on kernels with CONFIG_MAX_SKB_FRAGS=45, due to the
> increased skb_shared_info frags[] array length.
> 
>         gso_test_func: ASSERTION FAILED at net/core/gso_test.c:210
>         Expected alloc_size <= ((1UL) << 12), but
>             alloc_size == 5075 (0x13d3)
>             ((1UL) << 12) == 4096 (0x1000)
> 
> [...]

Here is the summary with links:
  - [net] net: gso_test: support CONFIG_MAX_SKB_FRAGS up to 45
    https://git.kernel.org/netdev/net/c/e6daf129ccb7

You are awesome, thank you!

Patch

diff --git a/net/core/gso_test.c b/net/core/gso_test.c
index ceb684be4cbf..4c2e77bd12f4 100644
--- a/net/core/gso_test.c
+++ b/net/core/gso_test.c
@@ -180,18 +180,17 @@  static void gso_test_func(struct kunit *test)
 	}
 
 	if (tcase->frag_skbs) {
-		unsigned int total_size = 0, total_true_size = 0, alloc_size = 0;
+		unsigned int total_size = 0, total_true_size = 0;
 		struct sk_buff *frag_skb, *prev = NULL;
 
-		page = alloc_page(GFP_KERNEL);
-		KUNIT_ASSERT_NOT_NULL(test, page);
-		page_ref_add(page, tcase->nr_frag_skbs - 1);
-
 		for (i = 0; i < tcase->nr_frag_skbs; i++) {
 			unsigned int frag_size;
 
+			page = alloc_page(GFP_KERNEL);
+			KUNIT_ASSERT_NOT_NULL(test, page);
+
 			frag_size = tcase->frag_skbs[i];
-			frag_skb = build_skb(page_address(page) + alloc_size,
+			frag_skb = build_skb(page_address(page),
 					     frag_size + shinfo_size);
 			KUNIT_ASSERT_NOT_NULL(test, frag_skb);
 			__skb_put(frag_skb, frag_size);
@@ -204,11 +203,8 @@  static void gso_test_func(struct kunit *test)
 
 			total_size += frag_size;
 			total_true_size += frag_skb->truesize;
-			alloc_size += frag_size + shinfo_size;
 		}
 
-		KUNIT_ASSERT_LE(test, alloc_size, PAGE_SIZE);
-
 		skb->len += total_size;
 		skb->data_len += total_size;
 		skb->truesize += total_true_size;