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

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

[H. Peter Anvin]

On 04/29/2012 01:54 PM, Linux Kernel Mailing List wrote:
>     
>     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.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Linus Torvalds]

On Sun, Apr 29, 2012 at 5:15 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> 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.

                     Linus

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

[H. Peter Anvin]

On 04/29/2012 05:25 PM, Linus Torvalds wrote:
> On Sun, Apr 29, 2012 at 5:15 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> 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.
> 

git://git.fsl.cs.sunysb.edu/am-utils-6.2.git
conf/autofs/autofs_linux.c

static ssize_t
autofs_get_pkt(int fd, void *buf, size_t bytes)
{
  ssize_t i;

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

    if (i <= 0)
      break;

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

  return bytes;
}

...

#if AUTOFS_MAX_PROTO_VERSION >= 5
    if (fh->version < 5) {
      len = sizeof(p.pkt);
    } else {
      len = sizeof(p.pkt5);
    }
#else
    len = sizeof(p.pkt);
#endif /* AUTOFS_MAX_PROTO_VERSION >= 5 */

    if (autofs_get_pkt(fh->fd, &p, len))
      continue;

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

[Linus Torvalds]

On Sun, Apr 29, 2012 at 5:28 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
>  do {
>    i = read(fd, buf, bytes);
>
>    if (i <= 0)
>      break;
>
>    buf = (char *)buf + i;
>    bytes -= i;
>  } while (bytes);
>
>  return bytes;

Ok, that should be fine. It will always get the full packet in one
read, so the short read case will never actually happen.

In fact, automount has this exact same pattern, except it calls the
function "fullread()".

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).

                       Linus

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

[H. Peter Anvin]

On 04/29/2012 05:33 PM, Linus Torvalds wrote:
> 
> 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.

	-hpa

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

[Linus Torvalds]

On Sun, Apr 29, 2012 at 5:35 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> 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

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

[H. Peter Anvin]

On 04/29/2012 05:43 PM, Linus Torvalds wrote:
> 
> (There's a "len" field in the v5 packet, but that's not the packet
> length, that's the length of the name component)
> 

Yes, that dates back from autofs v2/3 where there was only one packet type.

	-hpa

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

[Ian Kent]

On Sun, 2012-04-29 at 17:43 -0700, Linus Torvalds wrote:
> On Sun, Apr 29, 2012 at 5:35 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> >
> > 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.

autodir looks ok as well.

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

> 
>                       Linus

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

[Linus Torvalds]

On Sun, Apr 29, 2012 at 6:29 PM, Ian Kent <raven@themaw.net> wrote:
>
> 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.

                     Linus

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

[Ian Kent]

On Sun, 2012-04-29 at 18:56 -0700, Linus Torvalds wrote:
> On Sun, Apr 29, 2012 at 6:29 PM, Ian Kent <raven@themaw.net> wrote:
> >
> > 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.

I'm only aware that autodir uses the module and wasn't involved in the
development.

I didn't look closely at it at all, just enough to check for reads to
the kernel pipe, so I don't know either.

I'm not sure that autodir is still maintained so the only thing I'll do
is include a cc to the autodir mailing list in a any follow up mail to
the autofs list explaining the outcome of this work.

Ian

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

[H. Peter Anvin]

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.

	-hpa

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

[Linus Torvalds]

On Sun, Apr 29, 2012 at 5:30 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> 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.

That I agree with - making a v6 that sends a minimum-sized packets,
and where the readers just always try to read the maximum size.

That said, this is not exactly something high-performance, and copying
the pointless padding to make a fixed-size packet isn't a big deal.
We're talking just 240 extra bytes or so (NAME_MAX is 255, and most
names are obviously much shorter), so I do believe that we might as
well wait unless there are other good cleanups too.

                   Linus

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

[H. Peter Anvin]

On 04/29/2012 05:30 PM, H. Peter Anvin wrote:
> 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.

	-hpa

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

[Linus Torvalds]

On Mon, Apr 30, 2012 at 1:03 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> 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.

Yes, doing multiple streaming packets is good, but then you really do
need to design the protocol for streaming, which very much includes
having that size early in the header. And then you'd better make the
packets really have different sizes, so that you cannot possibly get
it wrong in an app that "knows" that there's one size.

However, in the case of autofs, I don't actually see multiple packets
being that common. Sure, they can happen, but it really looks like the
normal case will always be the trivial ping-pong scenario ("read one
packet, react to it, rinse and repeat").

But using a big buffer and trying to read lots at one time is valid even then.

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.

                  Linus

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

[H. Peter Anvin]

> 
> 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.
> 

Yes, and that was my fault, originally... back when there was one packet
size and long before compat was a concern.

	-hpa

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

[Michael Tokarev]

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

> On 04/29/2012 01:54 PM, Linux Kernel Mailing List wrote:
>>     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.

> +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;
> +}
> +

@@ -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?

Maybe something like the attached should be applied?

Thanks,

/mjt

[-- Attachment #2: autofs-enable-workaround-for-v5-only.diff --]
[-- Type: text/x-patch, Size: 2543 bytes --]

Enable packetized mode for autofs v5+ only

In commit 64f371bc3107e69efce563a3d0f0e6880de0d537 "autofs: make the
autofsv5 packet file descriptor use a packetized pipe", we enable
the packetized mode of the pipe unconditionally for all versions
of autofs protocol, but we actually only tested v5 version, and
it is unknown how it worked for previous versions of the protocol
and older binaries, who may actually read the packed piece by piece.
Enable the packetized mode only if client protocol is >= 5.

Note: the current function autofs_prepare_pipe() is called from 2
places which does the same thing with other fields of sbi structure,
so all this work may be put into a common function.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>


diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h
index 908e184..72e315a 100644
--- a/fs/autofs4/autofs_i.h
+++ b/fs/autofs4/autofs_i.h
@@ -269,14 +269,17 @@ 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)
+static inline int autofs_prepare_pipe(struct autofs_sb_info *sbi,
+				       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;
+	if (sbi->version >= 5)
+		/* We want a packet pipe */
+		pipe->f_flags |= O_DIRECT;
+	sbi->pipe = pipe;
 	return 0;
 }
 
diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index aa9103f..cdf9dbc 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -376,14 +376,13 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 			err = -EBADF;
 			goto out;
 		}
-		if (autofs_prepare_pipe(pipe) < 0) {
+		if (autofs_prepare_pipe(sbi, pipe) < 0) {
 			err = -EPIPE;
 			fput(pipe);
 			goto out;
 		}
 		sbi->oz_pgrp = task_pgrp_nr(current);
 		sbi->pipefd = pipefd;
-		sbi->pipe = pipe;
 		sbi->catatonic = 0;
 	}
 out:
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index 6e488eb..3b1eb5c 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -290,9 +290,8 @@ 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 (autofs_prepare_pipe(pipe) < 0)
+	if (autofs_prepare_pipe(sbi, pipe) < 0)
 		goto fail_fput;
-	sbi->pipe = pipe;
 	sbi->pipefd = pipefd;
 	sbi->catatonic = 0;
 

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

[Michael Tokarev]

On 30.04.2012 10:27, Michael Tokarev wrote:
>> On 04/29/2012 01:54 PM, Linux Kernel Mailing List wrote:
>>>     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.
> 
>> +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;
>> +}
>> +
> 
> @@ -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.

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

Thanks,

/mjt

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

[H. Peter Anvin]

On 04/29/2012 11:43 PM, Michael Tokarev wrote:
> 
> 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.
> 

Yup, the original braindamage is all mine.

	-hpa

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

[Ian Kent]

On Mon, 2012-04-30 at 10:43 +0400, Michael Tokarev wrote:
> On 30.04.2012 10:27, Michael Tokarev wrote:
> >> On 04/29/2012 01:54 PM, Linux Kernel Mailing List wrote:
> >>>     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.
> > 
> >> +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;
> >> +}
> >> +
> > 
> > @@ -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.
> 
> I didn't test if any of these actually work with any current
> kernel, however ;)

Yep, all the autofs releases use a sizeof() the relevant structure when
reading the kernel pipe, so that should be ok.

Ian