svn commit: r349243 - head/sys/cam

[Alexander Motin]

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 <mav at FreeBSD.org> 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 <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;
>>>> }
>>>>
>>>
>>
>> -- 
>> Alexander Motin
> 

-- 
Alexander Motin