autofs: make the autofsv5 packet file descriptor use a packetized pipe

With both automount and systemd doing a single read() system call, and verifying that they get *exactly* the size they expect but using different sizes, it seemed that fixing one of them inevitably seemed to break the other. At one point, a patch I seriously considered applying from Michael Tokarev did a "strcmp()" to see if it was automount that was doing the operation. Ugly, ugly.

However, a prettier solution exists now thanks to the packetized pipe mode. By marking the communication pipe as being packetized (by simply setting the O_DIRECT flag), we can always just write the bigger packet size, and if user-space does a smaller read, it will just get that partial end result and the extra alignment padding will simply be thrown away.

This makes both automount and systemd happy, since they now get the size they asked for, and the kernel side of autofs simply no longer needs to care - it could pad out the packet arbitrarily.

Of course, if there is some *other* user of autofs (please, please, please tell me it ain't so - and we haven't heard of any) that tries to read the packets with multiple writes, that other user will now be broken - the whole point of the packetized mode is that one system call gets exactly one packet, and you cannot read a packet in pieces.

I just looked at am-utils; am-utils *does* use autofs v5, and *will* loop back and read more data on a short read.

Quoted 3 lines · H. Peter Anvin

I just looked at am-utils; am-utils *does* use autofs v5, and *will* loop back and read more data on a short read.

Can you point to the sources?

If it's "short read" as in "I didn't get as much as I expected", that's fine.

If it is "short read" as in "I don't even try to read 300 bytes, I read it in 8-byte chunks", we're screwed.

 do {
   i = read(fd, buf, bytes);

   if (i <= 0)
     break;

   buf = (char *)buf + i;
   bytes -= i;
 } while (bytes);

 return bytes;

The problem is only if it starts out by reading just the header of the packet, and then reads the rest of the packet as a second read. *THAT* won't work with the packetized pipe approach, because reading the header of the packet will then discard the rest of it, and the second read would try to read the *next* packet (which under many normal loads won't even exist, of course).

Which is funny/sad, because that's actually the intended way the interface is meant to work.

