netdev - Re: [PATCH net] net: try harder to not reuse ifindex when moving interfaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 22 Oct 2015 17:21:52 +0200
From:	Thomas Haller <thaller@...hat.com>
To:	nicolas.dichtel@...nd.com,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	David Miller <davem@...emloft.net>, jbenc@...hat.com
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH net] net: try harder to not reuse ifindex when moving
 interfaces

On Thu, 2015-10-22 at 16:52 +0200, Nicolas Dichtel wrote:
> Le 21/10/2015 19:12, Hannes Frederic Sowa a écrit :
> > Hello,
> > 
> > On Wed, Oct 21, 2015, at 17:56, David Miller wrote:
> > > From: Jiri Benc <jbenc@...hat.com>
> > > Date: Wed, 21 Oct 2015 17:25:02 +0200
> > > 
> > > > On Wed, 21 Oct 2015 08:32:14 -0700 (PDT), David Miller wrote:
> > > > > As you say the apps are broken, so file a bug and have them
> > > > > fixed.
> > > > > 
> > > > > The assumption is clearly invalid, so apps cannot make such
> > > > > an
> > > > > assumption.
> > > > 
> > > > Does it mean you would be okay with a patch that always
> > > > allocates and
> > > > assigns a new ifindex in the target netns when interface is
> > > > moved
> > > > between name spaces?
> > > 
> > > I think you're misunderstanding me if you're still recommending
> > > kernel changes.
> > > 
> > > I'm plainly saying to remove the assumption in the apps.
> > > 
> > > If you don't show me exactly how some kernel change can lead to
> > > the apps implementing things properly, without the invalid
> > > assumptions, then I can only assume you didn't hear what I
> > > said.
> > 
> > I think the reason why ifindexes exists as ints is that we want to
> > have
> > lightweight way to refer to interfaces without taking references or
> > timestamps or generation ids which completely remove the
> > possibility for
> > races. But the racy nature in ifindexes is something we actually
> > want,
> > otherwise a user space program acquiring an ifindex would need to
> > get a
> > reference on the device and either during socket close or program
> > termination release it, that would be very costly.
> > 
> > This patch minimizes the race quite a lot, from something we could
> > actually see in everydays container creation to probably something
> > only
> > some users will expire with depleting the ifindex pool or playing
> > around
> > with CRIU.
> > 
> > We could come up with more heavy machinery to close the race
> > further for
> > CRIU by keeping track of "poisoned" ifindexes, which would need a
> > hashmap which could become pretty big and we could recycle when
> > ifindex
> > wraps around, but this seems too heavy weight to me.
> > 
> > I am in favor of a solution to minimize this race in the kernel
> > even
> > though we cannot ever close it completely.
> I probably miss something, but if the app listens netlink, I don't
> see how such
> app may have a race window.


Userspace uses the ifindex as identifier for the interface, for example
when changing a link or adding and IP address.

(1) Say, the application listens on netlink and learns about an
interface (and its ifindex).

(2) Immediately it does something with the ifindex (e.g. IF_UP the
interface).


Between (1) and (2) is a possibility of a race, that the application
cannot avoid. It can only hurry and hope for the best.



Thomas





Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)