[REVIEW] patch for copyout on ioctl error
Martin Matuska
mm at FreeBSD.org
Sun Apr 7 19:29:20 UTC 2013
Hi all,
ZFS expects a copyout of zfs_cmd_t on a ioctl error. Our sys_ioctl()
doesn't copyout in this case.
I am suggesting the attached patch for review to fix (or workaround)
this situation.
The idea is easy - zfs ioctl's won't send zfs_cmd_t anymore but a new
struct zfs_iocparm_t consisting of just three elements:
pointer to zfs_cmd_t
size of zfs_cmd_t
zfs_ioctl_version
Size of zfs_cmd_t is intended for the kernel to re-verify if talking to
the correct version. zfs_ioctl_version is for future purposes to make it
easier for the
kernel to identify ioctl changes.
The kernel module will do the copyin/copyout of zfs_cmd_t itself (like
illumos does). This makes future compatibility more simple and
zfsdev_ioctl() behaves much more like in illumos. The patch retains
compatibility with all previous states.
Cheers,
mm
--
Martin Matuska
FreeBSD committer
http://blog.vx.sk
-------------- next part --------------
Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_compat.c
===================================================================
--- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_compat.c (revision 249226)
+++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_compat.c (working copy)
@@ -72,7 +72,9 @@ zcmd_ioctl(int fd, int request, zfs_cmd_t *zc)
if (zfs_ioctl_version == ZFS_IOCVER_UNDEF)
zfs_ioctl_version = get_zfs_ioctl_version();
- if (zfs_ioctl_version == ZFS_IOCVER_DEADMAN)
+ if (zfs_ioctl_version == ZFS_IOCVER_LZC)
+ cflag = ZFS_CMD_COMPAT_LZC;
+ else if (zfs_ioctl_version == ZFS_IOCVER_DEADMAN)
cflag = ZFS_CMD_COMPAT_DEADMAN;
/*
Index: sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.c
===================================================================
--- sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.c (revision 249226)
+++ sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.c (working copy)
@@ -575,11 +575,21 @@ int
zcmd_ioctl_compat(int fd, int request, zfs_cmd_t *zc, const int cflag)
{
int nc, ret;
+ zfs_iocparm_t *zfs_iocparm;
void *zc_c;
unsigned long ncmd;
switch (cflag) {
case ZFS_CMD_COMPAT_NONE:
+ ncmd = _IOWR('Z', request, struct zfs_iocparm);
+ zfs_iocparm = malloc(sizeof(zfs_iocparm_t));
+ zfs_iocparm->zfs_cmd = zc;
+ zfs_iocparm->zfs_cmd_size = sizeof(zfs_cmd_t);
+ zfs_iocparm->zfs_ioctl_version = ZFS_IOCVER_CURRENT;
+ ret = ioctl(fd, ncmd, zfs_iocparm);
+ free(zfs_iocparm);
+ return (ret);
+ case ZFS_CMD_COMPAT_LZC:
ncmd = _IOWR('Z', request, struct zfs_cmd);
ret = ioctl(fd, ncmd, zc);
return (ret);
@@ -677,7 +687,7 @@ zfs_ioctl_compat_innvl(zfs_cmd_t *zc, nvlist_t * i
char *poolname, *snapname;
int err;
- if (cflag == ZFS_CMD_COMPAT_NONE)
+ if (cflag == ZFS_CMD_COMPAT_NONE || cflag == ZFS_CMD_COMPAT_LZC)
goto out;
switch (vec) {
@@ -828,7 +838,7 @@ zfs_ioctl_compat_outnvl(zfs_cmd_t *zc, nvlist_t *
{
nvlist_t *tmpnvl;
- if (cflag == ZFS_CMD_COMPAT_NONE)
+ if (cflag == ZFS_CMD_COMPAT_NONE || cflag == ZFS_CMD_COMPAT_LZC)
return (outnvl);
switch (vec) {
Index: sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.h
===================================================================
--- sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.h (revision 249226)
+++ sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.h (working copy)
@@ -49,19 +49,27 @@ extern "C" {
#define ZFS_IOCVER_NONE 0
#define ZFS_IOCVER_DEADMAN 1
#define ZFS_IOCVER_LZC 2
-#define ZFS_IOCVER_CURRENT ZFS_IOCVER_LZC
+#define ZFS_IOCVER_ZCMD 3
+#define ZFS_IOCVER_CURRENT ZFS_IOCVER_ZCMD
/* compatibility conversion flag */
#define ZFS_CMD_COMPAT_NONE 0
#define ZFS_CMD_COMPAT_V15 1
#define ZFS_CMD_COMPAT_V28 2
#define ZFS_CMD_COMPAT_DEADMAN 3
+#define ZFS_CMD_COMPAT_LZC 4
#define ZFS_IOC_COMPAT_PASS 254
#define ZFS_IOC_COMPAT_FAIL 255
#define ZFS_IOCREQ(ioreq) ((ioreq) & 0xff)
+typedef struct zfs_iocparm {
+ uint64_t zfs_cmd;
+ uint64_t zfs_cmd_size;
+ uint16_t zfs_ioctl_version;
+} zfs_iocparm_t;
+
typedef struct zinject_record_v15 {
uint64_t zi_objset;
uint64_t zi_object;
Index: sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c
===================================================================
--- sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c (revision 249226)
+++ sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c (working copy)
@@ -5713,11 +5713,13 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
{
zfs_cmd_t *zc;
uint_t vecnum;
+ int error, rc, len;
#ifdef illumos
- int error, rc, len;
minor_t minor = getminor(dev);
#else
- int error, len, cflag, cmd, oldvecnum;
+ zfs_iocparm_t *zc_iocparm;
+ int cflag, cmd, oldvecnum;
+ boolean_t newioc, compat;
cred_t *cr = td->td_ucred;
#endif
const zfs_ioc_vec_t *vec;
@@ -5725,6 +5727,9 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
nvlist_t *innvl = NULL;
cflag = ZFS_CMD_COMPAT_NONE;
+ compat = B_FALSE;
+ newioc = B_TRUE;
+
len = IOCPARM_LEN(zcmd);
cmd = zcmd & 0xff;
@@ -5732,19 +5737,26 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
* Check if we are talking to supported older binaries
* and translate zfs_cmd if necessary
*/
- if (len != sizeof(zfs_cmd_t))
- if (len == sizeof(zfs_cmd_deadman_t)) {
+ if (len != sizeof(zfs_iocparm_t)) {
+ newioc = B_FALSE;
+ if (len == sizeof(zfs_cmd_t)) {
+ cflag = ZFS_CMD_COMPAT_LZC;
+ vecnum = cmd;
+ } else if (len == sizeof(zfs_cmd_deadman_t)) {
cflag = ZFS_CMD_COMPAT_DEADMAN;
+ compat = B_TRUE;
vecnum = cmd;
} else if (len == sizeof(zfs_cmd_v28_t)) {
cflag = ZFS_CMD_COMPAT_V28;
+ compat = B_TRUE;
vecnum = cmd;
} else if (len == sizeof(zfs_cmd_v15_t)) {
cflag = ZFS_CMD_COMPAT_V15;
+ compat = B_TRUE;
vecnum = zfs_ioctl_v15_to_v28[cmd];
} else
return (EINVAL);
- else
+ } else
vecnum = cmd;
#ifdef illumos
@@ -5752,7 +5764,7 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
ASSERT3U(getmajor(dev), ==, ddi_driver_major(zfs_dip));
#endif
- if (cflag != ZFS_CMD_COMPAT_NONE) {
+ if (compat) {
if (vecnum == ZFS_IOC_COMPAT_PASS)
return (0);
else if (vecnum == ZFS_IOC_COMPAT_FAIL)
@@ -5777,13 +5789,33 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
error = SET_ERROR(EFAULT);
goto out;
}
-#else
- error = 0;
-#endif
-
- if (cflag != ZFS_CMD_COMPAT_NONE) {
+#else /* !illumos */
+ /*
+ * We don't alloc/free zc only if talking to library ioctl version 2
+ */
+ if (cflag != ZFS_CMD_COMPAT_LZC) {
zc = kmem_zalloc(sizeof(zfs_cmd_t), KM_SLEEP);
bzero(zc, sizeof(zfs_cmd_t));
+ } else {
+ zc = (void *)arg;
+ error = 0;
+ }
+
+ if (newioc) {
+ zc_iocparm = (void *)arg;
+ if (zc_iocparm->zfs_cmd_size != sizeof(zfs_cmd_t)) {
+ error = SET_ERROR(EFAULT);
+ goto out;
+ }
+ error = ddi_copyin((void *)zc_iocparm->zfs_cmd, zc,
+ sizeof(zfs_cmd_t), flag);
+ if (error != 0) {
+ error = SET_ERROR(EFAULT);
+ goto out;
+ }
+ }
+
+ if (compat) {
zfs_cmd_compat_get(zc, arg, cflag);
oldvecnum = vecnum;
error = zfs_ioctl_compat_pre(zc, &vecnum, cflag);
@@ -5791,8 +5823,8 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
goto out;
if (oldvecnum != vecnum)
vec = &zfs_ioc_vec[vecnum];
- } else
- zc = (void *)arg;
+ }
+#endif /* !illumos */
zc->zc_iflags = flag & FKIOCTL;
if (zc->zc_nvlist_src_size != 0) {
@@ -5803,7 +5835,7 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
}
/* rewrite innvl for backwards compatibility */
- if (cflag != ZFS_CMD_COMPAT_NONE)
+ if (compat)
innvl = zfs_ioctl_compat_innvl(zc, innvl, vecnum, cflag);
/*
@@ -5880,7 +5912,7 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
fnvlist_free(lognv);
/* rewrite outnvl for backwards compatibility */
- if (cflag != ZFS_CMD_COMPAT_NONE)
+ if (cflag != ZFS_CMD_COMPAT_NONE && cflag != ZFS_CMD_COMPAT_LZC)
outnvl = zfs_ioctl_compat_outnvl(zc, outnvl, vecnum,
cflag);
@@ -5904,10 +5936,23 @@ zfsdev_ioctl(struct cdev *dev, u_long zcmd, caddr_
out:
nvlist_free(innvl);
+
+ if (compat) {
+ zfs_ioctl_compat_post(zc, cmd, cflag);
+ zfs_cmd_compat_put(zc, arg, vecnum, cflag);
+ }
+
#ifdef illumos
rc = ddi_copyout(zc, (void *)arg, sizeof (zfs_cmd_t), flag);
if (error == 0 && rc != 0)
error = SET_ERROR(EFAULT);
+#else
+ if (newioc) {
+ rc = ddi_copyout(zc, (void *)zc_iocparm->zfs_cmd,
+ sizeof (zfs_cmd_t), flag);
+ if (error == 0 && rc != 0)
+ error = SET_ERROR(EFAULT);
+ }
#endif
if (error == 0 && vec->zvec_allow_log) {
char *s = tsd_get(zfs_allow_log_key);
@@ -5919,14 +5964,14 @@ out:
strfree(saved_poolname);
}
- if (cflag != ZFS_CMD_COMPAT_NONE) {
- zfs_ioctl_compat_post(zc, cmd, cflag);
- zfs_cmd_compat_put(zc, arg, vecnum, cflag);
- kmem_free(zc, sizeof (zfs_cmd_t));
- }
-
#ifdef illumos
kmem_free(zc, sizeof (zfs_cmd_t));
+#else
+ /*
+ * We don't alloc/free zc only if talking to library ioctl version 2
+ */
+ if (cflag != ZFS_CMD_COMPAT_LZC)
+ kmem_free(zc, sizeof (zfs_cmd_t));
#endif
return (error);
}
More information about the zfs-devel
mailing list