[PATCH iproute2] ip-address: properly display zero IPv4 peer address

Kernel allows for zero IPv4 peer addresses (IFA_ADDRESS):

   ip address add 192.168.5.1 peer 0.0.0.0/24 dev dummy

which is distinct from a usual address like:

   ip address add 192.168.5.1/24 dev dummy
   ip address add 192.168.5.1 peer 192.168.5.1/24 dev dummy

For IPv4, a missing IFA_ADDRESS attribute means that the peer
is 0.0.0.0. See inet_fill_ifaddr(), which does:

  if ((ifa->ifa_address &&
       nla_put_in_addr(skb, IFA_ADDRESS, ifa->ifa_address)) ||

Signed-off-by: Thomas Haller <redacted>
---
 ip/ipaddress.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index f290205..6fc4520 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c

@@ -949,7 +949,8 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 
 	if (!rta_tb[IFA_LOCAL])
 		rta_tb[IFA_LOCAL] = rta_tb[IFA_ADDRESS];
-	if (!rta_tb[IFA_ADDRESS])
+	if (!rta_tb[IFA_ADDRESS] &&
+	    ifa->ifa_family != AF_INET)
 		rta_tb[IFA_ADDRESS] = rta_tb[IFA_LOCAL];
 
 	if (filter.ifindex && filter.ifindex != ifa->ifa_index)

@@ -1034,16 +1035,18 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 						RTA_DATA(rta_tb[IFA_LOCAL]),
 						abuf, sizeof(abuf)));
 
-		if (rta_tb[IFA_ADDRESS] == NULL ||
+		if (rta_tb[IFA_ADDRESS] != NULL &&
 		    memcmp(RTA_DATA(rta_tb[IFA_ADDRESS]), RTA_DATA(rta_tb[IFA_LOCAL]),
 			   ifa->ifa_family == AF_INET ? 4 : 16) == 0) {
 			fprintf(fp, "/%d ", ifa->ifa_prefixlen);
 		} else {
 			fprintf(fp, " peer %s/%d ",
-				format_host(ifa->ifa_family,
-					    RTA_PAYLOAD(rta_tb[IFA_ADDRESS]),
-					    RTA_DATA(rta_tb[IFA_ADDRESS]),
-					    abuf, sizeof(abuf)),
+				rta_tb[IFA_ADDRESS]
+					? format_host(ifa->ifa_family,
+						      RTA_PAYLOAD(rta_tb[IFA_ADDRESS]),
+						      RTA_DATA(rta_tb[IFA_ADDRESS]),
+						      abuf, sizeof(abuf))
+					: "0.0.0.0",
 				ifa->ifa_prefixlen);
 		}
 	}

-- 
2.4.3

On Thu, Oct 22, 2015 at 10:34:28AM +0200, Thomas Haller wrote:

Kernel allows for zero IPv4 peer addresses (IFA_ADDRESS):

   ip address add 192.168.5.1 peer 0.0.0.0/24 dev dummy

which is distinct from a usual address like:

   ip address add 192.168.5.1/24 dev dummy
   ip address add 192.168.5.1 peer 192.168.5.1/24 dev dummy

For IPv4, a missing IFA_ADDRESS attribute means that the peer
is 0.0.0.0. See inet_fill_ifaddr(), which does:

  if ((ifa->ifa_address &&
       nla_put_in_addr(skb, IFA_ADDRESS, ifa->ifa_address)) ||

Signed-off-by: Thomas Haller <redacted>

Acked-by: Phil Sutter <phil@nwl.cc>

I wonder why this is not the case for IPv6. What is the functional
effect of setting a zero peer with non-zero netmask?

Cheers, Phil

On Thu, 22 Oct 2015 10:34:28 +0200
Thomas Haller [off-list ref] wrote:

Kernel allows for zero IPv4 peer addresses (IFA_ADDRESS):

   ip address add 192.168.5.1 peer 0.0.0.0/24 dev dummy

which is distinct from a usual address like:

   ip address add 192.168.5.1/24 dev dummy
   ip address add 192.168.5.1 peer 192.168.5.1/24 dev dummy

For IPv4, a missing IFA_ADDRESS attribute means that the peer
is 0.0.0.0. See inet_fill_ifaddr(), which does:

  if ((ifa->ifa_address &&
       nla_put_in_addr(skb, IFA_ADDRESS, ifa->ifa_address)) ||

Signed-off-by: Thomas Haller <redacted>

I would prefer that this apply to both IPv4 and IPv6.
If the kernel sends back an address, then display it.

On Mon, Nov 23, 2015 at 04:04:50PM -0800, Stephen Hemminger wrote:

On Thu, 22 Oct 2015 10:34:28 +0200
Thomas Haller [off-list ref] wrote:

quoted

Kernel allows for zero IPv4 peer addresses (IFA_ADDRESS):

   ip address add 192.168.5.1 peer 0.0.0.0/24 dev dummy

which is distinct from a usual address like:

   ip address add 192.168.5.1/24 dev dummy
   ip address add 192.168.5.1 peer 192.168.5.1/24 dev dummy

For IPv4, a missing IFA_ADDRESS attribute means that the peer
is 0.0.0.0. See inet_fill_ifaddr(), which does:

  if ((ifa->ifa_address &&
       nla_put_in_addr(skb, IFA_ADDRESS, ifa->ifa_address)) ||

Signed-off-by: Thomas Haller <redacted>

I would prefer that this apply to both IPv4 and IPv6.

The case that patch handles does not happen in IPv6.

If the kernel sends back an address, then display it.

It's rather "if the kernel *does not* send back an address ...".

When reviewing this patch, I tried to find an easier (and less ugly)
solution, but failed.  Here's the result from testing all variants:

1) ip a a 192.168.1.1/24                             dev test0
2) ip a a 192.168.2.1        peer 192.168.2.1/24     dev test0
3) ip a a 192.168.3.1        peer 0.0.0.0/24         dev test0
4) ip a a 192.168.4.1        peer 192.168.4.2        dev test0
5) ip a a feed:babe::1:1/112                         dev test0
6) ip a a feed:babe::2:1     peer feed:babe::2:1/112 dev test0
7) ip a a feed:babe::3:1     peer ::/112             dev test0
8) ip a a feed:babe::4:1     peer feed:babe::4:2     dev test0

