Re: [PATCH 1/1] Add 'Transport Interface' (triface) option. This can be used to specify the IP interface to use for the connection. The driver uses that to set SO_BINDTODEVICE on the socket before connecting.

[Hannes Reinecke <hare@suse.de>]

On 5/5/21 4:31 PM, Belanger, Martin wrote:
>> -----Original Message-----
>> From: Hannes Reinecke <hare@suse.de>
>> Sent: Wednesday, May 5, 2021 4:47 AM
>> To: Sagi Grimberg; Martin Belanger; linux-nvme@lists.infradead.org
>> Cc: kbusch@kernel.org; axboe@fb.com; hch@lst.de; Belanger, Martin
>> Subject: Re: [PATCH 1/1] Add 'Transport Interface' (triface) option. This can
>> be used to specify the IP interface to use for the connection. The driver uses
>> that to set SO_BINDTODEVICE on the socket before connecting.
>>
>>
>> [EXTERNAL EMAIL]
>>
>> On 5/4/21 9:56 PM, Sagi Grimberg wrote:
>>>
>>>> From: Martin Belanger <martin.belanger@dell.com>
>>>
>>> Change log is missing...
>>>
>>>>
>>>> ---
>>>>    drivers/nvme/host/core.c    |  5 +++++
>>>>    drivers/nvme/host/fabrics.c | 14 +++++++++++++
>>>>    drivers/nvme/host/fabrics.h |  6 +++++-
>>>>    drivers/nvme/host/tcp.c     | 41
>>>> ++++++++++++++++++++++++++++++++++---
>>>>    4 files changed, 62 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>> index 288ac47ff5b4..91ae11a1ae26 100644
>>>> --- a/drivers/nvme/host/core.c
>>>> +++ b/drivers/nvme/host/core.c
>>>> @@ -3961,6 +3961,11 @@ static int nvme_class_uevent(struct device
>>>> *dev, struct kobj_uevent_env *env)
>>>>              ret = add_uevent_var(env, "NVME_HOST_TRADDR=%s",
>>>>                    opts->host_traddr ?: "none");
>>>> +        if (ret)
>>>> +            return ret;
>>>> +
>>>> +        ret = add_uevent_var(env, "NVME_HOST_TRIFACE=%s",
>>>> +                opts->host_triface ?: "none");
>>>
>>> Given that this was the original intent for host_traddr, why not have
>>> host_traddr resolve the iface from the address and set sockopt
>>> SO_BINDTODEVICE on it?
>>>
>> That was my question, too.
>>
>> I would vastly prefer to not have another option to deal with (as it raises the
>> question whether to add it eg during 'nvme connect-all') And one could
>> argue that this was the intention of _having_ the host_traddr argument in
>> the first place ...
>>
>> Cheers,
>>
>> Hannes
>> --
>> Dr. Hannes Reinecke		        Kernel Storage Architect
>> hare@suse.de			               +49 911 74053 688
>> SUSE Software Solutions Germany GmbH, 90409 Nürnberg
>> GF: F. Imendörffer, HRB 36809 (AG Nürnberg)
> 
> Hi Sagi and Hannes,
> 
> Correct me if I'm wrong, but it sounds like host_traddr was primarily added for FC (at least it wasn't tested for TCP since it does not work in its current state). I'm not an expert on FC and maybe specifying an address is the right (and only) way to specify and interface for FC. For TCP, however, it's not advisable. Specifying an interface by its associated IP address is less intuitive than specifying the actual interface name and, in some cases, it simply won't work. That's because the association between interfaces and IP addresses is not predictable. IP addresses can be changed or can change by themselves over time (e.g. DHCP). Interface names are predictable [1] and will persist over time. Consider the following configuration.
> 
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
>      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>      inet 100.0.0.100/24 scope global lo
>         valid_lft forever preferred_lft forever
> 2: enp0s3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000
>      link/ether 08:00:27:21:65:ec brd ff:ff:ff:ff:ff:ff
>      inet 100.0.0.100/24 scope global enp0s3
>         valid_lft forever preferred_lft forever
> 3: enp0s8: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000
>      link/ether 08:00:27:4f:95:5c brd ff:ff:ff:ff:ff:ff
>      inet 100.0.0.100/24 scope global enp0s8
>         valid_lft forever preferred_lft forever
> 
> The above is a VM that I configured with the same IP address (100.0.0.100) on all interfaces. Doing a reverse
> lookup to identify the unique interface associated with 100.0.0.100 would simply not work here. And this is why
> the option host_iface is required. I understand that the above config does not represent a standard host system,
> but I'm using this to prove a point: "we can never know how a user will configure their system and the above
> configuration is perfectly fine by Linux".
> 

... and messing up any switch MAC address caching when doing so. I guess 
the network admin will come down hard on you if you try that on a 
production system.
And I sincerely question whether this is a valid use-case; I'm already 
getting grief from our network admins if I dare to put two network 
interfaces from the same machine in the same network.

> The current TCP implementation for host_traddr uses bind()-before-connect(). This is a common construct to set the
> source IP address on the socket before connecting. This has no effect on how Linux will select the interface for the
> connection. That's because Linux uses the Weak End System model as described in RFC1122 [2]. Setting the source address
> on a connection is a common requirement that linux-nvme needs to support. In fact, specifying the Source IP address
> is a mandatory FedGov requirement (e.g. connection to a RADIUS/TACACS+ server). Consider the following configuration.
> 
> $ ip addr list dev enp0s8
> 3: enp0s8: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000
>      link/ether 08:00:27:4f:95:5c brd ff:ff:ff:ff:ff:ff
>      inet 192.168.56.101/24 brd 192.168.56.255 scope global dynamic noprefixroute enp0s8
>         valid_lft 426sec preferred_lft 426sec
>      inet 192.168.56.102/24 scope global secondary enp0s8
>         valid_lft forever preferred_lft forever
>      inet 192.168.56.103/24 scope global secondary enp0s8
>         valid_lft forever preferred_lft forever
>      inet 192.168.56.104/24 scope global secondary enp0s8
>         valid_lft forever preferred_lft forever
> 
> Here we can see that several addresses are associated with interface enp0s8. By default, Linux will select the
> default IP address, 192.168.56.101, as the source address when connecting over interface enp0s8. Some users,
> however, want the ability to specify a different address (e.g., 192.168.56.103) to be used as the source address.
> The option host_traddr can be used as-is to perform this function (I tested it).
> 

No disagreement here.

> In conclusion, I believe that for TCP we need 2 options. One that can be used to specify an interface. And one
> that can be used to set the source address. And users should be allowed to use one or the other, or both, or none.
> Of course, the documentation for host_traddr will need some clarification. It should state that when used for TCP
> connection, this option only sets the source address. And the documentation for host_iface should say that this
> option only applies to TCP connections.
> 

I'm with James Smart here. I do fail to see the need for 'host_iface' 
_without_ 'host_traddr'; especially for IPv6 where several addresses are 
standard just specifying 'host_iface' simply is not enough, and one has 
to specify 'host_traddr' additionally.

So 'host_iface' should be contingent on 'host_traddr', meaning we can 
just expand the syntax of 'host_traddr'.
One easy possibility would be to add ',nobind' to the host_traddr syntax 
which would indicate that we should _not_ bind to the underlying 
interface; I do think that binding to the respective interface should be 
the default.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme