* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe [not found] <20120429205429.63CCD7C0064@ra.kernel.org> @ 2012-04-30 0:15 ` H. Peter Anvin 2012-04-30 0:25 ` Linus Torvalds 2012-04-30 6:27 ` Michael Tokarev 0 siblings, 2 replies; 19+ messages in thread From: H. Peter Anvin @ 2012-04-30 0:15 UTC (permalink / raw) To: Linux Kernel Mailing List Cc: Linus Torvalds, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:15 ` autofs: make the autofsv5 packet file descriptor use a packetized pipe H. Peter Anvin @ 2012-04-30 0:25 ` Linus Torvalds 2012-04-30 0:28 ` H. Peter Anvin 2012-04-30 0:30 ` H. Peter Anvin 2012-04-30 6:27 ` Michael Tokarev 1 sibling, 2 replies; 19+ messages in thread From: Linus Torvalds @ 2012-04-30 0:25 UTC (permalink / raw) To: H. Peter Anvin Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:25 ` Linus Torvalds @ 2012-04-30 0:28 ` H. Peter Anvin 2012-04-30 0:33 ` Linus Torvalds 2012-04-30 0:30 ` H. Peter Anvin 1 sibling, 1 reply; 19+ messages in thread From: H. Peter Anvin @ 2012-04-30 0:28 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:28 ` H. Peter Anvin @ 2012-04-30 0:33 ` Linus Torvalds 2012-04-30 0:35 ` H. Peter Anvin 0 siblings, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2012-04-30 0:33 UTC (permalink / raw) To: H. Peter Anvin Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:33 ` Linus Torvalds @ 2012-04-30 0:35 ` H. Peter Anvin 2012-04-30 0:43 ` Linus Torvalds 0 siblings, 1 reply; 19+ messages in thread From: H. Peter Anvin @ 2012-04-30 0:35 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:35 ` H. Peter Anvin @ 2012-04-30 0:43 ` Linus Torvalds 2012-04-30 0:45 ` H. Peter Anvin 2012-04-30 1:29 ` Ian Kent 0 siblings, 2 replies; 19+ messages in thread From: Linus Torvalds @ 2012-04-30 0:43 UTC (permalink / raw) To: H. Peter Anvin Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:43 ` Linus Torvalds @ 2012-04-30 0:45 ` H. Peter Anvin 2012-04-30 1:29 ` Ian Kent 1 sibling, 0 replies; 19+ messages in thread From: H. Peter Anvin @ 2012-04-30 0:45 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:43 ` Linus Torvalds 2012-04-30 0:45 ` H. Peter Anvin @ 2012-04-30 1:29 ` Ian Kent 2012-04-30 1:56 ` Linus Torvalds 1 sibling, 1 reply; 19+ messages in thread From: Ian Kent @ 2012-04-30 1:29 UTC (permalink / raw) To: Linus Torvalds Cc: H. Peter Anvin, Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 1:29 ` Ian Kent @ 2012-04-30 1:56 ` Linus Torvalds 2012-04-30 5:57 ` Ian Kent 0 siblings, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2012-04-30 1:56 UTC (permalink / raw) To: Ian Kent Cc: H. Peter Anvin, Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 1:56 ` Linus Torvalds @ 2012-04-30 5:57 ` Ian Kent 0 siblings, 0 replies; 19+ messages in thread From: Ian Kent @ 2012-04-30 5:57 UTC (permalink / raw) To: Linus Torvalds Cc: H. Peter Anvin, Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:25 ` Linus Torvalds 2012-04-30 0:28 ` H. Peter Anvin @ 2012-04-30 0:30 ` H. Peter Anvin 2012-04-30 0:37 ` Linus Torvalds 2012-04-30 20:03 ` H. Peter Anvin 1 sibling, 2 replies; 19+ messages in thread From: H. Peter Anvin @ 2012-04-30 0:30 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:30 ` H. Peter Anvin @ 2012-04-30 0:37 ` Linus Torvalds 2012-04-30 20:03 ` H. Peter Anvin 1 sibling, 0 replies; 19+ messages in thread From: Linus Torvalds @ 2012-04-30 0:37 UTC (permalink / raw) To: H. Peter Anvin Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:30 ` H. Peter Anvin 2012-04-30 0:37 ` Linus Torvalds @ 2012-04-30 20:03 ` H. Peter Anvin 2012-04-30 20:11 ` Linus Torvalds 1 sibling, 1 reply; 19+ messages in thread From: H. Peter Anvin @ 2012-04-30 20:03 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 20:03 ` H. Peter Anvin @ 2012-04-30 20:11 ` Linus Torvalds 2012-04-30 20:14 ` H. Peter Anvin 0 siblings, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2012-04-30 20:11 UTC (permalink / raw) To: H. Peter Anvin Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 20:11 ` Linus Torvalds @ 2012-04-30 20:14 ` H. Peter Anvin 0 siblings, 0 replies; 19+ messages in thread From: H. Peter Anvin @ 2012-04-30 20:14 UTC (permalink / raw) To: Linus Torvalds Cc: Linux Kernel Mailing List, Michael Tokarev, Alan Cox, Ian Kent, Thomas Meyer, autofs > > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 0:15 ` autofs: make the autofsv5 packet file descriptor use a packetized pipe H. Peter Anvin 2012-04-30 0:25 ` Linus Torvalds @ 2012-04-30 6:27 ` Michael Tokarev 2012-04-30 6:43 ` Michael Tokarev 1 sibling, 1 reply; 19+ messages in thread From: Michael Tokarev @ 2012-04-30 6:27 UTC (permalink / raw) To: Linus Torvalds Cc: H. Peter Anvin, Linux Kernel Mailing List, Alan Cox, Ian Kent, Thomas Meyer, autofs [-- 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; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 6:27 ` Michael Tokarev @ 2012-04-30 6:43 ` Michael Tokarev 2012-04-30 6:48 ` H. Peter Anvin 2012-04-30 6:55 ` Ian Kent 0 siblings, 2 replies; 19+ messages in thread From: Michael Tokarev @ 2012-04-30 6:43 UTC (permalink / raw) To: Linus Torvalds Cc: H. Peter Anvin, Linux Kernel Mailing List, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 6:43 ` Michael Tokarev @ 2012-04-30 6:48 ` H. Peter Anvin 2012-04-30 6:55 ` Ian Kent 1 sibling, 0 replies; 19+ messages in thread From: H. Peter Anvin @ 2012-04-30 6:48 UTC (permalink / raw) To: Michael Tokarev Cc: Linus Torvalds, Linux Kernel Mailing List, Alan Cox, Ian Kent, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: autofs: make the autofsv5 packet file descriptor use a packetized pipe 2012-04-30 6:43 ` Michael Tokarev 2012-04-30 6:48 ` H. Peter Anvin @ 2012-04-30 6:55 ` Ian Kent 1 sibling, 0 replies; 19+ messages in thread From: Ian Kent @ 2012-04-30 6:55 UTC (permalink / raw) To: Michael Tokarev Cc: Linus Torvalds, H. Peter Anvin, Linux Kernel Mailing List, Alan Cox, Thomas Meyer, autofs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-04-30 20:15 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20120429205429.63CCD7C0064@ra.kernel.org> 2012-04-30 0:15 ` autofs: make the autofsv5 packet file descriptor use a packetized pipe H. Peter Anvin 2012-04-30 0:25 ` Linus Torvalds 2012-04-30 0:28 ` H. Peter Anvin 2012-04-30 0:33 ` Linus Torvalds 2012-04-30 0:35 ` H. Peter Anvin 2012-04-30 0:43 ` Linus Torvalds 2012-04-30 0:45 ` H. Peter Anvin 2012-04-30 1:29 ` Ian Kent 2012-04-30 1:56 ` Linus Torvalds 2012-04-30 5:57 ` Ian Kent 2012-04-30 0:30 ` H. Peter Anvin 2012-04-30 0:37 ` Linus Torvalds 2012-04-30 20:03 ` H. Peter Anvin 2012-04-30 20:11 ` Linus Torvalds 2012-04-30 20:14 ` H. Peter Anvin 2012-04-30 6:27 ` Michael Tokarev 2012-04-30 6:43 ` Michael Tokarev 2012-04-30 6:48 ` H. Peter Anvin 2012-04-30 6:55 ` Ian Kent
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).