All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 01/12] autofs: Remove obsolete sb fields
@ 2016-07-10  8:07 Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 02/12] autofs: Don't fail to free_dev_ioctl(param) Tomohiro Kusumi
                   ` (10 more replies)
  0 siblings, 11 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

These two were left from aa55ddf3 which removed unused ioctls.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 fs/autofs4/autofs_i.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index eb87055..73e3d38 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -111,8 +111,6 @@ struct autofs_sb_info {
 	int max_proto;
 	unsigned long exp_timeout;
 	unsigned int type;
-	int reghost_enabled;
-	int needs_reghost;
 	struct super_block *sb;
 	struct mutex wq_mutex;
 	struct mutex pipe_mutex;
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 02/12] autofs: Don't fail to free_dev_ioctl(param)
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value Tomohiro Kusumi
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

Returning -ENOTTY here fails to free dynamically allocated param.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 fs/autofs4/dev-ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index c7fcc74..d47b35a 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -662,7 +662,8 @@ static int _autofs_dev_ioctl(unsigned int command,
 	fn = lookup_dev_ioctl(cmd);
 	if (!fn) {
 		pr_warn("unknown command 0x%08x\n", command);
-		return -ENOTTY;
+		err = -ENOTTY;
+		goto out;
 	}
 
 	fp = NULL;
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 02/12] autofs: Don't fail to free_dev_ioctl(param) Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-19  1:34   ` Ian Kent
  2016-07-10  8:07 ` [PATCH 04/12] autofs: Change _LINUX_AUTO_FS4_H to _UAPI_LINUX_AUTO_FS4_H Tomohiro Kusumi
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

According to include/uapi/linux/auto_fs.h, include/uapi/linux/auto_fs4.h,
and include/linux/auto_dev-ioctl.h, nr part of
AUTOFS_IOC_XXX starts off from 0x60 and
AUTOFS_DEV_IOCTL_XXX starts off from 0x71 which is 0x60+0x11.

From the way above macros are defined, it seems
AUTOFS_DEV_IOCTL_IOC_COUNT should be (0x20-0x11) instead of (0x20-11).
(Otherwise it's hard to figure out where 11 comes from)

COUNT macros are being used to test distance of an ioctl command
in question from the smallest one, so we don't want it to be
larger than necessary.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 fs/autofs4/autofs_i.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 73e3d38..7ef4d08 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -20,7 +20,7 @@
 #define AUTOFS_IOC_COUNT     32
 
 #define AUTOFS_DEV_IOCTL_IOC_FIRST	(AUTOFS_DEV_IOCTL_VERSION)
-#define AUTOFS_DEV_IOCTL_IOC_COUNT	(AUTOFS_IOC_COUNT - 11)
+#define AUTOFS_DEV_IOCTL_IOC_COUNT	(AUTOFS_IOC_COUNT - 0x11)
 
 #include <linux/kernel.h>
 #include <linux/slab.h>
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 04/12] autofs: Change _LINUX_AUTO_FS4_H to _UAPI_LINUX_AUTO_FS4_H
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 02/12] autofs: Don't fail to free_dev_ioctl(param) Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-19  1:40   ` Ian Kent
  2016-07-10  8:07 ` [PATCH 05/12] autofs: Remove AUTOFS_DEVID_LEN Tomohiro Kusumi
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

607ca46e which introduced uapi to Linux kernel added "_UAPI" to
include/uapi/linux/auto_fs.h, but not to include/uapi/linux/auto_fs4.h.

Headers under include/linux which only include uapi headers
generally have different names for include guard without "_UAPI",
so follow that.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 include/uapi/linux/auto_fs4.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
index 8f8f1bd..3c7ed14 100644
--- a/include/uapi/linux/auto_fs4.h
+++ b/include/uapi/linux/auto_fs4.h
@@ -6,8 +6,8 @@
  * option, any later version, incorporated herein by reference.
  */
 
-#ifndef _LINUX_AUTO_FS4_H
-#define _LINUX_AUTO_FS4_H
+#ifndef _UAPI_LINUX_AUTO_FS4_H
+#define _UAPI_LINUX_AUTO_FS4_H
 
 /* Include common v3 definitions */
 #include <linux/types.h>
@@ -154,4 +154,4 @@ union autofs_v5_packet_union {
 #define AUTOFS_IOC_PROTOSUBVER		_IOR(0x93, 0x67, int)
 #define AUTOFS_IOC_ASKUMOUNT		_IOR(0x93, 0x70, int)
 
-#endif /* _LINUX_AUTO_FS4_H */
+#endif /* _UAPI_LINUX_AUTO_FS4_H */
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 05/12] autofs: Remove AUTOFS_DEVID_LEN
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
                   ` (2 preceding siblings ...)
  2016-07-10  8:07 ` [PATCH 04/12] autofs: Change _LINUX_AUTO_FS4_H to _UAPI_LINUX_AUTO_FS4_H Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 06/12] autofs: Fix Documentation regarding devid on ioctl Tomohiro Kusumi
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

This macro was never used by neither kernel nor userspace,
and also doesn't represent "devid length" in bytes.
(unless it was added to mean something else).

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 include/linux/auto_dev-ioctl.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/auto_dev-ioctl.h b/include/linux/auto_dev-ioctl.h
index 7caaf29..bf82e3a 100644
--- a/include/linux/auto_dev-ioctl.h
+++ b/include/linux/auto_dev-ioctl.h
@@ -18,8 +18,6 @@
 #define AUTOFS_DEV_IOCTL_VERSION_MAJOR	1
 #define AUTOFS_DEV_IOCTL_VERSION_MINOR	0
 
-#define AUTOFS_DEVID_LEN		16
-
 #define AUTOFS_DEV_IOCTL_SIZE		sizeof(struct autofs_dev_ioctl)
 
 /*
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 06/12] autofs: Fix Documentation regarding devid on ioctl
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
                   ` (3 preceding siblings ...)
  2016-07-10  8:07 ` [PATCH 05/12] autofs: Remove AUTOFS_DEVID_LEN Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 07/12] autofs: Update struct autofs_dev_ioctl in Documentation Tomohiro Kusumi
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

The explanation on how ioctl handles devid seems incorrect.
Userspace who calls this ioctl has no input regarding devid,
and ioctl implementation retrieves devid via superblock.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 Documentation/filesystems/autofs4-mount-control.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/autofs4-mount-control.txt b/Documentation/filesystems/autofs4-mount-control.txt
index aff2211..540d9a7 100644
--- a/Documentation/filesystems/autofs4-mount-control.txt
+++ b/Documentation/filesystems/autofs4-mount-control.txt
@@ -323,9 +323,8 @@ mount on the given path dentry.
 
 The call requires an initialized struct autofs_dev_ioctl with the path
 field set to the mount point in question and the size field adjusted
-appropriately as well as the arg1 field set to the device number of the
-containing autofs mount. Upon return the struct field arg1 contains the
-uid and arg2 the gid.
+appropriately. Upon return the struct field arg1 contains the uid and
+arg2 the gid.
 
 When reconstructing an autofs mount tree with active mounts we need to
 re-connect to mounts that may have used the original process uid and
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 07/12] autofs: Update struct autofs_dev_ioctl in Documentation
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
                   ` (4 preceding siblings ...)
  2016-07-10  8:07 ` [PATCH 06/12] autofs: Fix Documentation regarding devid on ioctl Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 08/12] autofs: Fix pr_debug() message Tomohiro Kusumi
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

Sync with changes made by 730c9eec which introduced an union for
various ioctl commands instead of having statically named arg1,2.

This commit simply replaces arg1,2 with the corresponding fields
without changing semantics.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 .../filesystems/autofs4-mount-control.txt          | 70 +++++++++++++---------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/Documentation/filesystems/autofs4-mount-control.txt b/Documentation/filesystems/autofs4-mount-control.txt
index 540d9a7..50a3e01 100644
--- a/Documentation/filesystems/autofs4-mount-control.txt
+++ b/Documentation/filesystems/autofs4-mount-control.txt
@@ -179,8 +179,19 @@ struct autofs_dev_ioctl {
 				 * including this struct */
 	__s32 ioctlfd;          /* automount command fd */
 
-	__u32 arg1;             /* Command parameters */
-	__u32 arg2;
+	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];
 };
@@ -192,8 +203,8 @@ optionally be used to check a specific mount corresponding to a given
 mount point file descriptor, and when requesting the uid and gid of the
 last successful mount on a directory within the autofs file system.
 
-The fields arg1 and arg2 are used to communicate parameters and results of
-calls made as described below.
+The union is used to communicate parameters and results of calls made
+as described below.
 
 The path field is used to pass a path where it is needed and the size field
 is used account for the increased structure length when translating the
@@ -245,9 +256,9 @@ AUTOFS_DEV_IOCTL_PROTOVER_CMD and AUTOFS_DEV_IOCTL_PROTOSUBVER_CMD
 Get the major and minor version of the autofs4 protocol version understood
 by loaded module. This call requires an initialized struct autofs_dev_ioctl
 with the ioctlfd field set to a valid autofs mount point descriptor
-and sets the requested version number in structure field arg1. These
-commands return 0 on success or one of the negative error codes if
-validation fails.
+and sets the requested version number in version field of struct args_protover
+or sub_version field of struct args_protosubver. These commands return
+0 on success or one of the negative error codes if validation fails.
 
 
 AUTOFS_DEV_IOCTL_OPENMOUNT and AUTOFS_DEV_IOCTL_CLOSEMOUNT
@@ -256,9 +267,9 @@ AUTOFS_DEV_IOCTL_OPENMOUNT and AUTOFS_DEV_IOCTL_CLOSEMOUNT
 Obtain and release a file descriptor for an autofs managed mount point
 path. The open call requires an initialized struct autofs_dev_ioctl with
 the path field set and the size field adjusted appropriately as well
-as the arg1 field set to the device number of the autofs mount. The
-device number can be obtained from the mount options shown in
-/proc/mounts. The close call requires an initialized struct
+as the devid field of struct args_openmount set to the device number of
+the autofs mount. The device number can be obtained from the mount options
+shown in /proc/mounts. The close call requires an initialized struct
 autofs_dev_ioct with the ioctlfd field set to the descriptor obtained
 from the open call. The release of the file descriptor can also be done
 with close(2) so any open descriptors will also be closed at process exit.
@@ -272,10 +283,10 @@ AUTOFS_DEV_IOCTL_READY_CMD and AUTOFS_DEV_IOCTL_FAIL_CMD
 Return mount and expire result status from user space to the kernel.
 Both of these calls require an initialized struct autofs_dev_ioctl
 with the ioctlfd field set to the descriptor obtained from the open
-call and the arg1 field set to the wait queue token number, received
-by user space in the foregoing mount or expire request. The arg2 field
-is set to the status to be returned. For the ready call this is always
-0 and for the fail call it is set to the errno of the operation.
+call and the token field of struct args_ready or struct args_fail set
+to the wait queue token number, received by user space in the foregoing
+mount or expire request. The status field of struct args_fail is set to
+the errno of the operation. It is set to 0 on success.
 
 
 AUTOFS_DEV_IOCTL_SETPIPEFD_CMD
@@ -290,9 +301,10 @@ mount be catatonic (see next call).
 
 The call requires an initialized struct autofs_dev_ioctl with the
 ioctlfd field set to the descriptor obtained from the open call and
-the arg1 field set to descriptor of the pipe. On success the call
-also sets the process group id used to identify the controlling process
-(eg. the owning automount(8) daemon) to the process group of the caller.
+the pipefd field of struct args_setpipefd set to descriptor of the pipe.
+On success the call also sets the process group id used to identify the
+controlling process (eg. the owning automount(8) daemon) to the process
+group of the caller.
 
 
 AUTOFS_DEV_IOCTL_CATATONIC_CMD
@@ -323,8 +335,8 @@ mount on the given path dentry.
 
 The call requires an initialized struct autofs_dev_ioctl with the path
 field set to the mount point in question and the size field adjusted
-appropriately. Upon return the struct field arg1 contains the uid and
-arg2 the gid.
+appropriately. Upon return the uid field of struct args_requester contains
+the uid and gid field the gid.
 
 When reconstructing an autofs mount tree with active mounts we need to
 re-connect to mounts that may have used the original process uid and
@@ -342,8 +354,9 @@ this ioctl is called until no further expire candidates are found.
 The call requires an initialized struct autofs_dev_ioctl with the
 ioctlfd field set to the descriptor obtained from the open call. In
 addition an immediate expire, independent of the mount timeout, can be
-requested by setting the arg1 field to 1. If no expire candidates can
-be found the ioctl returns -1 with errno set to EAGAIN.
+requested by setting the how field of struct args_expire to 1. If no
+expire candidates can be found the ioctl returns -1 with errno set to
+EAGAIN.
 
 This call causes the kernel module to check the mount corresponding
 to the given ioctlfd for mounts that can be expired, issues an expire
@@ -356,7 +369,8 @@ Checks if an autofs mount point is in use.
 
 The call requires an initialized struct autofs_dev_ioctl with the
 ioctlfd field set to the descriptor obtained from the open call and
-it returns the result in the arg1 field, 1 for busy and 0 otherwise.
+it returns the result in the may_umount field of struct args_askumount,
+1 for busy and 0 otherwise.
 
 
 AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD
@@ -368,12 +382,12 @@ The call requires an initialized struct autofs_dev_ioctl. There are two
 possible variations. Both use the path field set to the path of the mount
 point to check and the size field adjusted appropriately. One uses the
 ioctlfd field to identify a specific mount point to check while the other
-variation uses the path and optionally arg1 set to an autofs mount type.
-The call returns 1 if this is a mount point and sets arg1 to the device
-number of the mount and field arg2 to the relevant super block magic
-number (described below) or 0 if it isn't a mountpoint. In both cases
-the the device number (as returned by new_encode_dev()) is returned
-in field arg1.
+variation uses the path and optionally in.type field of struct args_ismountpoint
+set to an autofs mount type. The call returns 1 if this is a mount point
+and sets out.devid field to the device number of the mount and out.magic
+field to the relevant super block magic number (described below) or 0 if
+it isn't a mountpoint. In both cases the the device number (as returned
+by new_encode_dev()) is returned in out.devid field.
 
 If supplied with a file descriptor we're looking for a specific mount,
 not necessarily at the top of the mounted stack. In this case the path
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 08/12] autofs: Fix pr_debug() message
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
                   ` (5 preceding siblings ...)
  2016-07-10  8:07 ` [PATCH 07/12] autofs: Update struct autofs_dev_ioctl in Documentation Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 09/12] autofs: Fix wrong file/function/macro names in comments Tomohiro Kusumi
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

This isn't a return value, so change the message to indicate
the status is the result of may_umount().

(or locate pr_debug() after put_user() with the same message)

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 fs/autofs4/root.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index 5a877bf..b5e6f64 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -842,7 +842,7 @@ static inline int autofs4_ask_umount(struct vfsmount *mnt, int __user *p)
 	if (may_umount(mnt))
 		status = 1;
 
-	pr_debug("returning %d\n", status);
+	pr_debug("may umount %d\n", status);
 
 	status = put_user(status, p);
 
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 09/12] autofs: Fix wrong file/function/macro names in comments
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
                   ` (6 preceding siblings ...)
  2016-07-10  8:07 ` [PATCH 08/12] autofs: Fix pr_debug() message Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 10/12] autofs: Add autofs_dev_ioctl_version() for AUTOFS_DEV_IOCTL_VERSION_CMD Tomohiro Kusumi
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 fs/autofs4/root.c              | 2 +-
 include/linux/auto_dev-ioctl.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index b5e6f64..a00e71b 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -645,7 +645,7 @@ static int autofs4_dir_unlink(struct inode *dir, struct dentry *dentry)
  * Version 4 of autofs provides a pseudo direct mount implementation
  * that relies on directories at the leaves of a directory tree under
  * an indirect mount to trigger mounts. To allow for this we need to
- * set the DMANAGED_AUTOMOUNT and DMANAGED_TRANSIT flags on the leaves
+ * set the DCACHE_NEED_AUTOMOUNT and DCACHE_MANAGE_TRANSIT flags on the leaves
  * of the directory tree. There is no need to clear the automount flag
  * following a mount or restore it after an expire because these mounts
  * are always covered. However, it is necessary to ensure that these
diff --git a/include/linux/auto_dev-ioctl.h b/include/linux/auto_dev-ioctl.h
index bf82e3a..f7e1f2c 100644
--- a/include/linux/auto_dev-ioctl.h
+++ b/include/linux/auto_dev-ioctl.h
@@ -122,7 +122,7 @@ static inline void init_autofs_dev_ioctl(struct autofs_dev_ioctl *in)
 
 /*
  * If you change this make sure you make the corresponding change
- * to autofs-dev-ioctl.c:lookup_ioctl()
+ * to fs/autofs4/dev-ioctl.c:lookup_dev_ioctl()
  */
 enum {
 	/* Get various version info */
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 10/12] autofs: Add autofs_dev_ioctl_version() for AUTOFS_DEV_IOCTL_VERSION_CMD
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
                   ` (7 preceding siblings ...)
  2016-07-10  8:07 ` [PATCH 09/12] autofs: Fix wrong file/function/macro names in comments Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 11/12] autofs: Fix print format for ioctl warning message Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 12/12] autofs: Move inclusion of linux/limits.h to uapi Tomohiro Kusumi
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

No functional changes, based on the following justification.

1. Make the code more consistent using the ioctl vector _ioctls[],
   rather than assigning NULL only for this ioctl command.
2. Remove goto done; for better maintainability in the long run.
3. The existing code is based on the fact that validate_dev_ioctl()
   sets ioctl version for any command, but AUTOFS_DEV_IOCTL_VERSION_CMD
   should explicitly set it regardless of the default behavior.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 fs/autofs4/dev-ioctl.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index d47b35a..3a1a969 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -172,6 +172,17 @@ static struct autofs_sb_info *autofs_dev_ioctl_sbi(struct file *f)
 	return sbi;
 }
 
+/* Return autofs dev ioctl version */
+static int autofs_dev_ioctl_version(struct file *fp,
+				    struct autofs_sb_info *sbi,
+				    struct autofs_dev_ioctl *param)
+{
+	/* This should have already been set. */
+	param->ver_major = AUTOFS_DEV_IOCTL_VERSION_MAJOR;
+	param->ver_minor = AUTOFS_DEV_IOCTL_VERSION_MINOR;
+	return 0;
+}
+
 /* Return autofs module protocol version */
 static int autofs_dev_ioctl_protover(struct file *fp,
 				     struct autofs_sb_info *sbi,
@@ -590,7 +601,8 @@ static ioctl_fn lookup_dev_ioctl(unsigned int cmd)
 		int cmd;
 		ioctl_fn fn;
 	} _ioctls[] = {
-		{cmd_idx(AUTOFS_DEV_IOCTL_VERSION_CMD), NULL},
+		{cmd_idx(AUTOFS_DEV_IOCTL_VERSION_CMD),
+			 autofs_dev_ioctl_version},
 		{cmd_idx(AUTOFS_DEV_IOCTL_PROTOVER_CMD),
 			 autofs_dev_ioctl_protover},
 		{cmd_idx(AUTOFS_DEV_IOCTL_PROTOSUBVER_CMD),
@@ -655,10 +667,6 @@ static int _autofs_dev_ioctl(unsigned int command,
 	if (err)
 		goto out;
 
-	/* The validate routine above always sets the version */
-	if (cmd == AUTOFS_DEV_IOCTL_VERSION_CMD)
-		goto done;
-
 	fn = lookup_dev_ioctl(cmd);
 	if (!fn) {
 		pr_warn("unknown command 0x%08x\n", command);
@@ -672,9 +680,11 @@ static int _autofs_dev_ioctl(unsigned int command,
 	/*
 	 * For obvious reasons the openmount can't have a file
 	 * descriptor yet. We don't take a reference to the
-	 * file during close to allow for immediate release.
+	 * file during close to allow for immediate release,
+	 * and the same for retrieving ioctl version.
 	 */
-	if (cmd != AUTOFS_DEV_IOCTL_OPENMOUNT_CMD &&
+	if (cmd != AUTOFS_DEV_IOCTL_VERSION_CMD &&
+	    cmd != AUTOFS_DEV_IOCTL_OPENMOUNT_CMD &&
 	    cmd != AUTOFS_DEV_IOCTL_CLOSEMOUNT_CMD) {
 		fp = fget(param->ioctlfd);
 		if (!fp) {
@@ -707,7 +717,6 @@ cont:
 
 	if (fp)
 		fput(fp);
-done:
 	if (err >= 0 && copy_to_user(user, param, AUTOFS_DEV_IOCTL_SIZE))
 		err = -EFAULT;
 out:
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 11/12] autofs: Fix print format for ioctl warning message
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
                   ` (8 preceding siblings ...)
  2016-07-10  8:07 ` [PATCH 10/12] autofs: Add autofs_dev_ioctl_version() for AUTOFS_DEV_IOCTL_VERSION_CMD Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  2016-07-10  8:07 ` [PATCH 12/12] autofs: Move inclusion of linux/limits.h to uapi Tomohiro Kusumi
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

