netdev - Re: [PATCH net] net: gso_test: support CONFIG_MAX_SKB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-Lzu+LHAah+dEM0doFZZnO+TfHPhQNWEy2UYS28o+4jsw@mail.gmail.com>
Date: Sun, 12 Nov 2023 09:23:27 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	edumazet@...gle.com, pabeni@...hat.com, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] net: gso_test: support CONFIG_MAX_SKB_FRAGS up to 45

On Sun, Nov 12, 2023 at 5:14 AM Simon Horman <horms@...nel.org> wrote:
>
> On Fri, Nov 10, 2023 at 10:36:00AM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@...gle.com>
> >
> > 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 <willemb@...gle.com>
>
> 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 <horms@...nel.org>

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.