netdev - Re: [PATCH net-next v2 07/12] selftests: ncdevmem: Properly reset flow steering

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: <Zv8W1dfhsmtsw5oT@mini-arch>
Date: Thu, 3 Oct 2024 15:12:37 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [PATCH net-next v2 07/12] selftests: ncdevmem: Properly reset
 flow steering

On 10/03, Mina Almasry wrote:
> On Thu, Oct 3, 2024 at 9:42 AM Stanislav Fomichev <stfomichev@...il.com> wrote:
> >
> > On 10/03, Mina Almasry wrote:
> > > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@...ichev.me> wrote:
> > > >
> > > > ntuple off/on might be not enough to do it on all NICs.
> > > > Add a bunch of shell crap to explicitly remove the rules.
> > > >
> > > > Cc: Mina Almasry <almasrymina@...gle.com>
> > > > Signed-off-by: Stanislav Fomichev <sdf@...ichev.me>
> > > > ---
> > > >  tools/testing/selftests/net/ncdevmem.c | 13 ++++++-------
> > > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > > > index 47458a13eff5..48cbf057fde7 100644
> > > > --- a/tools/testing/selftests/net/ncdevmem.c
> > > > +++ b/tools/testing/selftests/net/ncdevmem.c
> > > > @@ -207,13 +207,12 @@ void validate_buffer(void *line, size_t size)
> > > >
> > > >  static int reset_flow_steering(void)
> > > >  {
> > > > -       int ret = 0;
> > > > -
> > > > -       ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> > > > -       if (ret)
> > > > -               return ret;
> > > > -
> > > > -       return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> > > > +       run_command("sudo ethtool -K %s ntuple off >&2", ifname);
> > > > +       run_command("sudo ethtool -K %s ntuple on >&2", ifname);
> > > > +       run_command(
> > > > +               "sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2",
> > > > +               ifname, ifname);
> > > > +       return 0;
> > >
> > > Any reason to remove the checking of the return codes? Silent failures
> > > can waste time if the test fails and someone has to spend time finding
> > > out its the flow steering reset that failed (it may not be very
> > > obvious without the checking of the return code.
> >
> > IIRC, for me the 'ntuple off' part fails because the NIC doesn't let me
> > turn it of. And the new "ethtool .. | grep 'Filter: ' | ..." also fails
> > when there are no existing filters.
> >
> > I will add a comment to clarify..
> 
> Ah, understood. Seems this area is fraught with subtleties.
> 
> If you have time, maybe to counter these subtleties we can do a get of
> ntuple filters and confirm they're 0 somehow at the end of the
> function. That would offset not checking the return code.
> 
> But, I don't think it's extremely likely to run into errors here? So,
> this is probably good and can easily be improved later if we run into
> issues:
> 
> Reviewed-by: Mina Almasry <almasrymina@...gle.com>

Ack, I'll keep it as is with a comment. Ideally we should do proper
ethtool netlink/ioctl instead of shelling out, but I don' think
ntuple API is exposed to netlink and I'm too lazy to dive into how
the old ioctl-based ntuple API works :-D