[PATCH 1/2] i915: convert to new mount API

[PATCH 1/2] i915: convert to new mount API

[Sergey Senozhatsky]

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;
 }
 
-- 
2.22.0

[PATCH 2/2] i915: do not leak module ref counter

[Sergey Senozhatsky]

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
-- 
2.22.0

Re: [PATCH 2/2] i915: do not leak module ref counter

[Chris Wilson]

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
> 

Re: [PATCH 2/2] i915: do not leak module ref counter

[Chris Wilson]

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

Re: [PATCH 2/2] i915: do not leak module ref counter

[Sergey Senozhatsky]

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

Re: [PATCH 2/2] i915: do not leak module ref counter

[Sergey Senozhatsky]

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

Re: [PATCH 2/2] i915: do not leak module ref counter

[Chris Wilson]

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

Re: [PATCH 2/2] i915: do not leak module ref counter

[Sergey Senozhatsky]

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);

Re: [PATCH 2/2] i915: do not leak module ref counter

[Chris Wilson]

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

Re: [PATCH 1/2] i915: convert to new mount API

[Chris Wilson]

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

Re: [PATCH 1/2] i915: convert to new mount API

[Sergey Senozhatsky]

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