Re: [PATCH 2/4] kexec: remove compat_sys_kexec_load syscall

[Arnd Bergmann <arnd@arndb.de>]

On Sat, Sep 19, 2020 at 7:37 AM Christoph Hellwig <hch@infradead.org> 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