netdev - Re: ipmr: MFC routes when VIF deleted

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:   Sun, 11 Jun 2017 19:34:19 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Yotam Gigi <yotamg@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     Donald Sharp <sharpd@...ulusnetworks.com>
Subject: Re: ipmr: MFC routes when VIF deleted

On 11/06/17 11:55, Yotam Gigi wrote:
> I have been looking into some weird behavior, and I am not sure whether it is
> a bug or a feature.
> 
> When a VIF with index v gets deleted, the MFC routes does not get updated, which
> means that there can be routes pointing to that VIF. On datapath, when packet
> hits that route, the VIF validity will be checked and will not be sent to that
> device (but still, the route does not get updated).  Now, if the user creates
> another VIF with the same index v but different underlay device, the same route
> will forward the traffic to that device.
> 
> It is relevant to mention that when user adds a MFC route, only the active VIFs
> are used, so the flow of adding a route with dummy VIF indices and then
> connecting those VIF indices to real device is not supported. The only way to
> create a MFC route that has non existing VIFs is to create one with existing
> VIFs and then delete them.
> 
> Do we really want to support that?  To me, it looks like a buggy flow and I
> suggest that upon VIF deletion, the MFC routes will be updated to not point to
> any non existing VIF indices.
> 

Hi Yotam,
I'm not strongly against such change but my feeling is that we shouldn't change it.
I think we shouldn't change it because we cannot guarantee that we won't break some
user-space app that relies on this behaviour or uses it as an optimization.
User-space ipmr apps work in sync with the kernel and are usually the only ones
doing such changes so their internal state will be always valid, and I'd guess
they already deal with this one way or another.
My second argument is a minor one and is about performance. There are some apps
(e.g. pimd) which use interface add/del on interface state change (up/down) and
this could make these ops slower on large setups.

Again I see how this could be helpful and should've probably been like that from the
start, so if other people feel confident we won't break anything then I wouldn't
mind the change.

Thanks,
 Nik