On 2018-12-13, Brian Foster <bfoster@xxxxxxxxxx> wrote:
> On Wed, Dec 12, 2018 at 08:29:59PM -0500, Nick Bowler wrote:
>> The bulkstat family of ioctls are problematic on x32, because there is
>> a mixup of native 32-bit and 64-bit conventions: the xfs_bulkstat struct
>> contains pointers and 32-bit integers so that fits the native 32-bit
>> layout fine. However, one of those pointers is subsequently used to
>> refer to one of several structs which, on x32, all follow the native
>> 64-bit way.
>>
>> Fortunately the pointer chasing seems to end there, and the functions to
>> deal with this abstract things pretty well. We just need a little tweak
>> to pass the right formatting function if called from x32 mode.
>>
> > Could you be a bit more specific on the problem? What
> pointers/structures are problematic? What exactly is "the xfs_bulkstat
> struct?"
A mistake. I meant struct xfs_fsop_bulkreq; I'll fix the commit message.
The problem is:
- xfs_fsop_bulkreq on x32 matches IA-32 layout on x32, so the
ioctl cmd number matches and the implementation calls
xfs_compat_ioc_bulkstat.
- The 'ubuffer' pointer in that structure refers to either struct
xfs_bstat or struct xfs_inogrp. On x32 both of these structures
match the native 64-bit layout; the compat path writes out the
IA-32 layout which is incorrectly formatted for x32 userspace.
The proposed solution is:
- Use in_x32_syscall to distinguish the IA-32 and x32 cases, the
functions which do this have a easy way to select which output
format is required, so we just need to pick the right one on x32.
>> ---
>> fs/xfs/xfs_ioctl32.c | 25 +++++++++++++++++++++----
>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
>> index fba115f4103a..6a759c0ffb54 100644
>> --- a/fs/xfs/xfs_ioctl32.c
>> +++ b/fs/xfs/xfs_ioctl32.c
>> @@ -241,6 +241,23 @@ xfs_compat_ioc_bulkstat(
>> int done;
>> int error;
>>
>> + /*
>> + * These functions and size are used later to handle individual
>> + * entries; x32 is annoying and needs different functions.
>> + */
>
> Same here, this describes the change but doesn't help me understand the
> problem.
I'll think about a better way to write this comment.
>> + inumbers_fmt_pf inumbers_func = xfs_inumbers_fmt_compat;
>> + bulkstat_one_pf bs_one_func = xfs_bulkstat_one_compat;
>> + size_t bs_one_size = sizeof(compat_xfs_bstat_t);
>> +
>> +#ifdef CONFIG_X86_X32
>> + if (in_x32_syscall()) {
>> + /* x32 matches native amd64 bstat and inogrp layout */
>> + inumbers_func = xfs_inumbers_fmt;
>> + bs_one_func = xfs_bulkstat_one;
>> + bs_one_size = sizeof(xfs_bstat_t);
>> + }
>> +#endif
>> +
>
> Would this be necessary if the higher level x32 code called into
> xfs_ioc_bulkstat() instead of the compat variant, or is there some
> other reason x32 wouldn't work through that path?
The xfs_compat_ioc_bulkstat function does two things: it reads in the
xfs_fsop_bulkreq structure (matches ia-32 layout on x32), then it writes
out the xfs_inorgp or xfs_bstat structures depending on what operation
was requested; both of these structures match amd64 layout on x32.
So the goal of the change is to adjust the second behaviour only.
Cheers,
Nick
