[PATCH v3 1/2] tracing: fix bad use of igrab in trace_uprobe.c

[PATCH v3 1/2] tracing: fix bad use of igrab in trace_uprobe.c

[Song Liu]

As Miklos reported and suggested:

  This pattern repeats two times in trace_uprobe.c and in
  kernel/events/core.c as well:

      ret = kern_path(filename, LOOKUP_FOLLOW, &path);
      if (ret)
          goto fail_address_parse;

      inode = igrab(d_inode(path.dentry));
      path_put(&path);

  And it's wrong.  You can only hold a reference to the inode if you
  have an active ref to the superblock as well (which is normally
  through path.mnt) or holding s_umount.

  This way unmounting the containing filesystem while the tracepoint is
  active will give you the "VFS: Busy inodes after unmount..." message
  and a crash when the inode is finally put.

  Solution: store path instead of inode.

This patch fixes two instances in trace_uprobe.c. struct path is added to
struct trace_uprobe to keep the inode and containing mount point
referenced.

Fixes: f3f096cfedf8 ("tracing: Provide trace events interface for uprobes")
Fixes: 33ea4b24277b ("perf/core: Implement the 'perf_uprobe' PMU")
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Howard McLauchlan <hmclauchlan@fb.com>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Reported-by: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Song Liu <songliubraving@fb.com>
---
 kernel/trace/trace_uprobe.c | 55 +++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 34fd0e0..e7ffb5e 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -55,6 +55,7 @@ struct trace_uprobe {
 	struct list_head		list;
 	struct trace_uprobe_filter	filter;
 	struct uprobe_consumer		consumer;
+	struct path			path;
 	struct inode			*inode;
 	char				*filename;
 	unsigned long			offset;
@@ -289,7 +290,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
 	for (i = 0; i < tu->tp.nr_args; i++)
 		traceprobe_free_probe_arg(&tu->tp.args[i]);
 
-	iput(tu->inode);
+	path_put(&tu->path);
 	kfree(tu->tp.call.class->system);
 	kfree(tu->tp.call.name);
 	kfree(tu->filename);
@@ -363,7 +364,6 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
 static int create_trace_uprobe(int argc, char **argv)
 {
 	struct trace_uprobe *tu;
-	struct inode *inode;
 	char *arg, *event, *group, *filename;
 	char buf[MAX_EVENT_NAME_LEN];
 	struct path path;
@@ -371,7 +371,6 @@ static int create_trace_uprobe(int argc, char **argv)
 	bool is_delete, is_return;
 	int i, ret;
 
-	inode = NULL;
 	ret = 0;
 	is_delete = false;
 	is_return = false;
@@ -437,24 +436,14 @@ static int create_trace_uprobe(int argc, char **argv)
 	}
 	/* Find the last occurrence, in case the path contains ':' too. */
 	arg = strrchr(argv[1], ':');
-	if (!arg) {
-		ret = -EINVAL;
-		goto fail_address_parse;
-	}
+	if (!arg)
+		return -EINVAL;
 
 	*arg++ = '\0';
 	filename = argv[1];
 	ret = kern_path(filename, LOOKUP_FOLLOW, &path);
 	if (ret)
-		goto fail_address_parse;
-
-	inode = igrab(d_real_inode(path.dentry));
-	path_put(&path);
-
-	if (!inode || !S_ISREG(inode->i_mode)) {
-		ret = -EINVAL;
-		goto fail_address_parse;
-	}
+		return ret;
 
 	ret = kstrtoul(arg, 0, &offset);
 	if (ret)
@@ -490,7 +479,7 @@ static int create_trace_uprobe(int argc, char **argv)
 		goto fail_address_parse;
 	}
 	tu->offset = offset;
