Message-ID: <0e8be7be-69fb-fdc0-12c7-fb21285867bf@vyatta.att-mail.com>
Date:   Mon, 1 Oct 2018 09:48:31 +0100
From:   Mike Manning <mmanning@...tta.att-mail.com>
To:     David Ahern <dsahern@...il.com>, netdev@...r.kernel.org
Cc:     Robert Shearman <rshearma@...tta.att-mail.com>
Subject: Re: [PATCH net-next v1 1/5] net: allow binding socket in a VRF when
 there's an unbound socket

On 25/09/2018 18:16, David Ahern wrote:
> On 9/25/18 9:26 AM, Mike Manning wrote:
>> On 24/09/2018 23:44, David Ahern wrote:
>>> On 9/24/18 10:13 AM, Mike Manning wrote:
>>>> From: Robert Shearman <rshearma@...tta.att-mail.com>
>>>>
>>>> There is no easy way currently for applications that want to receive
>>>> packets in the default VRF to be isolated from packets arriving in
>>>> VRFs, which makes using VRF-unaware applications in a VRF-aware system
>>>> a potential security risk.
>>> That comment is not correct.
>>>
>>> The point of the l3mdev sysctl's is to prohibit this case. Setting
>>> net.ipv4.{tcp,udp}_l3mdev_accept=0 means that a packet arriving on an
>>> interface enslaved to a VRF can not be received by a global socket.
>> Hi David, thanks for reviewing this. The converse does not hold though,
>> i.e. there is no guarantee that the unbound socket will be selected for
>> packets when not in a VRF, if there is an unbound socket and a socket
>> bound to a VRF. Also, such packets should not be handled by the socket
> I need an explicit example here. You are saying a packet arriving on an
> interface not enslaved to a VRF might match a socket bound to a VRF?
This problem occurs when different service instances are listening on an
unbound socket and sockets bound to VRFs respectively. Received packets
that are not in a VRF are not guaranteed to be handled by the unbound
socket.
>> in the VRF if there is no unbound socket. We also had an issue with raw
>> socket lookup device bind matching. I can break this particular patch
>> into smaller patches and provide more detail, would this help? I will
>> also update/break up the other patches according to your comments.
> Why not add an l3mdev sysctl for raw sockets then?
I have now added this, see patch 4/10.
> Yes, please send smaller patches. A diff stat of:
>     15 files changed, 109 insertions(+), 62 deletions(-)
> is a bit harsh.
I have removed the 2 patches you are ok with and have submitted them
separately , and have split the remaining 3 into 10 smaller patches.
>>> Setting the l3mdev to 1 allows the default socket to work across VRFs.
>>> If that is not what you want for a given app or a given VRF, then one
>>> option is to add netfilter rules on the VRF device to prohibit it. I
>>> just verified this works for both tcp and udp.
>> Netfilter is per application and so does not scale. I have not checked
>> if it is suitable for packet handling on raw sockets.
>>
>>> Further, overlapping binds are allowed using SO_REUSEPORT meaning I can
>>> have a server running in the default vrf bound to a port AND a server
>>> running bound to a specific vrf and the same port:
>>>
>>> udp    UNCONN     0      0      *%red:12345                 *:*
>>>             users:(("vrf-test",pid=1376,fd=3))
>>> udp    UNCONN     0      0       *:12345                 *:*
>>>          users:(("vrf-test",pid=1375,fd=3))
>>>
>>> tcp    LISTEN     0      1      *%red:12345                 *:*
>>>             users:(("vrf-test",pid=1356,fd=3))
>>> tcp    LISTEN     0      1       *:12345                 *:*
>>>          users:(("vrf-test",pid=1352,fd=3))
>>>
>>> For packets arriving on an interface enslaved to a VRF the socket lookup
>>> will pick the VRF server over the global one.
>> Agreed, but the converse is not guaranteed to hold i.e. packets that are
>> not in a VRF may be handled by a socket bound to a VRF.
>>
>> We do use SO_REUSEPORT for our own applications so as to run instances
>> in the default and other VRFs, but still require these patches (or
>> similar) due to how packets are handled when there is an unbound socket
>> and sockets bound to different VRFs.
> Why can't compute_score be adjusted to account for that case?
Yes, this is what we are doing. This is now in patch 2/10 for stream and
3/10 for datagram sockets, and see 5/10 for raw sockets.
>>> -- 
>>>
>>> With this patch set I am seeing a number of tests failing -- socket
>>> connections working when they should not or not working when they
>>> should. I only skimmed the results. I am guessing this patch is the
>>> reason, but that is just a guess.
>>>
>>> You need to make sure all permutations of:
>>> 1. net.ipv4.{tcp,udp}_l3mdev_accept={0,1},
>>> 2. connection in the default VRF and in a VRF,
>>> 3. locally originated and remote traffic,
>>> 4. ipv4 and ipv6
>>>
>> We are using raw, datagram and stream sockets for ipv4 & ipv6, require
>> connectivity for local and remote addresses where appropriate and need
>> route leaking between VRFs when configured, we are unaware of any
>> outstanding bugs. Is there some way that I can run/analyze the tests
>> that are failing for you?
> I am not distributing my vrf tests right now. Before sending the
> response I quickly verified one case is easy for you to see: set the udp
> sysctl to 0, start a global server, send a packet to it via an interface
> enslaved to a VRF. It should fail ECONNREFUSED (no socket match) but
> instead packet reaches the server.
I have tried with netcat & socat to check this, and while the
connectivity handling is correct, this does not show the failure error
(it doesn't without the changes either). Before I look further into
this, can I check whether you had only the udp sysctl set to 0 i.e. the
tcp sysctl to 1? The reason I ask is that in the original series, we
were using a common function inet_sk_bound_dev_eq() for
raw/stream/datagram sockets, and this fn was checking only the tcp
sysctl (as we always have the udp and tcp sysctl set to 0). In series
v2, I have provided separate fns which check the relevant raw, tcp or
udp sysctl.
>> Also cf patch 2/5 note that ping to link-local addresses is handled
>> consistently with that to global addresses in a VRF, so this now
>> succeeds if ping is done in the VRF, i.e. 'sudo ip vrf exec <vrf> ping
>> <ll> -I <intf>
> Shifting packets destined to a LLA from the real device to the vrf
> device is a change in behavior. It is not clear to me at the moment that
> it will not cause a problem.
I have provided a clearer description of why we have had to do this in
patch 7/10.
>>> continue to work as expected meaning packets flow when they should and
>>> fail with the right error when they should not. I believe the UDP cases
>>> were the main ones failing.
>>>
>>> Given the test failures, I did not look at the code changes in the patch.
>>>
> A couple of the patches are fine as is - or need a small change. It
> might be easier for you to send those outside of the socket lookup set.
Thank you for reviewing these.

Powered by blists - more mailing lists