On Fri, Jul 30, 2010 at 11:47 -0500, H Hartley Sweeten wrote:
> On Friday, July 30, 2010 4:07 AM, Kulikov Vasiliy wrote:
> > put_user() may fail, if so return -EFAULT.
> > Also compare count with copied data size, not size of struct with these
> > fields.
> >
> > Signed-off-by: Kulikov Vasiliy <segooon@xxxxxxxxx>
> > ---
> > drivers/staging/dt3155/dt3155_drv.c | 121 ++++++++++++++++------------------
> > 1 files changed, 57 insertions(+), 64 deletions(-)
>
> As Jiri mentioned, whitespace cleanups should be a separate patch.
>
> Also, please base your patch on the current linux-next tree. This patch will
> not apply to the current source.
No problem, I'll be able to do it on Monday or a bit later.
>
> I do have a couple more comments below.
>
> > + struct frame_info *frame_info;
> > + u32 *buffer = (u32 *)buf;
>
> Why are you recasting the __user buffer? This will cause sparse warnings like:
>
> warning: cast removes address space of expression
> warning: incorrect type in argument 1 (different address spaces)
> expected void const volatile [noderef] <asn:1>*<noident>
> got unsigned int [usertype] *buffer
>
> For each put_user and copy_to_user call.
Right, I've thought that it's just a zero macro, but now I see that
it is compiler __attribute__().
>
> > +
> > + /* TODO: this should check the error flag and */
> > + /* return an error on hardware failures */
> > + if (count != 4*3 + sizeof(struct frame_info)) {
> > + pr_err("DT3155 ERROR (NJC): count is not right\n");
> > + return -EINVAL;
> > + }
>
> This is prone to breakage. It's safer to check against the struct size. Also,
> the 4*3 is a bit ambiguous.
>
I think current variant is ambiguous too as it copies concrete data
chunks, but measures struct size. In generic case sizeof(struct) != sum
of sizeof(fields) (current case is ok).
> >
> > - put_user(offset, (unsigned int __user *)buf);
> > - buf += sizeof(u32);
> > - put_user(fb->frame_count, (unsigned int __user *)buf);
> > - buf += sizeof(u32);
> > - put_user(dts->state, (unsigned int __user *)buf);
> > - buf += sizeof(u32);
> > - if (copy_to_user(buf, frame_info, sizeof(*frame_info)))
> > - return -EFAULT;
> > + if (put_user(offset, buffer) ||
> > + put_user(fb->frame_count, buffer + 1) ||
> > + put_user(dts->state, buffer + 2) ||
> > + copy_to_user(buffer + 3, frame_info, sizeof(*frame_info)))
> > + return -EFAULT;
>
> It would be better to just create a temporary 'struct dt3155_read' variable, fill
> in the fields, and then just do one copy_to_user(). That's basically what the
> three put_user calls followed by the copy_to_user() are doing. By using a local
> variable you also prevent any problems with the data order getting changed.
Yes, using struct for copying is the best variant IMO, but its second field
is named frame_seq, not frame_count:
struct dt3155_read {
u32 offset;
u32 frame_seq;
u32 state;
struct frame_info frame_info;
};
maybe it should be renamed to frame_count?
>
> >
> > - return sizeof(struct dt3155_read);
> > + return count;
> > }
> >
> > static unsigned int dt3155_poll (struct file * filp, poll_table *wait)
>
> Regards,
> Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