-	tu->inode = inode;
+	tu->path = path;
 	tu->filename = kstrdup(filename, GFP_KERNEL);
 
 	if (!tu->filename) {
@@ -558,7 +547,7 @@ static int create_trace_uprobe(int argc, char **argv)
 	return ret;
 
 fail_address_parse:
-	iput(inode);
+	path_put(&path);
 
 	pr_info("Failed to parse address or file.\n");
 
@@ -922,7 +911,14 @@ probe_event_enable(struct trace_uprobe *tu, struct trace_event_file *file,
 		goto err_flags;
 
 	tu->consumer.filter = filter;
-	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
+	tu->inode = d_real_inode(tu->path.dentry);
+	if (!tu->inode || !S_ISREG(tu->inode->i_mode)) {
+		tu->inode = NULL;
+		ret = -EINVAL;
+		goto err_buffer;
+	}
+	ret = uprobe_register(tu->inode, tu->offset,
+			      &tu->consumer);
 	if (ret)
 		goto err_buffer;
 
@@ -966,7 +962,8 @@ probe_event_disable(struct trace_uprobe *tu, struct trace_event_file *file)
 
 	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
 
-	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
+	uprobe_unregister(d_inode(tu->path.dentry), tu->offset, &tu->consumer);
+	tu->inode = NULL;
 	tu->tp.flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
 
 	uprobe_buffer_disable();
@@ -1041,7 +1038,8 @@ static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
 	write_unlock(&tu->filter.rwlock);
 
 	if (!done)
-		return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
+		return uprobe_apply(d_inode(tu->path.dentry), tu->offset,
+				    &tu->consumer, false);
 
 	return 0;
 }
@@ -1073,7 +1071,8 @@ static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
 
 	err = 0;
 	if (!done) {
-		err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+		err = uprobe_apply(d_inode(tu->path.dentry),
+				   tu->offset, &tu->consumer, true);
 		if (err)
 			uprobe_perf_close(tu, event);
 	}
@@ -1337,7 +1336,6 @@ struct trace_event_call *
 create_local_trace_uprobe(char *name, unsigned long offs, bool is_return)
 {
 	struct trace_uprobe *tu;
-	struct inode *inode;
 	struct path path;
 	int ret;
 
@@ -1345,14 +1343,6 @@ create_local_trace_uprobe(char *name, unsigned long offs, bool is_return)
 	if (ret)
 		return ERR_PTR(ret);
 
-	inode = igrab(d_inode(path.dentry));
-	path_put(&path);
-
-	if (!inode || !S_ISREG(inode->i_mode)) {
-		iput(inode);
-		return ERR_PTR(-EINVAL);
-	}
-
 	/*
 	 * local trace_kprobes are not added to probe_list, so they are never
 	 * searched in find_trace_kprobe(). Therefore, there is no concern of
@@ -1364,11 +1354,12 @@ create_local_trace_uprobe(char *name, unsigned long offs, bool is_return)
 	if (IS_ERR(tu)) {
 		pr_info("Failed to allocate trace_uprobe.(%d)\n",
 			(int)PTR_ERR(tu));
+		path_put(&path);
 		return ERR_CAST(tu);
 	}
 
 	tu->offset = offs;
-	tu->inode = inode;
+	tu->path = path;
 	tu->filename = kstrdup(name, GFP_KERNEL);
 	init_trace_event_call(tu, &tu->tp.call);
 
-- 
2.9.5

[PATCH v3 2/2] tracing: remove igrab() iput() call from uprobes.c

[Song Liu]

Caller of uprobe_register is required to keep the inode and containing
mount point referenced.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Howard McLauchlan <hmclauchlan@fb.com>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: Song Liu <songliubraving@fb.com>
---
 kernel/events/uprobes.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index ce6848e..20486bb 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -491,7 +491,6 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
 	if (!uprobe)
 		return NULL;
 
-	uprobe->inode = igrab(inode);
 	uprobe->offset = offset;
 	init_rwsem(&uprobe->register_rwsem);
 	init_rwsem(&uprobe->consumer_rwsem);
@@ -502,7 +501,6 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
 	if (cur_uprobe) {
 		kfree(uprobe);
 		uprobe = cur_uprobe;
-		iput(inode);
 	}
 
 	return uprobe;
@@ -701,7 +699,6 @@ static void delete_uprobe(struct uprobe *uprobe)
 	rb_erase(&uprobe->rb_node, &uprobes_tree);
 	spin_unlock(&uprobes_treelock);
 	RB_CLEAR_NODE(&uprobe->rb_node); /* for uprobe_is_active() */
-	iput(uprobe->inode);
 	put_uprobe(uprobe);
 }
 
@@ -873,7 +870,8 @@ static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *u
  * tuple).  Creation refcount stops uprobe_unregister from freeing the
  * @uprobe even before the register operation is complete. Creation
  * refcount is released when the last @uc for the @uprobe
- * unregisters.
+ * unregisters. Caller of uprobe_register() is required to keep @inode
+ * (and the containing mount) referenced.
  *
  * Return errno if it cannot successully install probes
  * else return 0 (success)
-- 
2.9.5

Re: [PATCH v3 2/2] tracing: remove igrab() iput() call from uprobes.c

[Steven Rostedt]

