Hi, it looks like the latest changes with respect to
userspace/kernelspace and dk_lookup interface in ccd/cgd drivers:
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ccd.c?rev=1.120&content-type=text/x-cvsweb-markup
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/cgd.c?rev=1.42.2.1&content-type=text/x-cvsweb-markup

have beaten me in terms of talking about these issues from my previous
experience (i.e. I too had recetnly found a problem when using cgd on
macppc port and have subsequently "hacked up" some thoughts/ideas on
the subject)... But for the sake of "doing things the most optimal
way" or just for the sake of my learning of the netbsd's kernel
internals a would like to post that message anyway with the following
question as a prelude:

Are we sure that modifying the dk_lookup is the best option (as
opposed to making cgd/ccd use copyinstr functionality in their
respective ccd.c/cgd.c code - especially considering that cgd.c
already uses copyinstr for other text fields in "cgd_ioctl" structure
*and* that the "userspace" to "kernelspace" conversion for the
"ci_disk" member of "cgd_ioctl" is already done in "cgdinit" code
anyway...

And before going on with the "meat of the message" one side-question
in relation to ccd:
"
		error = copyin(ccio->ccio_disks, cpp,
		    ccio->ccio_ndisks * sizeof(char **));
"
should it not be:
"ccio->ccio_ndisks * sizeof(char *); "
albeit practically sizeof (char **) == sizeof (char *), it might be
more "pedantically" correct since the ccio_disks appears to be treated
as an array of "char *" (i.e. an array of "pointers to character"
hence ccio_disks is "char **") ... could be just my sleepy brain after
14hrs ride on a train without a single snooze :-)

Anyway, here goes the message that I wanted to post in relation to
cgd/dk_lookup and copyinstr:

Hi all,
I have a problem running cgd (cryptographic disk driver) on macppc,
NetBSD-current.

Specifically, the following script:
"
dd if=/dev/zero of=./deleteMe bs=1m count=100
vnconfig vnd0 ./deleteMe
cgdconfig -s cgd0 /dev/vnd0c aes-cbc 128 < /dev/urandom
"
generates the following error:
"cgdconfig: ioctl: No such file or directory"

When looking at the source code of various things (cgdconfig utility,
cgd.c driver, etc.):
in /usr/src/sbin/cgdconfig/cgdconfig.c,  the

configure_params(int fd, const char *cgd, const char *dev, struct params *p)
calls ioctl as something like:

