netdev - Re: [PATCH net-next v2] selftests: drv-net: replace the nsim ring test with a drv-net one

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] [day] [month] [year] [list]

Message-ID: <a55df0a7-4adf-45bf-b448-eeb68ba5592e@lunn.ch>
Date: Tue, 28 Oct 2025 03:05:03 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
	shuah@...nel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v2] selftests: drv-net: replace the nsim ring
 test with a drv-net one

On Mon, Oct 27, 2025 at 05:15:39PM -0700, Jakub Kicinski wrote:
> On Mon, 27 Oct 2025 20:46:00 +0100 Andrew Lunn wrote:
> > > +    def test_config(config):
> > > +        try:
> > > +            cfg.eth.channels_set(ehdr | config)
> > > +            get = cfg.eth.channels_get(ehdr)
> > > +            for k, v in config.items():
> > > +                ksft_eq(get.get(k, 0), v)
> > > +        except NlError as e:
> > > +            failed.append(mix)
> > > +            ksft_pr("Can't set", config, e)
> > > +        else:
> > > +            ksft_pr("Okay", config)  
> > 
> > We expect failure to leave the configuration unchanged. So i would
> > actually do:
> > 
> > try:
> > 	before = get()
> > 	set()
> > except:
> > 	after = get()
> > 	fail(after != before)
> 
> Please allow me to introduce you to the magic of defer() ;)

That is why i don't like magic, especially in tests which have no
documentation of the expected results. For me, tests should be dumb,
often boringly repetitive, and at least 50% comments, explaining what
is being tested, what the expected outcome is, and most importantly,
why that is the expected outcome.

> This registers a command to run after the test completely exits:
> 
> +    defer(cfg.eth.channels_set, ehdr | restore)
> 
> > Also, does nlError contain the error code?
> > 
> >         fail(e.errcode not in (EINVAL, EOPNOTSUPP))
> > 
> > It would be good to detect and fail ENOTSUPP, which does appear every
> > so often, when it should not.
> 
> Dunno, checkpatch warns about ENOTSUPP. I don't that think checking 
> for funny error codes in every test scales :(

How about in the nlError constructor? That gives you a single
location, and you can accept EINVAL, EOPNOTSUPP, ENODEV, ENOMEM, maybe
ETOOBIG. Cause the test to fail for everything else. If anybody
reports test failures with other errno values, the list can be
expanded, if they are sensible.

> > > +    # Try to reach min on all settings
> > > +    for param in params:
> > > +        val = rings[param]
> > > +        while True:
> > > +            try:
> > > +                cfg.eth.rings_set({'header':{'dev-index': cfg.ifindex},
> > > +                                   param: val // 2})
> > > +                val //= 2
> > > +                if val <= 1:
> > > +                    break
> > > +            except NlError:
> > > +                break  
> > 
> > Is 0 ever valid? I would actually test 0 and make sure it fails with
> > EINVAL, or EOPNOTSUPP. Getting range checks wrong is a typical bug, so
> > it is good to test them. The happy days cases are boring because
> > developers tend to test those, so they are hardly worth testings. Its
> > the edge cases which should be tested.
> 
> I believe that 0 is a valid settings. I don't have much experience with
> devices which support it. But presumably using 0 to disable mini/jumbo
> rings would make sense for example? And max validation is done by the
> core so nothing interesting to explore there at the driver level :(

Looking at the code, it seems to cost nothing to actually test 0, if
you say it could be valid. That might also find an off-by-one error,
if it causes something to go negative/large positive.

	Andrew