| Message ID | 20181213013000.15716-2-nbowler@draconx.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixing xfs ioctls on x32 | expand |
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?" > Signed-off-by: Nick Bowler <nbowler@draconx.ca> > --- > 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. > + 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? Brian > /* done = 1 if there are more stats to get and if bulkstat */ > /* should be called again (unused here, but used in dmapi) */ > > @@ -272,15 +289,15 @@ xfs_compat_ioc_bulkstat( > > if (cmd == XFS_IOC_FSINUMBERS_32) { > error = xfs_inumbers(mp, &inlast, &count, > - bulkreq.ubuffer, xfs_inumbers_fmt_compat); > + bulkreq.ubuffer, inumbers_func); > } else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32) { > int res; > > - error = xfs_bulkstat_one_compat(mp, inlast, bulkreq.ubuffer, > - sizeof(compat_xfs_bstat_t), NULL, &res); > + error = bs_one_func(mp, inlast, bulkreq.ubuffer, > + bs_one_size, NULL, &res); > } else if (cmd == XFS_IOC_FSBULKSTAT_32) { > error = xfs_bulkstat(mp, &inlast, &count, > - xfs_bulkstat_one_compat, sizeof(compat_xfs_bstat_t), > + bs_one_func, bs_one_size, > bulkreq.ubuffer, &done); > } else > error = -EINVAL; > -- > 2.16.1 >
On 2018-12-13, Brian Foster <bfoster@redhat.com> 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
On Thu, Dec 13, 2018 at 12:44:16PM -0500, Nick Bowler wrote: > On 2018-12-13, Brian Foster <bfoster@redhat.com> 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. > I assume that this is because xfs_fsop_bulkreq includes pointers, which is where x32 and x86_64 actually start to differ..? So in this particular case, the two ioctl() structs actually are different between x32 and x86_64. > - 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. > Ok. > 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. > A little hairy, but it makes more sense now. Thanks. > >> --- > >> 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. > I'd suggest to use parts of what you've just described above. Brian > >> + 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
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. + */ + 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 + /* done = 1 if there are more stats to get and if bulkstat */ /* should be called again (unused here, but used in dmapi) */ @@ -272,15 +289,15 @@ xfs_compat_ioc_bulkstat( if (cmd == XFS_IOC_FSINUMBERS_32) { error = xfs_inumbers(mp, &inlast, &count, - bulkreq.ubuffer, xfs_inumbers_fmt_compat); + bulkreq.ubuffer, inumbers_func); } else if (cmd == XFS_IOC_FSBULKSTAT_SINGLE_32) { int res; - error = xfs_bulkstat_one_compat(mp, inlast, bulkreq.ubuffer, - sizeof(compat_xfs_bstat_t), NULL, &res); + error = bs_one_func(mp, inlast, bulkreq.ubuffer, + bs_one_size, NULL, &res); } else if (cmd == XFS_IOC_FSBULKSTAT_32) { error = xfs_bulkstat(mp, &inlast, &count, - xfs_bulkstat_one_compat, sizeof(compat_xfs_bstat_t), + bs_one_func, bs_one_size, bulkreq.ubuffer, &done); } else error = -EINVAL;
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. Signed-off-by: Nick Bowler <nbowler@draconx.ca> --- fs/xfs/xfs_ioctl32.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)