All other warnings use "cmd(0x%08x)" and this is the only one with "cmd(%d)".
(below comes from my userspace debug program, but not automount daemon)

[ 1139.905676] autofs4:pid:1640:check_dev_ioctl_version: ioctl control interface version mismatch: kernel(1.0), user(0.0), cmd(-1072131215)

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 fs/autofs4/dev-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 3a1a969..53dfd7d 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -75,7 +75,7 @@ static int check_dev_ioctl_version(int cmd, struct autofs_dev_ioctl *param)
 	if ((param->ver_major != AUTOFS_DEV_IOCTL_VERSION_MAJOR) ||
 	    (param->ver_minor > AUTOFS_DEV_IOCTL_VERSION_MINOR)) {
 		pr_warn("ioctl control interface version mismatch: "
-			"kernel(%u.%u), user(%u.%u), cmd(%d)\n",
+			"kernel(%u.%u), user(%u.%u), cmd(0x%08x)\n",
 			AUTOFS_DEV_IOCTL_VERSION_MAJOR,
 			AUTOFS_DEV_IOCTL_VERSION_MINOR,
 			param->ver_major, param->ver_minor, cmd);
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* [PATCH 12/12] autofs: Move inclusion of linux/limits.h to uapi
  2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
                   ` (9 preceding siblings ...)
  2016-07-10  8:07 ` [PATCH 11/12] autofs: Fix print format for ioctl warning message Tomohiro Kusumi
@ 2016-07-10  8:07 ` Tomohiro Kusumi
  10 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-10  8:07 UTC (permalink / raw)
  To: raven, autofs; +Cc: Tomohiro Kusumi

linux/limits.h should be included by uapi instead of linux/auto_fs.h
so as not to cause compile error in userspace.

 # cat << EOF > ./test1.c
 > #include <stdio.h>
 > #include <linux/auto_fs.h>
 > int main(void) {
 >     return 0;
 > }
 > EOF
 # gcc -Wall -g ./test1.c
 In file included from ./test1.c:2:0:
 /usr/include/linux/auto_fs.h:54:12: error: 'NAME_MAX' undeclared here (not in a function)
   char name[NAME_MAX+1];
             ^

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 include/linux/auto_fs.h      | 1 -
 include/uapi/linux/auto_fs.h | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/auto_fs.h b/include/linux/auto_fs.h
index b4066bb..b8f814c 100644
--- a/include/linux/auto_fs.h
+++ b/include/linux/auto_fs.h
@@ -10,7 +10,6 @@
 #define _LINUX_AUTO_FS_H
 
 #include <linux/fs.h>
-#include <linux/limits.h>
 #include <linux/ioctl.h>
 #include <uapi/linux/auto_fs.h>
 #endif /* _LINUX_AUTO_FS_H */
diff --git a/include/uapi/linux/auto_fs.h b/include/uapi/linux/auto_fs.h
index 9175a1b..1bfc3ed 100644
--- a/include/uapi/linux/auto_fs.h
+++ b/include/uapi/linux/auto_fs.h
@@ -12,6 +12,7 @@
 #define _UAPI_LINUX_AUTO_FS_H
 
 #include <linux/types.h>
+#include <linux/limits.h>
 #ifndef __KERNEL__
 #include <sys/ioctl.h>
 #endif /* __KERNEL__ */
-- 
2.5.5

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* Re: [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
  2016-07-10  8:07 ` [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value Tomohiro Kusumi
@ 2016-07-19  1:34   ` Ian Kent
  2016-07-19 19:29     ` Tomohiro Kusumi
  0 siblings, 1 reply; 19+ messages in thread
From: Ian Kent @ 2016-07-19  1:34 UTC (permalink / raw)
  To: Tomohiro Kusumi, autofs

On Sun, 2016-07-10 at 17:07 +0900, Tomohiro Kusumi wrote:
> According to include/uapi/linux/auto_fs.h, include/uapi/linux/auto_fs4.h,
> and include/linux/auto_dev-ioctl.h, nr part of
> AUTOFS_IOC_XXX starts off from 0x60 and
> AUTOFS_DEV_IOCTL_XXX starts off from 0x71 which is 0x60+0x11.
> 
> From the way above macros are defined, it seems
> AUTOFS_DEV_IOCTL_IOC_COUNT should be (0x20-0x11) instead of (0x20-11).
> (Otherwise it's hard to figure out where 11 comes from)

You're correct, this is wrong but I think this change is also wrong.

And so is the range check in dev-ioctl.c.

There's no check for an ioctl number greater than the range claimed by autofs
and the check in dev-ioctl.c seeks to check the number used is within the range
used by the dev ioctls.

Essentially

AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD = 0xd

14 ioctls ranging from 0x71 - 0x7e (inclusive).

The range check in dev-ioctl.c is:

        if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST) ||
            cmd - cmd_first >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
                return -ENOTTY;
        }

so I think AUTOFS_DEV_IOCTL_IOC_COUNT should be:

#define AUTOFS_DEV_IOCTL_IOC_COUNT \
	(AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)

and the check should be:

        if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST) ||
      
      cmd_first - cmd >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
                return 
