Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Michael Tokarev]

On 23.04.2012 20:55, Michael Tokarev wrote:
> On 23.04.2012 20:29, Michael Tokarev wrote:
>> On 23.04.2012 13:19, Ian Kent wrote:
>>> On Sun, 2012-04-22 at 19:22 +0400, Michael Tokarev wrote:
>>>> This is JFYI for now, I only diagnozed the issue but had
>>>> no time to see what's going on.
>>>>
>>>> The short story is that with 3.0.24 and later kernels,
>>>> 32bit autofs stopped working on 64bit kernels, while
>>>> it worked before just fine.
>>>>
>>>> The userspace is of version 5.0.4.  Platform is x86.
>>>>
>>>> The sympthoms of the issue is that automount daemon,
>>>> on every access to a (not yet mounted) autofs mount
>>>> point, stalls forever, with the process trying to
>>>> access it stalling forever too -- this is why the
>>>> system didn't reboot, -- it stayed on an attempt to
>>>> access one of automount subdirs, somewhere in the
>>>> middle.
>>
>> This is what happens with 3.0.24+:
>>
>> [pid 15207] read(4, "\5\0\0\0\3\0\0\0\2\0\0\0\35\0\0\0@\17\246\3\0\0\0\0\350\3\0\0\350\3\0\0"..., 304) = 300
>> [pid 15207] read(4,  <unfinished ...> -- forever.
>>
>> It wants to read 304 bytes, but kernel only supplies 300
>> bytes.
>>
>> $ file /usr/sbin/automount
>> /usr/sbin/automount: ELF 32-bit LSB shared object, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.18, stripped
>> $ uname -a
>> Linux gandalf 3.0.0-amd64 #3.0.27 SMP Tue Apr 3 16:23:45 MSK 2012 x86_64 GNU/Linux
>>
>> This is a32744d4abae24572eff7269bc17895c41bd0085
>> "autofs: work around unhappy compat problem on x86-64",
>> included in 3.0.24:
>>
>> +static noinline 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;
>> +}
>> +
>>
>> "End result: the "packed" size of the structure is 300 bytes: 4-byte, but
>> not 8-byte aligned.
>>
>> As a result, despite all the fields being in the same place on all
>> architectures, sizeof() will round up that size to 304 bytes on
>> architectures that have 8-byte alignment for u64."
>>
>> So I'm not sure how it worked before this change (from the
>> description of the patch it looks like it should not work).
>> But it definitely worked for me, and this patch broke it
>> for me.
> 
> It worked because the userspace apparently does the "rigtht"
> think already compensating for the kernel bitness mismatch.
> This is a bit funky too, but it should work:
> 
> static size_t get_kpkt_len(void)
> {
>         size_t pkt_len = sizeof(struct autofs_v5_packet);
>         struct utsname un;
> 
>         uname(&un);
> 
>         if (pkt_len % 8) {
>                 if (strcmp(un.machine, "alpha") == 0 ||
>                     strcmp(un.machine, "ia64") == 0 ||
>                     strcmp(un.machine, "x86_64") == 0 ||
>                     strcmp(un.machine, "ppc64") == 0)
>                         pkt_len += 4;
> 
>         }
> 
>         return pkt_len;
> }
> 
> So it looks like the kernel/user interface had an error,
> userspace adopted to the kernel by doing something, so
> it started working.  And next kernel adopted to the un-
> patched userspace, thus breaking patched userspace.
> 
> That's quite messy... :(  And I'm not sure what to do
> about this, -- the only solution - in my opinion anyway -
> is to revert this kernel patch and maybe switch to a
> new protocol version.

Okay, even if messy, there are quite easy pure userspace
solutions to all this.

But first of all, I want to question this change in the
first place, hence CC'ing to LKML too.

Kernel has been shipping with this brokeness for quite
some time, namely, since introduction of autofs4, dated
Mon Mar 27 01:14:55 2006 -0800 (commit 5c0a32fc2cd0).

This is 6 whole years by now.

Main userspace user of this interface adopted long ago
too, and has been in wide use for years as well.

This kernel change, even if right in theory, actually
breaks all existing userspace which was working fine
for years.

It looks like this is very much against main linux
principle of not breaking stuff even if the change is
"obviously right".

So it pretty much looks like this very fix has to be
reverted in mainline and in all stable kernels.  At least
that's what I'd do if you ask me.


Now, back to "pure userspace" solution.

