netdev - Re: [PATCH net-next] selftests: pmtu: Simplify cleanup and namespace names

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: <8b642166-e0f9-cfb7-8e19-5a46f58549b6@gmail.com>
Date:   Thu, 23 May 2019 09:41:59 -0600
From:   David Ahern <dsahern@...il.com>
To:     Stefano Brivio <sbrivio@...hat.com>,
        David Ahern <dsahern@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] selftests: pmtu: Simplify cleanup and namespace
 names

On 5/23/19 1:58 AM, Stefano Brivio wrote:
> Hi David,
> 
> On Wed, 22 May 2019 12:11:06 -0700
> David Ahern <dsahern@...nel.org> wrote:
> 
>> From: David Ahern <dsahern@...il.com>
>>
>> The point of the pause-on-fail argument is to leave the setup as is after
>> a test fails to allow a user to debug why it failed. Move the cleanup
>> after posting the result to the user to make it so.
>>
>> Random names for the namespaces are not user friendly when trying to
>> debug a failure. Make them simpler and more direct for the tests. Run
>> cleanup at the beginning to ensure they are cleaned up if they already
>> exist.
> 
> The reasons for picking per-instance unique names were:
> 
> - one can run multiple instances of the script in parallel. I
>   couldn't trigger any bug this way *so far*, though
> 
> - cleanup might fail because of e.g. device reference count leaks (this
>   happened quite frequently in the past), which are anyway visible in
>   kernel logs. Unique names avoid the need to reboot
> 
> Sure, it's a trade-off with usability, and I also see the value of
> having fixed names, so I'm fine with this too. I just wanted to make
> sure you considered these points.
> 
> By the way, the comment to nsname() (that I would keep, it's still
> somewhat convenient) is now inconsistent.
> 

I have been using the namespace override for a while now. I did consider
impacts to the above, but my thinking is this: exceptions are per FIB
entry (per fib6_nh with my latest patch set, but point still holds), FIB
entries are per FIB table, FIB tables are per network namespace. Running
multiple pmtu.sh sessions in parallel can not trigger an interdependent
bug because of that separation. The cleanup within a namespace teardown
(reference count leaks) should not be affected.

Now that we have good set of functional tests, we do need more complex
tests but those will still be contained within the namespace separation.
If you look at my current patch set on the list I add an icmp_redirect
test script. It actually does redirect, verify, mtu on top of redirect,
verify and then resets and inverts the order - going after an exception
entry with an update for both use cases.

For the pmtu script, perhaps the next step is something as simple as
configuring the setup and routing once and then run all of the
individual tests (or multiple of them) to generate multiple exceptions
within a single FIB table and then tests to generate multiple exceptions
with different addresses per entry.