netdev - Re: [RFC v4 3/5] vxlan: add support for underlay in non-default VRF

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: <CAJieiUiA=2JTsLD_siZnVemQEgoyepMWi2YoQ+ixxAEzvF3o3w@mail.gmail.com>
Date:   Mon, 26 Nov 2018 10:26:36 -0800
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     David Ahern <dsa@...ulusnetworks.com>
Cc:     abauvin@...leway.com, netdev <netdev@...r.kernel.org>,
        akherbouche@...leway.com
Subject: Re: [RFC v4 3/5] vxlan: add support for underlay in non-default VRF

On Mon, Nov 26, 2018 at 9:54 AM David Ahern <dsa@...ulusnetworks.com> wrote:
>
> On 11/26/18 9:32 AM, Alexis Bauvin wrote:
> > Thanks for the review. I’ll send a v5 if you have no other comment on
> > this version!
>
> A few comments on the test script; see attached which has the changes.
>
> Mainly the cleanup does not need to be called at the end since you setup
> the exit trap. The cleanup calls ip to delete veth-hv-1 and veth-tap but
> those are moved to other namespaces. 'ip netns exec NAME ip ...' is more
> efficiently done as 'ip -netns NAME ...'. The test results should align
> like this:
>
> Checking HV connectivity                                          [ OK ]
> Check VM connectivity through VXLAN (underlay in the default VRF) [ OK ]
> Check VM connectivity through VXLAN (underlay in a VRF)           [ OK ]
>
> So it is easy for users to see the PASS/FAIL.
>
> It would be good to copy the topology ascii art into the test script as
> well for future users.
>
> Also, add the test as a separate patch at the end and include it in
> tools/testing/selftests/net/Makefile
>
> Finally, I think you should drop the RFC and send it as a 'ready for
> inclusion'.

I cant seem to find patch 5 in my mail box... so commenting here
(Using reference to patch5 from here
https://marc.info/?l=linux-netdev&m=154284885815549&w=2)

Still not convinced that the auto reopen is justified here IMO because
it can be done from user-space and there are many cases where this is
already done from user-space. A few questions for alexis on that,
- What is the reason for handling NETDEV_CHANGE on the vxlan device
from the notifier handler. It can be really done in the changelink
handler, correct  ?
- Also, IIUC, patch5 blindly re-opens the vxlan device without
considering if the admin had set it to down last (ie the last state on
it was vxlan_close). is that correct ?

(Don't want to block the entire series for just patch5. Patch5 can be
done incrementally after we converge on it. The rest of the series
looks good as David has already reviewed.  And nice to see the test!).