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

[Thomas Meyer]

Am 14.11.2011 um 03:35 schrieb Ian Kent <raven at themaw.net>:

> 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.
> 

Another solution would be to explicitly add 4 filler bytes to the structure to have an 8 byte alignment, like the compiler does it now implicitly on 64 bit.

Another question: is there an ioctl available that returns the min and max supported protocol version of the driver? I think user space currently needs to mount with min/maxproto=6 and see it fail, then try to mount again with version=5.

>> 
>>>> 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
> 
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel