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@xxxxxxxxxx> > > Cc: Ian Kent <raven@xxxxxxxxxx> > > Cc: Thomas Meyer <thomas@xxxxxxxx> > > Cc: stable@xxxxxxxxxx > > Cc: autofs@xxxxxxxxxxxxxxx > > --- > > 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); > -- To unsubscribe from this list: send the line "unsubscribe autofs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
