Re: 32 bit (socket layer) ioctl emulation for 64 bit kernels

[Arnd Bergmann <arnd@arndb.de>]

On Monday 16 January 2006 06:59, Shaun Pereira wrote:
> 
> I was wondering if this the compat_sock_get_timestamp function is
> needed? If I were to remove the SIOCGSTAMP case from the
> compat_x25_ioctl function, then a SIOCGSTAMP ioctl system call would
> return -ENOIOCTLCMD which could  then be handled by do_siocgstamp
> handler in the ioctl32_hash_table? (fs/compat_ioctl.c)
> In which case I could remove this patch from the rest of the series.

Yes, that would also work, as I already mentioned (or tried to)
in one of my earlier comments. I would prefer to have this patch
though, because in the long term, I think we should migrate more
stuff away from the hash table and having the function there
means that others can use it as well.

> +       err = -EFAULT;
> +       if(access_ok(VERIFTY_WRITE, ctv, sizeof(*ctv))) {
> +               err = __put_user(sk->sk_stamp.tv_sec, &ctv->tv_sec);
> +               err != __put_user(sk->sk_stamp.tv_usec, &ctv->tv_usec);
> +       }
> +       return err;
> +}

This copies the correct data down to user space now, but might result
in returning an invalid error code.
In the second line you now have 'err != __put_user(...);', which is
a comparison, not an assignment!
For readability, I would simply write that as:

	ret = 0;
	if (put_user(sk->sk_stamp.tv_sec, &ctv->tv_sec) |
	    put_user(sk->sk_stamp.tv_usec, &ctv->tv_usec))
		err = -EFAULT;

You can also write it like your code, but with '|' instead of '!', but
that requires the additional knowledge that __put_user can only ever
return '0' or '-EFAULT' itself and that the bitwise or of those is
therefore also one of these two.

	Arnd <><