Date: Thu, 20 Jun 2019 16:59:48 -0600 From: Scott Long <scottl@samsco.org> To: Alexander Motin <mav@FreeBSD.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: <6B266DFC-87FF-4742-ACEE-0D37EABEFD7A@samsco.org> In-Reply-To: <201906202029.x5KKThjn017867@repo.freebsd.org> References: <201906202029.x5KKThjn017867@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Alexander, 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=99= 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: >=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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6B266DFC-87FF-4742-ACEE-0D37EABEFD7A>