From owner-svn-src-head@freebsd.org Wed Dec 14 16:47:13 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E6FFAC80CBC; Wed, 14 Dec 2016 16:47:13 +0000 (UTC) (envelope-from royger@gmail.com) Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 8C9ABE23; Wed, 14 Dec 2016 16:47:13 +0000 (UTC) (envelope-from royger@gmail.com) Received: by mail-wm0-x241.google.com with SMTP id g23so378666wme.1; Wed, 14 Dec 2016 08:47:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=T97Gta3wstkBhWadqzKBpNgMkkx5ZcpptRdI9wMSsKA=; b=FexxSBQzthoS+hGAfEszg11QBgRxknM3vlmKL5Y01+1/oA0wZD/85FXtoq9GjCOyQm G+c6/gdJo+hJPHh0SWXkN84wu8SHqxWhwaEkINAGxecY6km9rHfSlIXckLFUUBcFj7ps Xi6RrzU0Z4fXm8mUk6Gl+QbldP7RLPvp68QRxgyxo1llvW8NNGg33XoJOCFuiPNrg5yo nEbwzO1lIDSrANMjOMUqCzBcEhkwVcyAdORl/mtCnZbk8ZeUp5qrg1WZXHRdaJGTuzuS TxUvUSZLKFyAvEuJdoNN1M/Rp+e91g+kvZTuHEEewXkmP0rd0EINNyak25gJKJiU4sf6 //3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=T97Gta3wstkBhWadqzKBpNgMkkx5ZcpptRdI9wMSsKA=; b=KBUgnBlx4hOBomMDf1MTdmEPETQBBRTdleGDbpqlt76OJS8LEE7vxYOg61ZJQ3OD4V 5I5VEDpXHH7+DVO5nT8bVVfbzwujaGaHbKAzRozXp2iPd46CS4Pg0Qk71Q6vJbtk65Wf r/Ec3LdSLmCX3s0Ve3QBg5PJa3iJDhf7AJcWsnSTQHAT30ktLY8qv/re87QhAvfbpcoi 9YmC28rNdhyS4CXmkThkunVRjhHDmMugB6txL2jRgXIYNqrB0joZxt6/+xgbJx+qQz1M w8loJx49WeR1khpOGtM+VvDZd6DRK/LMagvSEU5H2UgZffRYfjS7FzCcnX9JNRyZu3Qd 7LeA== X-Gm-Message-State: AKaTC01ypfPj2IuarznMUKXtFrEeeiM6VuJbiE4zinE6ryAyN1Myu1/mDirGd7Dda0Cm0A== X-Received: by 10.28.45.212 with SMTP id t203mr8104397wmt.46.1481734031234; Wed, 14 Dec 2016 08:47:11 -0800 (PST) Received: from localhost (default-46-102-197-194.interdsl.co.uk. [46.102.197.194]) by smtp.gmail.com with ESMTPSA id x7sm23961661wjp.18.2016.12.14.08.47.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 14 Dec 2016 08:47:10 -0800 (PST) Sender: =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= Date: Wed, 14 Dec 2016 16:47:05 +0000 From: Roger Pau =?iso-8859-1?Q?Monn=E9?= To: Dimitry Andric Cc: Colin Percival , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r310013 - head/sys/dev/xen/blkfront Message-ID: <20161214164704.grn5gvsm3u6difs2@dhcp-3-221.uk.xensource.com> References: <201612130654.uBD6sDtF073358@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20161126 (1.7.1) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Dec 2016 16:47:14 -0000 On Tue, Dec 13, 2016 at 09:28:21PM +0100, Dimitry Andric wrote: > On 13 Dec 2016, at 07:54, Colin Percival wrote: > > > > Author: cperciva > > Date: Tue Dec 13 06:54:13 2016 > > New Revision: 310013 > > URL: https://svnweb.freebsd.org/changeset/base/310013 > > > > Log: > > Check that blkfront devices have a non-zero number of sectors and a > > non-zero sector size. Such a device would be a virtual disk of zero > > bytes; clearly not useful, and not something we should try to attach. > > > > As a fortuitous side effect, checking that these values are non-zero > > here results in them not *becoming* zero later on the function. This > > odd behaviour began with r309124 (clang 3.9.0) but is challenging to > > debug; making any changes to this function whatsoever seems to affect > > the llvm optimizer behaviour enough to make the unexpected zeroing of > > the sector_size variable cease. > > I've been having some fun debugging a kernel under Xen, and what I found > is that sector_size changes from 512 to 0 because of an xs_gather() call > in xbd_connect(). The function starts as follows: > > 1222 static void > 1223 xbd_connect(struct xbd_softc *sc) > 1224 { > 1225 device_t dev = sc->xbd_dev; > 1226 unsigned long sectors, sector_size, phys_sector_size; > 1227 unsigned int binfo; > 1228 int err, feature_barrier, feature_flush; > ... > 1237 err = xs_gather(XST_NIL, xenbus_get_otherend_path(dev), > 1238 "sectors", "%lu", §ors, > 1239 "info", "%u", &binfo, > 1240 "sector-size", "%lu", §or_size, > 1241 NULL); > > After this first call to xs_gather(), sectors is 44040322 (in my case of > a ~21G disk), binfo is zero, and sector_size is 512. During execution > of the following few lines, sector_size stays 512, until just after the > fourth call to xs_gather(): > > 1259 err = xs_gather(XST_NIL, xenbus_get_otherend_path(dev), > 1260 "feature-flush-cache", "%lu", &feature_flush, > 1261 NULL); > > and then it becomes zero! This is because the %lu format scans a 64 bit > unsigned integer, while the feature_flush variable is a 32 bit signed > integer, thus overwriting an adjacent location on the stack, which > happens to contain sector_size: in the assembly output, sector_size is > at -88(%rbp), while feature_flush is at -92(%rbp). > > There is another instance of such an incorrect format, a few lines > before this, but it doesn't seem to scribble over important data (or we > didn't panic because of it, yet): > > 1253 err = xs_gather(XST_NIL, xenbus_get_otherend_path(dev), > 1254 "feature-barrier", "%lu", &feature_barrier, > 1255 NULL); > > Here, feature_barrier is a 32 bit signed integer, while %lu again scans > a 64 bit unsigned integer. > > In short, I think the following change would also fix the problem: > > Index: sys/dev/xen/blkfront/blkfront.c > =================================================================== > --- sys/dev/xen/blkfront/blkfront.c (revision 309817) > +++ sys/dev/xen/blkfront/blkfront.c (working copy) > @@ -1251,13 +1251,13 @@ xbd_connect(struct xbd_softc *sc) > if (err || phys_sector_size <= sector_size) > phys_sector_size = 0; > err = xs_gather(XST_NIL, xenbus_get_otherend_path(dev), > - "feature-barrier", "%lu", &feature_barrier, > + "feature-barrier", "%d", &feature_barrier, > NULL); > if (err == 0 && feature_barrier != 0) > sc->xbd_flags |= XBDF_BARRIER; > > err = xs_gather(XST_NIL, xenbus_get_otherend_path(dev), > - "feature-flush-cache", "%lu", &feature_flush, > + "feature-flush-cache", "%d", &feature_flush, > NULL); > if (err == 0 && feature_flush != 0) > sc->xbd_flags |= XBDF_FLUSH; > > However, I have some difficulty getting a custom-built kernel booting on > my very rudimentary Xen setup. I am using a snapshot raw image, with no > idea how to insert a kernel into it. > > E.g, can you please try this out, instead of the zero-check fix? I am > very curious whether that fixes the panics. :) Yes, this indeed fixes the panic, I've reverted Colin's patch and applied yours, and everything seems fine. Please go ahead and commit it. xs_gather should really go away and we should use something that we can sanitize at compile time using __scanflike & friends. Thanks, Roger.