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

Message-ID: <20251027171539.565e63f2@kernel.org>
Date: Mon, 27 Oct 2025 17:15:39 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Andrew Lunn <andrew@...n.ch>
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, 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() ;)
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 :(

> > +    # 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 :(