[systemd-devel] [PATCH] Circumvent autofs_v5_packet_union size bug
Ian Kent
raven at themaw.net
Sun Nov 13 18:26:57 PST 2011
On Sun, 2011-11-13 at 13:23 +0100, Thomas Meyer wrote:
> Am Freitag, den 11.11.2011, 14:33 +0100 schrieb Kay Sievers:
> > 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.
> >
> > 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.
>
> Hi,
>
> just did this quick hack. It compiles, but I didn't test it:
>
> From 3e71ead349d179302452798fd3bc9305a5477d22 Mon Sep 17 00:00:00 2001
> From: Thomas Meyer <thomas at m3y3r.de>
> Date: Sun, 13 Nov 2011 13:13:57 +0100
> Subject: [PATCH] Add autofs_v6_packet with same packet size on x86 and
> x86_64.
>
> autofs_v5_packet is 300 bytes on x86 and 304 bytes on x86_64.
> This leads to an error in at least systemd when running a x86_64 kernel
> on an x86 userspace. Fix this by adding a new protocol version 6 packet
> that has the same size on x86 and x86_64.
I don't think that's quite right.
It's not only intel arch that we see the problem.
It is only evident when user space is not the native arch, ie. x86 user
space compatibility packages on x86_64 system. IIRC when the whole
system in x86 on x86_64 hardware the problem doesn't show up.
I wasn't keen on bumping the protocol version but it is a clean way of
doing it. The user space application will need to check the version it
gets, of course, and use the work around if it doesn't get the version
it wants.
The thing I don't much like about bumping the version is that, so far
the protocol version has matched the autofs daemon version, ie. version
5, so it's potentially further even more confusing given that the module
name is still autofs4.
But, of course, the version doesn't have to match.
>
> Signed-off-by: Thomas Meyer <thomas at m3y3r.de>
> ---
> fs/autofs4/inode.c | 30 ++++++++++++++++++++++------
> fs/autofs4/waitq.c | 47 +++++++++++++++++++++++++++++++--------------
> include/linux/auto_fs4.h | 19 ++++++++++++++++-
> 3 files changed, 72 insertions(+), 24 deletions(-)
>
> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c
> index 8179f1a..10a57fc 100644
> --- a/fs/autofs4/inode.c
> +++ b/fs/autofs4/inode.c
> @@ -197,6 +197,28 @@ static int parse_options(char *options, int *pipefd, uid_t *uid, gid_t *gid,
> return (*pipefd < 0);
> }
>
> +static bool check_protocol_version(int minproto, int maxproto)
> +{
> + bool rc = true;
> +
> + if (minproto > maxproto || maxproto < minproto) {
> + printk("autofs: protocol min(%d)/max(%d) version error!",
> + minproto, maxproto);
> + rc = false;
> + }
> +
> + if (maxproto < AUTOFS_MIN_PROTO_VERSION ||
> + minproto > AUTOFS_MAX_PROTO_VERSION) {
> + printk("autofs: kernel does not match daemon version "
> + "daemon (%d, %d) kernel (%d, %d)\n",
> + minproto, maxproto,
> + AUTOFS_MIN_PROTO_VERSION, AUTOFS_MAX_PROTO_VERSION);
> + rc = false;
> + }
> +
> + return rc;
> +}
> +
> int autofs4_fill_super(struct super_block *s, void *data, int silent)
> {
> struct inode * root_inode;
> @@ -269,14 +291,8 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
> root_inode->i_op = &autofs4_dir_inode_operations;
>
> /* Couldn't this be tested earlier? */
> - if (sbi->max_proto < AUTOFS_MIN_PROTO_VERSION ||
> - sbi->min_proto > AUTOFS_MAX_PROTO_VERSION) {
> - printk("autofs: kernel does not match daemon version "
> - "daemon (%d, %d) kernel (%d, %d)\n",
> - sbi->min_proto, sbi->max_proto,
> - AUTOFS_MIN_PROTO_VERSION, AUTOFS_MAX_PROTO_VERSION);
> + if(check_protocol_version(sbi->min_proto, sbi->max_proto) == false)
> goto fail_dput;
> - }
>
> /* Establish highest kernel protocol version */
> if (sbi->max_proto > AUTOFS_MAX_PROTO_VERSION)
> diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
> index e1fbdee..2e455be 100644
> --- a/fs/autofs4/waitq.c
> +++ b/fs/autofs4/waitq.c
> @@ -99,6 +99,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
> struct autofs_packet_hdr hdr;
> union autofs_packet_union v4_pkt;
> union autofs_v5_packet_union v5_pkt;
> + struct autofs_v6_packet v6_pkt;
I'm not sure that adding a packed struct to the union will actually work
without causing other problems. I can't remember now what I thought the
problem was so I'll need to check.
> } pkt;
> struct file *pipe = NULL;
> size_t pktsz;
> @@ -137,7 +138,7 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
> break;
> }
> /*
> - * Kernel protocol v5 packet for handling indirect and direct
> + * Kernel protocol v5/v6 packet for handling indirect and direct
> * mount missing and expire requests
> */
> case autofs_ptype_missing_indirect:
> @@ -145,20 +146,36 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
> case autofs_ptype_missing_direct:
> case autofs_ptype_expire_direct:
> {
> - struct autofs_v5_packet *packet = &pkt.v5_pkt.v5_packet;
> -
> - pktsz = sizeof(*packet);
> -
> - packet->wait_queue_token = wq->wait_queue_token;
> - packet->len = wq->name.len;
> - memcpy(packet->name, wq->name.name, wq->name.len);
> - packet->name[wq->name.len] = '\0';
> - packet->dev = wq->dev;
> - packet->ino = wq->ino;
> - packet->uid = wq->uid;
> - packet->gid = wq->gid;
> - packet->pid = wq->pid;
> - packet->tgid = wq->tgid;
> + if(sbi->version == 5) {
> + struct autofs_v5_packet *packet = &pkt.v5_pkt.v5_packet;
> +
> + packet->wait_queue_token = wq->wait_queue_token;
> + packet->len = wq->name.len;
> + memcpy(packet->name, wq->name.name, wq->name.len);
> + packet->name[wq->name.len] = '\0';
> + packet->dev = wq->dev;
> + packet->ino = wq->ino;
> + packet->uid = wq->uid;
> + packet->gid = wq->gid;
> + packet->pid = wq->pid;
> + packet->tgid = wq->tgid;
> + pktsz = sizeof(packet);
> + }
I don't usually like "else if" but that might have been better here, or
maybe a switch to allow for higher versions later. Not sure the best
thing here.
> + if(sbi->version == 6) {
> + struct autofs_v6_packet *packet = &pkt.v6_pkt;
> +
> + packet->wait_queue_token = wq->wait_queue_token;
> + packet->len = wq->name.len;
> + memcpy(packet->name, wq->name.name, wq->name.len);
> + packet->name[wq->name.len] = '\0';
> + packet->dev = wq->dev;
> + packet->ino = wq->ino;
> + packet->uid = wq->uid;
> + packet->gid = wq->gid;
> + packet->pid = wq->pid;
> + packet->tgid = wq->tgid;
> + pktsz = sizeof(packet);
> + }
> break;
> }
> default:
> diff --git a/include/linux/auto_fs4.h b/include/linux/auto_fs4.h
> index e02982f..e16b105 100644
> --- a/include/linux/auto_fs4.h
> +++ b/include/linux/auto_fs4.h
> @@ -20,9 +20,9 @@
> #undef AUTOFS_MIN_PROTO_VERSION
> #undef AUTOFS_MAX_PROTO_VERSION
>
> -#define AUTOFS_PROTO_VERSION 5
> +#define AUTOFS_PROTO_VERSION 6
> #define AUTOFS_MIN_PROTO_VERSION 3
> -#define AUTOFS_MAX_PROTO_VERSION 5
> +#define AUTOFS_MAX_PROTO_VERSION 6
>
> #define AUTOFS_PROTO_SUBVERSION 2
This is going to get a bit difficult.
The subversion normally relates to major version but now we need to
remember the version 5 subversion and add version 6 and its subversion.
Again, not entirely sure the best way to go here either, but breaking up
the versions is one way I guess.
>
> @@ -154,6 +154,21 @@ union autofs_v5_packet_union {
> autofs_packet_expire_direct_t expire_direct;
> };
>
> +/* autofs v6 common packet struct */
> +/* packed structure for same packet size on x86 and x86_64 */
> +struct autofs_v6_packet {
> + struct autofs_packet_hdr hdr;
> + autofs_wqt_t wait_queue_token;
> + __u32 dev;
> + __u64 ino;
> + __u32 uid;
> + __u32 gid;
> + __u32 pid;
> + __u32 tgid;
> + __u32 len;
> + char name[NAME_MAX+1];
> +} __attribute__ ((packed));
> +
> #define AUTOFS_IOC_EXPIRE_MULTI _IOW(0x93,0x66,int)
> #define AUTOFS_IOC_EXPIRE_INDIRECT AUTOFS_IOC_EXPIRE_MULTI
> #define AUTOFS_IOC_EXPIRE_DIRECT AUTOFS_IOC_EXPIRE_MULTI
More information about the systemd-devel
mailing list