The kernel sends data to userspace, and the unit of
exchange is pretty small - this structure of either
300 or 304 bytes.  Can the read from userspace EVER
be partial, so that a fullread() function is necessary?
I'm not sure, but to me it looks like this fullread()
loop isn't needed in the first place, and just single
read() (maybe repeating it in case of EINTR) should
do the job already.  This way, we either handle the
data we've read, if we think the amount we read
is sufficient, and the packet looks sane, or just
error out.  The filename is at the end of the struct,
so actually there's no need to send whole struct from
kernel, it is sufficient for the kernel to write up
to the trailing zero, and userspace _may_ be able to
understand (and no, this is not what userspace expects
currently, so changing kernel to do so will break things --
I'm just saying what can be done in _userspace_).

Now, we know that this sizeof(v5_packet) is designed
wrongly.  But it is actually trivial for the userspace
to expect EITHER 300 bytes OR 304 bytes from the kernel --
again, all in one go.  For it to work, extend the struct
with a padding at the end so it will be real 304 bytes,
and request reading of these 304 bytes, but handle the
read of 300 bytes too as successful.

This way, it will work for BOTH "bad" and "good" kernel.

Ofcourse it all breaks again if, in case of several
requests going from the kernel, kernel will batch them
into single larger read, so we'll lose packet boundary.
If that's the case, my "solution" does not work obviously.

Still, the userspace may adapt again, by doing a probe,
forcing the kernel to send us a request before making
the mountpoint visible in the final location, checking
the size of the data kernel sends, and setting that
pkt_len variable to this value.  Ugly, but it will let
the application to work in either case.

But again, to me the current situation with outright
BREAKING of existing userspace is much uglier since
it affects many users instead of a single userspace
component...

Thanks,

/mjt

>> Should 3.3 work?  As far as I can see, it includes the
>> same change (which has been backported!), so it may have
>> the same _issue_...

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Ian Kent]

On Mon, 2012-04-23 at 22:07 +0400, Michael Tokarev wrote:
> On 23.04.2012 20:55, Michael Tokarev wrote:
> > On 23.04.2012 20:29, Michael Tokarev wrote:
> >> On 23.04.2012 13:19, Ian Kent wrote:
> >>> On Sun, 2012-04-22 at 19:22 +0400, Michael Tokarev wrote:
> >>>> This is JFYI for now, I only diagnozed the issue but had
> >>>> no time to see what's going on.
> >>>>
> >>>> The short story is that with 3.0.24 and later kernels,
> >>>> 32bit autofs stopped working on 64bit kernels, while
> >>>> it worked before just fine.
> >>>>
> >>>> The userspace is of version 5.0.4.  Platform is x86.
> >>>>
> >>>> The sympthoms of the issue is that automount daemon,
> >>>> on every access to a (not yet mounted) autofs mount
> >>>> point, stalls forever, with the process trying to
> >>>> access it stalling forever too -- this is why the
> >>>> system didn't reboot, -- it stayed on an attempt to
> >>>> access one of automount subdirs, somewhere in the
> >>>> middle.
> >>
> >> This is what happens with 3.0.24+:
> >>
> >> [pid 15207] read(4, "\5\0\0\0\3\0\0\0\2\0\0\0\35\0\0\0@\17\246\3\0\0\0\0\350\3\0\0\350\3\0\0"..., 304) = 300
> >> [pid 15207] read(4,  <unfinished ...> -- forever.
> >>
> >> It wants to read 304 bytes, but kernel only supplies 300
> >> bytes.
> >>
> >> $ file /usr/sbin/automount
> >> /usr/sbin/automount: ELF 32-bit LSB shared object, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.18, stripped
> >> $ uname -a
> >> Linux gandalf 3.0.0-amd64 #3.0.27 SMP Tue Apr 3 16:23:45 MSK 2012 x86_64 GNU/Linux
> >>
> >> This is a32744d4abae24572eff7269bc17895c41bd0085
> >> "autofs: work around unhappy compat problem on x86-64",
> >> included in 3.0.24:
> >>
> >> +static noinline 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;
> >> +}
> >> +
> >>
> >> "End result: the "packed" size of the structure is 300 bytes: 4-byte, but
> >> not 8-byte aligned.
> >>
> >> As a result, despite all the fields being in the same place on all
> >> architectures, sizeof() will round up that size to 304 bytes on
> >> architectures that have 8-byte alignment for u64."
> >>
> >> So I'm not sure how it worked before this change (from the
> >> description of the patch it looks like it should not work).
> >> But it definitely worked for me, and this patch broke it
> >> for me.
> > 
> > It worked because the userspace apparently does the "rigtht"
> > think already compensating for the kernel bitness mismatch.
> > This is a bit funky too, but it should work:
> > 
> > static size_t get_kpkt_len(void)
> > {
> >         size_t pkt_len = sizeof(struct autofs_v5_packet);
> >         struct utsname un;
> > 
> >         uname(&un);
> > 
> >         if (pkt_len % 8) {
> >                 if (strcmp(un.machine, "alpha") == 0 ||
> >                     strcmp(un.machine, "ia64") == 0 ||
> >                     strcmp(un.machine, "x86_64") == 0 ||
> >                     strcmp(un.machine, "ppc64") == 0)
> >                         pkt_len += 4;
> > 
> >         }
> > 
> >         return pkt_len;
> > }
> > 
> > So it looks like the kernel/user interface had an error,
> > userspace adopted to the kernel by doing something, so
> > it started working.  And next kernel adopted to the un-
> > patched userspace, thus breaking patched userspace.
> > 
> > That's quite messy... :(  And I'm not sure what to do
> > about this, -- the only solution - in my opinion anyway -
> > is to revert this kernel patch and maybe switch to a
> > new protocol version.
> 
> Okay, even if messy, there are quite easy pure userspace
> solutions to all this.
> 
> But first of all, I want to question this change in the
> first place, hence CC'ing to LKML too.