"
	struct cgd_ioctl ci;
	ci.ci_disk = dev;

	if (ioctl(fd, CGDIOCSET, &ci) == -1) {
"
the point being that "dev" arg (and subsequently the "ci_disk" member
of "ci" struct) appears to be allocated in user space.

But then, when we get to kernel space (drivers), in
/usr/src/sys/dev/cgd.c there is something like:
"
	cgd_ioctl_set(struct cgd_softc *cs, void *data, struct lwp *l)
	{
		struct	 cgd_ioctl *ci = data;

		const char *cp;

		cp = ci->ci_disk;
"
At this stage, I think, the "cp" still holds address from userspace...
but in the subsequent code (still in the "cgd_ioctl_set") there is a
call to:
"
	if ((ret = dk_lookup(cp, l, &vp)) != 0)
		return ret;
"
And (in /usr/src/sys/dev/dksubr.c) the dk_lookup(const char *path,
struct lwp *l, struct vnode **vpp)
has code like:
"
	struct nameidata nd;

	NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, path, l);
	if ((error = vn_open(&nd, FREAD | FWRITE, 0)) != 0) {
"
From the above code, it would appear that the "path" arg still points
to user-space, but the NDINIT indicates UIO_SYSSPACE (which is used in
vn_open which calls namei which subsequently fails)...

When the code:
"
	NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, path, l);
"
is changed to:
"
	NDINIT(&nd, LOOKUP, FOLLOW, UIO_USERSPACE, path, l);
"
then the command:
cgdconfig -s cgd0 /dev/vnd0c aes-cbc 128 < /dev/urandom
does not generate "ioctl: No such file or directory" error.

Of course, modifying "dk_lookup" to use UIO_USERSPACE is not right
(dk_lookup is also used by raidframe), but rather - would it make more
sense to use "copyinstr" for ci_disk member of cgd_ioctl (just like
cgd.c does for ci_alg, ci_ivmethod, ci_key) prior to callind
"dk_lookup"?

So the code would be changed from this:
"
#define MAX_KEYSIZE	1024
cgd_ioctl_set(struct cgd_softc *cs, void *data, struct lwp *l)
{
	struct	 cgd_ioctl *ci = data;
	struct	 vnode *vp;
	int	 ret;
	size_t	 keybytes;			/* key length in bytes */
	const char *cp;
	char	 *inbuf;

	cp = ci->ci_disk;
	if ((ret = dk_lookup(cp, l, &vp)) != 0)
		return ret;

	inbuf = malloc(MAX_KEYSIZE, M_TEMP, M_WAITOK);

	if ((ret = cgdinit(cs, cp, vp, l)) != 0)
		goto bail;

	(void)memset(inbuf, 0, MAX_KEYSIZE);
	ret = copyinstr(ci->ci_alg, inbuf, 256, NULL);
	if (ret)
		goto bail;
"
to something like:
"
#define MAX_KEYSIZE	max(1024, MAXPATHLEN)
cgd_ioctl_set(struct cgd_softc *cs, void *data, struct lwp *l)
{
	struct	 cgd_ioctl *ci = data;
	struct	 vnode *vp = NULL;
	int	 ret;
	size_t	 keybytes;			/* key length in bytes */
	char	 *inbuf = malloc(MAX_KEYSIZE, M_TEMP, M_WAITOK);

	(void)memset(inbuf, 0, MAX_KEYSIZE);
	ret = copyinstr(ci->ci_disk, inbuf, MAX_KEYSIZE, NULL);
	if (ret)
		goto bail;

	if ((ret = dk_lookup(inbuf, l, &vp)) != 0)
		goto bail;

	if ((ret = cgdinit(cs, inbuf, vp, l)) != 0)
		goto bail;

	(void)memset(inbuf, 0, MAX_KEYSIZE);
	ret = copyinstr(ci->ci_alg, inbuf, 256, NULL);
	if (ret)
		goto bail;
"
and in bail label:
instead of
"
		(void)vn_close(vp, FREAD|FWRITE, l->l_cred, l);
"
have:
"
	if (vp != NULL)
		(void)vn_close(vp, FREAD|FWRITE, l->l_cred, l);
"
and because "cgdinit" would already be called with kernel-spaced
"cpath" (after the aforementioned code modifications) then, perhaps
one may delete (?) the following code in
static int cgdinit(struct cgd_softc *cs, const char *cpath, struct
vnode *vp, struct lwp *l)
"
	tmppath = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
	ret = copyinstr(cpath, tmppath, MAXPATHLEN, &cs->sc_tpathlen);
	if (ret)
		goto bail;
	cs->sc_tpath = malloc(cs->sc_tpathlen, M_DEVBUF, M_WAITOK);
	memcpy(cs->sc_tpath, tmppath, cs->sc_tpathlen);
	memcpy(cs->sc_tpath, cpath, cs->sc_tpathlen);
"
and
"
	free(tmppath, M_TEMP);
	if (ret && cs->sc_tpath)
		free(cs->sc_tpath, M_DEVBUF);
"
and in
cgd_ioctl_clr(struct cgd_softc *cs, void *data, struct lwp *l) may one
delete the following?
"
	free(cs->sc_tpath, M_DEVBUF);
"
I am probably missing something (e.g. the reason to use malloc with
M_DEVBUF in one place [for sc_tpath] and M_TEMP in other places... - I
don't know anything, really, about kernel in general and cgd
specifically...)

Speaking more in terms of cgd.c - does anyone know how is "sc_tpath"
(a member of "cgd_softc" structure) used at the moment? And may it be
thrown away? - you see - I am missing something :-)

Kind regards
Leon