cmd ifa_local      ifa_address
---------------------------------
1)  192.168.1.1    192.168.1.1
2)  192.168.2.1    192.168.2.1
3)  192.168.3.1    unset
4)  192.168.4.1    192.168.4.2
5)  unset          feed:babe::1:1
6)  unset          feed:babe::2:1
7)  unset          feed:babe::3:1
8)  feed:babe::4:1 feed:babe::4:2

No idea how this looks for decnet and ipx. Looking only at IPv6 though,
the patch's check for !AF_INET before setting rta_tb[IFA_ADDRESS] =
rta_tb[IFA_LOCAL] could indeed be skipped.

On a side note, I'm pretty sure the later memcmp() could be skipped in
many cases, at least by comparing the pointer values of
rta_tb[IFA_ADDRESS] and rta_tb[IFA_LOCAL].

Cheers, Phil

On Tue, 2015-11-24 at 12:44 +0100, Phil Sutter wrote:

On Mon, Nov 23, 2015 at 04:04:50PM -0800, Stephen Hemminger wrote:

quoted

On Thu, 22 Oct 2015 10:34:28 +0200
Thomas Haller [off-list ref] wrote:

quoted

Kernel allows for zero IPv4 peer addresses (IFA_ADDRESS):

   ip address add 192.168.5.1 peer 0.0.0.0/24 dev dummy

which is distinct from a usual address like:

   ip address add 192.168.5.1/24 dev dummy
   ip address add 192.168.5.1 peer 192.168.5.1/24 dev dummy

For IPv4, a missing IFA_ADDRESS attribute means that the peer
is 0.0.0.0. See inet_fill_ifaddr(), which does:

  if ((ifa->ifa_address &&
       nla_put_in_addr(skb, IFA_ADDRESS, ifa->ifa_address)) ||

Signed-off-by: Thomas Haller <redacted>

I would prefer that this apply to both IPv4 and IPv6.

The case that patch handles does not happen in IPv6.

quoted

If the kernel sends back an address, then display it.

It's rather "if the kernel *does not* send back an address ...".

When reviewing this patch, I tried to find an easier (and less ugly)
solution, but failed.  Here's the result from testing all variants:

Thank you Phil for evaluating this.

I also tried to change the patch to come up with a cleaner/unified way,
but the result wasn't subjectively better.


Thomas






For reference, the relevant lines of code from kernel:



ipv4: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/ipv4/devinet.c?id=6a13feb9c82803e2b815eca72fa7a9f5561d7861#n1542

        if ((ifa->ifa_address &&
             nla_put_in_addr(skb, IFA_ADDRESS, ifa->ifa_address)) ||
            (ifa->ifa_local &&
             nla_put_in_addr(skb, IFA_LOCAL, ifa->ifa_local)) ||



ipv6: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/addrconf.c?id=6a13feb9c82803e2b815eca72fa7a9f5561d7861#n4310

        if (!ipv6_addr_any(&ifa->peer_addr)) {
                if (nla_put_in6_addr(skb, IFA_LOCAL, &ifa->addr) < 0 ||
                    nla_put_in6_addr(skb, IFA_ADDRESS, &ifa->peer_addr) < 0)
                        goto error;
        } else
                if (nla_put_in6_addr(skb, IFA_ADDRESS, &ifa->addr) < 0)
                        goto error;