Back porting the 3.3 change to current stable kernels was not a good
idea IMO and I probably should have NAKed the one that I actually saw,
which was devoid of version btw.

The change came about because there was a complaint about me not wanting
to fix this in the kernel, but wanting to continue using the user space
workaround. But it was insisted that it be fixed in the kernel, and so
it was done.

Consequently people will encounter this gotcha sooner or later and will
need to apply a patch to resolve it. The back port to stable kernels
just means it's sooner rather than later.

I have already told you that I will need to provide a (updated, since
there is already a commit in git that covers 3.3) patch for autofs. I
haven't done that yet because I've been a little ill.

A quick web search finds the post that I made to the autofs list
explaining this.
http://comments.gmane.org/gmane.linux.file-systems/61832

> 
> Kernel has been shipping with this brokeness for quite
> some time, namely, since introduction of autofs4, dated
> Mon Mar 27 01:14:55 2006 -0800 (commit 5c0a32fc2cd0).
> 
> This is 6 whole years by now.
> 
> Main userspace user of this interface adopted long ago
> too, and has been in wide use for years as well.
> 
> This kernel change, even if right in theory, actually
> breaks all existing userspace which was working fine
> for years.
> 
> It looks like this is very much against main linux
> principle of not breaking stuff even if the change is
> "obviously right".
> 
> So it pretty much looks like this very fix has to be
> reverted in mainline and in all stable kernels.  At least
> that's what I'd do if you ask me.
> 
> 
> Now, back to "pure userspace" solution.
> 
> The kernel sends data to userspace, and the unit of
> exchange is pretty small - this structure of either
> 300 or 304 bytes.  Can the read from userspace EVER
> be partial, so that a fullread() function is necessary?
> I'm not sure, but to me it looks like this fullread()
> loop isn't needed in the first place, and just single
> read() (maybe repeating it in case of EINTR) should
> do the job already.  This way, we either handle the
> data we've read, if we think the amount we read
> is sufficient, and the packet looks sane, or just
> error out.  The filename is at the end of the struct,
> so actually there's no need to send whole struct from
> kernel, it is sufficient for the kernel to write up
> to the trailing zero, and userspace _may_ be able to
> understand (and no, this is not what userspace expects
> currently, so changing kernel to do so will break things --
> I'm just saying what can be done in _userspace_).
> 
> Now, we know that this sizeof(v5_packet) is designed
> wrongly.  But it is actually trivial for the userspace
> to expect EITHER 300 bytes OR 304 bytes from the kernel --
> again, all in one go.  For it to work, extend the struct
> with a padding at the end so it will be real 304 bytes,
> and request reading of these 304 bytes, but handle the
> read of 300 bytes too as successful.
> 
> This way, it will work for BOTH "bad" and "good" kernel.
> 
> Ofcourse it all breaks again if, in case of several
> requests going from the kernel, kernel will batch them
> into single larger read, so we'll lose packet boundary.
> If that's the case, my "solution" does not work obviously.
> 
> Still, the userspace may adapt again, by doing a probe,
> forcing the kernel to send us a request before making
> the mountpoint visible in the final location, checking
> the size of the data kernel sends, and setting that
> pkt_len variable to this value.  Ugly, but it will let
> the application to work in either case.
> 
> But again, to me the current situation with outright
> BREAKING of existing userspace is much uglier since
> it affects many users instead of a single userspace
> component...
> 
> Thanks,
> 
> /mjt
> 
> >> Should 3.3 work?  As far as I can see, it includes the
> >> same change (which has been backported!), so it may have
> >> the same _issue_...
> 

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Ian Kent]