On Fri, 20 Apr 2018 09:56:25 -0700
Song Liu <songliubraving@fb.com> wrote:

> Caller of uprobe_register is required to keep the inode and containing
> mount point referenced.

I would add a little more background to why this is the case. Also a
possible link to the conversation?

Link: http://lkml.kernel.org/r/CAELBmZB2XX=qEOLAdvGG4cPx4GEntcSnWQquJLUK1ongRj35cA@mail.gmail.com

?

-- Steve

> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Howard McLauchlan <hmclauchlan@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  kernel/events/uprobes.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index ce6848e..20486bb 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -491,7 +491,6 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
>  	if (!uprobe)
>  		return NULL;
>  
> -	uprobe->inode = igrab(inode);
>  	uprobe->offset = offset;
>  	init_rwsem(&uprobe->register_rwsem);
>  	init_rwsem(&uprobe->consumer_rwsem);
> @@ -502,7 +501,6 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
>  	if (cur_uprobe) {
>  		kfree(uprobe);
>  		uprobe = cur_uprobe;
> -		iput(inode);
>  	}
>  
>  	return uprobe;
> @@ -701,7 +699,6 @@ static void delete_uprobe(struct uprobe *uprobe)
>  	rb_erase(&uprobe->rb_node, &uprobes_tree);
>  	spin_unlock(&uprobes_treelock);
>  	RB_CLEAR_NODE(&uprobe->rb_node); /* for uprobe_is_active() */
> -	iput(uprobe->inode);
>  	put_uprobe(uprobe);
>  }
>  
> @@ -873,7 +870,8 @@ static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *u
>   * tuple).  Creation refcount stops uprobe_unregister from freeing the
>   * @uprobe even before the register operation is complete. Creation
>   * refcount is released when the last @uc for the @uprobe
> - * unregisters.
> + * unregisters. Caller of uprobe_register() is required to keep @inode
> + * (and the containing mount) referenced.
>   *
>   * Return errno if it cannot successully install probes
>   * else return 0 (success)

Re: [PATCH v3 1/2] tracing: fix bad use of igrab in trace_uprobe.c

[Steven Rostedt]

On Fri, 20 Apr 2018 09:56:24 -0700
Song Liu <songliubraving@fb.com> wrote:

> s Miklos reported and suggested:
> 
>   This pattern repeats two times in trace_uprobe.c and in
>   kernel/events/core.c as well:
> 
>       ret = kern_path(filename, LOOKUP_FOLLOW, &path);
>       if (ret)
>           goto fail_address_parse;
> 
>       inode = igrab(d_inode(path.dentry));
>       path_put(&path);
> 
>   And it's wrong.  You can only hold a reference to the inode if you
>   have an active ref to the superblock as well (which is normally
>   through path.mnt) or holding s_umount.
> 
>   This way unmounting the containing filesystem while the tracepoint is
>   active will give you the "VFS: Busy inodes after unmount..." message
>   and a crash when the inode is finally put.
> 
>   Solution: store path instead of inode.
> 
> This patch fixes two instances in trace_uprobe.c. struct path is added to
> struct trace_uprobe to keep the inode and containing mount point
> referenced.
> 
> Fixes: f3f096cfedf8 ("tracing: Provide trace events interface for uprobes")
> Fixes: 33ea4b24277b ("perf/core: Implement the 'perf_uprobe' PMU")
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Howard McLauchlan <hmclauchlan@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Reported-by: Miklos Szeredi <miklos@szeredi.hu>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---

Can I get an Acked-by or Reviewed-by from someone?

Thanks!

-- Steve

Re: [PATCH v3 1/2] tracing: fix bad use of igrab in trace_uprobe.c

[Miklos Szeredi]

