From owner-freebsd-virtualization@FreeBSD.ORG Sun Jul 8 20:53:00 2012 Return-Path: Delivered-To: freebsd-virtualization@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 8F450106566C; Sun, 8 Jul 2012 20:53:00 +0000 (UTC) (envelope-from bzeeb-lists@lists.zabbadoz.net) Received: from mx1.sbone.de (bird.sbone.de [46.4.1.90]) by mx1.freebsd.org (Postfix) with ESMTP id 300678FC1F; Sun, 8 Jul 2012 20:53:00 +0000 (UTC) Received: from dhcp-128-232-132-170.eduroam.csx.cam.ac.uk (dhcp-128-232-132-170.eduroam.csx.cam.ac.uk [128.232.132.170]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mx1.sbone.de (Postfix) with ESMTPSA id 3524B25D3891; Sun, 8 Jul 2012 20:52:57 +0000 (UTC) Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: "Bjoern A. Zeeb" In-Reply-To: <86obnqq94x.fsf@kopusha.home.net> Date: Sun, 8 Jul 2012 20:52:55 +0000 Content-Transfer-Encoding: quoted-printable Message-Id: <50CFED43-7789-4F27-9EC7-85268B7F23D4@lists.zabbadoz.net> References: <4FF32FC4.6020701@delphij.net> <86wr2kau38.fsf@in138.ua3> <4FF5E87C.2020908@delphij.net> <86r4sqasrt.fsf@kopusha.home.net> <672D93D3-D4B1-432E-AE53-98E6C05B8BE4@lists.zabbadoz.net> <86zk7da10y.fsf@in138.ua3> <86obnqq94x.fsf@kopusha.home.net> To: Mikolaj Golub X-Mailer: Apple Mail (2.1084) Cc: d@delphij.net, FreeBSD virtualization mailing list Subject: Re: GPF when doing jail -r, possibly an use-after-free X-BeenThere: freebsd-virtualization@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Discussion of various virtualization techniques FreeBSD supports." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 08 Jul 2012 20:53:00 -0000 On 8. Jul 2012, at 20:40 , Mikolaj Golub wrote: >=20 > On Sat, 7 Jul 2012 20:38:23 +0000 Bjoern A. Zeeb wrote: >=20 > BAZ> On 6. Jul 2012, at 05:53 , Mikolaj Golub wrote: >=20 >>>=20 >>> On Thu, 5 Jul 2012 20:21:53 +0000 Bjoern A. Zeeb wrote: >>>=20 >>> BAZ> On 5. Jul 2012, at 19:53 , Mikolaj Golub wrote: >>>=20 >>>>>=20 >>>>> On Thu, 05 Jul 2012 12:18:20 -0700 Xin Li wrote: >>>>>=20 >>>>> XL> Hi, Mikolaj, >>>>>=20 >>>>> XL> On 07/04/12 00:00, Mikolaj Golub wrote: >>>>>>> Is this observed after destroying epair? There is an issue with >>>>>>> epair: on destroy, when epair_clone_destroy() calls >>>>>>> ether_ifdetach() for its second half it does not switch to its = vnet >>>>>>> and if_detach_internal() can't find the interface and just = returns. >>>>>>> As a result V_ifnet list is left with dead reference. >>>>>=20 >>>>> XL> Yes. >>>>>=20 >>>>>>> = http://lists.freebsd.org/pipermail/freebsd-virtualization/2011-January/000= 628.html >>>>>>>=20 >>>>>>> Here is an updated patch against CURRENT: >>>>>>>=20 >>>>>>> = http://people.freebsd.org/~trociny/if_epair.c.epair_clone_destroy.1.patch >>>>>=20 >>>>> XL> Your >>>>>>>=20 >>>>> XL> patch did fixed the problem, thanks! Are you going to commit = it >>>>> XL> against -HEAD and then MFC after a while? >>>>>=20 >>>>> I would like Bjoern review it before me committing, or at least = tell he does >>>>> not mind, if he does not have time to review -) >>>=20 >>> BAZ> To me the patch looks wrong; I am wondering if someone broke = some other central >>> BAZ> assumptions but given I cannot currently spend time on this and = if it fixes things >>> BAZ> feel free to go ahead. >>>=20 >>> If you told what looks wrong I could try to dig at that direction = and might be >>> back with a better solution, instead of committing a presumably = wrong fix. >=20 > BAZ> sorry; vnet.c:vnet_destroy() should dtrt already wrt to = interfaces moving to parents > BAZ> and being detached. >=20 > But this is not the issue with vnet_destroy() not moving interfaces to > parents. It does move them. It is with destroying epair. When epair = that has > its ends in different vnets is destroyed, and ether_ifdetach() is = called for > the second end without switching to its vnet it fails to find the = iface in the > wrong vnet and just silently returns (which I think is also wrong: > if_detach_internal() should panic here). As a result the pointer is = not > removed from vnet ifnet list. And later, when someone is traversing = this list > and tries to access the pointer (this is often vnet_destroy(), which = is > usually called after removing interfaces, but might be e.g. ifconfig) = dead > pointer dereference occurs. >=20 > My patch just makes epair_clone_destroy() switch to the proper vnet = before > calling ether_ifdetach(). >=20 > Or have I missed your point? Maybe: Situation 1) epairNa is in base, eiparNb is jail foo stop jail foo: jail -r foo both epairN[ab] will live in base and can be destiryed without = vnet switching Situation 2) epairNa is in base, eiparNb is jail foo you are in jail foo and type epairNb destroy; that should not = be allowed Situation 3) epairNa is in base, eiparNb is jail foo you are in base and type ifconfig epairNa destroy This is your case ... I am not sure what I'd expect in this = case, especailly given epair is special... You probably are right. Ideally I'd not allow it to be destroyed unless both are in the if_home_vnet. However it seems we allow this; so in that case I definitively make sure to use the CURVNET_SET_QUIET() version to avoid the expected noise otherwise. The moment cloners will handle this it'll all be centrally managed and individual device drivers shouldn't need to worry about it anymore. /bz --=20 Bjoern A. Zeeb You have to have visions! It does not matter how good you are. It matters what good you do!