Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 29 Jan 2019 11:12:18 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Todd Kjos <tkjos@android.com>
Cc:     tkjos@google.com, gregkh@linuxfoundation.org, arve@android.com,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        maco@google.com, joel@joelfernandes.org, kernel-team@android.com
Subject: Re: [PATCH 1/7] binder: create userspace-to-binder-buffer copy
 function
Message-ID: <20190129081218.GL1795@kadam>
References: <20190129004934.85885-1-tkjos@google.com>
 <20190129004934.85885-2-tkjos@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190129004934.85885-2-tkjos@google.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 28, 2019 at 04:49:28PM -0800, Todd Kjos wrote:
> +/**
> + * binder_alloc_copy_user_to_buffer() - copy src user to tgt user
> + * @alloc: binder_alloc for this proc
> + * @buffer: binder buffer to be accessed
> + * @buffer_offset: offset into @buffer data
> + * @from: userspace pointer to source buffer
> + * @bytes: bytes to copy
> + *
> + * Copy bytes from source userspace to target buffer.
> + *
> + * Return: bytes remaining to be copied
> + */
> +unsigned long
> +binder_alloc_copy_user_to_buffer(struct binder_alloc *alloc,
> +				 struct binder_buffer *buffer,
> +				 binder_size_t buffer_offset,
> +				 const void __user *from,
> +				 size_t bytes)
> +{
> +	if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> +		return bytes;
> +
> +	while (bytes) {
> +		unsigned long size;
> +		unsigned long ret;
> +		struct page *page;
> +		pgoff_t pgoff;
> +		void *kptr;
> +
> +		page = binder_alloc_get_page(alloc, buffer,
> +					     buffer_offset, &pgoff);
> +		size = min(bytes, (size_t)(PAGE_SIZE - pgoff));

This code has so much more casting than necessary.  To me casting says
that we haven't got the types correct or something.  I've just pulled
this function out as an example really, but none of the casts here are
required...  This could just be:

		size = min(bytes, PAGE_SIZE - pgoff);

Btw, if you really need to do a cast inside a min() (you don't in this
cast) then use min_t().

> +		kptr = (void *)((uintptr_t)kmap(page) + pgoff);

This would be a lot cleaner as:

		kptr = kmap(page) + pgoff;

> +		ret = copy_from_user(kptr, (const void __user *)(uintptr_t)from,
> +				     size);

Remove the cast:

		ret = copy_from_user(kptr, from, size);


> +		kunmap(page);
> +		if (ret)
> +			return bytes - size + ret;
> +		bytes -= size;
> +		from = (void __user *)(uintptr_t)from + size;

Remove the cast:

		from += size;

> +		buffer_offset += size;
> +	}
> +	return 0;
> +}

regards,
dan carpenter