On Fri, Apr 20, 2018 at 6:56 PM, Song Liu <songliubraving@fb.com> wrote:
> As Miklos reported and suggested:
>
>   This pattern repeats two times in trace_uprobe.c and in
>   kernel/events/core.c as well:
>
>       ret = kern_path(filename, LOOKUP_FOLLOW, &path);
>       if (ret)
>           goto fail_address_parse;
>
>       inode = igrab(d_inode(path.dentry));
>       path_put(&path);
>
>   And it's wrong.  You can only hold a reference to the inode if you
>   have an active ref to the superblock as well (which is normally
>   through path.mnt) or holding s_umount.
>
>   This way unmounting the containing filesystem while the tracepoint is
>   active will give you the "VFS: Busy inodes after unmount..." message
>   and a crash when the inode is finally put.
>
>   Solution: store path instead of inode.
>
> This patch fixes two instances in trace_uprobe.c. struct path is added to
> struct trace_uprobe to keep the inode and containing mount point
> referenced.
>
> Fixes: f3f096cfedf8 ("tracing: Provide trace events interface for uprobes")
> Fixes: 33ea4b24277b ("perf/core: Implement the 'perf_uprobe' PMU")
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Howard McLauchlan <hmclauchlan@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Reported-by: Miklos Szeredi <miklos@szeredi.hu>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  kernel/trace/trace_uprobe.c | 55 +++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 32 deletions(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 34fd0e0..e7ffb5e 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -55,6 +55,7 @@ struct trace_uprobe {
>         struct list_head                list;
>         struct trace_uprobe_filter      filter;
>         struct uprobe_consumer          consumer;
> +       struct path                     path;
>         struct inode                    *inode;
>         char                            *filename;
>         unsigned long                   offset;
> @@ -289,7 +290,7 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
>         for (i = 0; i < tu->tp.nr_args; i++)
>                 traceprobe_free_probe_arg(&tu->tp.args[i]);
>
> -       iput(tu->inode);
> +       path_put(&tu->path);
>         kfree(tu->tp.call.class->system);
>         kfree(tu->tp.call.name);
>         kfree(tu->filename);
> @@ -363,7 +364,6 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
>  static int create_trace_uprobe(int argc, char **argv)
>  {
>         struct trace_uprobe *tu;
> -       struct inode *inode;
>         char *arg, *event, *group, *filename;
>         char buf[MAX_EVENT_NAME_LEN];
>         struct path path;
> @@ -371,7 +371,6 @@ static int create_trace_uprobe(int argc, char **argv)
>         bool is_delete, is_return;
>         int i, ret;
>
> -       inode = NULL;
>         ret = 0;
>         is_delete = false;
>         is_return = false;
> @@ -437,24 +436,14 @@ static int create_trace_uprobe(int argc, char **argv)
>         }
>         /* Find the last occurrence, in case the path contains ':' too. */
>         arg = strrchr(argv[1], ':');
> -       if (!arg) {
> -               ret = -EINVAL;
> -               goto fail_address_parse;
> -       }
> +       if (!arg)
> +               return -EINVAL;
>
>         *arg++ = '\0';
>         filename = argv[1];
>         ret = kern_path(filename, LOOKUP_FOLLOW, &path);
>         if (ret)
> -               goto fail_address_parse;
> -
> -       inode = igrab(d_real_inode(path.dentry));
> -       path_put(&path);
> -
> -       if (!inode || !S_ISREG(inode->i_mode)) {

You probably should test for positive regular file here.  You can use
d_is_reg(path.dentry) for that, which actually checks both the above
conditions and ensures that latter you don't even need to test and
error out on the !inode case because that is simply impossible.

> -               ret = -EINVAL;
> -               goto fail_address_parse;
> -       }
> +               return ret;
>
>         ret = kstrtoul(arg, 0, &offset);
>         if (ret)
> @@ -490,7 +479,7 @@ static int create_trace_uprobe(int argc, char **argv)
>                 goto fail_address_parse;
>         }
>         tu->offset = offset;
> -       tu->inode = inode;
> +       tu->path = path;
>         tu->filename = kstrdup(filename, GFP_KERNEL);
>
>         if (!tu->filename) {
> @@ -558,7 +547,7 @@ static int create_trace_uprobe(int argc, char **argv)
>         return ret;
>
>  fail_address_parse:
> -       iput(inode);
> +       path_put(&path);
>
>         pr_info("Failed to parse address or file.\n");
>
> @@ -922,7 +911,14 @@ probe_event_enable(struct trace_uprobe *tu, struct trace_event_file *file,
>                 goto err_flags;
>
>         tu->consumer.filter = filter;
> -       ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
> +       tu->inode = d_real_inode(tu->path.dentry);
> +       if (!tu->inode || !S_ISREG(tu->inode->i_mode)) {
> +               tu->inode = NULL;
> +               ret = -EINVAL;
> +               goto err_buffer;
> +       }
> +       ret = uprobe_register(tu->inode, tu->offset,
> +                             &tu->consumer);

So this just becomes:
+       tu->inode = d_real_inode(tu->path.dentry);

No more changes necessary.


>         if (ret)
>                 goto err_buffer;
>
> @@ -966,7 +962,8 @@ probe_event_disable(struct trace_uprobe *tu, struct trace_event_file *file)
>
>         WARN_ON(!uprobe_filter_is_empty(&tu->filter));
>
> -       uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
> +       uprobe_unregister(d_inode(tu->path.dentry), tu->offset, &tu->consumer);

Huh?  This should also be using tu->inode.  Othewise what's the point
in storing it?

> +       tu->inode = NULL;
>         tu->tp.flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
>
>         uprobe_buffer_disable();
> @@ -1041,7 +1038,8 @@ static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
>         write_unlock(&tu->filter.rwlock);
>
>         if (!done)
> -               return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> +               return uprobe_apply(d_inode(tu->path.dentry), tu->offset,
> +                                   &tu->consumer, false);

And here.

>
>         return 0;
>  }
> @@ -1073,7 +1071,8 @@ static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
>
>         err = 0;
>         if (!done) {
> -               err = uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> +               err = uprobe_apply(d_inode(tu->path.dentry),
> +                                  tu->offset, &tu->consumer, true);

And here.

Just drop these hunks completely.  Using tu->inode is fine.

>                 if (err)
>                         uprobe_perf_close(tu, event);
>         }
> @@ -1337,7 +1336,6 @@ struct trace_event_call *
>  create_local_trace_uprobe(char *name, unsigned long offs, bool is_return)
>  {
>         struct trace_uprobe *tu;
> -       struct inode *inode;
>         struct path path;
>         int ret;
>
> @@ -1345,14 +1343,6 @@ create_local_trace_uprobe(char *name, unsigned long offs, bool is_return)
>         if (ret)
>                 return ERR_PTR(ret);
>
> -       inode = igrab(d_inode(path.dentry));
> -       path_put(&path);
> -
> -       if (!inode || !S_ISREG(inode->i_mode)) {
> -               iput(inode);
> -               return ERR_PTR(-EINVAL);
> -       }

And again, the test for being a regular file can stay here.

> -
>         /*
>          * local trace_kprobes are not added to probe_list, so they are never
>          * searched in find_trace_kprobe(). Therefore, there is no concern of
> @@ -1364,11 +1354,12 @@ create_local_trace_uprobe(char *name, unsigned long offs, bool is_return)
>         if (IS_ERR(tu)) {
>                 pr_info("Failed to allocate trace_uprobe.(%d)\n",
>                         (int)PTR_ERR(tu));
> +               path_put(&path);
>                 return ERR_CAST(tu);
>         }
>
>         tu->offset = offs;
> -       tu->inode = inode;
> +       tu->path = path;
>         tu->filename = kstrdup(name, GFP_KERNEL);
>         init_trace_event_call(tu, &tu->tp.call);
>
> --
> 2.9.5
>

Re: [PATCH v3 2/2] tracing: remove igrab() iput() call from uprobes.c

[Miklos Szeredi]

On Fri, Apr 20, 2018 at 6:56 PM, Song Liu <songliubraving@fb.com> wrote:
> Caller of uprobe_register is required to keep the inode and containing
> mount point referenced.
>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Howard McLauchlan <hmclauchlan@fb.com>
> Cc: Josef Bacik <jbacik@fb.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  kernel/events/uprobes.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index ce6848e..20486bb 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -491,7 +491,6 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
>         if (!uprobe)
>                 return NULL;
>
> -       uprobe->inode = igrab(inode);

Where has the assignment gone?

Testing your changes would not hurt...

Thanks,
Miklos

Re: [PATCH v3 2/2] tracing: remove igrab() iput() call from uprobes.c

[Song Liu]

> On Apr 23, 2018, at 3:03 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> 
> On Fri, Apr 20, 2018 at 6:56 PM, Song Liu <songliubraving@fb.com> wrote:
>> Caller of uprobe_register is required to keep the inode and containing
>> mount point referenced.
>> 
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Howard McLauchlan <hmclauchlan@fb.com>
>> Cc: Josef Bacik <jbacik@fb.com>
>> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Cc: Miklos Szeredi <miklos@szeredi.hu>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> kernel/events/uprobes.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
>> index ce6848e..20486bb 100644
>> --- a/kernel/events/uprobes.c
>> +++ b/kernel/events/uprobes.c
>> @@ -491,7 +491,6 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
>>        if (!uprobe)
>>                return NULL;
>> 
>> -       uprobe->inode = igrab(inode);
> 
> Where has the assignment gone?
> 
> Testing your changes would not hurt...
> 
> Thanks,
> Miklos

Oops.. I tested the trace_uprobe to uprobe part of it, but didn't test
the uprobe itself. (Tested) fix coming soon.

Song