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: <CAHS8izOtNP2DXHWd_NcXTbD=P9s055g-EWWhknv4VkPh2NXKvg@mail.gmail.com>
Date: Thu, 3 Oct 2024 11:54:26 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Stanislav Fomichev <stfomichev@...il.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 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>

-- 
Thanks,
Mina