linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/6] autofs4 - correct offset mount expire check
@ 2008-10-23  2:35 Ian Kent
  2008-10-23  2:35 ` [PATCH 2/6] autofs4 - remove string terminator check Ian Kent
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Ian Kent @ 2008-10-23  2:35 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Kernel Mailing List, linux-fsdevel, autofs mailing list

When checking a directory tree in autofs_tree_busy() we can incorrectly
decide that the tree isn't busy. This happens for the case of an active
offset mount as autofs4_follow_mount() follows past the active offset
mount, which has an open file handle used for expires, causing the file
handle not to count toward the busyness check.

Signed-off-by: Ian Kent <raven@themaw.net>
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

---

 fs/autofs4/expire.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)


diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index cde2f8e..4b6fb3f 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -56,12 +56,23 @@ static int autofs4_mount_busy(struct vfsmount *mnt, struct dentry *dentry)
 	mntget(mnt);
 	dget(dentry);
 
-	if (!autofs4_follow_mount(&mnt, &dentry))
+	if (!follow_down(&mnt, &dentry))
 		goto done;
 
-	/* This is an autofs submount, we can't expire it */
-	if (is_autofs4_dentry(dentry))
-		goto done;
+	if (is_autofs4_dentry(dentry)) {
+		struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
+
+		/* This is an autofs submount, we can't expire it */
+		if (sbi->type == AUTOFS_TYPE_INDIRECT)
+			goto done;
+
+		/*
+		 * Otherwise it's an offset mount and we need to check
+		 * if we can umount its mount, if there is one.
+		 */
+		if (!d_mountpoint(dentry))
+			goto done;
+	}
 
 	/* Update the expiry counter if fs is busy */
 	if (!may_umount_tree(mnt)) {


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/6] autofs4 - remove string terminator check
  2008-10-23  2:35 [PATCH 1/6] autofs4 - correct offset mount expire check Ian Kent
@ 2008-10-23  2:35 ` Ian Kent
  2008-10-27 20:31   ` Andrew Morton
  2008-10-23  2:35 ` [PATCH 3/6] autofs4 - collect version check return Ian Kent
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Ian Kent @ 2008-10-23  2:35 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Kernel Mailing List, linux-fsdevel, autofs mailing list

Remove unnecessary string terminator check.

Signed-off-by: Ian Kent <raven@themaw.net>
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

---

 fs/autofs4/dev-ioctl.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)


diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 625abf5..304c1ff 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -51,18 +51,6 @@ static int check_name(const char *name)
 }
 
 /*
- * Check a string doesn't overrun the chunk of
- * memory we copied from user land.
- */
-static int invalid_str(char *str, void *end)
-{
-	while ((void *) str <= end)
-		if (!*str++)
-			return 0;
-	return -EINVAL;
-}
-
-/*
  * Check that the user compiled against correct version of autofs
  * misc device code.
  *
@@ -143,14 +131,6 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param)
 				    cmd);
 			goto out;
 		}
-
-		err = invalid_str(param->path,
-				 (void *) ((size_t) param + param->size));
-		if (err) {
-			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
-				    cmd);
-			goto out;
-		}
 	}
 
 	err = 0;


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/6] autofs4 - collect version check return
  2008-10-23  2:35 [PATCH 1/6] autofs4 - correct offset mount expire check Ian Kent
  2008-10-23  2:35 ` [PATCH 2/6] autofs4 - remove string terminator check Ian Kent
@ 2008-10-23  2:35 ` Ian Kent
  2008-10-23  2:35 ` [PATCH 4/6] autofs4 - make autofs type usage explicit Ian Kent
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Ian Kent @ 2008-10-23  2:35 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Kernel Mailing List, linux-fsdevel, autofs mailing list

The function check_dev_ioctl_version() returns an error code upon fail
but it isn't captured and returned in validate_dev_ioctl() as it should
be.

Signed-off-by: Ian Kent <raven@themaw.net>
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

---

 fs/autofs4/dev-ioctl.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 304c1ff..63bfb5d 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -116,9 +116,9 @@ static inline void free_dev_ioctl(struct autofs_dev_ioctl *param)
  */
 static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param)
 {
-	int err = -EINVAL;
+	int err;
 
-	if (check_dev_ioctl_version(cmd, param)) {
+	if ((err = check_dev_ioctl_version(cmd, param))) {
 		AUTOFS_WARN("invalid device control module version "
 		     "supplied for cmd(0x%08x)", cmd);
 		goto out;


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/6] autofs4 - make autofs type usage explicit
  2008-10-23  2:35 [PATCH 1/6] autofs4 - correct offset mount expire check Ian Kent
  2008-10-23  2:35 ` [PATCH 2/6] autofs4 - remove string terminator check Ian Kent
  2008-10-23  2:35 ` [PATCH 3/6] autofs4 - collect version check return Ian Kent
@ 2008-10-23  2:35 ` Ian Kent
  2008-10-27 20:40   ` Andrew Morton
  2008-10-23  2:35 ` [PATCH 5/6] autofs4 - improve parameter usage Ian Kent
  2008-10-23  2:35 ` [PATCH 6/6] autofs4 - cleanup expire code duplication Ian Kent
  4 siblings, 1 reply; 14+ messages in thread
From: Ian Kent @ 2008-10-23  2:35 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Kernel Mailing List, linux-fsdevel, autofs mailing list

This patch further improves autofs mount type usage and provides
supplementry explanation of the changes made in the previous patch
"autofs4 - cleanup autofs mount type usage".

Changes introduced in "autofs4 - cleanup autofs mount type usage":

- the type assigned at mount when no type is given is changed
  from 0 to AUTOFS_TYPE_INDIRECT. This was done because 0 and
  AUTOFS_TYPE_INDIRECT were being treated implicitly as the same
  type.

- previously, an offset mount had it's type set to
  AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET but the mount control
  re-implementation needs to be able distinguish all three types.
  So this was changed to make the type setting explicit.

- a type AUTOFS_TYPE_ANY was added for use by the re-implementation
  when checking if a given path is a mountpoint. It's not really a
  type as we use this to ask if a given path is a mountpoint in the
  autofs_dev_ioctl_ismountpoint() function.

Changes introduced in this patch:

- macros to set and test the autofs mount types have been added to
  improve readability and make the type usage explicit.

- the mount type is used from user space for the mount control
  re-implementtion so, for consistency, all the definitions have
  been moved to the user space include file include/linux/auto_fs4.h.

Signed-off-by: Ian Kent <raven@themaw.net>
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

---

 fs/autofs4/autofs_i.h    |    2 --
 fs/autofs4/dev-ioctl.c   |    4 ++--
 fs/autofs4/expire.c      |    4 ++--
 fs/autofs4/inode.c       |   14 +++++++-------
 fs/autofs4/waitq.c       |    8 ++++----
 include/linux/auto_fs4.h |   28 ++++++++++++++++++++++++----
 6 files changed, 39 insertions(+), 21 deletions(-)


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index e0f16da..a768031 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -25,8 +25,6 @@
 #define AUTOFS_DEV_IOCTL_IOC_FIRST	(AUTOFS_DEV_IOCTL_VERSION)
 #define AUTOFS_DEV_IOCTL_IOC_COUNT	(AUTOFS_IOC_COUNT - 11)
 
-#define AUTOFS_TYPE_TRIGGER	(AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET)
-
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/time.h>
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 63bfb5d..5a35502 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -510,7 +510,7 @@ static int autofs_dev_ioctl_expire(struct file *fp,
 	how = param->arg1;
 	mnt = fp->f_path.mnt;
 
-	if (sbi->type & AUTOFS_TYPE_TRIGGER)
+	if (autofs_type_trigger(sbi->type))
 		dentry = autofs4_expire_direct(sbi->sb, mnt, sbi, how);
 	else
 		dentry = autofs4_expire_indirect(sbi->sb, mnt, sbi, how);
@@ -592,7 +592,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 	param->arg2 = 0;
 
 	if (!fp || param->ioctlfd == -1) {
-		if (type == AUTOFS_TYPE_ANY) {
+		if (autofs_type_any(type)) {
 			struct super_block *sb;
 
 			err = path_lookup(path, LOOKUP_FOLLOW, &nd);
diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index 4b6fb3f..e3bd507 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -63,7 +63,7 @@ static int autofs4_mount_busy(struct vfsmount *mnt, struct dentry *dentry)
 		struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
 
 		/* This is an autofs submount, we can't expire it */
-		if (sbi->type == AUTOFS_TYPE_INDIRECT)
+		if (autofs_type_indirect(sbi->type))
 			goto done;
 
 		/*
@@ -490,7 +490,7 @@ int autofs4_expire_multi(struct super_block *sb, struct vfsmount *mnt,
 	if (arg && get_user(do_now, arg))
 		return -EFAULT;
 
-	if (sbi->type & AUTOFS_TYPE_TRIGGER)
+	if (autofs_type_trigger(sbi->type))
 		dentry = autofs4_expire_direct(sb, mnt, sbi, do_now);
 	else
 		dentry = autofs4_expire_indirect(sb, mnt, sbi, do_now);
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 9408507..e014916 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -197,9 +197,9 @@ static int autofs4_show_options(struct seq_file *m, struct vfsmount *mnt)
 	seq_printf(m, ",minproto=%d", sbi->min_proto);
 	seq_printf(m, ",maxproto=%d", sbi->max_proto);
 
-	if (sbi->type & AUTOFS_TYPE_OFFSET)
+	if (autofs_type_offset(sbi->type))
 		seq_printf(m, ",offset");
-	else if (sbi->type & AUTOFS_TYPE_DIRECT)
+	else if (autofs_type_direct(sbi->type))
 		seq_printf(m, ",direct");
 	else
 		seq_printf(m, ",indirect");
@@ -284,13 +284,13 @@ static int parse_options(char *options, int *pipefd, uid_t *uid, gid_t *gid,
 			*maxproto = option;
 			break;
 		case Opt_indirect:
-			*type = AUTOFS_TYPE_INDIRECT;
+			set_autofs_type_indirect(*type);
 			break;
 		case Opt_direct:
-			*type = AUTOFS_TYPE_DIRECT;
+			set_autofs_type_direct(*type);
 			break;
 		case Opt_offset:
-			*type = AUTOFS_TYPE_OFFSET;
+			set_autofs_type_offset(*type);
 			break;
 		default:
 			return 1;
@@ -338,7 +338,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	sbi->sb = s;
 	sbi->version = 0;
 	sbi->sub_version = 0;
-	sbi->type = AUTOFS_TYPE_INDIRECT;
+	set_autofs_type_indirect(sbi->type);
 	sbi->min_proto = 0;
 	sbi->max_proto = 0;
 	mutex_init(&sbi->wq_mutex);
@@ -380,7 +380,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	}
 
 	root_inode->i_fop = &autofs4_root_operations;
-	root_inode->i_op = sbi->type & AUTOFS_TYPE_TRIGGER ?
+	root_inode->i_op = autofs_type_trigger(sbi->type) ?
 			&autofs4_direct_root_inode_operations :
 			&autofs4_indirect_root_inode_operations;
 
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 4b67c2a..acf88e8 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -337,7 +337,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		 * is very similar for indirect mounts except only dentrys
 		 * in the root of the autofs file system may be negative.
 		 */
-		if (sbi->type & AUTOFS_TYPE_TRIGGER)
+		if (autofs_type_trigger(sbi->type))
 			return -ENOENT;
 		else if (!IS_ROOT(dentry->d_parent))
 			return -ENOENT;
@@ -348,7 +348,7 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 		return -ENOMEM;
 
 	/* If this is a direct mount request create a dummy name */
-	if (IS_ROOT(dentry) && sbi->type & AUTOFS_TYPE_TRIGGER)
+	if (IS_ROOT(dentry) && autofs_type_trigger(sbi->type))
 		qstr.len = sprintf(name, "%p", dentry);
 	else {
 		qstr.len = autofs4_getpath(sbi, dentry, &name);
@@ -406,11 +406,11 @@ int autofs4_wait(struct autofs_sb_info *sbi, struct dentry *dentry,
 				type = autofs_ptype_expire_multi;
 		} else {
 			if (notify == NFY_MOUNT)
-				type = (sbi->type & AUTOFS_TYPE_TRIGGER) ?
+				type = autofs_type_trigger(sbi->type) ?
 					autofs_ptype_missing_direct :
 					 autofs_ptype_missing_indirect;
 			else
-				type = (sbi->type & AUTOFS_TYPE_TRIGGER) ?
+				type = autofs_type_trigger(sbi->type) ?
 					autofs_ptype_expire_direct :
 					autofs_ptype_expire_indirect;
 		}
diff --git a/include/linux/auto_fs4.h b/include/linux/auto_fs4.h
index 2253716..d827765 100644
--- a/include/linux/auto_fs4.h
+++ b/include/linux/auto_fs4.h
@@ -29,10 +29,30 @@
 #define AUTOFS_EXP_IMMEDIATE		1
 #define AUTOFS_EXP_LEAVES		2
 
-#define AUTOFS_TYPE_ANY			0x0000
-#define AUTOFS_TYPE_INDIRECT		0x0001
-#define AUTOFS_TYPE_DIRECT		0x0002
-#define AUTOFS_TYPE_OFFSET		0x0004
+#define AUTOFS_TYPE_ANY			0U
+#define AUTOFS_TYPE_INDIRECT		1U
+#define AUTOFS_TYPE_DIRECT		2U
+#define AUTOFS_TYPE_OFFSET		4U
+
+#define set_autofs_type_indirect(type)		(type = AUTOFS_TYPE_INDIRECT)
+#define autofs_type_indirect(type)		(type == AUTOFS_TYPE_INDIRECT)
+
+#define set_autofs_type_direct(type)		(type = AUTOFS_TYPE_DIRECT)
+#define autofs_type_direct(type)		(type == AUTOFS_TYPE_DIRECT)
+
+#define set_autofs_type_offset(type)		(type = AUTOFS_TYPE_OFFSET)
+#define autofs_type_offset(type)		(type == AUTOFS_TYPE_OFFSET)
+
+#define autofs_type_trigger(type) \
+	(type == AUTOFS_TYPE_DIRECT || type == AUTOFS_TYPE_OFFSET)
+
+/*
+ * This isn't really a type as we use it to say "no type set" to
+ * indicate we want to search for "any" mount in the
+ * autofs_dev_ioctl_ismountpoint() device ioctl function.
+ */
+#define set_autofs_type_any(type)		(type = AUTOFS_TYPE_ANY)
+#define autofs_type_any(type)			(type == AUTOFS_TYPE_ANY)
 
 /* Daemon notification packet types */
 enum autofs_notify {


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/6] autofs4 - improve parameter usage
  2008-10-23  2:35 [PATCH 1/6] autofs4 - correct offset mount expire check Ian Kent
                   ` (2 preceding siblings ...)
  2008-10-23  2:35 ` [PATCH 4/6] autofs4 - make autofs type usage explicit Ian Kent
@ 2008-10-23  2:35 ` Ian Kent
  2008-10-23  2:35 ` [PATCH 6/6] autofs4 - cleanup expire code duplication Ian Kent
  4 siblings, 0 replies; 14+ messages in thread
From: Ian Kent @ 2008-10-23  2:35 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Kernel Mailing List, linux-fsdevel, autofs mailing list

The parameter usage in the device node ioctl code uses arg1 and arg2
as parameter names. This patch redefines the parameter names to reflect
what they actually are in an effort to make the code more readable.

Signed-off-by: Ian Kent <raven@themaw.net>
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

---

 fs/autofs4/dev-ioctl.c         |   54 +++++++++++++++--------------
 include/linux/auto_dev-ioctl.h |   75 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 100 insertions(+), 29 deletions(-)


diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 5a35502..9b78be9 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -159,7 +159,7 @@ static int autofs_dev_ioctl_protover(struct file *fp,
 				     struct autofs_sb_info *sbi,
 				     struct autofs_dev_ioctl *param)
 {
-	param->arg1 = sbi->version;
+	param->protover.version = sbi->version;
 	return 0;
 }
 
@@ -168,7 +168,7 @@ static int autofs_dev_ioctl_protosubver(struct file *fp,
 					struct autofs_sb_info *sbi,
 					struct autofs_dev_ioctl *param)
 {
-	param->arg1 = sbi->sub_version;
+	param->protosubver.sub_version = sbi->sub_version;
 	return 0;
 }
 
@@ -313,13 +313,13 @@ static int autofs_dev_ioctl_openmount(struct file *fp,
 	int err, fd;
 
 	/* param->path has already been checked */
-	if (!param->arg1)
+	if (!param->openmount.devid)
 		return -EINVAL;
 
 	param->ioctlfd = -1;
 
 	path = param->path;
-	devid = param->arg1;
+	devid = param->openmount.devid;
 
 	err = 0;
 	fd = autofs_dev_ioctl_open_mountpoint(path, devid);
@@ -351,7 +351,7 @@ static int autofs_dev_ioctl_ready(struct file *fp,
 {
 	autofs_wqt_t token;
 
-	token = (autofs_wqt_t) param->arg1;
+	token = (autofs_wqt_t) param->ready.token;
 	return autofs4_wait_release(sbi, token, 0);
 }
 
@@ -366,8 +366,8 @@ static int autofs_dev_ioctl_fail(struct file *fp,
 	autofs_wqt_t token;
 	int status;
 
-	token = (autofs_wqt_t) param->arg1;
-	status = param->arg2 ? param->arg2 : -ENOENT;
+	token = (autofs_wqt_t) param->fail.token;
+	status = param->fail.status ? param->fail.status : -ENOENT;
 	return autofs4_wait_release(sbi, token, status);
 }
 
@@ -390,10 +390,10 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 	int pipefd;
 	int err = 0;
 
-	if (param->arg1 == -1)
+	if (param->setpipefd.pipefd == -1)
 		return -EINVAL;
 
-	pipefd = param->arg1;
+	pipefd = param->setpipefd.pipefd;
 
 	mutex_lock(&sbi->wq_mutex);
 	if (!sbi->catatonic) {
@@ -435,8 +435,8 @@ static int autofs_dev_ioctl_timeout(struct file *fp,
 {
 	unsigned long timeout;
 
-	timeout = param->arg1;
-	param->arg1 = sbi->exp_timeout / HZ;
+	timeout = param->timeout.timeout;
+	param->timeout.timeout = sbi->exp_timeout / HZ;
 	sbi->exp_timeout = timeout * HZ;
 	return 0;
 }
@@ -467,7 +467,7 @@ static int autofs_dev_ioctl_requester(struct file *fp,
 	path = param->path;
 	devid = sbi->sb->s_dev;
 
-	param->arg1 = param->arg2 = -1;
+	param->requester.uid = param->requester.gid = -1;
 
 	/* Get nameidata of the parent directory */
 	err = path_lookup(path, LOOKUP_PARENT, &nd);
@@ -483,8 +483,8 @@ static int autofs_dev_ioctl_requester(struct file *fp,
 		err = 0;
 		autofs4_expire_wait(nd.path.dentry);
 		spin_lock(&sbi->fs_lock);
-		param->arg1 = ino->uid;
-		param->arg2 = ino->gid;
+		param->requester.uid = ino->uid;
+		param->requester.gid = ino->gid;
 		spin_unlock(&sbi->fs_lock);
 	}
 
@@ -507,7 +507,7 @@ static int autofs_dev_ioctl_expire(struct file *fp,
 	int err = -EAGAIN;
 	int how;
 
-	how = param->arg1;
+	how = param->expire.how;
 	mnt = fp->f_path.mnt;
 
 	if (autofs_type_trigger(sbi->type))
@@ -543,9 +543,9 @@ static int autofs_dev_ioctl_askumount(struct file *fp,
 				      struct autofs_sb_info *sbi,
 				      struct autofs_dev_ioctl *param)
 {
-	param->arg1 = 0;
+	param->askumount.may_umount = 0;
 	if (may_umount(fp->f_path.mnt))
-		param->arg1 = 1;
+		param->askumount.may_umount = 1;
 	return 0;
 }
 
@@ -578,6 +578,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 	struct nameidata nd;
 	const char *path;
 	unsigned int type;
+	unsigned int devid, magic;
 	int err = -ENOENT;
 
 	if (param->size <= sizeof(*param)) {
@@ -586,10 +587,10 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 	}
 
 	path = param->path;
-	type = param->arg1;
+	type = param->ismountpoint.in.type;
 
-	param->arg1 = 0;
-	param->arg2 = 0;
+	param->ismountpoint.out.devid = devid = 0;
+	param->ismountpoint.out.magic = magic = 0;
 
 	if (!fp || param->ioctlfd == -1) {
 		if (autofs_type_any(type)) {
@@ -600,7 +601,7 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 				goto out;
 
 			sb = nd.path.dentry->d_sb;
-			param->arg1 = new_encode_dev(sb->s_dev);
+			devid = new_encode_dev(sb->s_dev);
 		} else {
 			struct autofs_info *ino;
 
@@ -613,14 +614,14 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 				goto out_release;
 
 			ino = autofs4_dentry_ino(nd.path.dentry);
-			param->arg1 = autofs4_get_dev(ino->sbi);
+			devid = autofs4_get_dev(ino->sbi);
 		}
 
 		err = 0;
 		if (nd.path.dentry->d_inode &&
 		    nd.path.mnt->mnt_root == nd.path.dentry) {
 			err = 1;
-			param->arg2 = nd.path.dentry->d_inode->i_sb->s_magic;
+			magic = nd.path.dentry->d_inode->i_sb->s_magic;
 		}
 	} else {
 		dev_t devid = new_encode_dev(sbi->sb->s_dev);
@@ -633,18 +634,21 @@ static int autofs_dev_ioctl_ismountpoint(struct file *fp,
 		if (err)
 			goto out_release;
 
-		param->arg1 = autofs4_get_dev(sbi);
+		devid = autofs4_get_dev(sbi);
 
 		err = have_submounts(nd.path.dentry);
 
 		if (nd.path.mnt->mnt_mountpoint != nd.path.mnt->mnt_root) {
 			if (follow_down(&nd.path.mnt, &nd.path.dentry)) {
 				struct inode *inode = nd.path.dentry->d_inode;
-				param->arg2 = inode->i_sb->s_magic;
+				magic = inode->i_sb->s_magic;
 			}
 		}
 	}
 
+	param->ismountpoint.out.devid = devid;
+	param->ismountpoint.out.magic = magic;
+
 out_release:
 	path_put(&nd.path);
 out:
diff --git a/include/linux/auto_dev-ioctl.h b/include/linux/auto_dev-ioctl.h
index f4d05cc..91a7739 100644
--- a/include/linux/auto_dev-ioctl.h
+++ b/include/linux/auto_dev-ioctl.h
@@ -10,6 +10,7 @@
 #ifndef _LINUX_AUTO_DEV_IOCTL_H
 #define _LINUX_AUTO_DEV_IOCTL_H
 
+#include <linux/string.h>
 #include <linux/types.h>
 
 #define AUTOFS_DEVICE_NAME		"autofs"
@@ -25,6 +26,60 @@
  * An ioctl interface for autofs mount point control.
  */
 
+struct args_protover {
+	__u32	version;
+};
+
+struct args_protosubver {
+	__u32	sub_version;
+};
+
+struct args_openmount {
+	__u32	devid;
+};
+
+struct args_ready {
+	__u32	token;
+};
+
+struct args_fail {
+	__u32	token;
+	__s32	status;
+};
+
+struct args_setpipefd {
+	__s32	pipefd;
+};
+
+struct args_timeout {
+	__u64	timeout;
+};
+
+struct args_requester {
+	__u32	uid;
+	__u32	gid;
+};
+
+struct args_expire {
+	__u32	how;
+};
+
+struct args_askumount {
+	__u32	may_umount;
+};
+
+struct args_ismountpoint {
+	union {
+		struct args_in {
+			__u32	type;
+		} in;
+		struct args_out {
+			__u32	devid;
+			__u32	magic;
+		} out;
+	};
+};
+
 /*
  * All the ioctls use this structure.
  * When sending a path size must account for the total length
@@ -39,20 +94,32 @@ struct autofs_dev_ioctl {
 				 * including this struct */
 	__s32 ioctlfd;		/* automount command fd */
 
-	__u32 arg1;		/* Command parameters */
-	__u32 arg2;
+	/* Command parameters */
+
+	union {
+		struct args_protover		protover;
+		struct args_protosubver		protosubver;
+		struct args_openmount		openmount;
+		struct args_ready		ready;
+		struct args_fail		fail;
+		struct args_setpipefd		setpipefd;
+		struct args_timeout		timeout;
+		struct args_requester		requester;
+		struct args_expire		expire;
+		struct args_askumount		askumount;
+		struct args_ismountpoint	ismountpoint;
+	};
 
 	char path[0];
 };
 
 static inline void init_autofs_dev_ioctl(struct autofs_dev_ioctl *in)
 {
+	memset(in, 0, sizeof(struct autofs_dev_ioctl));
 	in->ver_major = AUTOFS_DEV_IOCTL_VERSION_MAJOR;
 	in->ver_minor = AUTOFS_DEV_IOCTL_VERSION_MINOR;
 	in->size = sizeof(struct autofs_dev_ioctl);
 	in->ioctlfd = -1;
-	in->arg1 = 0;
-	in->arg2 = 0;
 	return;
 }
 


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 6/6] autofs4 - cleanup expire code duplication
  2008-10-23  2:35 [PATCH 1/6] autofs4 - correct offset mount expire check Ian Kent
                   ` (3 preceding siblings ...)
  2008-10-23  2:35 ` [PATCH 5/6] autofs4 - improve parameter usage Ian Kent
@ 2008-10-23  2:35 ` Ian Kent
  4 siblings, 0 replies; 14+ messages in thread
From: Ian Kent @ 2008-10-23  2:35 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Kernel Mailing List, linux-fsdevel, autofs mailing list

A significant portion of the autofs_dev_ioctl_expire() and
autofs4_expire_multi() functions is duplicated code. This patch
cleans that up.

Signed-off-by: Ian Kent <raven@themaw.net>
Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

---

 fs/autofs4/autofs_i.h  |    2 ++
 fs/autofs4/dev-ioctl.c |   29 +----------------------------
 fs/autofs4/expire.c    |   27 +++++++++++++++++----------
 3 files changed, 20 insertions(+), 38 deletions(-)


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index a768031..b7ff33c 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -186,6 +186,8 @@ int autofs4_expire_wait(struct dentry *dentry);
 int autofs4_expire_run(struct super_block *, struct vfsmount *,
 			struct autofs_sb_info *,
 			struct autofs_packet_expire __user *);
+int autofs4_do_expire_multi(struct super_block *sb, struct vfsmount *mnt,
+			    struct autofs_sb_info *sbi, int when);
 int autofs4_expire_multi(struct super_block *, struct vfsmount *,
 			struct autofs_sb_info *, int __user *);
 struct dentry *autofs4_expire_direct(struct super_block *sb,
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 9b78be9..a040639 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -502,40 +502,13 @@ static int autofs_dev_ioctl_expire(struct file *fp,
 				   struct autofs_sb_info *sbi,
 				   struct autofs_dev_ioctl *param)
 {
-	struct dentry *dentry;
 	struct vfsmount *mnt;
-	int err = -EAGAIN;
 	int how;
 
 	how = param->expire.how;
 	mnt = fp->f_path.mnt;
 
-	if (autofs_type_trigger(sbi->type))
-		dentry = autofs4_expire_direct(sbi->sb, mnt, sbi, how);
-	else
-		dentry = autofs4_expire_indirect(sbi->sb, mnt, sbi, how);
-
-	if (dentry) {
-		struct autofs_info *ino = autofs4_dentry_ino(dentry);
-
-		/*
-		 * This is synchronous because it makes the daemon a
-		 * little easier
-		*/
-		err = autofs4_wait(sbi, dentry, NFY_EXPIRE);
-
-		spin_lock(&sbi->fs_lock);
-		if (ino->flags & AUTOFS_INF_MOUNTPOINT) {
-			ino->flags &= ~AUTOFS_INF_MOUNTPOINT;
-			sbi->sb->s_root->d_mounted++;
-		}
-		ino->flags &= ~AUTOFS_INF_EXPIRING;
-		complete_all(&ino->expire_complete);
-		spin_unlock(&sbi->fs_lock);
-		dput(dentry);
-	}
-
-	return err;
+	return autofs4_do_expire_multi(sbi->sb, mnt, sbi, how);
 }
 
 /* Check if autofs mount point is in use */
diff --git a/fs/autofs4/expire.c b/fs/autofs4/expire.c
index e3bd507..75f7dda 100644
--- a/fs/autofs4/expire.c
+++ b/fs/autofs4/expire.c
@@ -478,22 +478,16 @@ int autofs4_expire_run(struct super_block *sb,
 	return ret;
 }
 
-/* Call repeatedly until it returns -EAGAIN, meaning there's nothing
-   more to be done */
-int autofs4_expire_multi(struct super_block *sb, struct vfsmount *mnt,
-			struct autofs_sb_info *sbi, int __user *arg)
+int autofs4_do_expire_multi(struct super_block *sb, struct vfsmount *mnt,
+			    struct autofs_sb_info *sbi, int when)
 {
 	struct dentry *dentry;
 	int ret = -EAGAIN;
-	int do_now = 0;
-
-	if (arg && get_user(do_now, arg))
-		return -EFAULT;
 
 	if (autofs_type_trigger(sbi->type))
-		dentry = autofs4_expire_direct(sb, mnt, sbi, do_now);
+		dentry = autofs4_expire_direct(sb, mnt, sbi, when);
 	else
-		dentry = autofs4_expire_indirect(sb, mnt, sbi, do_now);
+		dentry = autofs4_expire_indirect(sb, mnt, sbi, when);
 
 	if (dentry) {
 		struct autofs_info *ino = autofs4_dentry_ino(dentry);
@@ -516,3 +510,16 @@ int autofs4_expire_multi(struct super_block *sb, struct vfsmount *mnt,
 	return ret;
 }
 
+/* Call repeatedly until it returns -EAGAIN, meaning there's nothing
+   more to be done */
+int autofs4_expire_multi(struct super_block *sb, struct vfsmount *mnt,
+			struct autofs_sb_info *sbi, int __user *arg)
+{
+	int do_now = 0;
+
+	if (arg && get_user(do_now, arg))
+		return -EFAULT;
+
+	return autofs4_do_expire_multi(sb, mnt, sbi, do_now);
+}
+


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/6] autofs4 - remove string terminator check
  2008-10-23  2:35 ` [PATCH 2/6] autofs4 - remove string terminator check Ian Kent
@ 2008-10-27 20:31   ` Andrew Morton
  2008-10-28  0:27     ` Ian Kent
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Morton @ 2008-10-27 20:31 UTC (permalink / raw)
  To: Ian Kent; +Cc: linux-kernel, linux-fsdevel, autofs

On Thu, 23 Oct 2008 10:35:22 +0800
Ian Kent <raven@themaw.net> wrote:

> Remove unnecessary string terminator check.

Why is it unnecessary?

Does this change alter behaviour in any way?

Does it fix a bug?

Better changelogs, please....

> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -51,18 +51,6 @@ static int check_name(const char *name)
>  }
>  
>  /*
> - * Check a string doesn't overrun the chunk of
> - * memory we copied from user land.
> - */
> -static int invalid_str(char *str, void *end)
> -{
> -	while ((void *) str <= end)
> -		if (!*str++)
> -			return 0;
> -	return -EINVAL;
> -}
> -
> -/*
>   * Check that the user compiled against correct version of autofs
>   * misc device code.
>   *
> @@ -143,14 +131,6 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param)
>  				    cmd);
>  			goto out;
>  		}
> -
> -		err = invalid_str(param->path,
> -				 (void *) ((size_t) param + param->size));
> -		if (err) {
> -			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
> -				    cmd);
> -			goto out;
> -		}
>  	}
>  
>  	err = 0;

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/6] autofs4 - make autofs type usage explicit
  2008-10-23  2:35 ` [PATCH 4/6] autofs4 - make autofs type usage explicit Ian Kent
@ 2008-10-27 20:40   ` Andrew Morton
  2008-10-28  0:28     ` Ian Kent
  2008-10-28 13:24     ` Jeff Moyer
  0 siblings, 2 replies; 14+ messages in thread
From: Andrew Morton @ 2008-10-27 20:40 UTC (permalink / raw)
  To: Ian Kent; +Cc: linux-kernel, linux-fsdevel, autofs

On Thu, 23 Oct 2008 10:35:32 +0800
Ian Kent <raven@themaw.net> wrote:

> This patch further improves autofs mount type usage and provides
> supplementry explanation of the changes made in the previous patch
> "autofs4 - cleanup autofs mount type usage".
> 
> Changes introduced in "autofs4 - cleanup autofs mount type usage":
> 
> - the type assigned at mount when no type is given is changed
>   from 0 to AUTOFS_TYPE_INDIRECT. This was done because 0 and
>   AUTOFS_TYPE_INDIRECT were being treated implicitly as the same
>   type.
> 
> - previously, an offset mount had it's type set to
>   AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET but the mount control
>   re-implementation needs to be able distinguish all three types.
>   So this was changed to make the type setting explicit.
> 
> - a type AUTOFS_TYPE_ANY was added for use by the re-implementation
>   when checking if a given path is a mountpoint. It's not really a
>   type as we use this to ask if a given path is a mountpoint in the
>   autofs_dev_ioctl_ismountpoint() function.
> 
> Changes introduced in this patch:
> 
> - macros to set and test the autofs mount types have been added to
>   improve readability and make the type usage explicit.

    ^^^^^^^^^^^^^^^^^^^  <<-- ??

> - the mount type is used from user space for the mount control
>   re-implementtion so, for consistency, all the definitions have
>   been moved to the user space include file include/linux/auto_fs4.h.
>
> ...
> 
> -		if (sbi->type == AUTOFS_TYPE_INDIRECT)
> +		if (autofs_type_indirect(sbi->type))

spose so.

> -			*type = AUTOFS_TYPE_INDIRECT;
> +			set_autofs_type_indirect(*type);

That's pretty nasty.  One doesn't expect a "function" to modify a
variable which was passed by value.

This interface _requires_ that set_autofs_type_indirect() be
implemented as a macro.

This didn't improve readability.

>
> ...
>
> +#define set_autofs_type_indirect(type)		(type = AUTOFS_TYPE_INDIRECT)

You'll find very few places in the kernel pull tricks like this, for
good reasons.  The obnoxious exceptions include local_irq_save() and
friends.

> +#define autofs_type_indirect(type)		(type == AUTOFS_TYPE_INDIRECT)

I guess that's OK.

But why was it implemented as a macro?  It didn't _need_ to be
implemented in cpp - it could have been implemented in C.

> +
> +#define set_autofs_type_direct(type)		(type = AUTOFS_TYPE_DIRECT)
> +#define autofs_type_direct(type)		(type == AUTOFS_TYPE_DIRECT)
> +
> +#define set_autofs_type_offset(type)		(type = AUTOFS_TYPE_OFFSET)
> +#define autofs_type_offset(type)		(type == AUTOFS_TYPE_OFFSET)
> +
> +#define autofs_type_trigger(type) \
> +	(type == AUTOFS_TYPE_DIRECT || type == AUTOFS_TYPE_OFFSET)

And this one is dangerous.  If passed an expression-with-side-effects
it will evaluate that expression either once or twice.  Bad.  Should be
implemented in C.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/6] autofs4 - remove string terminator check
  2008-10-27 20:31   ` Andrew Morton
@ 2008-10-28  0:27     ` Ian Kent
  2008-10-28  0:55       ` Andrew Morton
  2008-10-28  1:02       ` Ian Kent
  0 siblings, 2 replies; 14+ messages in thread
From: Ian Kent @ 2008-10-28  0:27 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, linux-fsdevel, autofs

On Mon, 2008-10-27 at 13:31 -0700, Andrew Morton wrote:
> On Thu, 23 Oct 2008 10:35:22 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > Remove unnecessary string terminator check.
> 
> Why is it unnecessary?
> 
> Does this change alter behaviour in any way?
> 
> Does it fix a bug?

Umm .... it was done in response to your comment, quoted below ...

> +/*
> + * Check a string doesn't overrun the chunk of
> + * memory we copied from user land.
> + */
> +static int invalid_str(char *str, void *end)
> +{
> +     while ((void *) str <= end)
> +             if (!*str++)
> +                     return 0;
> +     return -EINVAL;
> +}

What is this?  We copy strings in from userspace in 10000 different
places without needing checks such as this?

> 
> Better changelogs, please....
> 
> > --- a/fs/autofs4/dev-ioctl.c
> > +++ b/fs/autofs4/dev-ioctl.c
> > @@ -51,18 +51,6 @@ static int check_name(const char *name)
> >  }
> >  
> >  /*
> > - * Check a string doesn't overrun the chunk of
> > - * memory we copied from user land.
> > - */
> > -static int invalid_str(char *str, void *end)
> > -{
> > -	while ((void *) str <= end)
> > -		if (!*str++)
> > -			return 0;
> > -	return -EINVAL;
> > -}
> > -
> > -/*
> >   * Check that the user compiled against correct version of autofs
> >   * misc device code.
> >   *
> > @@ -143,14 +131,6 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param)
> >  				    cmd);
> >  			goto out;
> >  		}
> > -
> > -		err = invalid_str(param->path,
> > -				 (void *) ((size_t) param + param->size));
> > -		if (err) {
> > -			AUTOFS_WARN("invalid path supplied for cmd(0x%08x)",
> > -				    cmd);
> > -			goto out;
> > -		}
> >  	}
> >  
> >  	err = 0;


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/6] autofs4 - make autofs type usage explicit
  2008-10-27 20:40   ` Andrew Morton
@ 2008-10-28  0:28     ` Ian Kent
  2008-10-28 13:24     ` Jeff Moyer
  1 sibling, 0 replies; 14+ messages in thread
From: Ian Kent @ 2008-10-28  0:28 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, linux-fsdevel, autofs

On Mon, 2008-10-27 at 13:40 -0700, Andrew Morton wrote:
> On Thu, 23 Oct 2008 10:35:32 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > This patch further improves autofs mount type usage and provides
> > supplementry explanation of the changes made in the previous patch
> > "autofs4 - cleanup autofs mount type usage".
> > 
> > Changes introduced in "autofs4 - cleanup autofs mount type usage":
> > 
> > - the type assigned at mount when no type is given is changed
> >   from 0 to AUTOFS_TYPE_INDIRECT. This was done because 0 and
> >   AUTOFS_TYPE_INDIRECT were being treated implicitly as the same
> >   type.
> > 
> > - previously, an offset mount had it's type set to
> >   AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET but the mount control
> >   re-implementation needs to be able distinguish all three types.
> >   So this was changed to make the type setting explicit.
> > 
> > - a type AUTOFS_TYPE_ANY was added for use by the re-implementation
> >   when checking if a given path is a mountpoint. It's not really a
> >   type as we use this to ask if a given path is a mountpoint in the
> >   autofs_dev_ioctl_ismountpoint() function.
> > 
> > Changes introduced in this patch:
> > 
> > - macros to set and test the autofs mount types have been added to
> >   improve readability and make the type usage explicit.
> 
>     ^^^^^^^^^^^^^^^^^^^  <<-- ??
> 
> > - the mount type is used from user space for the mount control
> >   re-implementtion so, for consistency, all the definitions have
> >   been moved to the user space include file include/linux/auto_fs4.h.
> >
> > ...
> > 
> > -		if (sbi->type == AUTOFS_TYPE_INDIRECT)
> > +		if (autofs_type_indirect(sbi->type))
> 
> spose so.
> 
> > -			*type = AUTOFS_TYPE_INDIRECT;
> > +			set_autofs_type_indirect(*type);
> 
> That's pretty nasty.  One doesn't expect a "function" to modify a
> variable which was passed by value.
> 
> This interface _requires_ that set_autofs_type_indirect() be
> implemented as a macro.
> 
> This didn't improve readability.
> 
> >
> > ...
> >
> > +#define set_autofs_type_indirect(type)		(type = AUTOFS_TYPE_INDIRECT)
> 
> You'll find very few places in the kernel pull tricks like this, for
> good reasons.  The obnoxious exceptions include local_irq_save() and
> friends.
> 
> > +#define autofs_type_indirect(type)		(type == AUTOFS_TYPE_INDIRECT)
> 
> I guess that's OK.
> 
> But why was it implemented as a macro?  It didn't _need_ to be
> implemented in cpp - it could have been implemented in C.
> 
> > +
> > +#define set_autofs_type_direct(type)		(type = AUTOFS_TYPE_DIRECT)
> > +#define autofs_type_direct(type)		(type == AUTOFS_TYPE_DIRECT)
> > +
> > +#define set_autofs_type_offset(type)		(type = AUTOFS_TYPE_OFFSET)
> > +#define autofs_type_offset(type)		(type == AUTOFS_TYPE_OFFSET)
> > +
> > +#define autofs_type_trigger(type) \
> > +	(type == AUTOFS_TYPE_DIRECT || type == AUTOFS_TYPE_OFFSET)
> 
> And this one is dangerous.  If passed an expression-with-side-effects
> it will evaluate that expression either once or twice.  Bad.  Should be
> implemented in C.
> 

OK, more work needed then.
Ian



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/6] autofs4 - remove string terminator check
  2008-10-28  0:27     ` Ian Kent
@ 2008-10-28  0:55       ` Andrew Morton
  2008-10-28  1:04         ` Ian Kent
  2008-10-28  1:02       ` Ian Kent
  1 sibling, 1 reply; 14+ messages in thread
From: Andrew Morton @ 2008-10-28  0:55 UTC (permalink / raw)
  To: Ian Kent; +Cc: linux-kernel, linux-fsdevel, autofs

On Tue, 28 Oct 2008 09:27:02 +0900
Ian Kent <raven@themaw.net> wrote:

> On Mon, 2008-10-27 at 13:31 -0700, Andrew Morton wrote:
> > On Thu, 23 Oct 2008 10:35:22 +0800
> > Ian Kent <raven@themaw.net> wrote:
> > 
> > > Remove unnecessary string terminator check.
> > 
> > Why is it unnecessary?
> > 
> > Does this change alter behaviour in any way?
> > 
> > Does it fix a bug?
> 
> Umm .... it was done in response to your comment, quoted below ...

umm, the number of patches I've handled recently overflowed a u16. 
Please don't rely on my memory!  Plus "read akpm's mind" isn't a very
useful changelog.

Please send a new and complete changelog for this patch?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/6] autofs4 - remove string terminator check
  2008-10-28  0:27     ` Ian Kent
  2008-10-28  0:55       ` Andrew Morton
@ 2008-10-28  1:02       ` Ian Kent
  1 sibling, 0 replies; 14+ messages in thread
From: Ian Kent @ 2008-10-28  1:02 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, linux-fsdevel, autofs


This response is a bit confusing, let me try to fix that.

On Tue, 2008-10-28 at 09:27 +0900, Ian Kent wrote:
> On Mon, 2008-10-27 at 13:31 -0700, Andrew Morton wrote:
> > On Thu, 23 Oct 2008 10:35:22 +0800
> > Ian Kent <raven@themaw.net> wrote:
> > 
> > > Remove unnecessary string terminator check.
> > 
> > Why is it unnecessary?
> > 
> > Does this change alter behaviour in any way?
> > 
> > Does it fix a bug?
> 
> Umm .... it was done in response to your comment, quoted below ...
> 
<quote>
> > +/*
> > + * Check a string doesn't overrun the chunk of
> > + * memory we copied from user land.
> > + */
> > +static int invalid_str(char *str, void *end)
> > +{
> > +     while ((void *) str <= end)
> > +             if (!*str++)
> > +                     return 0;
> > +     return -EINVAL;
> > +}
> 
> What is this?  We copy strings in from userspace in 10000 different
> places without needing checks such as this?
</quote>



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/6] autofs4 - remove string terminator check
  2008-10-28  0:55       ` Andrew Morton
@ 2008-10-28  1:04         ` Ian Kent
  0 siblings, 0 replies; 14+ messages in thread
From: Ian Kent @ 2008-10-28  1:04 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, linux-fsdevel, autofs

On Mon, 2008-10-27 at 17:55 -0700, Andrew Morton wrote:
> On Tue, 28 Oct 2008 09:27:02 +0900
> Ian Kent <raven@themaw.net> wrote:
> 
> > On Mon, 2008-10-27 at 13:31 -0700, Andrew Morton wrote:
> > > On Thu, 23 Oct 2008 10:35:22 +0800
> > > Ian Kent <raven@themaw.net> wrote:
> > > 
> > > > Remove unnecessary string terminator check.
> > > 
> > > Why is it unnecessary?
> > > 
> > > Does this change alter behaviour in any way?
> > > 
> > > Does it fix a bug?
> > 
> > Umm .... it was done in response to your comment, quoted below ...
> 
> umm, the number of patches I've handled recently overflowed a u16. 
> Please don't rely on my memory!  Plus "read akpm's mind" isn't a very
> useful changelog.
> 
> Please send a new and complete changelog for this patch?

OK, a resend will be done.

Ian



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/6] autofs4 - make autofs type usage explicit
  2008-10-27 20:40   ` Andrew Morton
  2008-10-28  0:28     ` Ian Kent
@ 2008-10-28 13:24     ` Jeff Moyer
  1 sibling, 0 replies; 14+ messages in thread
From: Jeff Moyer @ 2008-10-28 13:24 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Ian Kent, linux-kernel, linux-fsdevel, autofs

Andrew Morton <akpm@linux-foundation.org> writes:

> On Thu, 23 Oct 2008 10:35:32 +0800
> Ian Kent <raven@themaw.net> wrote:
>
>> This patch further improves autofs mount type usage and provides
>> supplementry explanation of the changes made in the previous patch
>> "autofs4 - cleanup autofs mount type usage".
>> 
>> Changes introduced in "autofs4 - cleanup autofs mount type usage":
>> 
>> - the type assigned at mount when no type is given is changed
>>   from 0 to AUTOFS_TYPE_INDIRECT. This was done because 0 and
>>   AUTOFS_TYPE_INDIRECT were being treated implicitly as the same
>>   type.
>> 
>> - previously, an offset mount had it's type set to
>>   AUTOFS_TYPE_DIRECT|AUTOFS_TYPE_OFFSET but the mount control
>>   re-implementation needs to be able distinguish all three types.
>>   So this was changed to make the type setting explicit.
>> 
>> - a type AUTOFS_TYPE_ANY was added for use by the re-implementation
>>   when checking if a given path is a mountpoint. It's not really a
>>   type as we use this to ask if a given path is a mountpoint in the
>>   autofs_dev_ioctl_ismountpoint() function.
>> 
>> Changes introduced in this patch:
>> 
>> - macros to set and test the autofs mount types have been added to
>>   improve readability and make the type usage explicit.
>
>     ^^^^^^^^^^^^^^^^^^^  <<-- ??

Just for some background, I requested a change here.  The reason is that
I ran into problems in the user space daemon where there was this notion
of a bitfield that wasn't always treated as a bitfield.  So
AUTOFS_TYPE_ANY should be all bits set, right?  Nope.  Some places in
the code tested with binary operators, others with ==.  It was confusing
and error-prone.  The accessor functions at least normalize the interface.

Cheers,

Jeff

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2008-10-28 14:32 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-23  2:35 [PATCH 1/6] autofs4 - correct offset mount expire check Ian Kent
2008-10-23  2:35 ` [PATCH 2/6] autofs4 - remove string terminator check Ian Kent
2008-10-27 20:31   ` Andrew Morton
2008-10-28  0:27     ` Ian Kent
2008-10-28  0:55       ` Andrew Morton
2008-10-28  1:04         ` Ian Kent
2008-10-28  1:02       ` Ian Kent
2008-10-23  2:35 ` [PATCH 3/6] autofs4 - collect version check return Ian Kent
2008-10-23  2:35 ` [PATCH 4/6] autofs4 - make autofs type usage explicit Ian Kent
2008-10-27 20:40   ` Andrew Morton
2008-10-28  0:28     ` Ian Kent
2008-10-28 13:24     ` Jeff Moyer
2008-10-23  2:35 ` [PATCH 5/6] autofs4 - improve parameter usage Ian Kent
2008-10-23  2:35 ` [PATCH 6/6] autofs4 - cleanup expire code duplication Ian Kent

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).