On Tue, 2012-04-24 at 09:25 +0800, Ian Kent wrote:
> On Mon, 2012-04-23 at 22:07 +0400, Michael Tokarev wrote:
> > On 23.04.2012 20:55, Michael Tokarev wrote:
> > > On 23.04.2012 20:29, Michael Tokarev wrote:
> > >> On 23.04.2012 13:19, Ian Kent wrote:
> > >>> On Sun, 2012-04-22 at 19:22 +0400, Michael Tokarev wrote:
> > >>>> This is JFYI for now, I only diagnozed the issue but had
> > >>>> no time to see what's going on.
> > >>>>
> > >>>> The short story is that with 3.0.24 and later kernels,
> > >>>> 32bit autofs stopped working on 64bit kernels, while
> > >>>> it worked before just fine.
> > >>>>
> > >>>> The userspace is of version 5.0.4.  Platform is x86.
> > >>>>
> > >>>> The sympthoms of the issue is that automount daemon,
> > >>>> on every access to a (not yet mounted) autofs mount
> > >>>> point, stalls forever, with the process trying to
> > >>>> access it stalling forever too -- this is why the
> > >>>> system didn't reboot, -- it stayed on an attempt to
> > >>>> access one of automount subdirs, somewhere in the
> > >>>> middle.
> > >>
> > >> This is what happens with 3.0.24+:
> > >>
> > >> [pid 15207] read(4, "\5\0\0\0\3\0\0\0\2\0\0\0\35\0\0\0@\17\246\3\0\0\0\0\350\3\0\0\350\3\0\0"..., 304) = 300
> > >> [pid 15207] read(4,  <unfinished ...> -- forever.
> > >>
> > >> It wants to read 304 bytes, but kernel only supplies 300
> > >> bytes.
> > >>
> > >> $ file /usr/sbin/automount
> > >> /usr/sbin/automount: ELF 32-bit LSB shared object, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.18, stripped
> > >> $ uname -a
> > >> Linux gandalf 3.0.0-amd64 #3.0.27 SMP Tue Apr 3 16:23:45 MSK 2012 x86_64 GNU/Linux
> > >>
> > >> This is a32744d4abae24572eff7269bc17895c41bd0085
> > >> "autofs: work around unhappy compat problem on x86-64",
> > >> included in 3.0.24:
> > >>
> > >> +static noinline 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;
> > >> +}
> > >> +
> > >>
> > >> "End result: the "packed" size of the structure is 300 bytes: 4-byte, but
> > >> not 8-byte aligned.
> > >>
> > >> As a result, despite all the fields being in the same place on all
> > >> architectures, sizeof() will round up that size to 304 bytes on
> > >> architectures that have 8-byte alignment for u64."
> > >>
> > >> So I'm not sure how it worked before this change (from the
> > >> description of the patch it looks like it should not work).
> > >> But it definitely worked for me, and this patch broke it
> > >> for me.
> > > 
> > > It worked because the userspace apparently does the "rigtht"
> > > think already compensating for the kernel bitness mismatch.
> > > This is a bit funky too, but it should work:
> > > 
> > > static size_t get_kpkt_len(void)
> > > {
> > >         size_t pkt_len = sizeof(struct autofs_v5_packet);
> > >         struct utsname un;
> > > 
> > >         uname(&un);
> > > 
> > >         if (pkt_len % 8) {
> > >                 if (strcmp(un.machine, "alpha") == 0 ||
> > >                     strcmp(un.machine, "ia64") == 0 ||
> > >                     strcmp(un.machine, "x86_64") == 0 ||
> > >                     strcmp(un.machine, "ppc64") == 0)
> > >                         pkt_len += 4;
> > > 
> > >         }
> > > 
> > >         return pkt_len;
> > > }
> > > 
> > > So it looks like the kernel/user interface had an error,
> > > userspace adopted to the kernel by doing something, so
> > > it started working.  And next kernel adopted to the un-
> > > patched userspace, thus breaking patched userspace.
> > > 
> > > That's quite messy... :(  And I'm not sure what to do
> > > about this, -- the only solution - in my opinion anyway -
> > > is to revert this kernel patch and maybe switch to a
> > > new protocol version.
> > 
> > Okay, even if messy, there are quite easy pure userspace
> > solutions to all this.
> > 
> > But first of all, I want to question this change in the
> > first place, hence CC'ing to LKML too.
> 
> Back porting the 3.3 change to current stable kernels was not a good
> idea IMO and I probably should have NAKed the one that I actually saw,
> which was devoid of version btw.
> 
> The change came about because there was a complaint about me not wanting
> to fix this in the kernel, but wanting to continue using the user space
> workaround. But it was insisted that it be fixed in the kernel, and so
> it was done.
> 
> Consequently people will encounter this gotcha sooner or later and will
> need to apply a patch to resolve it. The back port to stable kernels
> just means it's sooner rather than later.
> 
> I have already told you that I will need to provide a (updated, since
> there is already a commit in git that covers 3.3) patch for autofs. I
> haven't done that yet because I've been a little ill.

AFAICT this patch is what's needed.
Can you check please.

autofs-5.0.4 - allow for kernel packet size change

From: Ian Kent <raven@themaw.net>

Kernel 3.3.0 has a patch to allow for the original missdesign of
the autofs v5 kernel packet. That change is being back ported to
earlier stable kernel releases which is causing autofs to hang in
mixed 32-bit user space/64-bit kernel

The problem is that while all the structure fields are alligned
correctly structure allignment on x86-64 causes the packet size
to be 4 bytes larger than on x86. So when running an x86 binary
on an x86-64 install the packet size did not match causing user
space pipe reads to hang.
---

 daemon/automount.c |    8 ++++++++
 include/mounts.h   |   17 +++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)


diff --git a/daemon/automount.c b/daemon/automount.c
index 863aac5..92236b1 100644
--- a/daemon/automount.c
+++ b/daemon/automount.c
@@ -38,6 +38,7 @@
 #include <dirent.h>
 #include <sys/vfs.h>
 #include <sys/utsname.h>
+#include "mounts.h"
 
 #include "automount.h"
 #ifdef LIBXML2_WORKAROUND
