From owner-svn-src-all@freebsd.org Fri Jun 21 20:02:05 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2EAB415BDC42; Fri, 21 Jun 2019 20:02:05 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 9F71276A6D; Fri, 21 Jun 2019 20:02:04 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 237BF22295; Fri, 21 Jun 2019 16:02:04 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Fri, 21 Jun 2019 16:02:04 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsco.org; h= content-type:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; s=fm3; bh=e 0qJDQNlRh0If7nhvaoDnFUD2F+nlA0CDplc1dt+Aj8=; b=oNb54BVF07iSWrtmU rOmLyClVZCoS+ykvsvDU05wFjLOYImZ4Auii0JPJO4Fx8RjpAw5ENvNbk0QeqQ/Z IB7KsWXN7yxjB5drRz3UVM5kmqdaGIrxtXuMtfU9yW8eezfPI9RldwSlddxqUXQg ww7rRDoGoe0rHwrnjH0fNQjJHJAW7QuAQS+SUd9KtjSkpcOAsg+hgcyiYGdpm1B6 A6T7gQmiFrTxucKmgHTHGcAmzbzIIFaRkBed2rfJEWvrDSAui0/Gwx4Vn8EKO0/1 5VMGtlxc6/T/050MBM9BLyt1qr5fDGIZizunD4eBzmreDmU+O/mQMgbuXbJ6kFSL ILknA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=e0qJDQNlRh0If7nhvaoDnFUD2F+nlA0CDplc1dt+A j8=; b=V5KQh8NI4nFaXn/TBVJlA4OlIPXTwMJE2XO5Vl/3ow/8gvDAeOoBsBME2 xZbvD0F8I4I4TZoOUlSR53f5e3Ki3Y2SreqOqeRkHqZKWTaYCWYd1C8kx2Md82mb /xHMjo/OM4Heg68ONFMkQoyWXhO3DEr0EDZKREQhD2sROkp3sTnHNw8ZQ80N/fEV YsjvwyOaL4/IX5iy/tN1l7Roewor9V7ozp56MrC1t6+c55JyjUBpgMWCQxlyQCwu zTOnsopkj04QgMdqZI7XYxs2TC7yM21GcRlp0IO3sfb2c+pcS4ugM3H7ryKeyBvy r2RjXESZyHfObe2JD+Wl8Ynxbez2g== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduvddrtdeigddugeekucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpegtggfuhfgjfffgkfhfvffosehtqh hmtdhhtdejnecuhfhrohhmpefutghothhtucfnohhnghcuoehstghothhtlhesshgrmhhs tghordhorhhgqeenucffohhmrghinhepfhhrvggvsghsugdrohhrghenucfkphepudelvd drheehrdehgedrheelnecurfgrrhgrmhepmhgrihhlfhhrohhmpehstghothhtlhesshgr mhhstghordhorhhgnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from [10.178.24.13] (unknown [192.55.54.59]) by mail.messagingengine.com (Postfix) with ESMTPA id 3A05A8005A; Fri, 21 Jun 2019 16:02:03 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: svn commit: r349243 - head/sys/cam From: Scott Long In-Reply-To: <42dbbc7f-51ea-bc3d-0432-5cc9b872d0e7@FreeBSD.org> Date: Fri, 21 Jun 2019 14:02:01 -0600 Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <201906202029.x5KKThjn017867@repo.freebsd.org> <6B266DFC-87FF-4742-ACEE-0D37EABEFD7A@samsco.org> <42dbbc7f-51ea-bc3d-0432-5cc9b872d0e7@FreeBSD.org> To: Alexander Motin X-Mailer: Apple Mail (2.3445.104.11) X-Rspamd-Queue-Id: 9F71276A6D X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.96 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.96)[-0.962,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Jun 2019 20:02:05 -0000 Hi Alexander, Thanks for the explanation, sorry I didn=E2=80=99t realize sooner that = it=E2=80=99s not used as a copyout buffer anymore. FWIW, that code is really hard to read on an 80 column window, would you be ok if it was refactored for better readability? Thanks, Scott > On Jun 20, 2019, at 6:04 PM, Alexander Motin wrote: >=20 > Hi Scott, >=20 > You may see this buffer content is not returned directly to user any > more. It is used only as temporary storage to pull > CDAI_TYPE_SCSI_DEVID, which is then converted into readable form, > returned to user. And the code is explicitly made to not read outside > the range that was copied to the buffer with simple and dumb memcpy(). >=20 > The effect is clearly visible in profiler when GEOM confxml calls it 3 > times for each disk, taking about 25% of the sysctl's CPU time. With > hundreds of disks it is not so small. >=20 > On 20.06.2019 18:59, Scott Long wrote: >> Hi Alexander, >>=20 >> I=E2=80=99m not a fan of removing the M_ZERO. I understand your = argument that >> lengths are provided elsewhere, but having the buffer be zero=E2=80=99d= is defensive >> against future bugs that might leak buffer contents. GETATTR isn=E2=80= =99t typically >> in a performance path, is there any measurable benefit to this? >>=20 >> Thanks, >> Scott >>=20 >>=20 >>> On Jun 20, 2019, at 2:29 PM, Alexander Motin = wrote: >>>=20 >>> Author: mav >>> Date: Thu Jun 20 20:29:42 2019 >>> New Revision: 349243 >>> URL: https://svnweb.freebsd.org/changeset/base/349243 >>>=20 >>> Log: >>> Optimize xpt_getattr(). >>>=20 >>> Do not allocate temporary buffer for attributes we are going to = return >>> as-is, just make sure to NUL-terminate them. Do not zero temporary = 64KB >>> buffer for CDAI_TYPE_SCSI_DEVID, XPT tells us how much data it = filled >>> and there are also length fields inside the returned data also. >>>=20 >>> MFC after: 2 weeks >>> Sponsored by: iXsystems, Inc. >>>=20 >>> Modified: >>> head/sys/cam/cam_xpt.c >>>=20 >>> Modified: head/sys/cam/cam_xpt.c >>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>> --- head/sys/cam/cam_xpt.c Thu Jun 20 20:06:19 2019 = (r349242) >>> +++ head/sys/cam/cam_xpt.c Thu Jun 20 20:29:42 2019 = (r349243) >>> @@ -1253,6 +1253,7 @@ xpt_getattr(char *buf, size_t len, const char = *attr, s >>> cdai.ccb_h.func_code =3D XPT_DEV_ADVINFO; >>> cdai.flags =3D CDAI_FLAG_NONE; >>> cdai.bufsiz =3D len; >>> + cdai.buf =3D buf; >>>=20 >>> if (!strcmp(attr, "GEOM::ident")) >>> cdai.buftype =3D CDAI_TYPE_SERIAL_NUM; >>> @@ -1262,14 +1263,14 @@ xpt_getattr(char *buf, size_t len, const = char *attr, s >>> strcmp(attr, "GEOM::lunname") =3D=3D 0) { >>> cdai.buftype =3D CDAI_TYPE_SCSI_DEVID; >>> cdai.bufsiz =3D CAM_SCSI_DEVID_MAXLEN; >>> + cdai.buf =3D malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT); >>> + if (cdai.buf =3D=3D NULL) { >>> + ret =3D ENOMEM; >>> + goto out; >>> + } >>> } else >>> goto out; >>>=20 >>> - cdai.buf =3D malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT|M_ZERO); >>> - if (cdai.buf =3D=3D NULL) { >>> - ret =3D ENOMEM; >>> - goto out; >>> - } >>> xpt_action((union ccb *)&cdai); /* can only be synchronous */ >>> if ((cdai.ccb_h.status & CAM_DEV_QFRZN) !=3D 0) >>> cam_release_devq(cdai.ccb_h.path, 0, 0, 0, FALSE); >>> @@ -1334,13 +1335,15 @@ xpt_getattr(char *buf, size_t len, const = char *attr, s >>> ret =3D EFAULT; >>> } >>> } else { >>> - ret =3D 0; >>> - if (strlcpy(buf, cdai.buf, len) >=3D len) >>> + if (cdai.provsiz < len) { >>> + cdai.buf[cdai.provsiz] =3D 0; >>> + ret =3D 0; >>> + } else >>> ret =3D EFAULT; >>> } >>>=20 >>> out: >>> - if (cdai.buf !=3D NULL) >>> + if ((char *)cdai.buf !=3D buf) >>> free(cdai.buf, M_CAMXPT); >>> return ret; >>> } >>>=20 >>=20 >=20 > --=20 > Alexander Motin