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

[Rich Felker <dalias@libc.org>]

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