@@ -599,6 +600,13 @@ static size_t get_kpkt_len(void)
 {
 	size_t pkt_len = sizeof(struct autofs_v5_packet);
 	struct utsname un;
+	int kern_vers;
+
+	kern_vers = linux_version_code();
+	if (kern_vers > KERNEL_VERSION(3, 2, 9) ||
+	   (kern_vers > KERNEL_VERSION(3, 0, 23) &&
+            kern_vers < KERNEL_VERSION(3, 1, 0)))
+		return pkt_len;
 
 	uname(&un);
 
diff --git a/include/mounts.h b/include/mounts.h
index 4d932ca..3947d83 100644
--- a/include/mounts.h
+++ b/include/mounts.h
@@ -16,6 +16,9 @@
 #ifndef MOUNTS_H
 #define MOUNTS_H
 
+#include <linux/version.h>
+#include <sys/utsname.h>
+
 #ifndef AUTOFS_TYPE_ANY
 #define AUTOFS_TYPE_ANY		0x0000
 #endif
@@ -72,6 +75,20 @@ struct mnt_list {
 	struct list_head ordered;
 };
 
+static inline unsigned int linux_version_code(void)
+{
+        struct utsname my_utsname;
+        unsigned int p, q, r;
+
+        if (uname(&my_utsname))
+                return 0;
+
+        p = (unsigned int)atoi(strtok(my_utsname.release, "."));
+        q = (unsigned int)atoi(strtok(NULL, "."));
+        r = (unsigned int)atoi(strtok(NULL, "."));
+        return KERNEL_VERSION(p, q, r);
+}
+
 unsigned int query_kproto_ver(void);
 unsigned int get_kver_major(void);
 unsigned int get_kver_minor(void);

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Michael Tokarev]

On 24.04.2012 07:40, Ian Kent wrote:
> On Tue, 2012-04-24 at 09:25 +0800, Ian Kent wrote:
>> On Mon, 2012-04-23 at 22:07 +0400, Michael Tokarev wrote:
[]
>>>> So it looks like the kernel/user interface had an error,

for 6 years,

>>>> userspace adopted to the kernel by doing something, so
>>>> it started working.  And next kernel adopted to the un-
>>>> patched userspace, thus breaking patched userspace.

(The commit in question is 499f286dc02cde6b668e2d757dfe100cb0c43445
from Feb 2012, released in 3.3, original bug went into kernel in
2006, as 5c0a32fc2cd0).

>>>>
>>>> That's quite messy... :(  And I'm not sure what to do
>>>> about this, -- the only solution - in my opinion anyway -
>>>> is to revert this kernel patch and maybe switch to a
>>>> new protocol version.
>>>
>>> Okay, even if messy, there are quite easy pure userspace
>>> solutions to all this.
>>>
>>> But first of all, I want to question this change in the
>>> first place, hence CC'ing to LKML too.
>>
>> Back porting the 3.3 change to current stable kernels was not a good
>> idea IMO and I probably should have NAKed the one that I actually saw,
>> which was devoid of version btw.
>>
>> The change came about because there was a complaint about me not wanting
>> to fix this in the kernel, but wanting to continue using the user space
>> workaround. But it was insisted that it be fixed in the kernel, and so
>> it was done.

Who insisted on this?

Note my question is not about stable series anymore, it is about the
original "fix" which went into 3.3 kernel, which, having in mind that
interface has been this way for 6 years and userspace adopted, should
not have been there in the first place.

>> Consequently people will encounter this gotcha sooner or later and will
>> need to apply a patch to resolve it. The back port to stable kernels
>> just means it's sooner rather than later.
>>
>> I have already told you that I will need to provide a (updated, since
>> there is already a commit in git that covers 3.3) patch for autofs. I
>> haven't done that yet because I've been a little ill.
> 
> AFAICT this patch is what's needed.
> Can you check please.
> 
> autofs-5.0.4 - allow for kernel packet size change

Please stop creating even more mess.  Just revert this patch from
kernel -- from BOTH stable and current mainline series.  Right
NOW, as an urgent fix, before 3.4 will be released (Cc'ing Linus
for this).

