Re: [PATCH net-next v1 1/5] net: allow binding socket in a VRF when there's an unbound socket

[David Ahern <dsahern@gmail.com>]

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@vyatta.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?


> 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?

Yes, please send smaller patches. A diff stat of:
    15 files changed, 109 insertions(+), 62 deletions(-)
is a bit harsh.

> 
>>
>> 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?

> 
>>
>> -- 
>>
>> 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.

> 
> 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.

> 
>> 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.