-ENOTTY;
        }

What do you think?

> 
> COUNT macros are being used to test distance of an ioctl command
> in question from the smallest one, so we don't want it to be
> larger than necessary.
> 
> Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
> ---
>  fs/autofs4/autofs_i.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> index 73e3d38..7ef4d08 100644
> --- a/fs/autofs4/autofs_i.h
> +++ b/fs/autofs4/autofs_i.h
> @@ -20,7 +20,7 @@
>  #define AUTOFS_IOC_COUNT     32
>  
>  #define AUTOFS_DEV_IOCTL_IOC_FIRST	(AUTOFS_DEV_IOCTL_VERSION)
> -#define AUTOFS_DEV_IOCTL_IOC_COUNT	(AUTOFS_IOC_COUNT - 11)
> +#define AUTOFS_DEV_IOCTL_IOC_COUNT	(AUTOFS_IOC_COUNT - 0x11)
>  
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* Re: [PATCH 04/12] autofs: Change _LINUX_AUTO_FS4_H to _UAPI_LINUX_AUTO_FS4_H
  2016-07-10  8:07 ` [PATCH 04/12] autofs: Change _LINUX_AUTO_FS4_H to _UAPI_LINUX_AUTO_FS4_H Tomohiro Kusumi
@ 2016-07-19  1:40   ` Ian Kent
  0 siblings, 0 replies; 19+ messages in thread
