svn commit: r349243 - head/sys/cam

[Scott Long]

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 at 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;
> }
>