From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/8929 Path: news.gmane.org!not-for-mail From: Timo Teras Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCHv2] properly handle point-to-point interfaces in getifaddrs() Date: Mon, 23 Nov 2015 11:12:49 +0200 Message-ID: <20151123111249.56da0630@vostro> References: <1447756429-2304-2-git-send-email-jow@openwrt.org> <1447965790-8151-1-git-send-email-jow@openwrt.org> <20151121111446.6d23921c@vostro> <20151121192035.GC3818@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1448270012 26216 80.91.229.3 (23 Nov 2015 09:13:32 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 23 Nov 2015 09:13:32 +0000 (UTC) Cc: musl@lists.openwall.com To: Rich Felker Original-X-From: musl-return-8942-gllmg-musl=m.gmane.org@lists.openwall.com Mon Nov 23 10:13:16 2015 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1a0nBL-0007Vb-R4 for gllmg-musl@m.gmane.org; Mon, 23 Nov 2015 10:13:07 +0100 Original-Received: (qmail 32249 invoked by uid 550); 23 Nov 2015 09:13:06 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 32231 invoked from network); 23 Nov 2015 09:13:05 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=GR2ZEjDCHf4gyXhGpTl7yHN5jjGPTkssRzFgdgV6tX0=; b=u3EXBPBRmO1ZeamxWEnO5cEuKDgbWh3ibzoq+6hTQGDBYWvkmpOw/zvwqNwmB34WBR AWQeIczjUOuXYUmctUswqNaIYyLs/RqDK0Me2BD1N+3nzbfdH0XEgyRhL8m/VIRo2ErC RSaCn8Zewg4yVcZaHQqY2rXuMKoGQSM5je/A9w5pWAtx20PAHwcjF4QlXof8zwnNLj3l 1Zsvlaq7S/0Nx5cQuED5Ks+0b+5meH0x0npArVFlz2loXtJy9e0eexTGew6ZqW0obcXd +1MhPA68pSmaiKvOgz3v/ZzEOHVgpq1/atnQpllTTeKFIIe4kHvyqksZno7vD68dRMmH 3plQ== X-Received: by 10.25.38.2 with SMTP id m2mr10852940lfm.113.1448269974266; Mon, 23 Nov 2015 01:12:54 -0800 (PST) Original-Sender: =?UTF-8?Q?Timo_Ter=C3=A4s?= In-Reply-To: <20151121192035.GC3818@brightrain.aerifal.cx> X-Mailer: Claws Mail 3.13.0 (GTK+ 2.24.28; x86_64-alpine-linux-musl) Xref: news.gmane.org gmane.linux.lib.musl.general:8929 Archived-At: On Sat, 21 Nov 2015 14:20:35 -0500 Rich Felker 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 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 > > > --- > > > 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?