On Sat, Sep 19, 2020 at 7:37 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> > + struct compat_kexec_segment __user *cs = (void __user *)segments;
> > + struct compat_kexec_segment segment;
> > + int i;
> > + for (i=0; i< nr_segments; i++) {
>
> Missing empty line after the variable declarations and really strange
> indentation.
>
> > + copy_from_user(&segment, &cs[i], sizeof(segment));
>
> Missing return value check.
>
> > + if (ret)
> > + break;
> > +
> > + image->segment[i] = (struct kexec_segment) {
> > + .buf = compat_ptr(segment.buf),
> > + .bufsz = segment.bufsz,
> > + .mem = segment.mem,
> > + .memsz = segment.memsz,
> > + };
> > + }
>
> I'd split the whole compat handling into a helper, and I'd probably
> use the unsafe_get/put user to optimize it a little more.
>
> > + } else {
> > + ret = copy_from_user(image->segment, segments, segment_bytes);
> > + }
> > if (ret)
> > ret = -EFAULT;
>
> Why not just
>
> if (copy_from_user(image->segment, segments, segment_bytes))
> ret = -EFAULT;
>
> ?
Addressed all of these now, thanks for the suggestions!
I had already fixed the missing error handling after the kbuild bot
pointed that out. The separate function does improve the error
handling.
I ended up not using unsafe_get/put since I find the copy_from_user
based loop more readable and it should lead to smaller object code in
most cases as well. kexec is not performance critical, so readability
seems more important here.
Arnd