(There's a "len" field in the v5 packet, but that's not the packet length, that's the length of the name component)

Quoted 3 lines · H. Peter Anvin

Which is funny/sad, because that's actually the intended way the interface is meant to work.

Well, the autofs packet model is actually horribly badly misdesigned even for that: the header doesn't contain the size of the packet. It contains the packet *type*, and from that you can then determine the size (of course, every other program would determine it *wrongly* for this whole x86-64 alignment reason), but that is actually a horrible model because it assumes you know all the packet types.

(There's a "len" field in the v5 packet, but that's not the packet length, that's the length of the name component)

And the reason nobody does that is that in practice there is only ever one single type of packet that is possible anyway, so there's no point in even reading the header to find the type.

So a much nicer model is one where the actual *size* of the packet is in the header. That would have allowed for not having that fixed maximum size of a name etc, and would have avoided the whole problem to begin with.

Of course, the nicest model of all is to just use a packetized interface to begin with, so that none of these issues exist. Which is what we're now effectively moving to, unless we can find some horrible program that makes that impossible due to it playing games and knowing it's a "stream".

Looking at am-utils, I think we're ok so far. But maybe you know of yet another crazy user of the autofs interfaces.

Linus

autodir looks ok as well.

http://sourceforge.net/projects/intraperson/files/latest/download

Quoted 4 lines · Ian Kent

autodir looks ok as well.

http://sourceforge.net/projects/intraperson/files/latest/download

That looks odd, but safe.

Why does the code make the pipe file descriptor non-blocking, when it then always reads it using that odd "poll_read()" function that does a loop with poll() and read(). I guess there is some 1-second timeout thing.

It's a bit odd in other ways too. The "handle_events()" function is passed the fd, but then it never actually uses it, and uses 'autodir.k_pipe' instead.

So I'm having a bit of trouble following the *logic* to any of that, but the only reads I found did seem to match the "read whole packet" model, so it does look ok too.

Incidentally, I think there is a good reason to add a v6 packet type regardless, for efficiency: with packetized pipes there really is no point in sending a packet which is mostly padding for no good reason.

Incidentally, I think there is a good reason to add a v6 packet type regardless, for efficiency: with packetized pipes there really is no point in sending a packet which is mostly padding for no good reason.

Thinking about it some more: for v6, I wouldn't use a packetized pipe at all (due to the unnecessary extra buffer consumption.)  Instead just put the message size in the header and read a large chunk, which may end up being more than one packet and may end up with a partial packet at the end.  *This is okay*, because there is only one reader, and any additional data needed will be gotten the next time around the loop.

The thing I *really* hate about the current autofs situation is how it really does everything you can possibly do wrong. Not at all the "let's just read as much as we can, and look at the return value to determine number of bytes/packets", but literally "I know a-priori how big the packet is, and I will read exactly that many bytes". That coupled with the psueod-fixed-size packets just made it a very very fragile thing.

Quoted 6 lines

However, a prettier solution exists now thanks to the packetized pipe mode. By marking the communication pipe as being packetized (by simply setting the O_DIRECT flag), we can always just write the bigger packet size, and if user-space does a smaller read, it will just get that partial end result and the extra alignment padding will simply be thrown away.

(quoted diff) +1 -0

+static inline int autofs_prepare_pipe(struct file *pipe)

Quoted 19 lines · Linux Kernel Mailing List

Quoted 6 lines

However, a prettier solution exists now thanks to the packetized pipe mode. By marking the communication pipe as being packetized (by simply setting the O_DIRECT flag), we can always just write the bigger packet size, and if user-space does a smaller read, it will just get that partial end result and the extra alignment padding will simply be thrown away.

(quoted diff) +11 -0

+static inline int autofs_prepare_pipe(struct file *pipe)
+{
+	if (!pipe->f_op || !pipe->f_op->write)
+		return -EINVAL;
+	if (!S_ISFIFO(pipe->f_dentry->d_inode->i_mode))
+		return -EINVAL;
+	/* We want a packet pipe */
+	pipe->f_flags |= O_DIRECT;
+	return 0;
+}
+

fs/autofs4/dev-ioctl.c +1 -1

@@ -376,7 +376,7 @@ static int autofs_dev_ioctl_setpipefd(st
 			err = -EBADF;
 			goto out;
 		}
-		if (!pipe->f_op || !pipe->f_op->write) {
+		if (autofs_prepare_pipe(pipe) < 0) {
 			err = -EPIPE;
 			fput(pipe);
 			goto out;

I've one more concern. I'm not sure but I think there's some risk still. This packetizing gets applied to all VERSIONS of the autofs PROTOCOL. Which means it will be applied to the lowest supported version (3) TOO, but did that version read whole packets too?

I think this is a false alarm. I checked autofs v3 and v4 userspace code (found on http://www.kernel.org/pub/linux/daemons/autofs ) and they both read whole packet at once, not piece-wise.

Quoted 30 lines · Michael Tokarev

Quoted 19 lines · Linux Kernel Mailing List

Quoted 6 lines

However, a prettier solution exists now thanks to the packetized pipe mode. By marking the communication pipe as being packetized (by simply setting the O_DIRECT flag), we can always just write the bigger packet size, and if user-space does a smaller read, it will just get that partial end result and the extra alignment padding will simply be thrown away.

(quoted diff) +11 -0

+static inline int autofs_prepare_pipe(struct file *pipe)
+{
+	if (!pipe->f_op || !pipe->f_op->write)
+		return -EINVAL;
+	if (!S_ISFIFO(pipe->f_dentry->d_inode->i_mode))
+		return -EINVAL;
+	/* We want a packet pipe */
+	pipe->f_flags |= O_DIRECT;
+	return 0;
+}
+

fs/autofs4/dev-ioctl.c +1 -1

@@ -376,7 +376,7 @@ static int autofs_dev_ioctl_setpipefd(st
 			err = -EBADF;
 			goto out;
 		}
-		if (!pipe->f_op || !pipe->f_op->write) {
+		if (autofs_prepare_pipe(pipe) < 0) {
 			err = -EPIPE;
 			fput(pipe);
 			goto out;

Quoted 5 lines

I've one more concern. I'm not sure but I think there's some risk still. This packetizing gets applied to all VERSIONS of the autofs PROTOCOL. Which means it will be applied to the lowest supported version (3) TOO, but did that version read whole packets too?

I think this is a false alarm. I checked autofs v3 and v4 userspace code (found on http://www.kernel.org/pub/linux/daemons/autofs ) and they both read whole packet at once, not piece-wise.

I didn't test if any of these actually work with any current kernel, however ;)