Fix handling of peer-to-peer interfaces in getifaddrs()

Fix handling of peer-to-peer interfaces in getifaddrs()

[Jo-Philipp Wich]

properly handle point-to-point interfaces in getifaddrs()

With point-to-point interfaces, the IFA_ADDRESS netlink attribute contains
the peer address while an extra attribute IFA_LOCAL carries the actual local
interface address.

Currently musl lacks any treatment of IFA_LOCAL leading to bogus results
when using getifaddrs() to obtain the local and remote IP addresses of
point-to-point interfaces like ppp ones.

The following test case illustrates the problem:

-- 8< --
#include <netinet/in.h>
#include <arpa/inet.h>
#include <string.h>
#include <sys/types.h>
#include <ifaddrs.h>
#include <stdio.h>
 
int main(int argc, char **argv) {
        struct ifaddrs *ifa, *ifap;
        char local[32];
        char remote[32];
 
        if (!getifaddrs(&ifa)) {
                for (ifap = ifa; ifap; ifap = ifap->ifa_next) {
                        if (!ifap->ifa_name || strcmp(ifap->ifa_name, argv[1]) ||
                            !ifap->ifa_addr || ifap->ifa_addr->sa_family != AF_INET)
                                continue;
 
                        inet_ntop(AF_INET, &((struct sockaddr_in *)ifap->ifa_addr)->sin_addr, local, sizeof(local));
 
                        if (ifap->ifa_dstaddr)
                                inet_ntop(AF_INET, &((struct sockaddr_in *)ifap->ifa_dstaddr)->sin_addr, remote, sizeof(remote));
                        else
                                strcpy(remote, "(null)");
 
                        printf("local addr = %s / remote addr = %s\n", local, remote);
                        break;
                }
 
                freeifaddrs(ifa);
        }
 
        return 0;
}
-- >8 --

I used the following command sequence to assert the results:

$ gcc -o /tmp/ifa /tmp/ifa.c
$ ip link add name dummy0 type dummy
$ ip addr add 1.1.1.1 peer 2.2.2.2 dev dummy0
$ /tmp/ifa dummy0

That led to the following result on musl:

  local addr = 2.2.2.2 / remote addr = (null)

Note that the local address is unexpectedly 2.2.2.2 while it should be 1.1.1.1
and the remote address is not set at all.

Running the same test on an ordinary glibc system (Debian) and on an uclibc
based OpenWrt gives the correct result:

  local addr = 1.1.1.1 / remote addr = 2.2.2.2


The proposed change adds special treatment of IFA_LOCAL to getifaddrs(),
following the logic used in uclibc and glibc implementations.

Reagrds,
Jo-Philipp

[PATCH] properly handle point-to-point interfaces in getifaddrs()

[Jo-Philipp Wich]

With point-to-point interfaces, the IFA_ADDRESS netlink attribute contains
the peer address while an extra attribute IFA_LOCAL carries the actual local
interface address.

Both the glibc and uclibc implementations of getifaddrs() handle this case
by moving the ifa_addr contents to the broadcast/remote address union and
overwriting ifa_addr upon receipt of an IFA_LOCAL attribute.

This patch adds the same special treatment logic of IFA_LOCAL to musl's
implementation of getifaddrs() in order to align its behaviour with that
of uclibc and musl.

Signed-off-by: Jo-Philipp Wich <jow@openwrt.org>
---
 src/network/getifaddrs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/network/getifaddrs.c b/src/network/getifaddrs.c