From: Ian Kent @ 2016-07-19  1:40 UTC (permalink / raw)
  To: Tomohiro Kusumi, autofs

On Sun, 2016-07-10 at 17:07 +0900, Tomohiro Kusumi wrote:
> 607ca46e which introduced uapi to Linux kernel added "_UAPI" to
> include/uapi/linux/auto_fs.h, but not to include/uapi/linux/auto_fs4.h.
> 
> Headers under include/linux which only include uapi headers
> generally have different names for include guard without "_UAPI",
> so follow that.

Yeah, and my series to rename autofs4 to autofs fixes this (if I can ever get to
posting it, it's been a long time), along with moving auto_dev-ioctl.h to
uapi/linux, where it should be.

Also it merges auto_fs4.h and auto_fs.h, leaving a stub for auto_fs4.h for
backward compatibility.

> 
> Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
> ---
>  include/uapi/linux/auto_fs4.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/auto_fs4.h b/include/uapi/linux/auto_fs4.h
> index 8f8f1bd..3c7ed14 100644
> --- a/include/uapi/linux/auto_fs4.h
> +++ b/include/uapi/linux/auto_fs4.h
> @@ -6,8 +6,8 @@
>   * option, any later version, incorporated herein by reference.
>   */
>  
> -#ifndef _LINUX_AUTO_FS4_H
> -#define _LINUX_AUTO_FS4_H
> +#ifndef _UAPI_LINUX_AUTO_FS4_H
> +#define _UAPI_LINUX_AUTO_FS4_H
>  
>  /* Include common v3 definitions */
>  #include <linux/types.h>
> @@ -154,4 +154,4 @@ union autofs_v5_packet_union {
>  #define AUTOFS_IOC_PROTOSUBVER		_IOR(0x93, 0x67, int)
>  #define AUTOFS_IOC_ASKUMOUNT		_IOR(0x93, 0x70, int)
>  
> -#endif /* _LINUX_AUTO_FS4_H */
> +#endif /* _UAPI_LINUX_AUTO_FS4_H */
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* Re: [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
  2016-07-19  1:34   ` Ian Kent
@ 2016-07-19 19:29     ` Tomohiro Kusumi
  2016-07-20  0:47       ` Ian Kent
  0 siblings, 1 reply; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-19 19:29 UTC (permalink / raw)
  To: Ian Kent; +Cc: autofs

Thanks for your review,

Yes, yours apparently works and that's exactly the right test based on
currently existing ioctl cmds.
I first thought about changing it like you did, but thought about
meaning of AUTOFS_IOC_COUNT which is 32.

I took this 32 as some sort of reserved range for autofs ioctl cmds,
where the whole ioctl cmds are designed to be somewhere in 0x60-0x80
range, and /dev/autofs ioctl cmds are in 0x71-0x80 range.
So in theory a potential ioctl cmd for /dev/autofs counting from the
smallest cmd must be no larger than 32-0x11 (or 0x80-0x71).

I've no idea what this reservation/limitation up to 0x80 is supposed
to mean though, as ioctl has 8 bits for nr part.
If you say this 32 is meaningless, I'll resubmit it with your proposed change.


     0x60               0x71            0x80
-----+------------------+---------------+------------------> NR
     <----------------------------------> AUTOFS_IOC_COUNT (32)
                        <---------------> AUTOFS_DEV_IOCTL_IOC_COUNT


2016-07-19 10:34 GMT+09:00 Ian Kent <raven@themaw.net>:
> On Sun, 2016-07-10 at 17:07 +0900, Tomohiro Kusumi wrote:
>> According to include/uapi/linux/auto_fs.h, include/uapi/linux/auto_fs4.h,
>> and include/linux/auto_dev-ioctl.h, nr part of
>> AUTOFS_IOC_XXX starts off from 0x60 and
>> AUTOFS_DEV_IOCTL_XXX starts off from 0x71 which is 0x60+0x11.
>>
>> From the way above macros are defined, it seems
>> AUTOFS_DEV_IOCTL_IOC_COUNT should be (0x20-0x11) instead of (0x20-11).
>> (Otherwise it's hard to figure out where 11 comes from)
>
> You're correct, this is wrong but I think this change is also wrong.
>
> And so is the range check in dev-ioctl.c.
>
> There's no check for an ioctl number greater than the range claimed by autofs
> and the check in dev-ioctl.c seeks to check the number used is within the range
> used by the dev ioctls.
>
> Essentially
>
> AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD = 0xd
>
> 14 ioctls ranging from 0x71 - 0x7e (inclusive).
>
> The range check in dev-ioctl.c is:
>
>         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST) ||
>             cmd - cmd_first >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
>                 return -ENOTTY;
>         }
>
> so I think AUTOFS_DEV_IOCTL_IOC_COUNT should be:
>
> #define AUTOFS_DEV_IOCTL_IOC_COUNT \
>         (AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)
>
> and the check should be:
>
>         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST) ||
>
>       cmd_first - cmd >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
>                 return
> -ENOTTY;
>         }
>
> What do you think?
>
>>
>> COUNT macros are being used to test distance of an ioctl command
>> in question from the smallest one, so we don't want it to be
>> larger than necessary.
>>
>> Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
>> ---
>>  fs/autofs4/autofs_i.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>> index 73e3d38..7ef4d08 100644
>> --- a/fs/autofs4/autofs_i.h
>> +++ b/fs/autofs4/autofs_i.h
>> @@ -20,7 +20,7 @@
>>  #define AUTOFS_IOC_COUNT     32
>>
>>  #define AUTOFS_DEV_IOCTL_IOC_FIRST   (AUTOFS_DEV_IOCTL_VERSION)
>> -#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 11)
>> +#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 0x11)
>>
>>  #include <linux/kernel.h>
>>  #include <linux/slab.h>
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* Re: [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
  2016-07-19 19:29     ` Tomohiro Kusumi
@ 2016-07-20  0:47       ` Ian Kent
  2016-07-20 17:53         ` Tomohiro Kusumi
  0 siblings, 1 reply; 19+ messages in thread
From: Ian Kent @ 2016-07-20  0:47 UTC (permalink / raw)
  To: Tomohiro Kusumi; +Cc: autofs

On Wed, 2016-07-20 at 04:29 +0900, Tomohiro Kusumi wrote:
> Thanks for your review,
> 
> Yes, yours apparently works and that's exactly the right test based on
> currently existing ioctl cmds.
> I first thought about changing it like you did, but thought about
> meaning of AUTOFS_IOC_COUNT which is 32.

I think the meaning of this is taken from Documentation/ioctl/ioctl-number.txt.

The define isn't actually used though, which is what I was saying.

> 
> I took this 32 as some sort of reserved range for autofs ioctl cmds,
> where the whole ioctl cmds are designed to be somewhere in 0x60-0x80
> range, and /dev/autofs ioctl cmds are in 0x71-0x80 range.

Yes, if ioctl numbers beyond 0x7e are to be used I think we need to update the
above file.

It's been a while since I did this but the highest miscellaneous device ioctl
number is 0x7e so if new ioctls were needed the usage registration file would
need to be updated and posted as a patch.

> So in theory a potential ioctl cmd for /dev/autofs counting from the
> smallest cmd must be no larger than 32-0x11 (or 0x80-0x71).
> 
> I've no idea what this reservation/limitation up to 0x80 is supposed
> to mean though, as ioctl has 8 bits for nr part.
> If you say this 32 is meaningless, I'll resubmit it with your proposed change.


The AUTOFS_IOC_COUNT should stay purely because it documents the "claimed"
autofs range (from 0x60-0x7f). Maybe a comment is needed to clear this up.

But AUTOFS_DEV_IOCTL_IOC_COUNT is supposed to be used to check the passed in
miscellaneous device ioctl number is between 0x71-0x7f (or would if it was
correct).

So I think the problem with the miscellaneous device ioctl number check you've
pointed out does need to be fixed and if you could update the patch to do that
it would be much appreciated.

Don't forget that the miscellaneous device ioctls are a newer set of ioctls with
added functionality to the older ioctls, hence the duplication you see with the
functions provided with each set, and the check for that specific range is
relevant to the later ones only.

Also, only one or the other set of ioctls can be used and the miscellaneous
device ioctls can only be used with version 5 and above of user space autofs.

I'd like to try (again) to re-implement this with netlink and add readdir and
asynchronous expire functionality but I don't know when I'll get time to do it s
ince it is a lot of work in kernel and user space, but I digress.

> 
> 
>      0x60               0x71            0x80
> -----+------------------+---------------+------------------> NR
>      <----------------------------------> AUTOFS_IOC_COUNT (32)
>                         <---------------> AUTOFS_DEV_IOCTL_IOC_COUNT

> 2016-07-19 10:34 GMT+09:00 Ian Kent <raven@themaw.net>:
> > On Sun, 2016-07-10 at 17:07 +0900, Tomohiro Kusumi wrote:
> > > According to include/uapi/linux/auto_fs.h, include/uapi/linux/auto_fs4.h,
> > > and include/linux/auto_dev-ioctl.h, nr part of
> > > AUTOFS_IOC_XXX starts off from 0x60 and
> > > AUTOFS_DEV_IOCTL_XXX starts off from 0x71 which is 0x60+0x11.
> > > 
> > > From the way above macros are defined, it seems
> > > AUTOFS_DEV_IOCTL_IOC_COUNT should be (0x20-0x11) instead of (0x20-11).
> > > (Otherwise it's hard to figure out where 11 comes from)
> > 
> > You're correct, this is wrong but I think this change is also wrong.
> > 
> > And so is the range check in dev-ioctl.c.
> > 
> > There's no check for an ioctl number greater than the range claimed by
> > autofs
> > and the check in dev-ioctl.c seeks to check the number used is within the
> > range
> > used by the dev ioctls.
> > 
> > Essentially
> > 
> > AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD = 0xd
> > 
> > 14 ioctls ranging from 0x71 - 0x7e (inclusive).
> > 
> > The range check in dev-ioctl.c is:
> > 
> >         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST) ||
> >             cmd - cmd_first >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
> >                 return -ENOTTY;
> >         }
> > 
> > so I think AUTOFS_DEV_IOCTL_IOC_COUNT should be:
> > 
> > #define AUTOFS_DEV_IOCTL_IOC_COUNT \
> >         (AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)
> > 
> > and the check should be:
> > 
> >         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST) ||
> > 
> >       cmd_first - cmd >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
> >                 return
> > -ENOTTY;
> >         }
> > 
> > What do you think?
> > 
> > > 
> > > COUNT macros are being used to test distance of an ioctl command
> > > in question from the smallest one, so we don't want it to be
> > > larger than necessary.
> > > 
> > > Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
> > > ---
> > >  fs/autofs4/autofs_i.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > > index 73e3d38..7ef4d08 100644
> > > --- a/fs/autofs4/autofs_i.h
> > > +++ b/fs/autofs4/autofs_i.h
> > > @@ -20,7 +20,7 @@
> > >  #define AUTOFS_IOC_COUNT     32
> > > 
> > >  #define AUTOFS_DEV_IOCTL_IOC_FIRST   (AUTOFS_DEV_IOCTL_VERSION)
> > > -#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 11)
> > > +#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 0x11)
> > > 
> > >  #include <linux/kernel.h>
> > >  #include <linux/slab.h>
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* Re: [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
  2016-07-20  0:47       ` Ian Kent
@ 2016-07-20 17:53         ` Tomohiro Kusumi
  2016-07-21  2:02           ` Ian Kent
  0 siblings, 1 reply; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-20 17:53 UTC (permalink / raw)
  To: Ian Kent; +Cc: autofs

> So I think the problem with the miscellaneous device ioctl number check you've
> pointed out does need to be fixed and if you could update the patch to do that
> it would be much appreciated.

OK, I'll take a look at it again based on your explanation,
and see if I can fix it, plus make things a bit more clear if possible.

> Don't forget that the miscellaneous device ioctls are a newer set of ioctls with
> added functionality to the older ioctls, hence the duplication you see with the
> functions provided with each set, and the check for that specific range is
> relevant to the later ones only.
>
> Also, only one or the other set of ioctls can be used and the miscellaneous
> device ioctls can only be used with version 5 and above of user space autofs.

Yes, I was wondering why they're implemented to both fs root and /dev/autofs
when I first looked at ioctl and userspace code.

>
> I'd like to try (again) to re-implement this with netlink and add readdir and
> asynchronous expire functionality but I don't know when I'll get time to do it s
> ince it is a lot of work in kernel and user space, but I digress.

Is there a publicly available list of TODO for autofs (kernel and userspace) ?

According to some of your recent posts to this mailing list,
you seem to have lots of patches that haven't been merged to upstream,
and also things yet to be implemented.



2016-07-20 9:47 GMT+09:00 Ian Kent <raven@themaw.net>:
> On Wed, 2016-07-20 at 04:29 +0900, Tomohiro Kusumi wrote:
>> Thanks for your review,
>>
>> Yes, yours apparently works and that's exactly the right test based on
>> currently existing ioctl cmds.
>> I first thought about changing it like you did, but thought about
>> meaning of AUTOFS_IOC_COUNT which is 32.
>
> I think the meaning of this is taken from Documentation/ioctl/ioctl-number.txt.
>
> The define isn't actually used though, which is what I was saying.
>
>>
>> I took this 32 as some sort of reserved range for autofs ioctl cmds,
>> where the whole ioctl cmds are designed to be somewhere in 0x60-0x80
>> range, and /dev/autofs ioctl cmds are in 0x71-0x80 range.
>
> Yes, if ioctl numbers beyond 0x7e are to be used I think we need to update the
> above file.
>
> It's been a while since I did this but the highest miscellaneous device ioctl
> number is 0x7e so if new ioctls were needed the usage registration file would
> need to be updated and posted as a patch.
>
>> So in theory a potential ioctl cmd for /dev/autofs counting from the
>> smallest cmd must be no larger than 32-0x11 (or 0x80-0x71).
>>
>> I've no idea what this reservation/limitation up to 0x80 is supposed
>> to mean though, as ioctl has 8 bits for nr part.
>> If you say this 32 is meaningless, I'll resubmit it with your proposed change.
>
>
> The AUTOFS_IOC_COUNT should stay purely because it documents the "claimed"
> autofs range (from 0x60-0x7f). Maybe a comment is needed to clear this up.
>
> But AUTOFS_DEV_IOCTL_IOC_COUNT is supposed to be used to check the passed in
> miscellaneous device ioctl number is between 0x71-0x7f (or would if it was
> correct).
>
> So I think the problem with the miscellaneous device ioctl number check you've
> pointed out does need to be fixed and if you could update the patch to do that
> it would be much appreciated.
>
> Don't forget that the miscellaneous device ioctls are a newer set of ioctls with
> added functionality to the older ioctls, hence the duplication you see with the
> functions provided with each set, and the check for that specific range is
> relevant to the later ones only.
>
> Also, only one or the other set of ioctls can be used and the miscellaneous
> device ioctls can only be used with version 5 and above of user space autofs.
>
> I'd like to try (again) to re-implement this with netlink and add readdir and
> asynchronous expire functionality but I don't know when I'll get time to do it s
> ince it is a lot of work in kernel and user space, but I digress.
>
>>
>>
>>      0x60               0x71            0x80
>> -----+------------------+---------------+------------------> NR
>>      <----------------------------------> AUTOFS_IOC_COUNT (32)
>>                         <---------------> AUTOFS_DEV_IOCTL_IOC_COUNT
>
>> 2016-07-19 10:34 GMT+09:00 Ian Kent <raven@themaw.net>:
>> > On Sun, 2016-07-10 at 17:07 +0900, Tomohiro Kusumi wrote:
>> > > According to include/uapi/linux/auto_fs.h, include/uapi/linux/auto_fs4.h,
>> > > and include/linux/auto_dev-ioctl.h, nr part of
>> > > AUTOFS_IOC_XXX starts off from 0x60 and
>> > > AUTOFS_DEV_IOCTL_XXX starts off from 0x71 which is 0x60+0x11.
>> > >
>> > > From the way above macros are defined, it seems
>> > > AUTOFS_DEV_IOCTL_IOC_COUNT should be (0x20-0x11) instead of (0x20-11).
>> > > (Otherwise it's hard to figure out where 11 comes from)
>> >
>> > You're correct, this is wrong but I think this change is also wrong.
>> >
>> > And so is the range check in dev-ioctl.c.
>> >
>> > There's no check for an ioctl number greater than the range claimed by
>> > autofs
>> > and the check in dev-ioctl.c seeks to check the number used is within the
>> > range
>> > used by the dev ioctls.
>> >
>> > Essentially
>> >
>> > AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD = 0xd
>> >
>> > 14 ioctls ranging from 0x71 - 0x7e (inclusive).
>> >
>> > The range check in dev-ioctl.c is:
>> >
>> >         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST) ||
>> >             cmd - cmd_first >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
>> >                 return -ENOTTY;
>> >         }
>> >
>> > so I think AUTOFS_DEV_IOCTL_IOC_COUNT should be:
>> >
>> > #define AUTOFS_DEV_IOCTL_IOC_COUNT \
>> >         (AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)
>> >
>> > and the check should be:
>> >
>> >         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST) ||
>> >
>> >       cmd_first - cmd >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
>> >                 return
>> > -ENOTTY;
>> >         }
>> >
>> > What do you think?
>> >
>> > >
>> > > COUNT macros are being used to test distance of an ioctl command
>> > > in question from the smallest one, so we don't want it to be
>> > > larger than necessary.
>> > >
>> > > Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
>> > > ---
>> > >  fs/autofs4/autofs_i.h | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >
>> > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>> > > index 73e3d38..7ef4d08 100644
>> > > --- a/fs/autofs4/autofs_i.h
>> > > +++ b/fs/autofs4/autofs_i.h
>> > > @@ -20,7 +20,7 @@
>> > >  #define AUTOFS_IOC_COUNT     32
>> > >
>> > >  #define AUTOFS_DEV_IOCTL_IOC_FIRST   (AUTOFS_DEV_IOCTL_VERSION)
>> > > -#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 11)
>> > > +#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 0x11)
>> > >
>> > >  #include <linux/kernel.h>
>> > >  #include <linux/slab.h>
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* Re: [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
  2016-07-20 17:53         ` Tomohiro Kusumi
@ 2016-07-21  2:02           ` Ian Kent
  2016-07-21 18:12             ` Tomohiro Kusumi
  0 siblings, 1 reply; 19+ messages in thread
From: Ian Kent @ 2016-07-21  2:02 UTC (permalink / raw)
  To: Tomohiro Kusumi; +Cc: autofs

On Thu, 2016-07-21 at 02:53 +0900, Tomohiro Kusumi wrote:
> > So I think the problem with the miscellaneous device ioctl number check
> > you've
> > pointed out does need to be fixed and if you could update the patch to do
> > that
> > it would be much appreciated.
> 
> OK, I'll take a look at it again based on your explanation,
> and see if I can fix it, plus make things a bit more clear if possible.
> 
> > Don't forget that the miscellaneous device ioctls are a newer set of ioctls
> > with
> > added functionality to the older ioctls, hence the duplication you see with
> > the
> > functions provided with each set, and the check for that specific range is
> > relevant to the later ones only.
> > 
> > Also, only one or the other set of ioctls can be used and the miscellaneous
> > device ioctls can only be used with version 5 and above of user space
> > autofs.
> 
> Yes, I was wondering why they're implemented to both fs root and /dev/autofs
> when I first looked at ioctl and userspace code.
> 
> > 
> > I'd like to try (again) to re-implement this with netlink and add readdir
> > and
> > asynchronous expire functionality but I don't know when I'll get time to do
> > it s
> > ince it is a lot of work in kernel and user space, but I digress.
> 
> Is there a publicly available list of TODO for autofs (kernel and userspace) ?
> 
> According to some of your recent posts to this mailing list,
> you seem to have lots of patches that haven't been merged to upstream,
> and also things yet to be implemented.

There isn't.

The truth is there haven't been other developers that have stuck with it so I've
pretty much done everything in isolation.

That's not too good because that means there isn't discussion about how things
should be done and there's very little review of changes too, since there's no
-one to review them.

If there were others, such as yourself, I would start doing things quite
differently, but clearly that would take time to start happening.

I detest writing and maintaining web presences so there is no wiki which is
another problem, and is the sort of place where a TODO list would reside.

As far as patches that are either not finished, did not solve a problem, or are
in progress, yes I have plenty of those and they have accumulated over many
years.

That's not uncommon when you work with something for a long time but they are not useful until you return to a problem they may be related too, so mostly not useful to others.

For example, I tried to implement the control interface using the netlink
mechanism, a long time ago now, and failed but I keep the work that I did in
case I can use it at some time is the future, that's my nature.

A netlink interface would be better than using ioctls, it would be more flexible
in the long run but, at the time, I was unwilling to fundamentally change the
way automount(8) works and there were other difficulties with asynchronous
expires so I failed to get it working and implemented the anonymous device ioctl
interface instead.

So these patches are not useful at all and are just a consequence of things I
have tried to do in the past.

But there is still the question of how to better deal with listing of
directories (using certain options) that can cause everything within the
directory to mount.

Over the years I've thought about it many times and at times I think a kernel
readdir implementation would be better and (often after thinking more about it)
I come back to there being no advantage over the existing pre-creation of mount
point directories.

If I was to, one day, believe that a kernel readdir implementation would make a
worthwhile difference then I think it would be better to implement that using
the netlink sub-system and the problems with that would need to be worked out to
do so.

So it ends up being little more than something for thought only, or possibly
discussion if there were others to take part in it, and what I have for this
would be extremely hard for anyone else to use as it is a mess of partially done
work.

For a long time now I accumulate patches and then commit and push those that
prove sound. And often it has been quite a while between pushes so there can end
up being quite a few, but that can change if there is reason to do so.

And generally all patches that are committed and pushed are also available on
kernel.org grouped by release. I've been trying my best to ensure they is as
accurate as possible so it is clear what has gone into each release.

Also, at times I have tried to put together a TODO list of longer term things
that I'd like to do for myself but haven't been very successful, ;)

