Re: [PATCH 1/7] binder: create userspace-to-binder-buffer copy function

[Dan Carpenter]

On Tue, Jan 29, 2019 at 04:32:09PM -0800, Todd Kjos wrote:
> On Tue, Jan 29, 2019 at 12:12 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> >
> > 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);
> 
> Dan,
> 
> Thanks for the feedback.
> 
> The one above: "size = min(bytes, PAGE_SIZE - pgoff);", results in
> 32-bit compile warnings:
> 
> ./include/linux/kernel.h:846:29: warning: comparison of distinct
> pointer types lacks a cast
>    (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> [...]
> drivers/android/binder_alloc.c:1157:10: note: in expansion of macro âminâ
>    size = min(bytes, PAGE_SIZE - pgoff);
> 
> so, I took your suggestion:
> 
>   size = min_t(size_t, 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;
> 
> It definitely is cleaner, but the clean version is doing arithmetic on
> a void pointer which is generally not considered good practice (but is
> supported by gcc). Is this acceptable in the kernel? I don't see any
> statement on it in Documentation/process/coding-style.rst.

Yeah, it's a GCCism but to me it seems so nice.  We do it all the time.

We'd like to get Clang to compile the kernel, but we've done very little
work to make that happen.  Removing variable length arrays was really
done for other reasons...

regards,
dan carpenter