Re: [PATCH v2] Introduce a version6 of autofs interface, to fix design error.

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2337 bytes --]

On Fri, Apr 27, 2012 at 12:24 PM, David Miller <davem@davemloft.net> wrote:
>
> If you can pull that off, it would certainly work and be the best
> solution.
>
> BTW, this has happened before, we had this same problem passing data
> over netlink sockets.  And since it's packetized already we had a
> slightly easier time dealing with it.

Ok, so here's a fairly quick hack.

NOTE! I haven't actually tested this, because I don't use autofs
(well, not knowingly - maybe I have a machine that actually does).

What it does is add a new "packetized" flag to pipes, which does two
simple things:
 - do not merge write data in pipe buffers when writing
 - when reading, consider a pipe buffer to be "spent" when any of the
data has been read

Of course, if you write more than PIPE_BUFFER bytes, the packetization
doesn't work, but hey, if you do that, it's your own damn problem.

Then, this just makes (for debugging!) autofs always write eight extra
bytes of garbage (NOTE NOTE NOTE! This will write uninitialized data,
so this is literally just for debugging this particular issue), so
that if the whole "kernel writes a few bytes extra" approach with pipe
packetization doesn't work, you would see it even on native 32-bit or
64-bit mode, without having to even test the special case of "32-bit
automount binary on a 64-bit kernel". So *anybody* who uses autofs can
test this patch and see if autofs still works for them despite the
extra padding on the write.

So note the "+8" in autofs_v5_packet_size(). It's there on purpose,
but it needs to be removed for actual final results if this works for
testing.

Comments? The patch looks fairly simple. The "packetized pipe" might
even be useful for other users and maybe we might want to expose it as
an actual pipe fcntl, but right now the only thing that sets that flag
is autofs.

Does this even work? I might well have missed some obvious thing, this
was put together pretty quickly, but I think the *concept* may be the
right approach to this whole mess.

(And yes, we could probably just pack the "struct pipe_inode_info"
better. We don't really need 32 bits for the pipe writer count etc.
But to make the patch simple, I just added a whole new bitfield, which
will just grow that silly structure).

                       Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 4469 bytes --]

 fs/autofs4/autofs_i.h     |   11 +++++++++++
 fs/autofs4/dev-ioctl.c    |    2 +-
 fs/autofs4/inode.c        |    2 +-
 fs/autofs4/waitq.c        |   14 ++++++--------
 fs/pipe.c                 |    6 +++---
 include/linux/pipe_fs_i.h |    1 +
 6 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index eb1cc92cd67d..f8642b8a1779 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -34,6 +34,7 @@
 #include <linux/sched.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/pipe_fs_i.h>
 #include <asm/current.h>
 #include <asm/uaccess.h>
 
@@ -270,6 +271,16 @@ int autofs4_fill_super(struct super_block *, void *, int);
 struct autofs_info *autofs4_new_ino(struct autofs_sb_info *);
 void autofs4_clean_ino(struct autofs_info *);
 
+static inline int autofs_prepare_pipe(struct file *pipe)
+{
+	if (!pipe->f_op || !pipe->f_op->write)
+		return -EINVAL;
+	if (!pipe->f_dentry->d_inode->i_pipe)
+		return -EINVAL;
+	pipe->f_dentry->d_inode->i_pipe->packetized = 1;
+	return 0;
+}
+
 /* Queue management functions */
 
 int autofs4_wait(struct autofs_sb_info *,struct dentry *, enum autofs_notify);
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 9dacb8586701..6259e7142032 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -376,7 +376,7 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 			err = -EBADF;
 			goto out;
 		}
-		if (!pipe->f_op || !pipe->f_op->write) {
+		if (autofs_prepare_pipe(pipe) < 0) {
 			err = -EPIPE;
 			fput(pipe);
 			goto out;
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index d8dc002e9cc3..c525b74deefd 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -292,7 +292,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 		printk("autofs: could not open pipe file descriptor\n");
 		goto fail_dput;
 	}
-	if (!pipe->f_op || !pipe->f_op->write)
+	if (autofs_prepare_pipe(pipe) < 0)
 		goto fail_fput;
 	sbi->pipe = pipe;
 	sbi->pipefd = pipefd;
diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 9c098db43344..5ce5026200b9 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -97,16 +97,14 @@ static int autofs4_write(struct autofs_sb_info *sbi,
  *
  * The packets are identical on x86-32 and x86-64, but have different
  * alignment. Which means that 'sizeof()' will give different results.
- * Fix it up for the case of running 32-bit user mode on a 64-bit kernel.
+ * However, we packetize the pipe, so just use the bigger size, the
+ * pipe read will discard the rest.
+ *
+ * This adds on 8 bytes of garbage FOR DEBUGGING ONLY!
  */
-static noinline size_t autofs_v5_packet_size(struct autofs_sb_info *sbi)
+static inline size_t autofs_v5_packet_size(struct autofs_sb_info *sbi)
 {
-	size_t pktsz = sizeof(struct autofs_v5_packet);
-#if defined(CONFIG_X86_64) && defined(CONFIG_COMPAT)
-	if (sbi->compat_daemon > 0)
-		pktsz -= 4;
-#endif
-	return pktsz;
+	return sizeof(struct autofs_v5_packet)+8;
 }
 
 static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
diff --git a/fs/pipe.c b/fs/pipe.c
index 25feaa3faac0..76febdfafcc3 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -407,7 +407,7 @@ redo:
 			ret += chars;
 			buf->offset += chars;
 			buf->len -= chars;
-			if (!buf->len) {
+			if (!buf->len || pipe->packetized) {
 				buf->ops = NULL;
 				ops->release(pipe, buf);
 				curbuf = (curbuf + 1) & (pipe->buffers - 1);
@@ -416,7 +416,7 @@ redo:
 				do_wakeup = 1;
 			}
 			total_len -= chars;
-			if (!total_len)
+			if (!total_len || pipe->packetized)
 				break;	/* common path: read succeeded */
 		}
 		if (bufs)	/* More to do? */
@@ -490,7 +490,7 @@ pipe_write(struct kiocb *iocb, const struct iovec *_iov,
 
 	/* We try to merge small writes */
 	chars = total_len & (PAGE_SIZE-1); /* size of the last buffer */
-	if (pipe->nrbufs && chars != 0) {
+	if (!pipe->packetized && pipe->nrbufs && chars != 0) {
 		int lastbuf = (pipe->curbuf + pipe->nrbufs - 1) &
 							(pipe->buffers - 1);
 		struct pipe_buffer *buf = pipe->bufs + lastbuf;
diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
index 6d626ff0cfd0..49f034aaca5e 100644
--- a/include/linux/pipe_fs_i.h
+++ b/include/linux/pipe_fs_i.h
@@ -43,6 +43,7 @@ struct pipe_buffer {
  **/
 struct pipe_inode_info {
 	wait_queue_head_t wait;
+	unsigned int packetized:1;
 	unsigned int nrbufs, curbuf, buffers;
 	unsigned int readers;
 	unsigned int writers;