IP address comparison may fail even when two addresses are equivalent
martin-belanger opened this issue · 10 comments
This is somehow related to PR: #647
IPv6 has a special syntax where consecutive sections of zeros can be replaced with two colons (::). In other words, the following two IPv6 addresses are equivalent:
2001:0db8:0000:0000:0000:ff00:0042:8329 == 2001:0db8::ff00:0042:8329
Also, IPv6 supports "IPv4-mapped IPv6 addresses" where an IPv4 address can be expressed as an IPv6 address. Again, this can lead to comparison failures when in fact two addresses match. Here's an example:
::ffff:192.0.2.128 == 192.0.2.128
Although unlikely, IPv4 addresses can include leading 0s.
192.0.2.128 == 192.000.002.128
Currently, libnvme uses case-insensitive string comparison (strcasecmp) to determine if two IP addresses are equal. This would fail for the above cases.
I don't have a solution for this. Python provides a module (ipaddress) that makes comparing IP addresses easy and covers all cases above. I don't know if there is an equivalent C library that we could use. Otherwise, we may have to write an IP address comparison function.
Just to give you an idea of what this is trying to solve.
##########################################################################
Step 1) Connect using compressed IPv6 (fe80::800:27ff:fe00:0)
and specify interface using scope syntax (%enp0s8).
##########################################################################
$ sudo nvme discover -p -t tcp -s 8009 -a fe80::800:27ff:fe00:0%enp0s8
$ nvme list -v
Subsystem Subsystem-NQN Controllers
---------------- ------------------------------------------------------------------------------------------------ ----------------
nvme-subsys0 nqn.2014-08.org.nvmexpress.discovery nvme0
Device SN MN FR TxPort Address Subsystem Namespaces
-------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
nvme0 139315012e3c3b70fe9c Linux 5.19.0-4 tcp traddr=fe80::800:27ff:fe00:0%enp0s8,trsvcid=8009,src_addr=fe80::c71b:9e1:c456:f855 nvme-subsys0
Device Generic NSID Usage Format Controllers
------------ ------------ ---------- -------------------------- ---------------- ----------------
##########################################################################
Step 2) Same as before, but specify interface using -f option.
We see that a new connection is created even though it's the same IPv6
address and interface.
##########################################################################
$ sudo nvme discover -p -t tcp -s 8009 -a fe80::800:27ff:fe00:0 -f enp0s8
$ nvme list -v
Subsystem Subsystem-NQN Controllers
---------------- ------------------------------------------------------------------------------------------------ ----------------
nvme-subsys1 nqn.2014-08.org.nvmexpress.discovery nvme1
nvme-subsys0 nqn.2014-08.org.nvmexpress.discovery nvme0
Device SN MN FR TxPort Address Subsystem Namespaces
-------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
nvme1 139315012e3c3b70fe9c Linux 5.19.0-4 tcp traddr=fe80::800:27ff:fe00:0,trsvcid=8009,host_iface=enp0s8,src_addr=fe80::c71b:9e1:c456:f855 nvme-subsys1
nvme0 139315012e3c3b70fe9c Linux 5.19.0-4 tcp traddr=fe80::800:27ff:fe00:0%enp0s8,trsvcid=8009,src_addr=fe80::c71b:9e1:c456:f855 nvme-subsys0
Device Generic NSID Usage Format Controllers
------------ ------------ ---------- -------------------------- ---------------- ----------------
##########################################################################
Step 3) Connect using expanded IPv6 address (fe80:0000:0000:0000:800:27ff:fe00:0)
and specify interface using -f option.
We see that a new connection is created even though it's the same IPv6
address and interface as previous 2 connections.
##########################################################################
$ sudo nvme discover -p -t tcp -s 8009 -a fe80:0000:0000:0000:800:27ff:fe00:0 -f enp0s8
$ nvme list -v
Subsystem Subsystem-NQN Controllers
---------------- ------------------------------------------------------------------------------------------------ ----------------
nvme-subsys2 nqn.2014-08.org.nvmexpress.discovery nvme2
nvme-subsys1 nqn.2014-08.org.nvmexpress.discovery nvme1
nvme-subsys0 nqn.2014-08.org.nvmexpress.discovery nvme0
Device SN MN FR TxPort Address Subsystem Namespaces
-------- -------------------- ---------------------------------------- -------- ------ -------------- ------------ ----------------
nvme2 139315012e3c3b70fe9c Linux 5.19.0-4 tcp traddr=fe80:0000:0000:0000:800:27ff:fe00:0,trsvcid=8009,host_iface=enp0s8,src_addr=fe80::c71b:9e1:c456:f855 nvme-subsys2
nvme1 139315012e3c3b70fe9c Linux 5.19.0-4 tcp traddr=fe80::800:27ff:fe00:0,trsvcid=8009,host_iface=enp0s8,src_addr=fe80::c71b:9e1:c456:f855 nvme-subsys1
nvme0 139315012e3c3b70fe9c Linux 5.19.0-4 tcp traddr=fe80::800:27ff:fe00:0%enp0s8,trsvcid=8009,src_addr=fe80::c71b:9e1:c456:f855 nvme-subsys0
Device Generic NSID Usage Format Controllers
------------ ------------ ---------- -------------------------- ---------------- ----------------
##########################################################################
Step 4) Repeat with more or less 0s in the IPv6 address:
fe80:0:0:0:800:27ff:fe00:0
fe80:00:0:0:800:27ff:fe00:0
fe80:00:00:0:800:27ff:fe00:0
fe80:00:00:00:800:27ff:fe00:0
etc.
Every time, a new connection is created because we do a string comparison
instead of checking whether the addresses are equivalent.
##########################################################################
Thanks for the detailed report. We should definitely fix this.
We may need to sanitize the IP address as soon as it is passed in by the user.
I noticed that even if libnvme is able to recognize that two IP addresses are identical with this new function (see #652), the kernel still thinks they are different IP addresses and will make new connections.
All that to say that I need to submit more changes in #652.
As mentioned in #652, the new compare function should probably prefixed with nvme_ and needs to be exported via libnvme.map if the aim is to be useful outside of the library. If this is not the intent that we should move the declaration to private.h
I will prefix the function with nvme_ and add it to libnvme.map. I think this is a good idea because nvme-cli may also need to check IP addresses, which will require this new function to be public.
BTW, don't know if you care, but there seems at least one combination of IP comparison which is not tested yet: https://app.codecov.io/gh/linux-nvme/libnvme/blob/master/src%2Fnvme%2Futil.c
(not important, just having fun looking at the report :))
Thanks for letting me know. I shall add one more test to cover that part of the code.
@igaw - This is just FYI.
I found that the function traddr_is_hostname() is duplicated (see links below). I plan to address this as part of the IP address sanitization effort.
https://github.com/linux-nvme/libnvme/blob/master/src/nvme/tree.c#L1000
https://github.com/linux-nvme/libnvme/blob/master/src/nvme/fabrics.c#L534
@martin-belanger are we good with this bug report? Is there anything still missing?
Yes we're good now. Closing.