index 89a8f72..9a610c2 100644
--- a/src/network/getifaddrs.c
+++ b/src/network/getifaddrs.c
@@ -161,6 +161,15 @@ static int netlink_msg_to_ifaddr(void *pctx, struct nlmsghdr *h)
 		ifs->ifa.ifa_flags = ifs0->ifa.ifa_flags;
 		for (rta = NLMSG_RTA(h, sizeof(*ifa)); NLMSG_RTAOK(rta, h); rta = RTA_NEXT(rta)) {
 			switch (rta->rta_type) {
+			case IFA_LOCAL:
+				/* If ifa_addr is set and we get IFA_LOCAL, assume we have
+				 * a point-to-point network. Move address to correct field.  */
+				if (ifs->ifa.ifa_addr) {
+					ifs->ifu = ifs->addr;
+					ifs->ifa.ifa_dstaddr = &ifs->ifu.sa;
+					memset(&ifs->addr, 0, sizeof(ifs->addr));
+				}
+				/* fall through */
 			case IFA_ADDRESS:
 				copy_addr(&ifs->ifa.ifa_addr, ifa->ifa_family, &ifs->addr, RTA_DATA(rta), RTA_DATALEN(rta), ifa->ifa_index);
 				break;
-- 
2.1.4

[PATCHv2] properly handle point-to-point interfaces in getifaddrs()

[Jo-Philipp Wich]

With point-to-point interfaces, the IFA_ADDRESS netlink attribute contains
the peer address while an extra attribute IFA_LOCAL carries the actual local
interface address.

Both the glibc and uclibc implementations of getifaddrs() handle this case
by moving the ifa_addr contents to the broadcast/remote address union and
overwriting ifa_addr upon receipt of an IFA_LOCAL attribute.

This patch adds the same special treatment logic of IFA_LOCAL to musl's
implementation of getifaddrs() in order to align its behaviour with that
of uclibc and musl.

Signed-off-by: Jo-Philipp Wich <jow@openwrt.org>
---
Changelog v2:
 * Handle IFA_LOCAL, IFA_ADDRESS in arbritary order
 * Remove misleading comment for IFA_BROADCAST, no such attribute on ptp links
---
 src/network/getifaddrs.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/network/getifaddrs.c b/src/network/getifaddrs.c
index 89a8f72..fed75bd 100644
--- a/src/network/getifaddrs.c
+++ b/src/network/getifaddrs.c
@@ -162,13 +162,26 @@ static int netlink_msg_to_ifaddr(void *pctx, struct nlmsghdr *h)
 		for (rta = NLMSG_RTA(h, sizeof(*ifa)); NLMSG_RTAOK(rta, h); rta = RTA_NEXT(rta)) {
 			switch (rta->rta_type) {
 			case IFA_ADDRESS:
-				copy_addr(&ifs->ifa.ifa_addr, ifa->ifa_family, &ifs->addr, RTA_DATA(rta), RTA_DATALEN(rta), ifa->ifa_index);
+				/* If ifa_addr is already set we, received an IFA_LOCAL before
+				 * so treat this as destination address */
+				if (ifs->ifa.ifa_addr)
+					copy_addr(&ifs->ifa.ifa_dstaddr, ifa->ifa_family, &ifs->ifu, RTA_DATA(rta), RTA_DATALEN(rta), ifa->ifa_index);
+				else
+					copy_addr(&ifs->ifa.ifa_addr, ifa->ifa_family, &ifs->addr, RTA_DATA(rta), RTA_DATALEN(rta), ifa->ifa_index);
 				break;
 			case IFA_BROADCAST:
-				/* For point-to-point links this is peer, but ifa_broadaddr
-				 * and ifa_dstaddr are union, so this works for both.  */
 				copy_addr(&ifs->ifa.ifa_broadaddr, ifa->ifa_family, &ifs->ifu, RTA_DATA(rta), RTA_DATALEN(rta), ifa->ifa_index);
 				break;
+			case IFA_LOCAL:
+				/* If ifa_addr is set and we get IFA_LOCAL, assume we have
+				 * a point-to-point network. Move address to correct field. */
+				if (ifs->ifa.ifa_addr) {
+					ifs->ifu = ifs->addr;
+					ifs->ifa.ifa_dstaddr = &ifs->ifu.sa;
+					memset(&ifs->addr, 0, sizeof(ifs->addr));
+				}
+				copy_addr(&ifs->ifa.ifa_addr, ifa->ifa_family, &ifs->addr, RTA_DATA(rta), RTA_DATALEN(rta), ifa->ifa_index);
+				break;
 			case IFA_LABEL:
 				if (RTA_DATALEN(rta) < sizeof(ifs->name)) {
 					memcpy(ifs->name, RTA_DATA(rta), RTA_DATALEN(rta));
-- 
2.1.4

Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs()

[Timo Teras]

On Thu, 19 Nov 2015 21:43:10 +0100
Jo-Philipp Wich <jow@openwrt.org> wrote:

> With point-to-point interfaces, the IFA_ADDRESS netlink attribute
> contains the peer address while an extra attribute IFA_LOCAL carries
> the actual local interface address.
> 
> Both the glibc and uclibc implementations of getifaddrs() handle this
> case by moving the ifa_addr contents to the broadcast/remote address
> union and overwriting ifa_addr upon receipt of an IFA_LOCAL attribute.
> 
> This patch adds the same special treatment logic of IFA_LOCAL to
> musl's implementation of getifaddrs() in order to align its behaviour
> with that of uclibc and musl.
> 
> Signed-off-by: Jo-Philipp Wich <jow@openwrt.org>
> ---
> Changelog v2:
>  * Handle IFA_LOCAL, IFA_ADDRESS in arbritary order
>  * Remove misleading comment for IFA_BROADCAST, no such attribute on
> ptp links ---
>  src/network/getifaddrs.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

I wrote the code before looking into how ptp links are reported, and
just assumed it'd be somehow consistent. But IFA_ADDRESS indeed is
the peer for ptp links. How nicely inconsistent from kernel side ;)

Seems iproute2 basically does:
 1. If IFA_LOCAL not set, copy IFA_ADDRESS to it
 2. If IFA_ADDRESS is not set, copy IFA_LOCAL to it
 3. Print IFA_LOCAL as local address
 4. Print IFA_ADDRESS as peer address if it's not equal to IFA_LOCAL

So this looks right to me.

Acked-by: Timo Teräs <timo.teras@iki.fi>

Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs()

[Rich Felker]

On Sat, Nov 21, 2015 at 11:14:46AM +0200, Timo Teras wrote:
> On Thu, 19 Nov 2015 21:43:10 +0100
> Jo-Philipp Wich <jow@openwrt.org> wrote:
> 
> > With point-to-point interfaces, the IFA_ADDRESS netlink attribute
> > contains the peer address while an extra attribute IFA_LOCAL carries
> > the actual local interface address.
> > 
> > Both the glibc and uclibc implementations of getifaddrs() handle this
> > case by moving the ifa_addr contents to the broadcast/remote address
> > union and overwriting ifa_addr upon receipt of an IFA_LOCAL attribute.
> > 
> > This patch adds the same special treatment logic of IFA_LOCAL to
> > musl's implementation of getifaddrs() in order to align its behaviour
> > with that of uclibc and musl.
> > 
> > Signed-off-by: Jo-Philipp Wich <jow@openwrt.org>
> > ---
> > Changelog v2:
> >  * Handle IFA_LOCAL, IFA_ADDRESS in arbritary order
> >  * Remove misleading comment for IFA_BROADCAST, no such attribute on
> > ptp links ---
> >  src/network/getifaddrs.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> I wrote the code before looking into how ptp links are reported, and
> just assumed it'd be somehow consistent. But IFA_ADDRESS indeed is
> the peer for ptp links. How nicely inconsistent from kernel side ;)
> 
> Seems iproute2 basically does:
>  1. If IFA_LOCAL not set, copy IFA_ADDRESS to it
>  2. If IFA_ADDRESS is not set, copy IFA_LOCAL to it
>  3. Print IFA_LOCAL as local address
>  4. Print IFA_ADDRESS as peer address if it's not equal to IFA_LOCAL
> 
> So this looks right to me.

Are you sure? The new patch seems to have exactly the same issue with
depending on the order of the records as the old patch had. To solve
it I think both need to be stored in temp storage during the loop,
then code after the loop has to resolve which to use. Am I missing
something?

Rich

Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs()

[Timo Teras]

On Sat, 21 Nov 2015 14:20:35 -0500
Rich Felker <dalias@libc.org> wrote:

> On Sat, Nov 21, 2015 at 11:14:46AM +0200, Timo Teras wrote:
> > On Thu, 19 Nov 2015 21:43:10 +0100
> > Jo-Philipp Wich <jow@openwrt.org> wrote:
> >   
> > > With point-to-point interfaces, the IFA_ADDRESS netlink attribute
> > > contains the peer address while an extra attribute IFA_LOCAL
> > > carries the actual local interface address.
> > > 
> > > Both the glibc and uclibc implementations of getifaddrs() handle
> > > this case by moving the ifa_addr contents to the broadcast/remote
> > > address union and overwriting ifa_addr upon receipt of an
> > > IFA_LOCAL attribute.
> > > 
> > > This patch adds the same special treatment logic of IFA_LOCAL to
> > > musl's implementation of getifaddrs() in order to align its
> > > behaviour with that of uclibc and musl.
> > > 
> > > Signed-off-by: Jo-Philipp Wich <jow@openwrt.org>
> > > ---
> > > Changelog v2:
> > >  * Handle IFA_LOCAL, IFA_ADDRESS in arbritary order
> > >  * Remove misleading comment for IFA_BROADCAST, no such attribute
> > > on ptp links ---
> > >  src/network/getifaddrs.c | 19 ++++++++++++++++---
> > >  1 file changed, 16 insertions(+), 3 deletions(-)  
> > 
> > I wrote the code before looking into how ptp links are reported, and
> > just assumed it'd be somehow consistent. But IFA_ADDRESS indeed is
> > the peer for ptp links. How nicely inconsistent from kernel side ;)
> > 
> > Seems iproute2 basically does:
> >  1. If IFA_LOCAL not set, copy IFA_ADDRESS to it
> >  2. If IFA_ADDRESS is not set, copy IFA_LOCAL to it
> >  3. Print IFA_LOCAL as local address
> >  4. Print IFA_ADDRESS as peer address if it's not equal to IFA_LOCAL
> > 
> > So this looks right to me.  
> 
> Are you sure? The new patch seems to have exactly the same issue with
> depending on the order of the records as the old patch had. To solve
> it I think both need to be stored in temp storage during the loop,
> then code after the loop has to resolve which to use. Am I missing
> something?

v2 patch in pseudo-code does:
  IFA_ADDRESS:
    if ifa_addr set earlier
      ifa_dstaddr = this
    else
      ifa_addr = this

  IFA_LOCAL:
    if ifa_addr set earlier
      ifa_dstaddr = ifa_addr
    ifa_addr = this   

so it does look right to me, and handles whatever order they are in:

IFA_ADDRESS then IFA_LOCAL:
  IFA_ADDRESS sets ifa_addr
  IFA_LOCAL moves ifa_addr to ifa_dstaddr and sets ifa_addr

IFA_LOCAL then IFA_ADDRESS:
  IFA_LOCAL sets ifa_addr
  IFA_ADDRESS sets ifa_dstaddr

The only side affect might be if you get two IFA_LOCAL addresses, the
first goes to ifa_dstaddr. But that's invalid input, kernel does not
create it, so I think we need to care about it.

It might be more obvious what is going on if we store the RTA info for
IFA_ADDRESS/IFA_LOCAL and do the logic after the loop. But functionally
it should be the same.

Or am I missing something?

Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs()

[Rich Felker]

On Mon, Nov 23, 2015 at 11:12:49AM +0200, Timo Teras wrote:
> On Sat, 21 Nov 2015 14:20:35 -0500
> Rich Felker <dalias@libc.org> wrote:
> 
> > On Sat, Nov 21, 2015 at 11:14:46AM +0200, Timo Teras wrote:
> > > On Thu, 19 Nov 2015 21:43:10 +0100
> > > Jo-Philipp Wich <jow@openwrt.org> wrote:
> > >   
> > > > With point-to-point interfaces, the IFA_ADDRESS netlink attribute
> > > > contains the peer address while an extra attribute IFA_LOCAL
> > > > carries the actual local interface address.
> > > > 
> > > > Both the glibc and uclibc implementations of getifaddrs() handle
> > > > this case by moving the ifa_addr contents to the broadcast/remote
> > > > address union and overwriting ifa_addr upon receipt of an
> > > > IFA_LOCAL attribute.
> > > > 
> > > > This patch adds the same special treatment logic of IFA_LOCAL to
> > > > musl's implementation of getifaddrs() in order to align its
> > > > behaviour with that of uclibc and musl.
> > > > 
> > > > Signed-off-by: Jo-Philipp Wich <jow@openwrt.org>
> > > > ---
> > > > Changelog v2:
> > > >  * Handle IFA_LOCAL, IFA_ADDRESS in arbritary order
> > > >  * Remove misleading comment for IFA_BROADCAST, no such attribute
> > > > on ptp links ---
> > > >  src/network/getifaddrs.c | 19 ++++++++++++++++---
> > > >  1 file changed, 16 insertions(+), 3 deletions(-)  
> > > 
> > > I wrote the code before looking into how ptp links are reported, and
> > > just assumed it'd be somehow consistent. But IFA_ADDRESS indeed is
> > > the peer for ptp links. How nicely inconsistent from kernel side ;)
> > > 
> > > Seems iproute2 basically does:
> > >  1. If IFA_LOCAL not set, copy IFA_ADDRESS to it
> > >  2. If IFA_ADDRESS is not set, copy IFA_LOCAL to it
> > >  3. Print IFA_LOCAL as local address
> > >  4. Print IFA_ADDRESS as peer address if it's not equal to IFA_LOCAL
> > > 
> > > So this looks right to me.  
> > 
> > Are you sure? The new patch seems to have exactly the same issue with
> > depending on the order of the records as the old patch had. To solve
> > it I think both need to be stored in temp storage during the loop,
> > then code after the loop has to resolve which to use. Am I missing
> > something?
> 
> v2 patch in pseudo-code does:
>   IFA_ADDRESS:
>     if ifa_addr set earlier
>       ifa_dstaddr = this
>     else
>       ifa_addr = this
> 
>   IFA_LOCAL:
>     if ifa_addr set earlier
>       ifa_dstaddr = ifa_addr
>     ifa_addr = this   
> 
> so it does look right to me, and handles whatever order they are in:
> 
> IFA_ADDRESS then IFA_LOCAL:
>   IFA_ADDRESS sets ifa_addr
>   IFA_LOCAL moves ifa_addr to ifa_dstaddr and sets ifa_addr
> 
> IFA_LOCAL then IFA_ADDRESS:
>   IFA_LOCAL sets ifa_addr
>   IFA_ADDRESS sets ifa_dstaddr
> 
> The only side affect might be if you get two IFA_LOCAL addresses, the
> first goes to ifa_dstaddr. But that's invalid input, kernel does not
> create it, so I think we need to care about it.

Thanks for explaining it. I think you're right.

> It might be more obvious what is going on if we store the RTA info for
> IFA_ADDRESS/IFA_LOCAL and do the logic after the loop. But functionally
> it should be the same.

Yes, this would be more clear and was the fix I expected, but I'm okay
with this version as long as it's correct.

Any further comments before I apply this?

Rich

Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs()

[Rich Felker]

On Thu, Nov 19, 2015 at 09:43:10PM +0100, Jo-Philipp Wich wrote:
> With point-to-point interfaces, the IFA_ADDRESS netlink attribute contains
> the peer address while an extra attribute IFA_LOCAL carries the actual local
> interface address.
> 
> Both the glibc and uclibc implementations of getifaddrs() handle this case
> by moving the ifa_addr contents to the broadcast/remote address union and
> overwriting ifa_addr upon receipt of an IFA_LOCAL attribute.
> 
> This patch adds the same special treatment logic of IFA_LOCAL to musl's
> implementation of getifaddrs() in order to align its behaviour with that
> of uclibc and musl.
                ^^^^ glibc

Committed, fixing this and line folding in the commit message;
otherwise, unchanged. Thanks!

Rich