[PATCH] unbreak automount daemon after fixing autofs5 ABI

[PATCH] unbreak automount daemon after fixing autofs5 ABI

[Michael Tokarev]

Patch 499f286dc02cde6b668e2d757dfe100cb0c43445, comitted
in Feb-2012, fixed a kernel<=>user interface which was
broken for mixed 32/64 user/kernel environment since its
introduction in 2006 by commit 5c0a32fc2cd0.  The problem
with this fix is that it broke userspace which were adopted
to the initial bug a year after its introduction, in
autofs-5.0.1.  So patch 499f286dc02cde6b broke a working
userspace.  This has been done because another user of
this interface emerged, systemd, which had the same prob
as old automount (<5.0.1) had.  So the end result was a
breakage of an old, widely used -- by all current and
previous distribution -- software (autofs5) in order to
unbreak a new not yet widely adopted software (systemd).

The real fix for this issue, in my opinion, is to adopt
the same fix in systemd as has been done in autofs5, and/or
to have a new interface without a bug wich both userspace
implementations will use.

Alternative is to find a way on the kernel side to see
which variant of userspace we're dealing with, by seeing
how many bytes for the structure in question the userspace
requests in read operation.  But since autofs uses regular
pipe code to read the data from kernel, and the said pipe
is opened in userspace before connecting it with the autofs
module, reads are not seen by autofs module directly, they're
seen by the pipe code.

So for now, below is a minimal quick fix which should sort
current mess.

Change 499f286dc02cde6b tries to fix the original interface
bug for 32bit userspace running on 64bit kernel.  But the
issue is that autofs5 already has a workaround for it, so
the end result is that the two (kernel & user) disagree
again.  "Fix" this by applying the in-kernel fix only if
the process is not named "automount" -- which is how autofs5
daemon is named.

This change should be applied to all 3.x stable series too,
since it breaks existing userspace for these kernels.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Cc: Ian Kent <raven@themaw.net>
Cc: Thomas Meyer <thomas@m3y3r.de>
Cc: stable@kernel.org
Cc: autofs@vger.kernel.org
---
 fs/autofs4/dev-ioctl.c |    3 ++-
 fs/autofs4/inode.c     |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 9dacb85..d5a4cb6 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -385,7 +385,8 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
-		sbi->compat_daemon = is_compat_task();
+		sbi->compat_daemon =
+			is_compat_task() && strcmp(current->comm, "automount");
 	}
 out:
 	mutex_unlock(&sbi->wq_mutex);
diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
index d8dc002..b59f6a0 100644
--- a/fs/autofs4/inode.c
+++ b/fs/autofs4/inode.c
@@ -225,7 +225,8 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	set_autofs_type_indirect(&sbi->type);
 	sbi->min_proto = 0;
 	sbi->max_proto = 0;
-	sbi->compat_daemon = is_compat_task();
+	sbi->compat_daemon =
+		is_compat_task() && strcmp(current->comm, "automount");
 	mutex_init(&sbi->wq_mutex);
 	mutex_init(&sbi->pipe_mutex);
 	spin_lock_init(&sbi->fs_lock);
-- 
1.7.10

Re: [PATCH] unbreak automount daemon after fixing autofs5 ABI

[Ian Kent]

On Thu, 2012-04-26 at 10:26 +0400, Michael Tokarev wrote:
> Patch 499f286dc02cde6b668e2d757dfe100cb0c43445, comitted
> in Feb-2012, fixed a kernel<=>user interface which was
> broken for mixed 32/64 user/kernel environment since its
> introduction in 2006 by commit 5c0a32fc2cd0.  The problem
> with this fix is that it broke userspace which were adopted
> to the initial bug a year after its introduction, in
> autofs-5.0.1.  So patch 499f286dc02cde6b broke a working
> userspace.  This has been done because another user of
> this interface emerged, systemd, which had the same prob
> as old automount (<5.0.1) had.  So the end result was a
> breakage of an old, widely used -- by all current and
> previous distribution -- software (autofs5) in order to
> unbreak a new not yet widely adopted software (systemd).
> 
> The real fix for this issue, in my opinion, is to adopt
> the same fix in systemd as has been done in autofs5, and/or
> to have a new interface without a bug wich both userspace
> implementations will use.
> 
> Alternative is to find a way on the kernel side to see
> which variant of userspace we're dealing with, by seeing
> how many bytes for the structure in question the userspace
> requests in read operation.  But since autofs uses regular
> pipe code to read the data from kernel, and the said pipe
> is opened in userspace before connecting it with the autofs
> module, reads are not seen by autofs module directly, they're
> seen by the pipe code.
> 
> So for now, below is a minimal quick fix which should sort
> current mess.
> 
> Change 499f286dc02cde6b tries to fix the original interface
> bug for 32bit userspace running on 64bit kernel.  But the
> issue is that autofs5 already has a workaround for it, so
> the end result is that the two (kernel & user) disagree
> again.  "Fix" this by applying the in-kernel fix only if
> the process is not named "automount" -- which is how autofs5
> daemon is named.