> 
> 
> 2016-07-20 9:47 GMT+09:00 Ian Kent <raven@themaw.net>:
> > On Wed, 2016-07-20 at 04:29 +0900, Tomohiro Kusumi wrote:
> > > Thanks for your review,
> > > 
> > > Yes, yours apparently works and that's exactly the right test based on
> > > currently existing ioctl cmds.
> > > I first thought about changing it like you did, but thought about
> > > meaning of AUTOFS_IOC_COUNT which is 32.
> > 
> > I think the meaning of this is taken from Documentation/ioctl/ioctl
> > -number.txt.
> > 
> > The define isn't actually used though, which is what I was saying.
> > 
> > > 
> > > I took this 32 as some sort of reserved range for autofs ioctl cmds,
> > > where the whole ioctl cmds are designed to be somewhere in 0x60-0x80
> > > range, and /dev/autofs ioctl cmds are in 0x71-0x80 range.
> > 
> > Yes, if ioctl numbers beyond 0x7e are to be used I think we need to update
> > the
> > above file.
> > 
> > It's been a while since I did this but the highest miscellaneous device
> > ioctl
> > number is 0x7e so if new ioctls were needed the usage registration file
> > would
> > need to be updated and posted as a patch.
> > 
> > > So in theory a potential ioctl cmd for /dev/autofs counting from the
> > > smallest cmd must be no larger than 32-0x11 (or 0x80-0x71).
> > > 
> > > I've no idea what this reservation/limitation up to 0x80 is supposed
> > > to mean though, as ioctl has 8 bits for nr part.
> > > If you say this 32 is meaningless, I'll resubmit it with your proposed
> > > change.
> > 
> > 
> > The AUTOFS_IOC_COUNT should stay purely because it documents the "claimed"
> > autofs range (from 0x60-0x7f). Maybe a comment is needed to clear this up.
> > 
> > But AUTOFS_DEV_IOCTL_IOC_COUNT is supposed to be used to check the passed in
> > miscellaneous device ioctl number is between 0x71-0x7f (or would if it was
> > correct).
> > 
> > So I think the problem with the miscellaneous device ioctl number check
> > you've
> > pointed out does need to be fixed and if you could update the patch to do
> > that
> > it would be much appreciated.
> > 
> > Don't forget that the miscellaneous device ioctls are a newer set of ioctls
> > with
> > added functionality to the older ioctls, hence the duplication you see with
> > the
> > functions provided with each set, and the check for that specific range is
> > relevant to the later ones only.
> > 
> > Also, only one or the other set of ioctls can be used and the miscellaneous
> > device ioctls can only be used with version 5 and above of user space
> > autofs.
> > 
> > I'd like to try (again) to re-implement this with netlink and add readdir
> > and
> > asynchronous expire functionality but I don't know when I'll get time to do
> > it s
> > ince it is a lot of work in kernel and user space, but I digress.
> > 
> > > 
> > > 
> > >      0x60               0x71            0x80
> > > -----+------------------+---------------+------------------> NR
> > >      <----------------------------------> AUTOFS_IOC_COUNT (32)
> > >                         <---------------> AUTOFS_DEV_IOCTL_IOC_COUNT
> > 
> > > 2016-07-19 10:34 GMT+09:00 Ian Kent <raven@themaw.net>:
> > > > On Sun, 2016-07-10 at 17:07 +0900, Tomohiro Kusumi wrote:
> > > > > According to include/uapi/linux/auto_fs.h,
> > > > > include/uapi/linux/auto_fs4.h,
> > > > > and include/linux/auto_dev-ioctl.h, nr part of
> > > > > AUTOFS_IOC_XXX starts off from 0x60 and
> > > > > AUTOFS_DEV_IOCTL_XXX starts off from 0x71 which is 0x60+0x11.
> > > > > 
> > > > > From the way above macros are defined, it seems
> > > > > AUTOFS_DEV_IOCTL_IOC_COUNT should be (0x20-0x11) instead of (0x20-11).
> > > > > (Otherwise it's hard to figure out where 11 comes from)
> > > > 
> > > > You're correct, this is wrong but I think this change is also wrong.
> > > > 
> > > > And so is the range check in dev-ioctl.c.
> > > > 
> > > > There's no check for an ioctl number greater than the range claimed by
> > > > autofs
> > > > and the check in dev-ioctl.c seeks to check the number used is within
> > > > the
> > > > range
> > > > used by the dev ioctls.
> > > > 
> > > > Essentially
> > > > 
> > > > AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD = 0xd
> > > > 
> > > > 14 ioctls ranging from 0x71 - 0x7e (inclusive).
> > > > 
> > > > The range check in dev-ioctl.c is:
> > > > 
> > > >         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST)
> > > > ||
> > > >             cmd - cmd_first >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
> > > >                 return -ENOTTY;
> > > >         }
> > > > 
> > > > so I think AUTOFS_DEV_IOCTL_IOC_COUNT should be:
> > > > 
> > > > #define AUTOFS_DEV_IOCTL_IOC_COUNT \
> > > >         (AUTOFS_DEV_IOCTL_VERSION_CMD -
> > > > AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)
> > > > 
> > > > and the check should be:
> > > > 
> > > >         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST)
> > > > ||
> > > > 
> > > >       cmd_first - cmd >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
> > > >                 return
> > > > -ENOTTY;
> > > >         }
> > > > 
> > > > What do you think?
> > > > 
> > > > > 
> > > > > COUNT macros are being used to test distance of an ioctl command
> > > > > in question from the smallest one, so we don't want it to be
> > > > > larger than necessary.
> > > > > 
> > > > > Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
> > > > > ---
> > > > >  fs/autofs4/autofs_i.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
> > > > > index 73e3d38..7ef4d08 100644
> > > > > --- a/fs/autofs4/autofs_i.h
> > > > > +++ b/fs/autofs4/autofs_i.h
> > > > > @@ -20,7 +20,7 @@
> > > > >  #define AUTOFS_IOC_COUNT     32
> > > > > 
> > > > >  #define AUTOFS_DEV_IOCTL_IOC_FIRST   (AUTOFS_DEV_IOCTL_VERSION)
> > > > > -#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 11)
> > > > > +#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 0x11)
> > > > > 
> > > > >  #include <linux/kernel.h>
> > > > >  #include <linux/slab.h>
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