[]
> @@ -599,6 +600,13 @@ static size_t get_kpkt_len(void)
>  {
>  	size_t pkt_len = sizeof(struct autofs_v5_packet);
>  	struct utsname un;
> +	int kern_vers;
> +
> +	kern_vers = linux_version_code();
> +	if (kern_vers > KERNEL_VERSION(3, 2, 9) ||
> +	   (kern_vers > KERNEL_VERSION(3, 0, 23) &&
> +            kern_vers < KERNEL_VERSION(3, 1, 0)))
> +		return pkt_len;

Yeah, mess like this is absolutely disgusting... :(

[]
 +static inline unsigned int linux_version_code(void)
> +{
> +        struct utsname my_utsname;
> +        unsigned int p, q, r;
> +
> +        if (uname(&my_utsname))
> +                return 0;
> +
> +        p = (unsigned int)atoi(strtok(my_utsname.release, "."));
> +        q = (unsigned int)atoi(strtok(NULL, "."));
> +        r = (unsigned int)atoi(strtok(NULL, "."));
> +        return KERNEL_VERSION(p, q, r);

And here there's the same bug repeated again: there's no
guarantee that 3rd strtok() will return non-NULL.  But
this is irrelevant, just do not do that.  Pretty please,
it is not too late still.


Thank you!

/mjt

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Linus Torvalds]

On Mon, Apr 23, 2012 at 11:07 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> Okay, even if messy, there are quite easy pure userspace
> solutions to all this.

Yes.

> But first of all, I want to question this change in the
> first place, hence CC'ing to LKML too.

So the reason for the change was to make it much easier for people to
upgrade their 32-bit environment to a 64-bit kernel, and then you can
- maybe even just one package at a time: you may only care about a few
things - upgrade your binaries.

And the 64-bit compat layer really *should* make that "just work". A
32-bit binary really should work exactly the same on a 32-bit kernel
as it does on a 64-bit kernel. The optimal situation is that the
binary couldn't really even tell the difference, with the possible
exception of just doing an uname (and quite frankly, even *that* I
think is debatable: if you run a 32-bit binary, I seriously think we
should have just returned "i386", but people argued against that).

> Kernel has been shipping with this brokeness for quite
> some time, namely, since introduction of autofs4, dated
> Mon Mar 27 01:14:55 2006 -0800 (commit 5c0a32fc2cd0).

Absolutely. That said, we I did have several reports of it making it
possible to use a 64-bit kernel with user land from a couple of
regular distributions. So it's a real fix, and I'd like to keep it
around some way too.

> Main userspace user of this interface adopted long ago
> too, and has been in wide use for years as well.

Actually, that's just not true. The *main* users of the interface seem
to have never fixed anything. As far as I know, neither the upstream
autofs tools nor several of the big distributions ever had patches to
make 32-bit autofs work with the old broken 64-bit compat layer.

So this is a regression, but it does seem to be the case that the
workarounds for the old broken kernel behavior were fairly limited in
distribution too. Which makes me wonder if

 (a) we could make it a kernel boot command line option (which would
be better than a hardcoded compile-time config option)

 (b) which distros did the work-around for the old broken 32-bit user
space compat behavior, and how widely spread is that (which would
likely imply which way the *default* behavior should be)

But yes, we'll need to fix it somehow in the kernel, even if it might
be just a horrible hack.

Sadly, the autofs interface is *disgusting*. It just uses a pipe, so
the kernel side of autofs doesn't even *see* how big the read will be.
It just sees a random pipe, never seeing the read itself. So we can't
just say "oh, he's doing a 300-byte read, so he must be the old broken
interface". But if somebody figures out how to detect that
automatically in the kernel, that would be really good too.

Anybody?

                  Linus

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Michael Tokarev]

On 24.04.2012 19:08, Linus Torvalds wrote:
> On Mon, Apr 23, 2012 at 11:07 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
[]
>> Kernel has been shipping with this brokeness for quite
>> some time, namely, since introduction of autofs4, dated
>> Mon Mar 27 01:14:55 2006 -0800 (commit 5c0a32fc2cd0).
> 
> Absolutely. That said, we I did have several reports of it making it
> possible to use a 64-bit kernel with user land from a couple of
> regular distributions. So it's a real fix, and I'd like to keep it
> around some way too.

I run 64bit kernel and 32bit userland for several years.  Initially
we had a bunch of machines (servers) not supporting 64bits at all,
and I wanted to keep userspace across all servers the same (so I
can roll eg updates to all them at once).  Nowadays it more historic,
but still works, -- I just updated the kernel (to support more
memory etc), and a few applications which actually _use_ that memory,
the rest of the userspace is 32bits still.

The same 32/64 environment is running on my workstations too.

>> Main userspace user of this interface adopted long ago
>> too, and has been in wide use for years as well.
> 
> Actually, that's just not true. The *main* users of the interface seem
> to have never fixed anything. As far as I know, neither the upstream
> autofs tools nor several of the big distributions ever had patches to
> make 32-bit autofs work with the old broken 64-bit compat layer.

And this is actually not the case -- that's what started this thread,
when I tried to upgrade a 64bit kernel on one of our production servers
to 3.0.28, 32bit autofs, which worked just fine before, stopped working.
Hence this bugreport/discussion.

The userspace is running debian stable (squeeze).  Debian has autofs
package based on upstream 5.0.4 version.  That (upstream) version has
the "bug-compat" code in it, in daemon/automount.c:

static size_t get_kpkt_len(void)
{
        size_t pkt_len = sizeof(struct autofs_v5_packet);
        struct utsname un;

        uname(&un);

        if (pkt_len % 8) {
                if (strcmp(un.machine, "alpha") == 0 ||
                    strcmp(un.machine, "ia64") == 0 ||
                    strcmp(un.machine, "x86_64") == 0 ||
                    strcmp(un.machine, "ppc64") == 0)
                        pkt_len += 4;

        }

        return pkt_len;
}

This is exactly the workaround for this very bug in kernel.

I don't know how old this code is, but it definitely is in the
5.0.1 upstream tarball, and the file there is dated
Feb-20, 2007 - at least 3 years after the initial bug in kernel.

> So this is a regression, but it does seem to be the case that the
> workarounds for the old broken kernel behavior were fairly limited in
> distribution too. Which makes me wonder if

No, this is unfortunately not the case: the whole issue here is that
the _only_ userspace of this interface has the fix for broken kernel
for at least 5 years already, or maybe more (with kernel bug being
6 years old).

>  (a) we could make it a kernel boot command line option (which would
> be better than a hardcoded compile-time config option)
> 
>  (b) which distros did the work-around for the old broken 32-bit user
> space compat behavior, and how widely spread is that (which would
> likely imply which way the *default* behavior should be)

That'd be all distros shipping 5.0.1 or later version, which, I think,
is all current and previous distros.  5.0.1-pre1 did not have this
workaround.

And previos, v4 version, did not know this interface.

> But yes, we'll need to fix it somehow in the kernel, even if it might
> be just a horrible hack.

I'm not sure it really needs fixing, since the only user of this interface
has a workarond for this alomst since the time the bug has been introduced.

The only real solution to this - in my humble opinion anyway - is to make
the _next_ version of the interface right, but keep current one broken but
compatible.

> Sadly, the autofs interface is *disgusting*. It just uses a pipe, so
> the kernel side of autofs doesn't even *see* how big the read will be.
> It just sees a random pipe, never seeing the read itself. So we can't
> just say "oh, he's doing a 300-byte read, so he must be the old broken
> interface". But if somebody figures out how to detect that
> automatically in the kernel, that would be really good too.

No, just keep it the way it is now.  And design new iface right :)

