Date: Thu, 20 Jun 2019 20:04:32 -0400 From: Alexander Motin <mav@FreeBSD.org> To: Scott Long <scottl@samsco.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349243 - head/sys/cam Message-ID: <42dbbc7f-51ea-bc3d-0432-5cc9b872d0e7@FreeBSD.org> In-Reply-To: <6B266DFC-87FF-4742-ACEE-0D37EABEFD7A@samsco.org> References: <201906202029.x5KKThjn017867@repo.freebsd.org> <6B266DFC-87FF-4742-ACEE-0D37EABEFD7A@samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mav@FreeBSD.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?42dbbc7f-51ea-bc3d-0432-5cc9b872d0e7>