* Re: [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value
  2016-07-21  2:02           ` Ian Kent
@ 2016-07-21 18:12             ` Tomohiro Kusumi
  0 siblings, 0 replies; 19+ messages in thread
From: Tomohiro Kusumi @ 2016-07-21 18:12 UTC (permalink / raw)
  To: Ian Kent; +Cc: autofs

Thanks for another detailed explanation.

> And generally all patches that are committed and pushed are also available on
> kernel.org grouped by release. I've been trying my best to ensure they is as
> accurate as possible so it is clear what has gone into each release.

I'm assuming you're talking about
https://www.kernel.org/pub/linux/daemons/autofs/
for userspace stuff.

> Also, at times I have tried to put together a TODO list of longer term things
> that I'd like to do for myself but haven't been very successful, ;)

It would be cool to know the long term goal, if you can (or want to)
make it public.



2016-07-21 11:02 GMT+09:00 Ian Kent <raven@themaw.net>:
> On Thu, 2016-07-21 at 02:53 +0900, Tomohiro Kusumi wrote:
>> > So I think the problem with the miscellaneous device ioctl number check
>> > you've
>> > pointed out does need to be fixed and if you could update the patch to do
>> > that
>> > it would be much appreciated.
>>
>> OK, I'll take a look at it again based on your explanation,
>> and see if I can fix it, plus make things a bit more clear if possible.
>>
>> > Don't forget that the miscellaneous device ioctls are a newer set of ioctls
>> > with
>> > added functionality to the older ioctls, hence the duplication you see with
>> > the
>> > functions provided with each set, and the check for that specific range is
>> > relevant to the later ones only.
>> >
>> > Also, only one or the other set of ioctls can be used and the miscellaneous
>> > device ioctls can only be used with version 5 and above of user space
>> > autofs.
>>
>> Yes, I was wondering why they're implemented to both fs root and /dev/autofs
>> when I first looked at ioctl and userspace code.
>>
>> >
>> > I'd like to try (again) to re-implement this with netlink and add readdir
>> > and
>> > asynchronous expire functionality but I don't know when I'll get time to do
>> > it s
>> > ince it is a lot of work in kernel and user space, but I digress.
>>
>> Is there a publicly available list of TODO for autofs (kernel and userspace) ?
>>
>> According to some of your recent posts to this mailing list,
>> you seem to have lots of patches that haven't been merged to upstream,
>> and also things yet to be implemented.
>
> There isn't.
>
> The truth is there haven't been other developers that have stuck with it so I've
> pretty much done everything in isolation.
>
> That's not too good because that means there isn't discussion about how things
> should be done and there's very little review of changes too, since there's no
> -one to review them.
>
> If there were others, such as yourself, I would start doing things quite
> differently, but clearly that would take time to start happening.
>
> I detest writing and maintaining web presences so there is no wiki which is
> another problem, and is the sort of place where a TODO list would reside.
>
> As far as patches that are either not finished, did not solve a problem, or are
> in progress, yes I have plenty of those and they have accumulated over many
> years.
>
> That's not uncommon when you work with something for a long time but they are not useful until you return to a problem they may be related too, so mostly not useful to others.
>
> For example, I tried to implement the control interface using the netlink
> mechanism, a long time ago now, and failed but I keep the work that I did in
> case I can use it at some time is the future, that's my nature.
>
> A netlink interface would be better than using ioctls, it would be more flexible
> in the long run but, at the time, I was unwilling to fundamentally change the
> way automount(8) works and there were other difficulties with asynchronous
> expires so I failed to get it working and implemented the anonymous device ioctl
> interface instead.
>
> So these patches are not useful at all and are just a consequence of things I
> have tried to do in the past.
>
> But there is still the question of how to better deal with listing of
> directories (using certain options) that can cause everything within the
> directory to mount.
>
> Over the years I've thought about it many times and at times I think a kernel
> readdir implementation would be better and (often after thinking more about it)
> I come back to there being no advantage over the existing pre-creation of mount
> point directories.
>
> If I was to, one day, believe that a kernel readdir implementation would make a
> worthwhile difference then I think it would be better to implement that using
> the netlink sub-system and the problems with that would need to be worked out to
> do so.
>
> So it ends up being little more than something for thought only, or possibly
> discussion if there were others to take part in it, and what I have for this
> would be extremely hard for anyone else to use as it is a mess of partially done
> work.
>
> For a long time now I accumulate patches and then commit and push those that
> prove sound. And often it has been quite a while between pushes so there can end
> up being quite a few, but that can change if there is reason to do so.
>
> And generally all patches that are committed and pushed are also available on
> kernel.org grouped by release. I've been trying my best to ensure they is as
> accurate as possible so it is clear what has gone into each release.
>
> Also, at times I have tried to put together a TODO list of longer term things
> that I'd like to do for myself but haven't been very successful, ;)
>
>>
>>
>> 2016-07-20 9:47 GMT+09:00 Ian Kent <raven@themaw.net>:
>> > On Wed, 2016-07-20 at 04:29 +0900, Tomohiro Kusumi wrote:
>> > > Thanks for your review,
>> > >
>> > > Yes, yours apparently works and that's exactly the right test based on
>> > > currently existing ioctl cmds.
>> > > I first thought about changing it like you did, but thought about
>> > > meaning of AUTOFS_IOC_COUNT which is 32.
>> >
>> > I think the meaning of this is taken from Documentation/ioctl/ioctl
>> > -number.txt.
>> >
>> > The define isn't actually used though, which is what I was saying.
>> >
>> > >
>> > > I took this 32 as some sort of reserved range for autofs ioctl cmds,
>> > > where the whole ioctl cmds are designed to be somewhere in 0x60-0x80
>> > > range, and /dev/autofs ioctl cmds are in 0x71-0x80 range.
>> >
>> > Yes, if ioctl numbers beyond 0x7e are to be used I think we need to update
>> > the
>> > above file.
>> >
>> > It's been a while since I did this but the highest miscellaneous device
>> > ioctl
>> > number is 0x7e so if new ioctls were needed the usage registration file
>> > would
>> > need to be updated and posted as a patch.
>> >
>> > > So in theory a potential ioctl cmd for /dev/autofs counting from the
>> > > smallest cmd must be no larger than 32-0x11 (or 0x80-0x71).
>> > >
>> > > I've no idea what this reservation/limitation up to 0x80 is supposed
>> > > to mean though, as ioctl has 8 bits for nr part.
>> > > If you say this 32 is meaningless, I'll resubmit it with your proposed
>> > > change.
>> >
>> >
>> > The AUTOFS_IOC_COUNT should stay purely because it documents the "claimed"
>> > autofs range (from 0x60-0x7f). Maybe a comment is needed to clear this up.
>> >
>> > But AUTOFS_DEV_IOCTL_IOC_COUNT is supposed to be used to check the passed in
>> > miscellaneous device ioctl number is between 0x71-0x7f (or would if it was
>> > correct).
>> >
>> > So I think the problem with the miscellaneous device ioctl number check
>> > you've
>> > pointed out does need to be fixed and if you could update the patch to do
>> > that
>> > it would be much appreciated.
>> >
>> > Don't forget that the miscellaneous device ioctls are a newer set of ioctls
>> > with
>> > added functionality to the older ioctls, hence the duplication you see with
>> > the
>> > functions provided with each set, and the check for that specific range is
>> > relevant to the later ones only.
>> >
>> > Also, only one or the other set of ioctls can be used and the miscellaneous
>> > device ioctls can only be used with version 5 and above of user space
>> > autofs.
>> >
>> > I'd like to try (again) to re-implement this with netlink and add readdir
>> > and
>> > asynchronous expire functionality but I don't know when I'll get time to do
>> > it s
>> > ince it is a lot of work in kernel and user space, but I digress.
>> >
>> > >
>> > >
>> > >      0x60               0x71            0x80
>> > > -----+------------------+---------------+------------------> NR
>> > >      <----------------------------------> AUTOFS_IOC_COUNT (32)
>> > >                         <---------------> AUTOFS_DEV_IOCTL_IOC_COUNT
>> >
>> > > 2016-07-19 10:34 GMT+09:00 Ian Kent <raven@themaw.net>:
>> > > > On Sun, 2016-07-10 at 17:07 +0900, Tomohiro Kusumi wrote:
>> > > > > According to include/uapi/linux/auto_fs.h,
>> > > > > include/uapi/linux/auto_fs4.h,
>> > > > > and include/linux/auto_dev-ioctl.h, nr part of
>> > > > > AUTOFS_IOC_XXX starts off from 0x60 and
>> > > > > AUTOFS_DEV_IOCTL_XXX starts off from 0x71 which is 0x60+0x11.
>> > > > >
>> > > > > From the way above macros are defined, it seems
>> > > > > AUTOFS_DEV_IOCTL_IOC_COUNT should be (0x20-0x11) instead of (0x20-11).
>> > > > > (Otherwise it's hard to figure out where 11 comes from)
>> > > >
>> > > > You're correct, this is wrong but I think this change is also wrong.
>> > > >
>> > > > And so is the range check in dev-ioctl.c.
>> > > >
>> > > > There's no check for an ioctl number greater than the range claimed by
>> > > > autofs
>> > > > and the check in dev-ioctl.c seeks to check the number used is within
>> > > > the
>> > > > range
>> > > > used by the dev ioctls.
>> > > >
>> > > > Essentially
>> > > >
>> > > > AUTOFS_DEV_IOCTL_VERSION_CMD - AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD = 0xd
>> > > >
>> > > > 14 ioctls ranging from 0x71 - 0x7e (inclusive).
>> > > >
>> > > > The range check in dev-ioctl.c is:
>> > > >
>> > > >         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST)
>> > > > ||
>> > > >             cmd - cmd_first >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
>> > > >                 return -ENOTTY;
>> > > >         }
>> > > >
>> > > > so I think AUTOFS_DEV_IOCTL_IOC_COUNT should be:
>> > > >
>> > > > #define AUTOFS_DEV_IOCTL_IOC_COUNT \
>> > > >         (AUTOFS_DEV_IOCTL_VERSION_CMD -
>> > > > AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)
>> > > >
>> > > > and the check should be:
>> > > >
>> > > >         if (_IOC_TYPE(command) != _IOC_TYPE(AUTOFS_DEV_IOCTL_IOC_FIRST)
>> > > > ||
>> > > >
>> > > >       cmd_first - cmd >= AUTOFS_DEV_IOCTL_IOC_COUNT) {
>> > > >                 return
>> > > > -ENOTTY;
>> > > >         }
>> > > >
>> > > > What do you think?
>> > > >
>> > > > >
>> > > > > COUNT macros are being used to test distance of an ioctl command
>> > > > > in question from the smallest one, so we don't want it to be
>> > > > > larger than necessary.
>> > > > >
>> > > > > Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
>> > > > > ---
>> > > > >  fs/autofs4/autofs_i.h | 2 +-
>> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > > >
>> > > > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
>> > > > > index 73e3d38..7ef4d08 100644
>> > > > > --- a/fs/autofs4/autofs_i.h
>> > > > > +++ b/fs/autofs4/autofs_i.h
>> > > > > @@ -20,7 +20,7 @@
>> > > > >  #define AUTOFS_IOC_COUNT     32
>> > > > >
>> > > > >  #define AUTOFS_DEV_IOCTL_IOC_FIRST   (AUTOFS_DEV_IOCTL_VERSION)
>> > > > > -#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 11)
>> > > > > +#define AUTOFS_DEV_IOCTL_IOC_COUNT   (AUTOFS_IOC_COUNT - 0x11)
>> > > > >
>> > > > >  #include <linux/kernel.h>
>> > > > >  #include <linux/slab.h>
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

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