No, you cannot be sure the autofs binary is named automount.
Not only that, this approach makes an unpleasant change even more
unpleasant.

I still think that my original proposed patches were the better approach
to handling this problem.

My recommendation was that the major autofs kernel protocol version be
bumped and a packed structure used from that version on. That allows
user space to request that protocol version for communication at mount
time. If systemd doesn't want to support earlier protocol versions then
it can complain, instructing the user to update to a later kernel.

I understand why that proposal was not well accepted but the fact
remains, I did make a mistake and I did chose to work around it in user
space rather than cope with the pain at the time. In hind site that may
have been the wrong thing to do but that's history. So I believe the
most sensible thing to do now is to take the approach that minimizes
breakage.

> 
> This change should be applied to all 3.x stable series too,
> since it breaks existing userspace for these kernels.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> Cc: Ian Kent <raven@themaw.net>
> Cc: Thomas Meyer <thomas@m3y3r.de>
> Cc: stable@kernel.org
> Cc: autofs@vger.kernel.org
> ---
>  fs/autofs4/dev-ioctl.c |    3 ++-
>  fs/autofs4/inode.c     |    3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> index 9dacb85..d5a4cb6 100644
> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -385,7 +385,8 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
>  		sbi->pipefd = pipefd;
>  		sbi->pipe = pipe;
>  		sbi->catatonic = 0;
> -		sbi->compat_daemon = is_compat_task();
> +		sbi->compat_daemon =
> +			is_compat_task() && strcmp(current->comm, "automount");

If this approach is used it might be better to use a length limited
comparison so that if a distribution needs to have multiple packages
side by side, perhaps v4 or 32 and 64 bit, they would most likely append
something on the end of the binary name.
 
>  	}
>  out:
>  	mutex_unlock(&sbi->wq_mutex);
> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> index d8dc002..b59f6a0 100644
> --- a/fs/autofs4/inode.c
> +++ b/fs/autofs4/inode.c
> @@ -225,7 +225,8 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
>  	set_autofs_type_indirect(&sbi->type);
>  	sbi->min_proto = 0;
>  	sbi->max_proto = 0;
> -	sbi->compat_daemon = is_compat_task();
> +	sbi->compat_daemon =
> +		is_compat_task() && strcmp(current->comm, "automount");
>  	mutex_init(&sbi->wq_mutex);
>  	mutex_init(&sbi->pipe_mutex);
>  	spin_lock_init(&sbi->fs_lock);

Re: [PATCH] unbreak automount daemon after fixing autofs5 ABI

[Michael Tokarev]

On 26.04.2012 12:21, Ian Kent wrote:
> On Thu, 2012-04-26 at 10:26 +0400, Michael Tokarev wrote:
[]
>> Change 499f286dc02cde6b tries to fix the original interface
>> bug for 32bit userspace running on 64bit kernel.  But the
>> issue is that autofs5 already has a workaround for it, so
>> the end result is that the two (kernel & user) disagree
>> again.  "Fix" this by applying the in-kernel fix only if
>> the process is not named "automount" -- which is how autofs5
>> daemon is named.
> 
> No, you cannot be sure the autofs binary is named automount.

Well, I checked several distributions, all have the binary named
this way.  Yes a user can rename it and it will break again, but
it is at least better than now when it does not work at all.  And
yes it is messy, just like current situation already is.

> Not only that, this approach makes an unpleasant change even more
> unpleasant.
> 
> I still think that my original proposed patches were the better approach
> to handling this problem.
> 
> My recommendation was that the major autofs kernel protocol version be
> bumped and a packed structure used from that version on. That allows
> user space to request that protocol version for communication at mount
> time. If systemd doesn't want to support earlier protocol versions then
> it can complain, instructing the user to update to a later kernel.

