From owner-svn-src-all@freebsd.org Fri Jun 21 20:38:36 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 53D3415BEECE; Fri, 21 Jun 2019 20:38:36 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-yb1-xb42.google.com (mail-yb1-xb42.google.com [IPv6:2607:f8b0:4864:20::b42]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id BC94E805AA; Fri, 21 Jun 2019 20:38:35 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: by mail-yb1-xb42.google.com with SMTP id x7so3185793ybg.5; Fri, 21 Jun 2019 13:38:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:subject:to:cc:references:from:openpgp:autocrypt:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=GLi4m4MNKCzuklIzblk8qZqKMPaWji2KHZrOoTUzM+s=; b=psOHJlgdzGgeARet0asZCd0bYIrADuizHbVADq5RJ8DPRdw2L7ho6pYOuC6ST2SyVv kymP24rEa2MB7GlR9SIU4i8LVCkw6UagaWmVRGRFYdNeHUyFauM5Rsz3myZnLkpXwXZP q3/P8Iy41LlHwQbBXJ+nCdOVcc33ADPGEo3tg6aTv5OeSf2hkigrufFZSajc4Cu656Ue wp/mr/yElwR+OBaEXJEuMuuLAEmuyuCfSPCWSbAqUtf6Zn6a3Qvf4ICOP59X9/wahIW4 XJqXvMJ0HJaaw4SlV+yeWK4X4u/yDwhudLyh+VVJf6+oL0+zX//Ck3eNea4foTXWlyjZ u/MA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:subject:to:cc:references:from:openpgp :autocrypt:message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=GLi4m4MNKCzuklIzblk8qZqKMPaWji2KHZrOoTUzM+s=; b=lVy2pt0rKCnKu93WrcCTat33lPGDSpKQTmywKJ2CwRgcto7GbSnuJw56qgKRp7YHz0 fyDhVSxL2iGZs3dyUToyMP+Vm/MbMfpKRhqROS0QX/pu1oqOfchbnOnhNEdtUDB+T4GU fjQIlo1HAPgiwphXkKS5eJb64qvZ4+sMSrQC2IAColgW4eMWLruWeMsi99nxc+4j7AhT Hy5M2lJwo9cm7+/ai77sBcnmxU5lpulwYXFKdAk0p1YK6wkMyTcTD7Go/aIyVaTsdZq6 xsLCBFfbkhOxtGQu1lqlP6JZbzXxRknOuDzBCnwhKkjboGiGnQCxFJuhytHXLPYE6aks K6iQ== X-Gm-Message-State: APjAAAVZ24fgxL/jhr8KbWwoLHWZ8zG4EnrhagDPlBUz5ZDMOxO9KZ2m dzZCudc6VvSdNngXPuGXdvJh9ddkQjU= X-Google-Smtp-Source: APXvYqzF7OvGTPQeaivwu7osfAdV1fEE70mgMP0qhjHQrFJK7HDPFG9PnZtIqKUCgjMhYlazgTkXpQ== X-Received: by 2002:a25:c505:: with SMTP id v5mr72987174ybe.492.1561149514639; Fri, 21 Jun 2019 13:38:34 -0700 (PDT) Received: from mavoffice.ixsystems.com ([12.189.233.129]) by smtp.gmail.com with ESMTPSA id p7sm918065ywd.26.2019.06.21.13.38.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 21 Jun 2019 13:38:33 -0700 (PDT) Sender: Alexander Motin Subject: Re: svn commit: r349243 - head/sys/cam To: Scott Long Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201906202029.x5KKThjn017867@repo.freebsd.org> <6B266DFC-87FF-4742-ACEE-0D37EABEFD7A@samsco.org> <42dbbc7f-51ea-bc3d-0432-5cc9b872d0e7@FreeBSD.org> From: Alexander Motin Openpgp: preference=signencrypt Autocrypt: addr=mav@FreeBSD.org; prefer-encrypt=mutual; keydata= xsBNBFOzxAwBCADkPrax0pI2W/ig0CK9nRJJwsHitAGEZ2HZiFEuti+6/4UVxj81yr4ak/4g 9bKUyC7rMEAp/ZHNhd+MFCPAAcHPvtovnfykqE/vuosCS3wlSLloix2iKVLks0CwbLHGAyne 46lTQW74Xl/33c3W1Z6d8jD9gVFT/xaVzZ0U9xdzOmsYAZaAj4ki0tuxO9F7L+ct9grRe7iP g8t9hai7BL4ee3VRwk2JXnKb7UvBiVITKYWKz1jRvZIrjPokgEcCLOSlv7x/1kjuFnj3xWZU 7HSFFT8J93epBbrSSCsYsppIk2fZH41kaaFXsMQfTPH8wkeM6qwrvOh4HiQM08R+9tThABEB AAHNIUFsZXhhbmRlciBNb3RpbiA8bWF2QEZyZWVCU0Qub3JnPsLAlwQTAQoAQQIbAwULCQgH AwUVCgkICwUWAwIBAAIeAQIXgAIZARYhBOmM88TmnMPNDledVYMYw5VbqyJ/BQJZYMKuBQkN McyiAAoJEIMYw5VbqyJ/tuUIAOG3ONOSNYqjK4eTZ1TVh9jdUBAhWk5nhDFnODN49Wj0AbYm 7aIqy8O1hnCDSZG5LttjSAo3UfXJZDKQM0BLb0gpRMBnAYqO6tdolLNqAbPGJBnGoPjsh24y 6KcbDaNnis+lD4GwPXwQM+92wZGhCUFElPV9NciZGVS65TNIgk7X+yEjjhD1MSWKKijZ1r9Z zIt4OzUTxxNOvzdlABZS88nNRdJkatOQJPmFdd1mpP6UzTNCiLUo1pIqOEtJgvVVDYq5WHY6 tciWWYdmZG/tIBexJmv2mV2OLVjXR6ZeKmntVH14H72/wRHJuYHQC+r5SVRcWWayrThsY6jZ Yr4+raTOwE0EU7PEDAEIAOZgWf2cJIu+58IzP2dkXE/urj3tr4OqrB/yHGWUf71Lz6D0Fi6Z AXgDtmcFLGPfMyWuLAvSM+xmoguk7zC4hRBYvQycmIhuqBq1jO1Wp/Z+lpoPM/1cDYLn8Flv mI/c40MhUZh345DA4jYWWaZNjQHUWVQ1fPf595vdVVMPT/abE8E5DaF6fSkRmqFTmfYRkfbt 3ytU8NdUapDcJVY7cEP2nJBVNZPnOIObR/ZIgSxjjrG5o34yXoqeup8JvwEv+/NylzzuyXEZ R1EdEIzQ/a1nh/0j4NXtzZEqKW4aTWlmSqb6wN8jh1OSOOqkYsfnE3nfxcZbxi4IRoNQYlm5 9R8AEQEAAcLAZQQYAQoADwUCU7PEDAIbDAUJBaOagAAKCRCDGMOVW6sif7FRB/4k9y/GaGqU fcJiXdQHRAKHCUvbKMFgeEDHOg33qx+POS2Ah85/PXVa2jYBldCZDmYc+zl48aEMd163a7s3 0gJaB7CYElwxlKUk6c+5gwoYIJuJJzSzW0JzSD5ch7RIRxbfxrKdsiHrUW8AeduZWzlK6VaW RmWILgLmxfLdhEVFWxbr99GSeVFZaZwn6tl/8CvBcgYoARvJvl0V5zS1akQfEISYkwL9EfUI W44EOHranL5qUXkedXBYp6fRsooGrIimfwYxaC8FbXhk3FMgMjDMRiVq4POHo1iGeYETsUrL NM6184E25gPVtX2fb3RhM8Xh6BkwCZ6ZYbQ+AcD4F/cKwsB8BBgBCgAmAhsMFiEE6YzzxOac w80OV51VgxjDlVurIn8FAllgwtgFCQ0xzMwACgkQgxjDlVurIn9OqAf9FAcKWS95wTTbraXA qg/+bQyHgjlMtGCgkmfxLsbUGeqiFgmSIuoDrF7q6sYPs6p00CXXZRuuNZt0lX7O95re8mgz gxm5iJisZpdbHMVepYlw/AxT2wCHwxGCEe64Lm+A9vjlOd+3D3/6fSLwZ9WFCE6p6lQZ1CDg 09xe+JKSgC+KDqmn0tzGKyfSCuhRAq3XkZyxL1hxBaDeP0eeKlzoy7jXodf3wVvXXc0cmpza B5McuRHK4EU6jIioHo30YqPM4AjPHGxV2X1N6/Aayungzj9EXNZtKCxs6dsTvjniWa5VkZ9F 4SOdSbxEen1DZRYpeWnd7GVmO86n+5USkKCXPg== Message-ID: <58b239ed-c119-7946-d601-1f46686cb7a8@FreeBSD.org> Date: Fri, 21 Jun 2019 16:38:33 -0400 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: BC94E805AA X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.99 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.99)[-0.989,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] 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:38:36 -0000 On 21.06.2019 16:02, Scott Long wrote: > Hi Alexander, > > Thanks for the explanation, sorry I didn’t realize sooner that it’s > 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? Sure, why not. >> On Jun 20, 2019, at 6:04 PM, Alexander Motin wrote: >> >> Hi Scott, >> >> 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(). >> >> 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. >> >> On 20.06.2019 18:59, Scott Long wrote: >>> Hi Alexander, >>> >>> I’m not a fan of removing the M_ZERO. I understand your argument that >>> lengths are provided elsewhere, but having the buffer be zero’d is defensive >>> against future bugs that might leak buffer contents. GETATTR isn’t typically >>> in a performance path, is there any measurable benefit to this? >>> >>> Thanks, >>> Scott >>> >>> >>>> On Jun 20, 2019, at 2:29 PM, Alexander Motin wrote: >>>> >>>> Author: mav >>>> Date: Thu Jun 20 20:29:42 2019 >>>> New Revision: 349243 >>>> URL: https://svnweb.freebsd.org/changeset/base/349243 >>>> >>>> Log: >>>> Optimize xpt_getattr(). >>>> >>>> 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. >>>> >>>> MFC after: 2 weeks >>>> Sponsored by: iXsystems, Inc. >>>> >>>> Modified: >>>> head/sys/cam/cam_xpt.c >>>> >>>> Modified: head/sys/cam/cam_xpt.c >>>> ============================================================================== >>>> --- 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 = XPT_DEV_ADVINFO; >>>> cdai.flags = CDAI_FLAG_NONE; >>>> cdai.bufsiz = len; >>>> + cdai.buf = buf; >>>> >>>> if (!strcmp(attr, "GEOM::ident")) >>>> cdai.buftype = CDAI_TYPE_SERIAL_NUM; >>>> @@ -1262,14 +1263,14 @@ xpt_getattr(char *buf, size_t len, const char *attr, s >>>> strcmp(attr, "GEOM::lunname") == 0) { >>>> cdai.buftype = CDAI_TYPE_SCSI_DEVID; >>>> cdai.bufsiz = CAM_SCSI_DEVID_MAXLEN; >>>> + cdai.buf = malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT); >>>> + if (cdai.buf == NULL) { >>>> + ret = ENOMEM; >>>> + goto out; >>>> + } >>>> } else >>>> goto out; >>>> >>>> - cdai.buf = malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT|M_ZERO); >>>> - if (cdai.buf == NULL) { >>>> - ret = ENOMEM; >>>> - goto out; >>>> - } >>>> xpt_action((union ccb *)&cdai); /* can only be synchronous */ >>>> if ((cdai.ccb_h.status & CAM_DEV_QFRZN) != 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 = EFAULT; >>>> } >>>> } else { >>>> - ret = 0; >>>> - if (strlcpy(buf, cdai.buf, len) >= len) >>>> + if (cdai.provsiz < len) { >>>> + cdai.buf[cdai.provsiz] = 0; >>>> + ret = 0; >>>> + } else >>>> ret = EFAULT; >>>> } >>>> >>>> out: >>>> - if (cdai.buf != NULL) >>>> + if ((char *)cdai.buf != buf) >>>> free(cdai.buf, M_CAMXPT); >>>> return ret; >>>> } >>>> >>> >> >> -- >> Alexander Motin > -- Alexander Motin