On Wed, 2019-11-20 at 20:46 +0100, Arnd Bergmann wrote:
> On Wed, Nov 20, 2019 at 8:27 PM Ben Hutchings
> <ben.hutchings@xxxxxxxxxxxxxxx> wrote:
> > On Fri, 2019-11-08 at 21:34 +0100, Arnd Bergmann wrote:
> > > The layout of struct timeval is different on sparc64 from
> > > anything else, and the patch I did long ago failed to take
> > > this into account.
> > >
> > > Change it now to handle sparc64 user space correctly again.
> > >
> > > Quite likely nobody cares about parallel ports on sparc64,
> > > but there is no reason not to fix it.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: 9a450484089d ("lp: support 64-bit time_t user space")
> > > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> > > ---
> > > drivers/char/lp.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> > > index 7c9269e3477a..bd95aba1f9fe 100644
> > > --- a/drivers/char/lp.c
> > > +++ b/drivers/char/lp.c
> > > @@ -713,6 +713,10 @@ static int lp_set_timeout64(unsigned int minor, void __user *arg)
> > > if (copy_from_user(karg, arg, sizeof(karg)))
> > > return -EFAULT;
> > >
> > > + /* sparc64 suseconds_t is 32-bit only */
> > > + if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
> > > + karg[1] >>= 32;
> > > +
> > > return lp_set_timeout(minor, karg[0], karg[1]);
> > > }
> > >
> >
> > It seems like it would make way more sense to use __kernel_old_timeval.
>
> Right, that would work. I tried to keep the patch small here, changing
> it to __kernel_old_timeval would require make it all more complicated
> since it would still need to check some conditional to tell the difference
> between sparc32 and sparc64.
Right.
> I think this patch (relative to the version I posted) would work the same:
>
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index bd95aba1f9fe..86994421ee97 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -713,13 +713,19 @@ static int lp_set_timeout64(unsigned int minor,
> void __user *arg)
> if (copy_from_user(karg, arg, sizeof(karg)))
> return -EFAULT;
>
> - /* sparc64 suseconds_t is 32-bit only */
> - if (IS_ENABLED(CONFIG_SPARC64) && !in_compat_syscall())
> - karg[1] >>= 32;
> -
> return lp_set_timeout(minor, karg[0], karg[1]);
> }
>
> +static int lp_set_timeout(unsigned int minor, void __user *arg)
That function name is already used! Maybe this should be
lp_set_timeout_old()?
> +{
> + __kernel_old_timeval tv;
> +
> + if (copy_from_user(tv, arg, sizeof(karg)))
> + return -EFAULT;
> +
> + return lp_set_timeout(minor, tv->tv_sec, tv->tv_usec);
> +}
> +
> static long lp_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -730,11 +736,8 @@ static long lp_ioctl(struct file *file, unsigned int cmd,
> mutex_lock(&lp_mutex);
> switch (cmd) {
> case LPSETTIMEOUT_OLD:
> - if (BITS_PER_LONG == 32) {
> - ret = lp_set_timeout32(minor, (void __user *)arg);
> - break;
> - }
> - /* fall through - for 64-bit */
> + ret = lp_set_timeout(minor, (void __user *)arg);
> + break;
> case LPSETTIMEOUT_NEW:
> ret = lp_set_timeout64(minor, (void __user *)arg);
> break;
>
> Do you like that better?
Yes. Aside from the duplicate function name, it looks correct and
cleaner than the current version.
> One difference here is the handling of
> LPSETTIMEOUT_NEW on sparc64, which would continue to use
> the 64/64 layout rather than the 64/32/pad layout, but that should
> be ok, since sparc64 user space using ppdev (if any exists)
> would use LPSETTIMEOUT_OLD, not LPSETTIMEOUT_NEW.
Right, that's a little weird but appears to be consistent with "new"
socket timestamps.
> > Then you don't have to explicitly handle the sparc64 oddity.
> >
> > As it is, this still over-reads from user-space which might result in a
> > spurious -EFAULT.
>
> I think you got this wrong: sparc64 like most architectures naturally
> aligns 64-bit members, so 'struct timeval' still uses 16 bytes including
> the four padding bytes at the end, it just has the nanoseconds in
> a different position from all other big-endian architectures.
Oh of course, yes.
Ben.
--
Ben Hutchings, Software Developer Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
Manchester, M1 2HF, United Kingdom