This is exactly what I was proposing when I started my thread about
3.0.24 autofs breakage -- to _revert_ the in-kernel fix and introduce
a new interface (or variation thereof, in means of a new mount option
or something).  This way, breakage and mess are both minimal.

Note that a major protocol changes aren't really needed, just a single
mount option, which uses a right size of the structure.  Unless at the
same time you want to change something else.

> I understand why that proposal was not well accepted but the fact

But I don't understand this ;)

> remains, I did make a mistake and I did chose to work around it in user
> space rather than cope with the pain at the time. In hind site that may
> have been the wrong thing to do but that's history. So I believe the
> most sensible thing to do now is to take the approach that minimizes
> breakage.

No one complains to you now about the history, and it is not relevant
now already, as we do have what we have.

And yes I 100% agree that reverting that "fix" change in kernel and
introducing a more sane "slightly new" interface is the way to go.

Thank you!

/mjt

Re: [PATCH] unbreak automount daemon after fixing autofs5 ABI

[Ian Kent]

On Thu, 2012-04-26 at 12:32 +0400, Michael Tokarev wrote:
> On 26.04.2012 12:21, Ian Kent wrote:
> > On Thu, 2012-04-26 at 10:26 +0400, Michael Tokarev wrote:
> []
> >> Change 499f286dc02cde6b tries to fix the original interface
> >> bug for 32bit userspace running on 64bit kernel.  But the
> >> issue is that autofs5 already has a workaround for it, so
> >> the end result is that the two (kernel & user) disagree
> >> again.  "Fix" this by applying the in-kernel fix only if
> >> the process is not named "automount" -- which is how autofs5
> >> daemon is named.
> > 
> > No, you cannot be sure the autofs binary is named automount.
> 
> Well, I checked several distributions, all have the binary named
> this way.  Yes a user can rename it and it will break again, but
> it is at least better than now when it does not work at all.  And
> yes it is messy, just like current situation already is.
> 
> > Not only that, this approach makes an unpleasant change even more
> > unpleasant.
> > 
> > I still think that my original proposed patches were the better approach
> > to handling this problem.
> > 
> > My recommendation was that the major autofs kernel protocol version be
> > bumped and a packed structure used from that version on. That allows
> > user space to request that protocol version for communication at mount
> > time. If systemd doesn't want to support earlier protocol versions then
> > it can complain, instructing the user to update to a later kernel.
> 
> This is exactly what I was proposing when I started my thread about
> 3.0.24 autofs breakage -- to _revert_ the in-kernel fix and introduce
> a new interface (or variation thereof, in means of a new mount option
> or something).  This way, breakage and mess are both minimal.
> 
> Note that a major protocol changes aren't really needed, just a single
> mount option, which uses a right size of the structure.  Unless at the
> same time you want to change something else.

There's no new interface needed, as such. Basically the structure used
for communication just needs to be declared packed and used instead of
the current one. Since it's not a backward compatible change the major
version needs to be increased to indicate that.

The autofs kernel protocol version (major version number) is specified
when mounting autofs mounts. The minor version is meant to be used to
identify the bug fix level. So increasing the major version number is
the normal way that change may be introduced without breaking existing
applications or requiring any changes to them. New applications simply
set the major version at mount time and can decide if they want to use a
lower version if the mount fails, assuming the lower version has the
functionality that's needed. And, yes, it isn't quite that simple at run
time but that is the way it's meant to work.

So using the word "major" probably gives the wrong impression even
though the term is correct usage.

Ian

Re: [PATCH] unbreak automount daemon after fixing autofs5 ABI

[Ian Kent]

