From owner-svn-src-head@freebsd.org Fri Dec 4 16:32:11 2015 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 53F9CA4116F; Fri, 4 Dec 2015 16:32:11 +0000 (UTC) (envelope-from ken@kdm.org) Received: from mithlond.kdm.org (mithlond.kdm.org [96.89.93.250]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "A1-33714", Issuer "A1-33714" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 286571906; Fri, 4 Dec 2015 16:32:10 +0000 (UTC) (envelope-from ken@kdm.org) Received: from mithlond.kdm.org (localhost [127.0.0.1]) by mithlond.kdm.org (8.15.2/8.14.9) with ESMTPS id tB4GW8lo093535 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 4 Dec 2015 11:32:08 -0500 (EST) (envelope-from ken@mithlond.kdm.org) Received: (from ken@localhost) by mithlond.kdm.org (8.15.2/8.14.9/Submit) id tB4GW8so093534; Fri, 4 Dec 2015 11:32:08 -0500 (EST) (envelope-from ken) Date: Fri, 4 Dec 2015 11:32:08 -0500 From: "Kenneth D. Merry" To: Ravi Pokala 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> References: <201512032054.tB3KsuUw037541@repo.freebsd.org> <75635FDB-E85F-4F0A-8EDC-8A29F8A095BE@panasas.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <75635FDB-E85F-4F0A-8EDC-8A29F8A095BE@panasas.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.4.3 (mithlond.kdm.org [127.0.0.1]); Fri, 04 Dec 2015 11:32:08 -0500 (EST) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS autolearn=ham autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on mithlond.kdm.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Dec 2015 16:32:11 -0000 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