[Crash-utility] [PATCH] Fix memory driver module build with kernel 5.8+

[Petr Tesarik]

On Fri, 11 Sep 2020 02:05:21 +0530
Bhupesh Sharma <bhsharma at redhat.com> wrote:

> Hi Petr,
> 
> Thanks for the patch. I have some nitpicks, please see inline:
> 
> On Fri, Sep 4, 2020 at 7:24 PM Petr Tesarik <ptesarik at suse.cz> wrote:
> >
> > Kernel commit fe557319aa06c23cffc9346000f119547e0f289a renamed
> > probe_kernel_{read,write} to copy_{from,to}_kernel_nofault.
> >
> > Additionally, commit 0493cb086353e786be56010780a0b7025b5db34c
> > unexported probe_kernel_write(), so writing kernel memory is
> > no longer possible from a module.
> >
> > I have renamed the functions in source, but I'm also adding wrappers to
> > allow building the module with older kernel versions.
> >
> > Without this patch, build with kernel 5.8 and later fails:
> >
> > kbuild/default/crash.c: In function 'crash_write':
> > kbuild/default/crash.c:189:12: error: implicit declaration of function 'probe_kernel_write'; did you mean 'kernel_write'? [-Werror=implicit-function-declaration]
> >   189 |        if (probe_kernel_write(vaddr, buffer, count)) {
> >       |            ^~~~~~~~~~~~~~~~~~
> >       |            kernel_write
> > kbuild/default/crash.c: In function 'crash_read':
> > kbuild/default/crash.c:225:13: error: implicit declaration of function 'probe_kernel_read'; did you mean 'kernel_read'? [-Werror=implicit-function-declaration]
> >   225 |         if (probe_kernel_read(buffer, vaddr, count)) {
> >       |             ^~~~~~~~~~~~~~~~~
> >       |             kernel_read
> >
> > Signed-off-by: Petr Tesarik <ptesarik at suse.com>
> >
> > ---
> >  memory_driver/crash.c |   27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > --- a/memory_driver/crash.c
> > +++ b/memory_driver/crash.c
> > @@ -25,6 +25,7 @@
> >   *****************************************************************************/
> >
> >  #include <linux/module.h>
> > +#include <linux/version.h>
> >  #include <linux/types.h>
> >  #include <linux/miscdevice.h>
> >  #include <linux/init.h>
> > @@ -37,6 +38,22 @@
> >
> >  extern int page_is_ram(unsigned long);
> >
> > +#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 8, 0)
> > +
> > +#define CAN_WRITE_KERNEL  
>       1
> 
> Hmmm.. I have no strong opinion about this, but can I suggest using a
> simple variable instead of a macro.
> Something like a 'unsigned int version_info':
> 
> unsigned int version_info = LINUX_VERSION_CODE;
> 
> and then perform the check in the code at run time against the 'version_info'.

I'm not sure how to implement this. Kernel 5.8+ does not export any
symbol that could be used for writing, so I cannot even build the code
that needs it.

Are you suggesting to use a function pointer and initialize it using
symbol_get()?

> To me, it seems more portable in future, as we are seeing such issues
> with newer kernels (due to function name renaming/deprecation).
> 
> Another minor nitpick: Can I suggest adding a comment to this code leg
> - this can help while backporting patches to distributions (which
> still work with older kernel versions) - as it helps making a better
> informed decision while backporting the patch.

OK, I get it that this would not be needed if I use symbol_get(),
correct?

Petr T

> > +
> > +static inline long copy_from_kernel_nofault(void *dst, const void
> > *src, size_t size) +{
> > +       return probe_kernel_read(dst, src, size);
> > +}
> > +
> > +static inline long copy_to_kernel_nofault(void *dst, const void
> > *src, size_t size) +{
> > +       return probe_kernel_write(dst, src, size);
> > +}
> > +
> > +#endif
> > +
> >  #ifdef CONFIG_S390
> >  /*
> >   * For swapped prefix pages get bounce buffer using
> > xlate_dev_mem_ptr() @@ -160,6 +177,8 @@ crash_llseek(struct file *
> > file, loff_t }
> >  }
> >
> > +#ifdef CAN_WRITE_KERNEL
> > +
> >  static ssize_t
> >  crash_write(struct file *file, const char *buf, size_t count,
> > loff_t *poff) {
> > @@ -186,7 +205,7 @@ crash_write(struct file *file, const cha
> >                 return -EFAULT;
> >         }
> >
> > -       if (probe_kernel_write(vaddr, buffer, count)) {
> > +       if (copy_to_kernel_nofault(vaddr, buffer, count)) {
> >                 unmap_virtual(page);
> >                 return -EFAULT;
> >         }
> > @@ -197,6 +216,8 @@ crash_write(struct file *file, const cha
> >         return written;
> >  }
> >
> > +#endif
> > +
> >  /*
> >   *  Determine the page address for an address offset value,
> >   *  get a virtual address for it, and copy it out.
> > @@ -222,7 +243,7 @@ crash_read(struct file *file, char *buf,
> >          * Use bounce buffer to bypass the CONFIG_HARDENED_USERCOPY
> >          * kernel text restriction.
> >          */
> > -        if (probe_kernel_read(buffer, vaddr, count)) {
> > +        if (copy_from_kernel_nofault(buffer, vaddr, count)) {
> >                  unmap_virtual(page);
> >                  return -EFAULT;
> >          }
> > @@ -294,7 +315,9 @@ static struct file_operations crash_fops
> >         .owner = THIS_MODULE,
> >         .llseek = crash_llseek,
> >         .read = crash_read,
> > +#ifdef CAN_WRITE_KERNEL
> >         .write = crash_write,
> > +#endif
> >         .unlocked_ioctl = crash_ioctl,
> >         .open = crash_open,
> >         .release = crash_release,
> > --
> > Crash-utility mailing list
> > Crash-utility at redhat.com
> > https://www.redhat.com/mailman/listinfo/crash-utility  
> 
> With the above points addressed, please feel free to add:
> 
> Acked-by: Bhupesh Sharma <bhsharma at redhat.com>
> 
> Thanks.
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: Digit?ln? podpis OpenPGP
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20200911/21b6f868/attachment.sig>