Re: [Linaro-mm-sig] [PATCHv3 2/2] staging: android: ion: Add ioctl to query available heaps

[Laura Abbott]

On 09/07/2016 12:37 PM, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 11:49:59 AM CEST Laura Abbott wrote:
>
> > -	if (dir & _IOC_WRITE)
> > -		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> > -			return -EFAULT;
> > +	/*
> > +	 * The copy_from_user is unconditional here for both read and write
> > +	 * to do the validate. If there is no write for the ioctl, the
> > +	 * buffer is cleared
> > +	 */
> > +	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> > +		return -EFAULT;
> > +
> > +	ret = validate_ioctl_arg(cmd, &data);
> > +	if (WARN_ON_ONCE(ret))
> > +		return ret;
>
> I noticed that the WARN_ON_ONCE warns about invalid user input,
> but I think we tend to normally just use WARN_ON for things that
> go wrong inside of the kernel or in hardware.
>
> Maybe better use printk_once() or printk_ratelimited.

Sure, the error code should hopefully be enough of a hint to
userspace to maybe check the log.

> Is there any noticeable overhead in always copying the structure?
> copy_from_user() can be a bit slow depending on debugging or
> security features, and it seems unnecessary if the validation
> is only done for one of the commands.

Good point. It made sense with some of the other ioctls (specifically
the ABI) but isn't necessary now. We can evaluate later when other
ioctls get added.

> Otherwise the patch looks good to me.
>
> 	Arnd

Thanks!

Laura