On Friday, December 30, 2011, Ben Hutchings wrote:
> On Thu, 2011-12-29 at 23:33 +0100, Rafael J. Wysocki wrote:
> > On Thursday, December 29, 2011, Ben Hutchings wrote:
> > > On Tue, 2011-12-27 at 21:33 +0100, Ben Hutchings wrote:
> > > > This allows uswsusp built for i386 to run on an x86_64 kernel (tested
> > > > with Debian package version 1.0+20110509-2).
> > >
> > > Now I wonder whether all this code was necessary.
> > >
> > > [...]
> > > > +#ifdef CONFIG_COMPAT
> > > > +
> > > > +struct compat_resume_swap_area {
> > > > + compat_loff_t offset;
> > > > + u32 dev;
> > > > +} __packed;
> > > [...]
> > >
> > > Is there actually any case where this doesn't have compatible layout
> > > with struct resume_swap_area? It may have lower alignment, but
> > > misalignment is dealt with automatically when copying in/out of
> > > userland.
> >
> > Quite frankly, I'm not 100% sure.
>
> If it wasn't for the '__packed' in the declaration of struct
> resume_swap_area, there would be 4 trailing bytes of padding on 64-bit
> architectures and none on most 32-bit architectures. But __packed
> should inhibit that padding.
>
> I was apparently mistaken about put_user(); that would need to be
> changed to put_user_unaligned() if we want to handle compat_loff_t
> without a wrapper.
>
> > > If the structure size was variable then the ioctl number would be as
> > > well, and commit cf007e3526a785a95a738d5a8fba44f1f4fe33e0 is supposed to
> > > have removed the last ioctls that had that problem.
> > >
> > > So maybe the compat function can be as simple as:
> > >
> > > static long
> > > snapshot_compat_ioctl(struct file *file, unsigned int cmd, unsigned long
> > > arg)
> > > {
> > > BUILD_BUG_ON(sizeof(loff_t) != sizeof(compat_loff_t));
> > > BUILD_BUG_ON(sizeof(struct resume_swap_area) !=
> > > sizeof(struct compat_resume_swap_area));
> > >
> >
> > Well, I'd prefer to fail the ioctl in the above situations instead of
> > crashing
> > the kernel.
>
> This is a compile-time check...
That's correct, sorry.
> > > switch (cmd) {
> > > case SNAPSHOT_SET_SWAP_AREA:
> > > case SNAPSHOT_GET_IMAGE_SIZE:
> > > case SNAPSHOT_CREATE_IMAGE:
> > > case SNAPSHOT_AVAIL_SWAP_SIZE:
> > > case SNAPSHOT_ALLOC_SWAP_PAGE:
> > > return snapshot_ioctl(file, cmd,
> > > (unsigned long) compat_ptr(arg));
> > >
> > > default:
> > > return snapshot_ioctl(file, cmd, arg);
> > > }
> > > }
> >
> > Does it acutally work?
>
> I haven't tested, but in any case testing on x86 wouldn't prove that
> this works for any other 32/64-bit architecture pair.
Well, if it doesn't work on x86, it will prove something. :-)
Thanks,
Rafael
--
To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/201112300052.11690....@sisk.pl
