tmpfs does not set ->remount_fs() anymore and its users need to be converted to new mount API.
BUG: kernel NULL pointer dereference, address: 0000000000000000 PF: supervisor instruction fetch in kernel mode PF: error_code(0x0010) - not-present page 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
Signed-off-by: Sergey Senozhatsky sergey.senozhatsky@gmail.com --- drivers/gpu/drm/i915/gem/i915_gemfs.c | 28 ++++++++++++++++++++------- 1 file changed, 21 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..cf05ba72df9d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -7,14 +7,17 @@ #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 = NULL; struct file_system_type *type; struct vfsmount *gemfs; + bool ok = true;
type = get_fs_type("tmpfs"); if (!type) @@ -36,18 +39,29 @@ 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)) { + ok = false; + goto out; } + + if (!fc->ops->parse_monolithic || + fc->ops->parse_monolithic(fc, options)) { + ok = false; + goto out; + } + + if (!fc->ops->reconfigure || fc->ops->reconfigure(fc)) + ok = false; }
+out: + if (!ok) + pr_err("i915 gemfs reconfiguration failed\n"); + if (!IS_ERR_OR_NULL(fc)) + put_fs_context(fc); i915->mm.gemfs = gemfs; - return 0; }
put_filesystem() before i915_gemfs_init() deals with kern_mount() error.
Signed-off-by: Sergey Senozhatsky sergey.senozhatsky@gmail.com --- drivers/gpu/drm/i915/gem/i915_gemfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index cf05ba72df9d..d437188d1736 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -24,8 +24,10 @@ int i915_gemfs_init(struct drm_i915_private *i915) return -ENODEV;
gemfs = kern_mount(type); - if (IS_ERR(gemfs)) + if (IS_ERR(gemfs)) { + put_filesystem(type); return PTR_ERR(gemfs); + }
/* * Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most
Quoting Sergey Senozhatsky (2019-08-02 13:39:56)
put_filesystem() before i915_gemfs_init() deals with kern_mount() error.
Signed-off-by: Sergey Senozhatsky sergey.senozhatsky@gmail.com
drivers/gpu/drm/i915/gem/i915_gemfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index cf05ba72df9d..d437188d1736 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -24,8 +24,10 @@ int i915_gemfs_init(struct drm_i915_private *i915) return -ENODEV;
gemfs = kern_mount(type);
Looking around, it looks like we always need to drop type after mounting. Should the put_filesystem(type); be here instead?
Anyway, nice catch.
if (IS_ERR(gemfs))
if (IS_ERR(gemfs)) {put_filesystem(type); return PTR_ERR(gemfs);} /* * Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most-- 2.22.0
Quoting Chris Wilson (2019-08-02 13:58:36)
Quoting Sergey Senozhatsky (2019-08-02 13:39:56)
put_filesystem() before i915_gemfs_init() deals with kern_mount() error.
Signed-off-by: Sergey Senozhatsky sergey.senozhatsky@gmail.com
drivers/gpu/drm/i915/gem/i915_gemfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index cf05ba72df9d..d437188d1736 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -24,8 +24,10 @@ int i915_gemfs_init(struct drm_i915_private *i915) return -ENODEV;
gemfs = kern_mount(type);Looking around, it looks like we always need to drop type after mounting. Should the put_filesystem(type); be here instead?
Anyway, nice catch.
Sigh. put_filesystem() is part of fs internals. I'd be tempted to add
static void put_filesystem(struct file_system_type *fs) { module_put(fs->owner); }
and cc that for stable. And send a patch to add EXPORT_SYMBOL and queue it for removal. Or just ignore the stable@ since it's unlikely to be ever met in the wild and just request the EXPORT_SYMBOL. -Chris
On (08/02/19 14:10), Chris Wilson wrote:
drivers/gpu/drm/i915/gem/i915_gemfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index cf05ba72df9d..d437188d1736 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -24,8 +24,10 @@ int i915_gemfs_init(struct drm_i915_private *i915) return -ENODEV;
gemfs = kern_mount(type);Looking around, it looks like we always need to drop type after mounting. Should the put_filesystem(type); be here instead?
Anyway, nice catch.
Sigh. put_filesystem() is part of fs internals. I'd be tempted to add
Good catch!
So we can switch to vfs_kern_mount(), I guess, but pass different options, depending on has_transparent_hugepage().
-ss
On (08/02/19 22:15), Sergey Senozhatsky wrote: [..]
Looking around, it looks like we always need to drop type after mounting. Should the put_filesystem(type); be here instead?
Anyway, nice catch.
Sigh. put_filesystem() is part of fs internals. I'd be tempted to add
Good catch!
So we can switch to vfs_kern_mount(), I guess, but pass different options, depending on has_transparent_hugepage().
Hmm. This doesn't look exactly right. It appears that vfs_kern_mount() has a slightly different purpose. It's for drivers which register their own fstype and fs_context/sb callbacks. A typical usage would be
static struct file_system_type nfsd_fs_type = { .owner→ → = THIS_MODULE, .name→ → = "nfsd", .init_fs_context = nfsd_init_fs_context, .kill_sb→ = nfsd_umount, }; MODULE_ALIAS_FS("nfsd");
vfs_kern_mount(&nfsd_fs_type, SB_KERNMOUNT, "nfsd", NULL);
i915 is a different beast, it just wants to mount fs and reconfigure it, it doesn't want to be an fs. So it seems that current kern_mount() is actually right.
Maybe we need to export put_filesystem() instead.
-ss
Quoting Sergey Senozhatsky (2019-08-02 14:35:03)
On (08/02/19 22:15), Sergey Senozhatsky wrote: [..]
Looking around, it looks like we always need to drop type after mounting. Should the put_filesystem(type); be here instead?
Anyway, nice catch.
Sigh. put_filesystem() is part of fs internals. I'd be tempted to add
Good catch!
So we can switch to vfs_kern_mount(), I guess, but pass different options, depending on has_transparent_hugepage().
Hmm. This doesn't look exactly right. It appears that vfs_kern_mount() has a slightly different purpose. It's for drivers which register their own fstype and fs_context/sb callbacks. A typical usage would be
static struct file_system_type nfsd_fs_type = { .owner→ → = THIS_MODULE, .name→ → = "nfsd", .init_fs_context = nfsd_init_fs_context, .kill_sb→ = nfsd_umount, }; MODULE_ALIAS_FS("nfsd"); vfs_kern_mount(&nfsd_fs_type, SB_KERNMOUNT, "nfsd", NULL);i915 is a different beast, it just wants to mount fs and reconfigure it, it doesn't want to be an fs. So it seems that current kern_mount() is actually right.
struct vfsmount *kern_mount(struct file_system_type *type) { struct vfsmount *mnt; mnt = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL); if (!IS_ERR(mnt)) { /* * it is a longterm mount, don't release mnt until * we unmount before file sys is unregistered */ real_mount(mnt)->mnt_ns = MNT_NS_INTERNAL; } return mnt; }
With the exception of fiddling with MNT_NS_INTERNAL, it seems amenable for our needs. -Chris
On (08/02/19 14:41), Chris Wilson wrote: [..]
struct vfsmount *kern_mount(struct file_system_type *type) { struct vfsmount *mnt; mnt = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL); if (!IS_ERR(mnt)) { /* * it is a longterm mount, don't release mnt until * we unmount before file sys is unregistered */ real_mount(mnt)->mnt_ns = MNT_NS_INTERNAL; } return mnt; }
With the exception of fiddling with MNT_NS_INTERNAL, it seems amenable for our needs.
Sorry, not sure I understand. i915 use kern_mount() at the moment.
Since we still need to put_filesystem(), I'd probably add one more patch - export put_filesystem()
so then we can put_filesystem() in i915. Wonder what would happen if someone would do modprobe i915 rmmod i916 In a loop.
So something like this (this is against current patch set).
--- drivers/gpu/drm/i915/gem/i915_gemfs.c | 5 ++--- fs/filesystems.c | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index d437188d1736..4ea7a6f750f4 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -24,10 +24,9 @@ int i915_gemfs_init(struct drm_i915_private *i915) return -ENODEV;
gemfs = kern_mount(type); - if (IS_ERR(gemfs)) { - put_filesystem(type); + put_filesystem(type); + if (IS_ERR(gemfs)) return PTR_ERR(gemfs); - }
/* * Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most diff --git a/fs/filesystems.c b/fs/filesystems.c index 9135646e41ac..4837eda748b5 100644 --- a/fs/filesystems.c +++ b/fs/filesystems.c @@ -45,6 +45,7 @@ void put_filesystem(struct file_system_type *fs) { module_put(fs->owner); } +EXPORT_SYMBOL(put_filesystem);
static struct file_system_type **find_filesystem(const char *name, unsigned len) { @@ -280,5 +281,4 @@ struct file_system_type *get_fs_type(const char *name) } return fs; } - EXPORT_SYMBOL(get_fs_type);
Quoting Sergey Senozhatsky (2019-08-02 14:49:55)
On (08/02/19 14:41), Chris Wilson wrote: [..]
struct vfsmount *kern_mount(struct file_system_type *type) { struct vfsmount *mnt; mnt = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL); if (!IS_ERR(mnt)) { /* * it is a longterm mount, don't release mnt until * we unmount before file sys is unregistered */ real_mount(mnt)->mnt_ns = MNT_NS_INTERNAL; } return mnt; }
With the exception of fiddling with MNT_NS_INTERNAL, it seems amenable for our needs.
Sorry, not sure I understand. i915 use kern_mount() at the moment.
Since we still need to put_filesystem(), I'd probably add one more patch - export put_filesystem()
so then we can put_filesystem() in i915. Wonder what would happen if someone would do modprobe i915 rmmod i916 In a loop.
So something like this (this is against current patch set).
Yes, that's what I in mind. I was thinking of whether we can replace our kern_mount + fc->ops->reconfigure() into a single vfs_kern_mount(), and whether or not that works with get_fs_type("tmpfs"). -Chris
Quoting Sergey Senozhatsky (2019-08-02 13:39:55)
tmpfs does not set ->remount_fs() anymore and its users need to be converted to new mount API.
BUG: kernel NULL pointer dereference, address: 0000000000000000 PF: supervisor instruction fetch in kernel mode PF: error_code(0x0010) - not-present page 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
Signed-off-by: Sergey Senozhatsky sergey.senozhatsky@gmail.com
drivers/gpu/drm/i915/gem/i915_gemfs.c | 28 ++++++++++++++++++++------- 1 file changed, 21 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..cf05ba72df9d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -7,14 +7,17 @@ #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 = NULL; struct file_system_type *type; struct vfsmount *gemfs;bool ok = true;
Start with ok = false, we only need to set to true if we succeed in reconfiguring.
type = get_fs_type("tmpfs"); if (!type)@@ -36,18 +39,29 @@ 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;
Hmm, we could avoid this if we used vfs_kernel_mount() directly rather than the kern_mount wrapper, as then we pass options through to parse_monotithic_mount_data(). Or am I barking up the wrong tree?
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)) {ok = false;goto out; }if (!fc->ops->parse_monolithic ||fc->ops->parse_monolithic(fc, options)) {
checkpatch.pl will complain that this should line up with the '('
ok = false;goto out;}if (!fc->ops->reconfigure || fc->ops->reconfigure(fc))ok = false; }+out:
if (!ok)pr_err("i915 gemfs reconfiguration failed\n");
Let's make it a bit more user friendly,
dev_err(i915->drm.dev, "Unable to reconfigure internal shmemfs for preferred" " allocation strategy; continuing, but performance may suffer.\n");
Assuming that we can't just use vfs_kern_mount() instead, with the nits Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On (08/02/19 13:54), Chris Wilson wrote: [..]
int i915_gemfs_init(struct drm_i915_private *i915) {
struct fs_context *fc = NULL; struct file_system_type *type; struct vfsmount *gemfs;bool ok = true;Start with ok = false, we only need to set to true if we succeed in reconfiguring.
OK, makes sense.
type = get_fs_type("tmpfs"); if (!type)@@ -36,18 +39,29 @@ 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;Hmm, we could avoid this if we used vfs_kernel_mount() directly rather than the kern_mount wrapper, as then we pass options through to parse_monotithic_mount_data(). Or am I barking up the wrong tree?
Hmm. Wouldn't this error on !TRANSPARENT_HUGE_PAGECACHE systems? "huge=never" should be an invalid option when system does not know about THP.
[..]
if (!fc->ops->parse_monolithic ||fc->ops->parse_monolithic(fc, options)) {checkpatch.pl will complain that this should line up with the '('
It doesn't.
------------------------------------------------- outgoing/0001-i915-convert-to-new-mount-API.patch ------------------------------------------------- total: 0 errors, 0 warnings, 53 lines checked
outgoing/0001-i915-convert-to-new-mount-API.patch has no obvious style problems and is ready for submission.
------------------------------------------------------- outgoing/0002-i915-do-not-leak-module-ref-counter.patch ------------------------------------------------------- total: 0 errors, 0 warnings, 11 lines checked
outgoing/0002-i915-do-not-leak-module-ref-counter.patch has no obvious style problems and is ready for submission.
[..]
if (!ok)pr_err("i915 gemfs reconfiguration failed\n");Let's make it a bit more user friendly,
dev_err(i915->drm.dev, "Unable to reconfigure internal shmemfs for preferred" " allocation strategy; continuing, but performance may suffer.\n");
I guess now checkpatch will complain :)
Assuming that we can't just use vfs_kern_mount() instead, with the nits Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Thanks.
I'll sit on it for several days, just to see if more feedback will come.
-ss
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Sergey Senozhatsky