Date: Fri, 4 Dec 2015 11:32:08 -0500 From: "Kenneth D. Merry" <ken@FreeBSD.ORG> To: Ravi Pokala <rpokala@mac.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r291716 - in head: share/man/man4 sys/cam sys/cam/ata sys/cam/scsi sys/dev/md sys/geom sys/kern sys/pc98/include sys/sys usr.sbin usr.sbin/camdd Message-ID: <20151204163208.GA93141@mithlond.kdm.org> In-Reply-To: <75635FDB-E85F-4F0A-8EDC-8A29F8A095BE@panasas.com> References: <201512032054.tB3KsuUw037541@repo.freebsd.org> <75635FDB-E85F-4F0A-8EDC-8A29F8A095BE@panasas.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Dec 03, 2015 at 23:55:14 -0800, Ravi Pokala wrote:
> Hi Ken,
>
> A few questions:
>
> > Although these ioctls do not have a declared argument, they
> > both take a union ccb pointer. If we declare a size here,
> > the ioctl code in sys/kern/sys_generic.c will malloc and free
> > a buffer for either the CCB or the CCB pointer (depending on
> > how it is declared). Since we have to keep a copy of the
> > CCB (which is fairly large) anyway, having the ioctl malloc
> > and free a CCB for each call is wasteful.
>
>
> (a) How does that work? That is, how does the argument get to the ioctl handler in the kernel?
>
In sys_ioctl(), in sys/kern/sys_generic.c, the pointer argument ("data") to
the ioctl syscall is passed through into kern_ioctl() and then on down
until it gets into the passioctl() call. It is passed through even when
the declared size of the ioctl is 0, as it is for the two new ioctls:
/*
* These two ioctls take a union ccb *, but that is not explicitly declared
* to avoid having the ioctl handling code malloc and free their own copy
* of the CCB or the CCB pointer.
*/
#define CAMIOQUEUE _IO(CAM_VERSION, 4)
#define CAMIOGET _IO(CAM_VERSION, 5)
Here's the code in question:
if (size > 0) {
if (com & IOC_VOID) {
/* Integer argument. */
arg = (intptr_t)uap->data;
data = (void *)&arg;
size = 0;
} else {
if (size > SYS_IOCTL_SMALL_SIZE)
data = malloc((u_long)size, M_IOCTLOPS, M_WAITOK
);
else
data = smalldata;
}
} else
data = (void *)&uap->data;
So in the size == 0 case, data is just passed through as is.
Prior to r274017, if the ioctl were declared as _IOWR, there would be a
malloc and copyin of however much data is declared in the ioctl, no matter
what the size. So, in this case, sizeof(union ccb *) or sizeof(union ccb).
The problem is, upon exit from the ioctl, that data is freed. With a
queueing interface, we need to keep a copy of the CCB around after the
ioctl exits. You have the same problem even after r274017, because that
just provides a small buffer on the stack. (And would only help in the
pointer case. And we don't need to copyin the pointer.)
So, to avoid that, we don't declare an argument, but we do pass in a
pointer and do the copy the user's CCB into a CCB that is allocated inside
the pass(4) driver.
> (b) The CCB is large, but the CCB pointer is just a pointer; shouldn't that be passed in as the arg?
>
It is. Here's what camdd(8) does:
>From camdd_pass_run():
union ccb *ccb;
...
/*
* Queue the CCB to the pass(4) driver.
*/
if (ioctl(pass_dev->dev->fd, CAMIOQUEUE, ccb) == -1) {
And from camdd_pass_fetch():
union ccb ccb;
...
while ((retval = ioctl(pass_dev->dev->fd, CAMIOGET, &ccb)) != -1) {
Ken
--
Kenneth Merry
ken@FreeBSD.ORG
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151204163208.GA93141>