| Message ID | 20190721142930.GA480@tigerII.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [linux-next] mm/i915: i915_gemfs_init() NULL dereference | expand |
On (07/21/19 23:29), Sergey Senozhatsky wrote: > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > #PF: supervisor instruction fetch in kernel mode > #PF: error_code(0x0010) - not-present page > PGD 0 P4D 0 > Oops: 0010 [#1] PREEMPT SMP PTI > RIP: 0010:0x0 > Code: Bad RIP value. > [..] > Call Trace: > i915_gemfs_init+0x6e/0xa0 [i915] > i915_gem_init_early+0x76/0x90 [i915] > i915_driver_probe+0x30a/0x1640 [i915] > ? kernfs_activate+0x5a/0x80 > ? kernfs_add_one+0xdd/0x130 > pci_device_probe+0x9e/0x110 > really_probe+0xce/0x230 > driver_probe_device+0x4b/0xc0 > device_driver_attach+0x4e/0x60 > __driver_attach+0x47/0xb0 > ? device_driver_attach+0x60/0x60 > bus_for_each_dev+0x61/0x90 > bus_add_driver+0x167/0x1b0 > driver_register+0x67/0xaa > ? 0xffffffffc0522000 > do_one_initcall+0x37/0x13f > ? kmem_cache_alloc+0x11a/0x150 > do_init_module+0x51/0x200 > __se_sys_init_module+0xef/0x100 > do_syscall_64+0x49/0x250 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 So "the new mount API" conversion probably looks like something below. But I'm not 100% sure. --- drivers/gpu/drm/i915/gem/i915_gemfs.c | 32 +++++++++++++++++++++------ 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index 099f3397aada..2e365b26f8ee 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -7,12 +7,14 @@ #include <linux/fs.h> #include <linux/mount.h> #include <linux/pagemap.h> +#include <linux/fs_context.h> #include "i915_drv.h" #include "i915_gemfs.h" int i915_gemfs_init(struct drm_i915_private *i915) { + struct fs_context *fc; struct file_system_type *type; struct vfsmount *gemfs; @@ -36,19 +38,35 @@ int i915_gemfs_init(struct drm_i915_private *i915) struct super_block *sb = gemfs->mnt_sb; /* FIXME: Disabled until we get W/A for read BW issue. */ char options[] = "huge=never"; - int flags = 0; int err; - err = sb->s_op->remount_fs(sb, &flags, options); - if (err) { - kern_unmount(gemfs); - return err; - } + fc = fs_context_for_reconfigure(sb->s_root, 0, 0); + if (IS_ERR(fc)) + goto err; + + if (!fc->ops->parse_monolithic) + goto err; + + err = fc->ops->parse_monolithic(fc, options); + if (err) + goto err; + + if (!fc->ops->reconfigure) + goto err; + + err = fc->ops->reconfigure(fc); + if (err) + goto err; } i915->mm.gemfs = gemfs; - return 0; + +err: + pr_err("i915 gemfs init() failed\n"); + put_fs_context(fc); + kern_unmount(gemfs); + return -EINVAL; } void i915_gemfs_fini(struct drm_i915_private *i915)
Quoting Sergey Senozhatsky (2019-07-31 17:48:29) > @@ -36,19 +38,35 @@ int i915_gemfs_init(struct drm_i915_private *i915) > struct super_block *sb = gemfs->mnt_sb; > /* FIXME: Disabled until we get W/A for read BW issue. */ > char options[] = "huge=never"; > - int flags = 0; > int err; > > - err = sb->s_op->remount_fs(sb, &flags, options); > - if (err) { > - kern_unmount(gemfs); > - return err; > - } > + fc = fs_context_for_reconfigure(sb->s_root, 0, 0); > + if (IS_ERR(fc)) > + goto err; > + > + if (!fc->ops->parse_monolithic) > + goto err; > + > + err = fc->ops->parse_monolithic(fc, options); > + if (err) > + goto err; > + > + if (!fc->ops->reconfigure) It would be odd for fs_context_for_reconfigure() to allow creation of a context if that context couldn't perform a reconfigre, nevertheless that seems to be the case. > + goto err; > + > + err = fc->ops->reconfigure(fc); > + if (err) > + goto err; Only thing that stands out is that we should put_fs_context() here as well. I guess it's better than poking at the SB_INFO directly ourselves. I think though we shouldn't bail if we can't change the thp setting, and just accept whatever with a warning. Looks like the API is already available in dinq, so we can apply this ahead of the next merge window. -Chris
On (08/01/19 18:30), Chris Wilson wrote: > Quoting Sergey Senozhatsky (2019-07-31 17:48:29) > > @@ -36,19 +38,35 @@ int i915_gemfs_init(struct drm_i915_private *i915) [..] > > + if (!fc->ops->parse_monolithic) > > + goto err; > > + > > + err = fc->ops->parse_monolithic(fc, options); > > + if (err) > > + goto err; > > + > > + if (!fc->ops->reconfigure) > > It would be odd for fs_context_for_reconfigure() to allow creation of a > context if that context couldn't perform a reconfigre, nevertheless that > seems to be the case. Well, I kept those checks just because fs/ code does the same. E.g. fs/super.c reconfigure_super() { if (fc->ops->reconfigure) fc->ops->reconfigure(fc) } do_emergency_remount_callback() { fc = fs_context_for_reconfigure(); reconfigure_super(fc); } > > + goto err; > > + > > + err = fc->ops->reconfigure(fc); > > + if (err) > > + goto err; > > Only thing that stands out is that we should put_fs_context() here as > well. Oh... Indeed, somehow I forgot to put_fs_context(). > I guess it's better than poking at the SB_INFO directly ourselves. > I think though we shouldn't bail if we can't change the thp setting, and > just accept whatever with a warning. OK. > Looks like the API is already available in dinq, so we can apply this > ahead of the next merge window. OK, will cook a formal patch then. Thanks! -ss
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index 099f3397aada..1f95d9ea319a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -39,6 +39,9 @@ int i915_gemfs_init(struct drm_i915_private *i915) int flags = 0; int err; + if (!sb->s_op->remount_fs) + return -ENODEV; + err = sb->s_op->remount_fs(sb, &flags, options); if (err) { kern_unmount(gemfs);
Hi, My laptop oopses early on with nothing on the screen; after some debugging I managed to obtain a backtrace: BUG: kernel NULL pointer dereference, address: 0000000000000000 #PF: supervisor instruction fetch in kernel mode #PF: error_code(0x0010) - not-present page PGD 0 P4D 0 Oops: 0010 [#1] PREEMPT SMP PTI RIP: 0010:0x0 Code: Bad RIP value. [..] Call Trace: i915_gemfs_init+0x6e/0xa0 [i915] i915_gem_init_early+0x76/0x90 [i915] i915_driver_probe+0x30a/0x1640 [i915] ? kernfs_activate+0x5a/0x80 ? kernfs_add_one+0xdd/0x130 pci_device_probe+0x9e/0x110 really_probe+0xce/0x230 driver_probe_device+0x4b/0xc0 device_driver_attach+0x4e/0x60 __driver_attach+0x47/0xb0 ? device_driver_attach+0x60/0x60 bus_for_each_dev+0x61/0x90 bus_add_driver+0x167/0x1b0 driver_register+0x67/0xaa ? 0xffffffffc0522000 do_one_initcall+0x37/0x13f ? kmem_cache_alloc+0x11a/0x150 do_init_module+0x51/0x200 __se_sys_init_module+0xef/0x100 do_syscall_64+0x49/0x250 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP is at 0x00, which is never good It sort of boils down to commit 144df3b288c4 (vfs: Convert ramfs, shmem, tmpfs, devtmpfs, rootfs to use the new mount API), which removed ->remount_fs from tmpfs' ops: === @@ -3736,7 +3849,6 @@ static const struct super_operations shmem_ops = { .destroy_inode = shmem_destroy_inode, #ifdef CONFIG_TMPFS .statfs = shmem_statfs, - .remount_fs = shmem_remount_fs, .show_options = shmem_show_options, #endif .evict_inode = shmem_evict_inode, === So i915 init executes NULL get_fs_type("tmpfs"); sb->s_op->remount_fs(sb, &flags, options); For the time being the following (obvious and wrong) patch at least boots -next: --- --- -ss