end of thread, other threads:[~2016-07-21 18:12 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-10  8:07 [PATCH 01/12] autofs: Remove obsolete sb fields Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 02/12] autofs: Don't fail to free_dev_ioctl(param) Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 03/12] autofs: Fix AUTOFS_DEV_IOCTL_IOC_COUNT value Tomohiro Kusumi
2016-07-19  1:34   ` Ian Kent
2016-07-19 19:29     ` Tomohiro Kusumi
2016-07-20  0:47       ` Ian Kent
2016-07-20 17:53         ` Tomohiro Kusumi
2016-07-21  2:02           ` Ian Kent
2016-07-21 18:12             ` Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 04/12] autofs: Change _LINUX_AUTO_FS4_H to _UAPI_LINUX_AUTO_FS4_H Tomohiro Kusumi
2016-07-19  1:40   ` Ian Kent
2016-07-10  8:07 ` [PATCH 05/12] autofs: Remove AUTOFS_DEVID_LEN Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 06/12] autofs: Fix Documentation regarding devid on ioctl Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 07/12] autofs: Update struct autofs_dev_ioctl in Documentation Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 08/12] autofs: Fix pr_debug() message Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 09/12] autofs: Fix wrong file/function/macro names in comments Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 10/12] autofs: Add autofs_dev_ioctl_version() for AUTOFS_DEV_IOCTL_VERSION_CMD Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 11/12] autofs: Fix print format for ioctl warning message Tomohiro Kusumi
2016-07-10  8:07 ` [PATCH 12/12] autofs: Move inclusion of linux/limits.h to uapi Tomohiro Kusumi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.