On Thu, 2012-04-26 at 16:21 +0800, Ian Kent wrote:
> On Thu, 2012-04-26 at 10:26 +0400, Michael Tokarev wrote:
> > Patch 499f286dc02cde6b668e2d757dfe100cb0c43445, comitted
> > in Feb-2012, fixed a kernel<=>user interface which was
> > broken for mixed 32/64 user/kernel environment since its
> > introduction in 2006 by commit 5c0a32fc2cd0.  The problem
> > with this fix is that it broke userspace which were adopted
> > to the initial bug a year after its introduction, in
> > autofs-5.0.1.  So patch 499f286dc02cde6b broke a working
> > userspace.  This has been done because another user of
> > this interface emerged, systemd, which had the same prob
> > as old automount (<5.0.1) had.  So the end result was a
> > breakage of an old, widely used -- by all current and
> > previous distribution -- software (autofs5) in order to
> > unbreak a new not yet widely adopted software (systemd).
> > 
> > The real fix for this issue, in my opinion, is to adopt
> > the same fix in systemd as has been done in autofs5, and/or
> > to have a new interface without a bug wich both userspace
> > implementations will use.
> > 
> > Alternative is to find a way on the kernel side to see
> > which variant of userspace we're dealing with, by seeing
> > how many bytes for the structure in question the userspace
> > requests in read operation.  But since autofs uses regular
> > pipe code to read the data from kernel, and the said pipe
> > is opened in userspace before connecting it with the autofs
> > module, reads are not seen by autofs module directly, they're
> > seen by the pipe code.
> > 
> > So for now, below is a minimal quick fix which should sort
> > current mess.
> > 
> > Change 499f286dc02cde6b tries to fix the original interface
> > bug for 32bit userspace running on 64bit kernel.  But the
> > issue is that autofs5 already has a workaround for it, so
> > the end result is that the two (kernel & user) disagree
> > again.  "Fix" this by applying the in-kernel fix only if
> > the process is not named "automount" -- which is how autofs5
> > daemon is named.
> 
> No, you cannot be sure the autofs binary is named automount.
> Not only that, this approach makes an unpleasant change even more
> unpleasant.
> 
> I still think that my original proposed patches were the better approach
> to handling this problem.

Actually, just to give credit (or blame) where it is due, while this was
my preferred way to handle this, Thomas wrote the original patch which I
changed a little before proposing.

> 
> My recommendation was that the major autofs kernel protocol version be
> bumped and a packed structure used from that version on. That allows
> user space to request that protocol version for communication at mount
> time. If systemd doesn't want to support earlier protocol versions then
> it can complain, instructing the user to update to a later kernel.
> 
> I understand why that proposal was not well accepted but the fact
> remains, I did make a mistake and I did chose to work around it in user
> space rather than cope with the pain at the time. In hind site that may
> have been the wrong thing to do but that's history. So I believe the
> most sensible thing to do now is to take the approach that minimizes
> breakage.
> 
> > 
> > This change should be applied to all 3.x stable series too,
> > since it breaks existing userspace for these kernels.
> > 
> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > Cc: Ian Kent <raven@themaw.net>
> > Cc: Thomas Meyer <thomas@m3y3r.de>
> > Cc: stable@kernel.org
> > Cc: autofs@vger.kernel.org
> > ---
> >  fs/autofs4/dev-ioctl.c |    3 ++-
> >  fs/autofs4/inode.c     |    3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
> > index 9dacb85..d5a4cb6 100644
> > --- a/fs/autofs4/dev-ioctl.c
> > +++ b/fs/autofs4/dev-ioctl.c
> > @@ -385,7 +385,8 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
> >  		sbi->pipefd = pipefd;
> >  		sbi->pipe = pipe;
> >  		sbi->catatonic = 0;
> > -		sbi->compat_daemon = is_compat_task();
> > +		sbi->compat_daemon =
> > +			is_compat_task() && strcmp(current->comm, "automount");
> 
> If this approach is used it might be better to use a length limited
> comparison so that if a distribution needs to have multiple packages
> side by side, perhaps v4 or 32 and 64 bit, they would most likely append
> something on the end of the binary name.
>  
> >  	}
> >  out:
> >  	mutex_unlock(&sbi->wq_mutex);
> > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> > index d8dc002..b59f6a0 100644
> > --- a/fs/autofs4/inode.c
> > +++ b/fs/autofs4/inode.c
> > @@ -225,7 +225,8 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
> >  	set_autofs_type_indirect(&sbi->type);
> >  	sbi->min_proto = 0;
> >  	sbi->max_proto = 0;
> > -	sbi->compat_daemon = is_compat_task();
> > +	sbi->compat_daemon =
> > +		is_compat_task() && strcmp(current->comm, "automount");
> >  	mutex_init(&sbi->wq_mutex);
> >  	mutex_init(&sbi->pipe_mutex);
> >  	spin_lock_init(&sbi->fs_lock);
>