netdev - Re: [PATCH net-next 0/7] net: add struct nexthop to fib{6}

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]

Date:   Thu, 30 May 2019 13:01:19 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     David Ahern <dsahern@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        David Ahern <dsahern@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        Ido Schimmel <idosch@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Martin KaFai Lau <kafai@...com>, Wei Wang <weiwan@...gle.com>
Subject: Re: [PATCH net-next 0/7] net: add struct nexthop to fib{6}_info

On Thu, May 30, 2019 at 11:52 AM David Ahern <dsahern@...il.com> wrote:
>
> On 5/30/19 12:27 PM, Alexei Starovoitov wrote:
> > On Thu, May 30, 2019 at 11:01 AM David Miller <davem@...emloft.net> wrote:
> >>
> >> From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> >> Date: Thu, 30 May 2019 08:18:10 -0700
> >>
> >>> On Thu, May 30, 2019 at 8:16 AM David Ahern <dsahern@...il.com> wrote:
> >>>>
> >>>> On 5/30/19 9:06 AM, Alexei Starovoitov wrote:
> >>>>> Huge number of core changes and zero tests.
> >>>>
> >>>> As mentioned in a past response, there are a number of tests under
> >>>> selftests that exercise the code paths affected by this change.
> >>>
> >>> I see zero new tests added.
> >>
> >> If the existing tests give sufficient coverage, your objections are not
> >> reasonable Alexei.
> >
> > I completely disagree. Existing tests are not sufficient.
> > It is a new feature for the kernel with corresponding iproute2 new features,
> > yet there are zero tests.
> >
>
> Your objection was based on changes to core code ("Huge number of core
> changes and zero tests"), not new code paths.
>
> The nexthop code paths are not live yet. More changes are needed before
> that can happen. I have been sending the patches in small, reviewable
> increments to be kind to reviewers than a single 27 patch set with
> everything (the remaining set which is over the limit BTW).
>
> Once iproute2 has the nexthop command (patches sent to the list) AND the
> RTA_NHID patches are in, I have this as the final set:
>
> 8c0b06b9813e selftests: Add test cases for nexthop objects
> ea5c19e4dc7c selftests: Add nexthop objects to router_multipath.sh
> 3be7b15d1e56 selftests: pmtu: Move running of test into a new function
> a896b2206ea5 selftests: pmtu: Move route installs to a new function
> cfa48193d0b8 selftests: pmtu: Add support for routing via nexthop objects
> 3d09a79208b9 selftests: icmp_redirect: Add support for routing via
> nexthop objects

cool. that's exactly what I was asking about.
Could you please post them to the mailing list as RFC or some
not-yet-to-be-merged tag?
And in the main patch set, just say in cover letter:
"here look at this set posted separately, because of such and such"

> As always, my patches can be viewed by anyone:
>     https://github.com/dsahern/linux

sure, but it's counter to the standard code review procedure we have.
Pointing a link to your tree doesn't make them reviewable on the
public mailing list.
I very much prefer to do code reviews by looking at the tests first.
It gives me a better idea on api, use cases, etc.

> That includes:
> 1. a test script doing functional validation of the nexthop code
> (net/ipv4/nexthop.c), along with stack integration (e.g., v6 nexthop
> with v4 routes) and traffic (ping).
>
> 2. updates to existing exception tests (pmtu and redirect) with the
> nexthop obects used for routing
>
> 3. updates to the existing router_multipath script with the nexthop
> obects used for routing and validating balanced selection in paths.

great.

>
> Latest branch is:
>    https://github.com/dsahern/linux/tree/5.2-next-nexthops-v14
>
> In addition to the functional and runtime tests listed above:
> 1. I have all kinds of MPLS and network functional tests that I run. I
> have already committed to sending those for inclusion, it is a matter of
> time.

cool. first time I hear.

> 2. The FRR team has had a kernel with these patches for 5+ months adding
> support for nexthops to FRR. They have their own test suites used for
> their development.

what is FRR team? First time I hear about them as well.

> 3. The patches are included in our 4.19 kernel and subjected to our
> suite of smoke and validation tests.

it's also great to hear, but your kernel tree is not a substitute
for netdev community with many companies contributing and running
those kernels, syzbots, buildbots, etc.
I have no doubt that you test your patches, but these patches also touch
the very core of networking that we all are care deeply about.
Hence we want to be able to run all those tests on our kernels too.