[systemd-devel] [PATCH] Circumvent autofs_v5_packet_union size bug

[Ian Kent]

On Mon, 2011-11-14 at 03:08 +0100, Kay Sievers wrote:
> On Mon, Nov 14, 2011 at 02:38, Ian Kent <raven at themaw.net> wrote:
> > On Fri, 2011-11-11 at 14:33 +0100, Kay Sievers wrote:
> >> On Tue, Sep 20, 2011 at 14:26, Kay Sievers <kay.sievers at vrfy.org> wrote:
> >> > On Tue, Sep 20, 2011 at 12:58, Lennart Poettering
> >> > <lennart at poettering.net> wrote:
> >> >> On Tue, 20.09.11 11:36, Ian Kent (raven at themaw.net) wrote:
> >> >>
> >> >>> > This is a bug in the kernel, and it should be fixed in the kernel (which
> >> >>> > would mean the kernel folks have to define a new version of this
> >> >>> > struct/ioctl which fixes this). If that's defined we can then add
> >> >>> > support for this into systemd. Could you file a bug about this please on
> >> >>> > the kernel bugzilla? (please cc me, or post the url here!)
> >> >>>
> >> >>> Yes, it is a mistake that I made but it is a bad idea to change
> >> >>> something that will cause widespread failure of existing binaries and
> >> >>> that's why the discrepancy persists and will continue to do so.
> >> >>
> >> >> I am not expecting this to be changed in the kernel. Instead I expect
> >> >> that the kernel API is extended with a correct version of the same data
> >> >> structure. If the kernel broke it the kernel should fix it.
> >> >>
> >> >>> I chose to compensate for it in user space and to advise anyone that
> >> >>> encountered the problem on how to handle it and I have had no reports of
> >> >>> problems in autofs since I added the size calculation (which was a long
> >> >>> time ago now).
> >> >>
> >> >> Well, it's a workaround in userspace. Fix the kernel. Add a second union
> >> >> or something which can be used by newer clients.
> >> >>
> >> >>> > I am sorry but I am pretty sure I want to keep the compat kludges for
> >> >>> > broken things in systemd at a minimum, adding such a work around looks
> >> >>> > like the wrong way to me. We generally try to fix problems where they
> >> >>> > are these days, not where we notice them.
> >> >>>
> >> >>> Sure, it is unfortunate but, as far as the autofs kernel module is
> >> >>> concerned the decision about this was made a long time ago and, yes it
> >> >>> isn't what we want but the breakage it would have caused then and the
> >> >>> breakage it would cause now is too great.
> >> >>
> >> >> Adding a corrected version will not break anything. This is not about
> >> >> replacing the current borked interface, but simply by adding a fixed
> >> >> version that we can use from userspace without ugly compat glue.
> >> >>
> >> >> Fix the problems where they are, don't work around them where they aren't.
> >> >
> >> > I can only second that.
> >> >
> >> > Please add a new definition, leave the old around, and do not require
> >> > userspace to do wild things, like compare static lists of
> >> > architectures to work around things that can be expected to just work.
> >>
> >> Ian, is this fixed in the meantime? If not, mind fixing the kernel bug
> >> and introduce an non-broken version of the kernel interface.
> >
> > I spent some time having a look at how I might do it but I didn't end up
> > with anything suitable.
> >
> > How exactly did you think I would be able to do it without introducing
> > breakage?
> 
> We thought by introducing a non-broken version of the ioctl. Like
> Thomas Meyer posted earlier in this thread. I did not look at the
> patch details, but what's the problem with that approach?

I saw that and have now commented on it.

We can do that but there are a couple of changes I'd like. I need to do
some testing and I also need to see if I can work out what I thought the
problem was with adding a packed struct to the union, but I may be
thinking about something different that I tried.

> 
> >> We like to have this fixed in systemd, but are still reluctant to
> >> compile lists of static 32 vs. 64 architecture bit matches in init.
> >
> > Why would you need to do that, it's easy to work out the expected packet
> > size at program start with the function I posted.
> 
> This is about 'init', which is far more exposed than an autofs daemon.
> We really don't want to compile static lists of architectures into
> 'init', to work around such kernel bugs. Linux has a new architecture
> every month, and some of them switch from 32 to 64 bit. That fix
> belongs into the kernel, not in paper-over fixes in all userspace
> tools which use autofs.
> 
> > Even if I had a way to
> > do it you would still need to use the function since you won't know what
> > kernel version your client users will be using.
> 
> We have no problem to require a recent kernel and a minimal protocol
> version of autofs to fix these issues. Is that what you mean?

Yes, that's all I meant.
That will be needed if you don't need to provide any backward
compatibility to earlier kernel versions.

Ian