Hi Kazu
Sorry for the delayed reply,
On Thu, Oct 29, 2020 at 2:34 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab@xxxxxxx> wrote:
>
> Hi Bhupesh,
>
> I looked this discussion over again, but I'm not sure what kind of
> implementation and code comment you were suggesting. As for
> backporting the patch, I thought the commit log would be enough..
>
> Do you think we should not merge this as is?
No problem. I only had some nitpicks.
We can go ahead and apply the same for now - maybe later on if someone
finds more time we can clean this up further.
Feel free to add:
Acked-by: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
Thanks.
>
> -----Original Message-----
> > On Fri, 11 Sep 2020 02:05:21 +0530
> > Bhupesh Sharma <bhsharma@xxxxxxxxxx> 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@xxxxxxx> 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@xxxxxxxx>
> > > >
> > > > ---
> > > > 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@xxxxxxxxxx
> > > > https://www.redhat.com/mailman/listinfo/crash-utility
> > >
> > > With the above points addressed, please feel free to add:
> > >
> > > Acked-by: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
> > >
> > > Thanks.
> > >
> > > --
> > > Crash-utility mailing list
> > > Crash-utility@xxxxxxxxxx
> > > https://www.redhat.com/mailman/listinfo/crash-utility
> > >
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