Thanks,

/mjt

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Michael Tokarev]

On 24.04.2012 20:12, Michael Tokarev wrote:
> On 24.04.2012 19:08, Linus Torvalds wrote:

>>> Kernel has been shipping with this brokeness for quite
>>> some time, namely, since introduction of autofs4, dated
>>> Mon Mar 27 01:14:55 2006 -0800 (commit 5c0a32fc2cd0).
>>

>> Actually, that's just not true. The *main* users of the interface seem
>> to have never fixed anything. As far as I know, neither the upstream
>> autofs tools nor several of the big distributions ever had patches to
>> make 32-bit autofs work with the old broken 64-bit compat layer.
[]
> I don't know how old this code is, but it definitely is in the
> 5.0.1 upstream tarball, and the file there is dated
> Feb-20, 2007 - at least 3 years after the initial bug in kernel.

I meant to say "about a year after the initial bug in kernel".

And I think it was me who found the original issue, -- i mean,
discovered that it does not work, not found the real bug - but
I don't remember anymore.  This stuff looks very familiar for
some reason however... ;)

Thanks,

/mjt

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Linus Torvalds]

On Tue, Apr 24, 2012 at 9:12 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> The userspace is running debian stable (squeeze).  Debian has autofs
> package based on upstream 5.0.4 version.  That (upstream) version has
> the "bug-compat" code in it, in daemon/automount.c:
>
> static size_t get_kpkt_len(void)
> {
>        size_t pkt_len = sizeof(struct autofs_v5_packet);
>        struct utsname un;
>
>        uname(&un);
>
>        if (pkt_len % 8) {
>                if (strcmp(un.machine, "alpha") == 0 ||
>                    strcmp(un.machine, "ia64") == 0 ||
>                    strcmp(un.machine, "x86_64") == 0 ||
>                    strcmp(un.machine, "ppc64") == 0)
>                        pkt_len += 4;

Ugh.

So how did this break for Thomas Mayer (there was another person
involved too, I forget the details)? Is this code somehow
config-dependent so that it doesn't even get compiled for some cases?

                    Linus

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Thomas Meyer]

Am Dienstag, den 24.04.2012, 12:53 -0700 schrieb Linus Torvalds:
> On Tue, Apr 24, 2012 at 9:12 AM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >
> > The userspace is running debian stable (squeeze).  Debian has autofs
> > package based on upstream 5.0.4 version.  That (upstream) version has
> > the "bug-compat" code in it, in daemon/automount.c:
> >
> > static size_t get_kpkt_len(void)
> > {
> >        size_t pkt_len = sizeof(struct autofs_v5_packet);
> >        struct utsname un;
> >
> >        uname(&un);
> >
> >        if (pkt_len % 8) {
> >                if (strcmp(un.machine, "alpha") == 0 ||
> >                    strcmp(un.machine, "ia64") == 0 ||
> >                    strcmp(un.machine, "x86_64") == 0 ||
> >                    strcmp(un.machine, "ppc64") == 0)
> >                        pkt_len += 4;
> 
> Ugh.
> 
> So how did this break for Thomas Mayer (there was another person
> involved too, I forget the details)? Is this code somehow
> config-dependent so that it doesn't even get compiled for some cases?

It broke the autofs feature of systemd (32bit) on a 64bit kernel, which
resulted in an endless wait in the boot process. I don't use the autofs
tools. I guess the fix in the autofs package to compensate the kernel
bug, which has now been fixed, breaks the autofs package.
The fix in the kernel makes systemd work correctly.

thomas

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Linus Torvalds]

On Tue, Apr 24, 2012 at 1:42 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
>
> It broke the autofs feature of systemd (32bit) on a 64bit kernel, which
> resulted in an endless wait in the boot process. I don't use the autofs
> tools. I guess the fix in the autofs package to compensate the kernel
> bug, which has now been fixed, breaks the autofs package.
> The fix in the kernel makes systemd work correctly.

Oh damn, I'd forgotten the details. Ok.

That does mean that we probably just have to do a kernel command line
option for this. And then the autofs4 code that currently does

    sbi->compat_daemon = is_compat_task();

needs to change that to take the command line option into account too.

We could make it a mount option, but since the whole point is that we
want to be compatible with existing binaries that don't *use* special
mount options, I suspect the only reasonable point is at the kernel
command line.

That is, unless somebody can figure out a way to auto-detect how big
the user space read is. The problem with that really is that the
autofs read() system call isn't done directly to some "real autofs"
file descriptor, autofs literally is using the standard pipe code.

So the code in fs/pipe.c does actually see the size of the read. But
the code in fs/autofs4/ does not. And we can't change the interface,
because the whole pipe is opened in user space iirc.

Ugly.

Anybody willing to write the patch?

                 Linus

Damn.

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Michael Tokarev]

On 25.04.2012 01:01, Linus Torvalds wrote:
> On Tue, Apr 24, 2012 at 1:42 PM, Thomas Meyer <thomas@m3y3r.de> wrote:
>>
>> It broke the autofs feature of systemd (32bit) on a 64bit kernel, which
>> resulted in an endless wait in the boot process. I don't use the autofs
>> tools. I guess the fix in the autofs package to compensate the kernel
>> bug, which has now been fixed, breaks the autofs package.
>> The fix in the kernel makes systemd work correctly.
> 
> Oh damn, I'd forgotten the details. Ok.
> 
> That does mean that we probably just have to do a kernel command line
> option for this. And then the autofs4 code that currently does
> 
>     sbi->compat_daemon = is_compat_task();
> 
> needs to change that to take the command line option into account too.

How about something like

     sbi->compat_daemon = is_compat_task() && strcmp(task->comm, "automount") != 0

instead?  (This task->comm is a pseudocode, just to show the idea, I don't
know where this info can be found in kernel)

Or maybe similar workaround (as in automount) can be placed into systemd
too.  I'd really prefer this one -- historically things has been that way
for quite some time (at least 5 years).

Or yet another solution: let the kernel to compensate, but add some code
in the _negotiation_ phase, when user<=>kernel space initializes things
and exchanges their capabilities - IF there is one.

> We could make it a mount option, but since the whole point is that we
> want to be compatible with existing binaries that don't *use* special
> mount options, I suspect the only reasonable point is at the kernel
> command line.

Well, the last my suggestion -- it will have to use some special option
or a flag or whatever.  But let just make it a part of the interface,
like we just introduced an autofs v6, so existing and currently in WIDE
use (I mean automount from autofs5 which broke) will be fine, and all
NEW binaries will work too.

Kernel command line is - in my opinion - too much for this very issue.
The same way a user can just replace his automount with automount64
(for example) - if something has to be done by the user anyway.

But a mount option which will be specified by all new binaries as a
part of standard interface should work - provided this is the
"negotiation" phase.

This way, we're breaking systemd _again_, but only till the next
version.  The difference between systemd and automount here is that
the latter is much older and is more likely to be used in a mixed
environment and has more "chances" to be seen in the future this
way too, when someone tries to run some old distribution code in
a chroot or a container.  Systemd here is much less used in mixed
environment now and is less widely used in general, so this way
we break less - IMHO anyway.

And/or systemd can adopt in the similar way as automount did --
like, treat this bug as an interface "feature", provide a "large
enough" buffer and just expect a read of up to the trailing zero
in filename as okay too -- this way it will work regardless of
anything else (this is what I proposed initially).

> That is, unless somebody can figure out a way to auto-detect how big
> the user space read is. The problem with that really is that the
> autofs read() system call isn't done directly to some "real autofs"
> file descriptor, autofs literally is using the standard pipe code.
> 
> So the code in fs/pipe.c does actually see the size of the read. But
> the code in fs/autofs4/ does not. And we can't change the interface,
> because the whole pipe is opened in user space iirc.

Yes, that'd be more sane solution - an ability to detect read size.

So, to sum it all up, we've the following options:

1) somehow detect the read size and do a magic in kernel so everything
   Just Work.

2) sbi->compat_daemon = is_compat_task() && strcmp(task->comm, "automount") != 0
   Everything _current_ works.

3) mount option or other change of the interface, reverting the
   patch.  Old automount will continue to work as it did last 5+
   years, systemd will break again.  Can be combined with 2), ie,
   do 2) as an "urgent fix" and implement 3) later after some
   thinking.

Thanks,

/mjt

Re: Breaking userspace? Re: 3.0.24 broke aufofs on mixed 32/64bit environment

[Linus Torvalds]

On Tue, Apr 24, 2012 at 9:05 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> How about something like
>
>     sbi->compat_daemon = is_compat_task() && strcmp(task->comm, "automount") != 0
>
> instead?  (This task->comm is a pseudocode, just to show the idea, I don't
> know where this info can be found in kernel)

Thomas, does that work for you?

                Linus