Mon, May 10, 2021 at 12:25:27PM CEST, jtluka(a)redhat.com wrote:
Fri, May 07, 2021 at 03:31:18PM CEST, olichtne(a)redhat.com wrote:
>On Tue, May 04, 2021 at 11:21:15AM +0200, Jan Tluka wrote:
>> The handler of RTM_NEWADDR incorrectly fetches the IFA_ADDRESS
>> attribute to update the device address. This is not an issue for IPv6 or
>> typical update of the device address. However it is a bug for the
>> assignment of address with peer address included (typical for point to
>> point networks), for example:
>>
>> ip address add 192.168.100.1/24 peer 192.169.100.2 dev enp7s0
>>
>> In this case, the IFA_ADDRESS attribute value is the peer address value
>> and the device address is the value of IFA_LOCAL.
>>
>> When the peer address is not specified, both IFA_LOCAL and IFA_ADDRESS
>> contain the same value which is the reason this has been working without
>> any issues previously.
>>
>> The fix is to use the IFA_LOCAL attribute value to update the device
>> address for IPv4 scenarios.
>>
>> Signed-off-by: Jan Tluka <jtluka(a)redhat.com>
>> ---
>> lnst/Devices/Device.py | 21 ++++++++++++++++++---
>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py
>> index c31a018f..68b5f1af 100644
>> --- a/lnst/Devices/Device.py
>> +++ b/lnst/Devices/Device.py
>> @@ -25,7 +25,7 @@ from lnst.Common.ExecCmd import exec_cmd, ExecCmdFail
>> from lnst.Common.DeviceError import DeviceError, DeviceDeleted, DeviceDisabled
>> from lnst.Common.DeviceError import DeviceConfigError, DeviceConfigValueError
>> from lnst.Common.DeviceError import DeviceFeatureNotSupported
>> -from lnst.Common.IpAddress import ipaddress
>> +from lnst.Common.IpAddress import ipaddress, AF_INET
>> from lnst.Common.HWAddress import hwaddress
>>
>> from pyroute2.netlink.rtnl import RTM_NEWLINK
>> @@ -205,8 +205,23 @@ class Device(object, metaclass=DeviceMeta):
>> if nl_msg['header']['type'] == RTM_NEWLINK:
>> self._nl_msg = nl_msg
>> elif nl_msg['header']['type'] == RTM_NEWADDR:
>> - addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'),
>> - flags=nl_msg.get_attr("IFA_FLAGS"))
>> + if nl_msg['family'] == AF_INET:
>> + """
>> + from if_addr.h:
>> + /*
>> + * Important comment:
>> + * IFA_ADDRESS is prefix address, rather than local interface
address.
>> + * It makes no difference for normally configured broadcast
interfaces,
>> + * but for point-to-point IFA_ADDRESS is DESTINATION address,
>> + * local address is supplied in IFA_LOCAL attribute.
>> + */
>> + """
>> + addr = ipaddress(nl_msg.get_attr('IFA_LOCAL'),
>> + flags=nl_msg.get_attr("IFA_FLAGS"))
>> + else:
>> + addr = ipaddress(nl_msg.get_attr('IFA_ADDRESS'),
>> + flags=nl_msg.get_attr("IFA_FLAGS"))
>
>Based on your explanation and the if_addr.h comment, shouldn't we
>**always** read the IFA_LOCAL attribute?
>
>-Ondrej
>
Yes, we should. We should query IFA_ADDRESS only for IPv6 [1].
J.
[1]
https://github.com/tgraf/libnl/blob/master/lib/route/addr.c#L259
FYI, I double checked this. The patch is correct.
For IPv4 we should read the IFA_LOCAL, but for IPv6 only the IFA_ADDRESS
is populated:
$ ip -4 addr add dev lo 192.168.10.10/24
{'attrs': [('IFA_ADDRESS', '192.168.10.10'),
('IFA_LOCAL', '192.168.10.10'),
('IFA_LABEL', 'lo'),
('IFA_FLAGS', 128),
[root@lnst-devel lnst]# ('IFA_CACHEINFO', {'ifa_preferred':
4294967295, 'ifa_valid': 4294967295, 'cstamp': 2507106, 'tstamp':
2507106})],
'event': 'RTM_NEWADDR',
'family': 2,
'flags': 128,
'header': {'error': None,
'flags': 0,
'length': 76,
'pid': 3993,
'sequence_number': 1620746196,
'stats': Stats(qsize=0, delta=0, delay=0),
'target': 'localhost',
'type': 20},
'index': 1,
'prefixlen': 24,
'scope': 0}
$ ip -6 addr add dev lo fc00::6/64
{'attrs': [('IFA_ADDRESS', 'fc00::6'),
('IFA_CACHEINFO', {'ifa_preferred': 4294967295,
'ifa_valid': 4294967295, 'cstamp': 2475861, 'tstamp': 2475861}),
('IFA_FLAGS', 192)],
'event': 'RTM_NEWADDR',
'family': 10,
'flags': 192,
'header': {'error': None,
'flags': 0,
'length': 72,
'pid': 0,
'sequence_number': 0,
'stats': Stats(qsize=0, delta=0, delay=0),
'target': 'localhost',
'type': 20},
'index': 1,
'prefixlen': 64,
'scope': 0}
-Jan