netdev - Re: [PATCH net-next 1/1] tc-testing: initial version of tunnel

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: <CAMDBHYLmhtmtNR00yiK2i9Pt=r7Z-mRxjj7X=bR6JiDgKvCEVA@mail.gmail.com>
Date:   Wed, 27 Jun 2018 14:50:18 -0400
From:   Lucas Bates <lucasb@...atatu.com>
To:     Davide Caratti <dcaratti@...hat.com>
Cc:     Keara Leibovitz <kleib@...atatu.com>,
        David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net-next 1/1] tc-testing: initial version of tunnel_key
 unit tests

On Tue, Jun 26, 2018 at 10:51 AM, Davide Caratti <dcaratti@...hat.com> wrote:
> On Tue, 2018-06-26 at 09:17 -0400, Keara Leibovitz wrote:
>> Create unittests for the tc tunnel_key action.
>>
>>
>> Signed-off-by: Keara Leibovitz <kleib@...atatu.com>
>> ---
>>  .../tc-testing/tc-tests/actions/tunnel_key.json    | 676 +++++++++++++++++++++
>>  1 file changed, 676 insertions(+)
>>  create mode 100644 tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
>>
>> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
>> new file mode 100644
>> index 000000000000..bfe522ac8177
>
> hello Keara!
>
> I think the 'teardown' stage in some of these tests should be reviewed.
> Those that are meant to test invalid configurations (like dc6b) should
> allow non-zero exit codes in the teardown stage, if the wrong
> configuration is catched by the userspace TC tool, before talking to the
> kernel.
>
> Otherwise, those tests will fail when they are invoked one by one with the
> act_tunnel_key module unloaded.
>
Hi Davide, I thought I'd weigh in here.

In the short term, I think this is reasonable, but it's not a feasible
long-term solution.  Here's why:

Allowing non-zero exit codes on setup and teardown was a precaution
that needed to be implemented as flushing actions in a freshly-booted
kernel returned errors - certain actions would only allow you to flush
after that action had been added.

But, doing this on so many test cases means that we can lose control
of the test environment, especially since a lot of commands get copied
between test cases.  One test's command under test becomes the next
test case's setup command, etc.  This can cause false results and
potentially waste a lot of time for someone trying to track down a
bug... Or cause bugs to be missed.

So, how to fix: we've had some discussions about it already.  Jiri had
requested the addition of a config file (like the one at
tools/testing/selftests/net/forwarding/config, and maybe an addition
to the README for tdc for explanation.  People would then possibly be
restricted to running one test case file at a time based on what
options they had loaded...  This is still not ideal.

I think the best possible fix is to add a new plugin for tdc to
exclude tests based on the kernel config.  This would require the
addition of a new optional field to the test case format, where any
and all included modules required for the test to work would be
listed.  The plugin would look at this information, do its best to
determine if the currently running kernel supports it, and allows the
test to run or be skipped as a result.

Let me show an example of the new field:
>> --- /dev/null
>> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
>> @@ -0,0 +1,676 @@
>>
> ...
>
>> +    {
>> +        "id": "dc6b",
>> +        "name": "Add tunnel_key set action with missing mandatory src_ip parameter",
>> +        "category": [
>> +            "actions",
>> +            "tunnel_key"
>> +        ],
               "reqModules": [
                   "CONFIG_NET_ACT_TUNNEL_KEY"
               ],
>> +        "setup": [
>> +            [
>> +                "$TC actions flush action tunnel_key",
>> +                0,
>> +                1,
>> +                255
>> +            ]
>> +        ],
>> +        "cmdUnderTest": "$TC actions add action tunnel_key set dst_ip 20.20.20.2 id 100",
>> +        "expExitCode": "255",
>> +        "verifyCmd": "$TC actions list action tunnel_key",
>> +        "matchPattern": "action order [0-9]+: tunnel_key set.*dst_ip 20.20.20.2.*key_id 100",
>> +        "matchCount": "0",
>> +        "teardown": [
>> +            "$TC actions flush action tunnel_key"
>> +        ]
>> +    },

As we venture into more and more complicated tests, where different
modules would start getting mixed together, this might be the most
effective route.

This plugin will require some changes I've made to our local version
of tdc that I've been testing out - they change the way tdc handles
its test results, and also give it the ability to skip tests without
affecting the rest of the test run.

Until I'm able to submit everything, I'd be OK with having Keara add
the non-zero exit codes to the teardown on her tests.  In the meantime
we'll get the README updated and config file added as well.

How does this sound?

- Lucas