[PATCH 0/7] uprobes: register/unregister preparations for filtering

[PATCH 0/7] uprobes: register/unregister preparations for filtering

[Oleg Nesterov]

Hello.

Srikar, it is not clear if you was convinced or not during the
last discussion. We will discuss this again probably. So far
I am sending the initial changes which (I think) make sense in
any case. Please review.

The next step is locking. Alas, we can not use consumer_rwsem
to protect the list in filter_chain(). Whatever we do, at least
uprobe_mmap() needs to do this under mm->mmap_sem. And we do
want to allow uc->handler() to play with current->mm.

Then we will actually immplement filter_chain(), this should be
trivial unless you insist we need for_each_mm_user() (cough, I
hope you do not ;).

Then we will try to solve the problem with fork.

Oleg.

[PATCH 1/7] uprobes: Move __set_bit(UPROBE_SKIP_SSTEP) into alloc_uprobe()

[Oleg Nesterov]

Cosmetic. __set_bit(UPROBE_SKIP_SSTEP) is the part of initialization,
it is not clear why it is set in insert_uprobe().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 5e22faf..e9f22ed 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -430,9 +430,6 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe)
 	u = __insert_uprobe(uprobe);
 	spin_unlock(&uprobes_treelock);
 
-	/* For now assume that the instruction need not be single-stepped */
-	__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
-
 	return u;
 }
 
@@ -454,6 +451,8 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
 	uprobe->offset = offset;
 	init_rwsem(&uprobe->consumer_rwsem);
 	mutex_init(&uprobe->copy_mutex);
+	/* For now assume that the instruction need not be single-stepped */
+	__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
 
 	/* add to uprobes_tree, sorted on inode:offset */
 	cur_uprobe = insert_uprobe(uprobe);
-- 
1.5.5.1

[PATCH 2/7] uprobes: Kill the "uprobe != NULL" check in uprobe_unregister()

[Oleg Nesterov]

Trivial. uprobe can't be NULL after mutex_unlock(), it was already used.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e9f22ed..13b247c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -900,8 +900,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
 	}
 
 	mutex_unlock(uprobes_hash(inode));
-	if (uprobe)
-		put_uprobe(uprobe);
+	put_uprobe(uprobe);
 }
 
 static struct rb_node *
-- 
1.5.5.1

[PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister

[Oleg Nesterov]

register/unregister verifies that inode/uc != NULL. For what?
This really looks like "hide the potential problem", the caller
should pass the valid data.

register() also checks uc->next == NULL, probably to prevent the
double-register but the caller can do other stupid/wrong things.
If we do this check, then we should document that uc->next should
be cleared before register() and add BUG_ON().

Also add the small comment about the i_size_read() check.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 13b247c..d8e930a 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -844,9 +844,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 	struct uprobe *uprobe;
 	int ret;
 
-	if (!inode || !uc || uc->next)
-		return -EINVAL;
-
+	/* Racy, just to catch the obvious mistakes */
 	if (offset > i_size_read(inode))
 		return -EINVAL;
 
@@ -883,9 +881,6 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
 {
 	struct uprobe *uprobe;
 
-	if (!inode || !uc)
-		return;
-
 	uprobe = find_uprobe(inode, offset);
 	if (!uprobe)
 		return;
-- 
1.5.5.1

[PATCH 4/7] uprobes: Kill uprobe_consumer->filter()

[Oleg Nesterov]

uprobe_consumer->filter() is pointless in its current form, kill it.

We will add it back, but with the different signature/semantics. Perhaps
we will even re-introduce the callsite in handler_chain(), but not to
just skip uc->handler().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/uprobes.h     |    5 -----
 kernel/events/uprobes.c     |    6 ++----
 kernel/trace/trace_uprobe.c |    1 -
 3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 4f628a6..83742b9 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -37,11 +37,6 @@ struct inode;
 
 struct uprobe_consumer {
 	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
-	/*
-	 * filter is optional; If a filter exists, handler is run
-	 * if and only if filter returns true.
-	 */
-	bool (*filter)(struct uprobe_consumer *self, struct task_struct *task);
 
 	struct uprobe_consumer *next;
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index d8e930a..e761974 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -477,10 +477,8 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 		return;
 
 	down_read(&uprobe->consumer_rwsem);
-	for (uc = uprobe->consumers; uc; uc = uc->next) {
-		if (!uc->filter || uc->filter(uc, current))
-			uc->handler(uc, regs);
-	}
+	for (uc = uprobe->consumers; uc; uc = uc->next)
+		uc->handler(uc, regs);
 	up_read(&uprobe->consumer_rwsem);
 }
 
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 03003cd..38eee92 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -546,7 +546,6 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 		return -EINTR;
 
 	utc->cons.handler = uprobe_dispatcher;
-	utc->cons.filter = NULL;
 	ret = uprobe_register(tu->inode, tu->offset, &utc->cons);
 	if (ret) {
 		kfree(utc);
-- 
1.5.5.1

[PATCH 5/7] uprobes: Introduce filter_chain()

[Oleg Nesterov]

Add the new helper filter_chain(). Currently it is only placeholder,
the comment explains what is should do. We will change it later to
consult every consumer to decide whether we need to install the swbp.
Until then it works as if any consumer returns true, this matches the
current behavior.

Change install_breakpoint() to call filter_chain() instead of checking
uprobe->consumers != NULL. We obviously need this, and this equally
closes the race with _unregister().

Change remove_breakpoint() to call this helper too. Currently this is
pointless because remove_breakpoint() is only called when the last
consumer goes away, but we will change this.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e761974..edc47ae 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -614,6 +614,18 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 	return ret;
 }
 
+static bool filter_chain(struct uprobe *uprobe)
+{
+	/*
+	 * TODO:
+	 *	for_each_consumer(uc)
+	 *		if (uc->filter(...))
+	 *			return true;
+	 *	return false;
+	 */
+	return uprobe->consumers != NULL;
+}
+
 static int
 install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 			struct vm_area_struct *vma, unsigned long vaddr)
@@ -624,11 +636,10 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 	/*
 	 * If probe is being deleted, unregister thread could be done with
 	 * the vma-rmap-walk through. Adding a probe now can be fatal since
-	 * nobody will be able to cleanup. Also we could be from fork or
-	 * mremap path, where the probe might have already been inserted.
-	 * Hence behave as if probe already existed.
+	 * nobody will be able to cleanup. But in this case filter_chain()
+	 * must return false, all consumers have gone away.
 	 */
-	if (!uprobe->consumers)
+	if (!filter_chain(uprobe))
 		return 0;
 
 	ret = prepare_uprobe(uprobe, vma->vm_file, mm, vaddr);
@@ -655,10 +666,12 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
 static int
 remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
 {
-	/* can happen if uprobe_register() fails */
 	if (!test_bit(MMF_HAS_UPROBES, &mm->flags))
 		return 0;
 
+	if (filter_chain(uprobe))
+		return 0;
+
 	set_bit(MMF_RECALC_UPROBES, &mm->flags);
 	return set_orig_insn(&uprobe->arch, mm, vaddr);
 }
@@ -1382,6 +1395,7 @@ static void mmf_recalc_uprobes(struct mm_struct *mm)
 		 * This is not strictly accurate, we can race with
 		 * uprobe_unregister() and see the already removed
 		 * uprobe if delete_uprobe() was not yet called.
+		 * Or this uprobe can be filtered out.
 		 */
 		if (vma_has_uprobes(vma, vma->vm_start, vma->vm_end))
 			return;
-- 
1.5.5.1

[PATCH 6/7] uprobes: _unregister() should always do register_for_each_vma(false)

[Oleg Nesterov]

uprobe_unregister() removes the breakpoints only if the last consumer
goes away. To support the filtering it should do this every time, we
want to remove the breakpoints which nobody else want to keep.

Note: given that filter_chain() is not actually implemented, this patch
itself doesn't change the behaviour yet, register_for_each_vma(false)
is a heavy "nop" unless there are no more consumers.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index edc47ae..7c98671 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -825,12 +825,20 @@ static int __uprobe_register(struct uprobe *uprobe)
 	return register_for_each_vma(uprobe, true);
 }
 
-static void __uprobe_unregister(struct uprobe *uprobe)
+static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
-	if (!register_for_each_vma(uprobe, false))
-		delete_uprobe(uprobe);
+	int err;
+
+	if (!consumer_del(uprobe, uc))	/* WARN? */
+		return;
 
-	/* TODO : cant unregister? schedule a worker thread */
+	err = register_for_each_vma(uprobe, false);
+	if (!uprobe->consumers) {
+		clear_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
+		/* TODO : cant unregister? schedule a worker thread */
+		if (!err)
+			delete_uprobe(uprobe);
+	}
 }
 
 /*
@@ -868,8 +876,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 	} else if (!consumer_add(uprobe, uc)) {
 		ret = __uprobe_register(uprobe);
 		if (ret) {
-			uprobe->consumers = NULL;
-			__uprobe_unregister(uprobe);
+			__uprobe_unregister(uprobe, uc);
 		} else {
 			set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
 		}
@@ -897,14 +904,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
 		return;
 
 	mutex_lock(uprobes_hash(inode));
-
-	if (consumer_del(uprobe, uc)) {
-		if (!uprobe->consumers) {
-			__uprobe_unregister(uprobe);
-			clear_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
-		}
-	}
-
+	__uprobe_unregister(uprobe, uc);
 	mutex_unlock(uprobes_hash(inode));
 	put_uprobe(uprobe);
 }
-- 
1.5.5.1

[PATCH 7/7] uprobes: _register() should always do register_for_each_vma(true)

[Oleg Nesterov]

To support the filtering uprobe_register() should do
register_for_each_vma(true) every time the new consumer comes,
we need to install the previously nacked breakpoints.

Note:
	- uprobes_mutex[] should die, what is actually protects is
	  alloc_uprobe().

	- UPROBE_RUN_HANDLER should die too, obviously it can't work
	  unless uprobe has a single consumer. The consumer should
	  serialize with _register/_unregister itself. Or this flag
	  should live in uprobe_consumer->state.

	- Perhaps we can do some optimizations later. For example, if
	  filter_chain() never returns false uprobe can record this
	  fact and avoid the unnecessary register_for_each_vma().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 7c98671..c80507d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -482,16 +482,12 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
 	up_read(&uprobe->consumer_rwsem);
 }
 
-/* Returns the previous consumer */
-static struct uprobe_consumer *
-consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
+static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
 	down_write(&uprobe->consumer_rwsem);
 	uc->next = uprobe->consumers;
 	uprobe->consumers = uc;
 	up_write(&uprobe->consumer_rwsem);
-
-	return uc->next;
 }
 
 /*
@@ -820,9 +816,15 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 	return err;
 }
 
-static int __uprobe_register(struct uprobe *uprobe)
+static int __uprobe_register(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
-	return register_for_each_vma(uprobe, true);
+	int err;
+
+	consumer_add(uprobe, uc);
+	err = register_for_each_vma(uprobe, true);
+	if (!err) /* TODO: pointless unless the first consumer */
+		set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
+	return err;
 }
 
 static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
@@ -867,21 +869,14 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 	if (offset > i_size_read(inode))
 		return -EINVAL;
 
-	ret = 0;
+	ret = -ENOMEM;
 	mutex_lock(uprobes_hash(inode));
 	uprobe = alloc_uprobe(inode, offset);
-
-	if (!uprobe) {
-		ret = -ENOMEM;
-	} else if (!consumer_add(uprobe, uc)) {
-		ret = __uprobe_register(uprobe);
-		if (ret) {
+	if (uprobe) {
+		ret = __uprobe_register(uprobe, uc);
+		if (ret)
 			__uprobe_unregister(uprobe, uc);
-		} else {
-			set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
-		}
 	}
-
 	mutex_unlock(uprobes_hash(inode));
 	if (uprobe)
 		put_uprobe(uprobe);
-- 
1.5.5.1

Re: [PATCH 5/7] uprobes: Introduce filter_chain()

[Oleg Nesterov]

On 11/23, Oleg Nesterov wrote:
>
> Change install_breakpoint() to call filter_chain() instead of checking
> uprobe->consumers != NULL. We obviously need this, and this equally
> closes the race with _unregister().
>
> Change remove_breakpoint() to call this helper too. Currently this is
> pointless because remove_breakpoint() is only called when the last
> consumer goes away, but we will change this.

Just in case...

This is only to make the initial change as simple as possible. filter_chain()
will have more arguments and more callers, say, perhaps build_map_info().
And perhaps these 2 callsites should be moved from install/remove to the
caller later.

Oleg.

Re: [PATCH 1/7] uprobes: Move __set_bit(UPROBE_SKIP_SSTEP) into alloc_uprobe()

[Srikar Dronamraju]

> Cosmetic. __set_bit(UPROBE_SKIP_SSTEP) is the part of initialization,
> it is not clear why it is set in insert_uprobe().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/events/uprobes.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 5e22faf..e9f22ed 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -430,9 +430,6 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe)
>  	u = __insert_uprobe(uprobe);
>  	spin_unlock(&uprobes_treelock);
> 
> -	/* For now assume that the instruction need not be single-stepped */
> -	__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
> -
>  	return u;
>  }
> 
> @@ -454,6 +451,8 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
>  	uprobe->offset = offset;
>  	init_rwsem(&uprobe->consumer_rwsem);
>  	mutex_init(&uprobe->copy_mutex);
> +	/* For now assume that the instruction need not be single-stepped */
> +	__set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
> 
>  	/* add to uprobes_tree, sorted on inode:offset */
>  	cur_uprobe = insert_uprobe(uprobe);
> -- 
> 1.5.5.1
> 

Re: [PATCH 2/7] uprobes: Kill the "uprobe != NULL" check in uprobe_unregister()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:02]:

> Trivial. uprobe can't be NULL after mutex_unlock(), it was already used.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/events/uprobes.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index e9f22ed..13b247c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -900,8 +900,7 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
>  	}
> 
>  	mutex_unlock(uprobes_hash(inode));
> -	if (uprobe)
> -		put_uprobe(uprobe);
> +	put_uprobe(uprobe);
>  }
> 
>  static struct rb_node *
> -- 
> 1.5.5.1
> 

Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:06]:

> register/unregister verifies that inode/uc != NULL. For what?
> This really looks like "hide the potential problem", the caller
> should pass the valid data.
> 

Agree that users should pass valid data.
I do understand that we expect the users to be knowledge-able. 
Also users are routed thro in-kernel api that does this check.

However from an api perspective, if a user passes invalid data, do we
want the system to crash.
Esp if kernel can identify that users has indeed passed wrong info. I do agree
that users can still pass invalid data that kernel maynot be able to
identify in most cases.


> register() also checks uc->next == NULL, probably to prevent the
> double-register but the caller can do other stupid/wrong things.

Users can surely do more stupid things. But this is again something that
kernel can identify. By allowing a double-register of a consumer, thats
already registered, we might end up allowing circular loop of consumers.

> If we do this check, then we should document that uc->next should
> be cleared before register() and add BUG_ON().
> 
> Also add the small comment about the i_size_read() check.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 13b247c..d8e930a 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -844,9 +844,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
>  	struct uprobe *uprobe;
>  	int ret;
> 
> -	if (!inode || !uc || uc->next)
> -		return -EINVAL;
> -
> +	/* Racy, just to catch the obvious mistakes */
>  	if (offset > i_size_read(inode))
>  		return -EINVAL;
> 
> @@ -883,9 +881,6 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
>  {
>  	struct uprobe *uprobe;
> 
> -	if (!inode || !uc)
> -		return;
> -
>  	uprobe = find_uprobe(inode, offset);
>  	if (!uprobe)
>  		return;
> -- 
> 1.5.5.1
> 

Re: [PATCH 4/7] uprobes: Kill uprobe_consumer->filter()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:10]:

> uprobe_consumer->filter() is pointless in its current form, kill it.
> 
> We will add it back, but with the different signature/semantics. Perhaps
> we will even re-introduce the callsite in handler_chain(), but not to
> just skip uc->handler().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  include/linux/uprobes.h     |    5 -----
>  kernel/events/uprobes.c     |    6 ++----
>  kernel/trace/trace_uprobe.c |    1 -
>  3 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 4f628a6..83742b9 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -37,11 +37,6 @@ struct inode;
> 
>  struct uprobe_consumer {
>  	int (*handler)(struct uprobe_consumer *self, struct pt_regs *regs);
> -	/*
> -	 * filter is optional; If a filter exists, handler is run
> -	 * if and only if filter returns true.
> -	 */
> -	bool (*filter)(struct uprobe_consumer *self, struct task_struct *task);
> 
>  	struct uprobe_consumer *next;
>  };
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index d8e930a..e761974 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -477,10 +477,8 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
>  		return;
> 
>  	down_read(&uprobe->consumer_rwsem);
> -	for (uc = uprobe->consumers; uc; uc = uc->next) {
> -		if (!uc->filter || uc->filter(uc, current))
> -			uc->handler(uc, regs);
> -	}
> +	for (uc = uprobe->consumers; uc; uc = uc->next)
> +		uc->handler(uc, regs);
>  	up_read(&uprobe->consumer_rwsem);
>  }
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 03003cd..38eee92 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -546,7 +546,6 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
>  		return -EINTR;
> 
>  	utc->cons.handler = uprobe_dispatcher;
> -	utc->cons.filter = NULL;
>  	ret = uprobe_register(tu->inode, tu->offset, &utc->cons);
>  	if (ret) {
>  		kfree(utc);
> -- 
> 1.5.5.1
> 

Re: [PATCH 5/7] uprobes: Introduce filter_chain()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:12]:

> Add the new helper filter_chain(). Currently it is only placeholder,
> the comment explains what is should do. We will change it later to
> consult every consumer to decide whether we need to install the swbp.
> Until then it works as if any consumer returns true, this matches the
> current behavior.
> 
> Change install_breakpoint() to call filter_chain() instead of checking
> uprobe->consumers != NULL. We obviously need this, and this equally
> closes the race with _unregister().
> 
> Change remove_breakpoint() to call this helper too. Currently this is
> pointless because remove_breakpoint() is only called when the last
> consumer goes away, but we will change this.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/events/uprobes.c |   24 +++++++++++++++++++-----
>  1 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index e761974..edc47ae 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -614,6 +614,18 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
>  	return ret;
>  }
> 
> +static bool filter_chain(struct uprobe *uprobe)
> +{
> +	/*
> +	 * TODO:
> +	 *	for_each_consumer(uc)
> +	 *		if (uc->filter(...))
> +	 *			return true;
> +	 *	return false;
> +	 */
> +	return uprobe->consumers != NULL;
> +}
> +
>  static int
>  install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  			struct vm_area_struct *vma, unsigned long vaddr)
> @@ -624,11 +636,10 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  	/*
>  	 * If probe is being deleted, unregister thread could be done with
>  	 * the vma-rmap-walk through. Adding a probe now can be fatal since
> -	 * nobody will be able to cleanup. Also we could be from fork or
> -	 * mremap path, where the probe might have already been inserted.
> -	 * Hence behave as if probe already existed.
> +	 * nobody will be able to cleanup. But in this case filter_chain()
> +	 * must return false, all consumers have gone away.
>  	 */
> -	if (!uprobe->consumers)
> +	if (!filter_chain(uprobe))
>  		return 0;
> 
>  	ret = prepare_uprobe(uprobe, vma->vm_file, mm, vaddr);
> @@ -655,10 +666,12 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
>  static int
>  remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
>  {
> -	/* can happen if uprobe_register() fails */
>  	if (!test_bit(MMF_HAS_UPROBES, &mm->flags))
>  		return 0;
> 
> +	if (filter_chain(uprobe))
> +		return 0;
> +
>  	set_bit(MMF_RECALC_UPROBES, &mm->flags);
>  	return set_orig_insn(&uprobe->arch, mm, vaddr);
>  }
> @@ -1382,6 +1395,7 @@ static void mmf_recalc_uprobes(struct mm_struct *mm)
>  		 * This is not strictly accurate, we can race with
>  		 * uprobe_unregister() and see the already removed
>  		 * uprobe if delete_uprobe() was not yet called.
> +		 * Or this uprobe can be filtered out.
>  		 */
>  		if (vma_has_uprobes(vma, vma->vm_start, vma->vm_end))
>  			return;
> -- 
> 1.5.5.1
> 

Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister

[Oleg Nesterov]

On 12/10, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:06]:
>
> > register/unregister verifies that inode/uc != NULL. For what?
> > This really looks like "hide the potential problem", the caller
> > should pass the valid data.
> >
>
> Agree that users should pass valid data.
> I do understand that we expect the users to be knowledge-able.
> Also users are routed thro in-kernel api that does this check.
>
> However from an api perspective, if a user passes invalid data, do we
> want the system to crash.
>
> Esp if kernel can identify that users has indeed passed wrong info. I do agree
> that users can still pass invalid data that kernel maynot be able to
> identify in most cases.

inode != NULL can't verify that it actually points to the valid inode,
NULL is only one example of invalid data.

I agree, sometimes it makes sense to protect against the stupid mistakes,
but if we want to check against NULL we should do

	if (WARN_ON(!inode))
		return;

Especially in uprobe_unregister(). The current code is really "hide
the possible problem" and nothing more. It is better to crash imho
than silently return.

> > register() also checks uc->next == NULL, probably to prevent the
> > double-register but the caller can do other stupid/wrong things.
>
> Users can surely do more stupid things. But this is again something that
> kernel can identify. By allowing a double-register of a consumer, thats
> already registered, we might end up allowing circular loop of consumers.

I understand. But in this case we should document that uc->next must
be cleared before uprobe_register(). Or add init_consumer().

And we should change uprobe_unregister() to clear uc->next as well.
I think that the code like this

	uprobe_register(uc);
	uprobe_unregister(uc);

	uprobe_register(uc);

should work. Currently it doesn't because of this check.


So I still think these checks are pointless and (at least in unregister)
even harmful.

But I won't insist too much, I can drop this patch if you do not change
your mind.

Oleg.

Re: [PATCH 7/7] uprobes: _register() should always do register_for_each_vma(true)

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:17]:

> To support the filtering uprobe_register() should do
> register_for_each_vma(true) every time the new consumer comes,
> we need to install the previously nacked breakpoints.
> 
> Note:
> 	- uprobes_mutex[] should die, what is actually protects is
> 	  alloc_uprobe().
> 
> 	- UPROBE_RUN_HANDLER should die too, obviously it can't work
> 	  unless uprobe has a single consumer. The consumer should
> 	  serialize with _register/_unregister itself. Or this flag
> 	  should live in uprobe_consumer->state.
> 
> 	- Perhaps we can do some optimizations later. For example, if
> 	  filter_chain() never returns false uprobe can record this
> 	  fact and avoid the unnecessary register_for_each_vma().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/events/uprobes.c |   31 +++++++++++++------------------
>  1 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 7c98671..c80507d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -482,16 +482,12 @@ static void handler_chain(struct uprobe *uprobe, struct pt_regs *regs)
>  	up_read(&uprobe->consumer_rwsem);
>  }
> 
> -/* Returns the previous consumer */
> -static struct uprobe_consumer *
> -consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
> +static void consumer_add(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
>  	down_write(&uprobe->consumer_rwsem);
>  	uc->next = uprobe->consumers;
>  	uprobe->consumers = uc;
>  	up_write(&uprobe->consumer_rwsem);
> -
> -	return uc->next;
>  }
> 
>  /*
> @@ -820,9 +816,15 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
>  	return err;
>  }
> 
> -static int __uprobe_register(struct uprobe *uprobe)
> +static int __uprobe_register(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
> -	return register_for_each_vma(uprobe, true);
> +	int err;
> +
> +	consumer_add(uprobe, uc);
> +	err = register_for_each_vma(uprobe, true);
> +	if (!err) /* TODO: pointless unless the first consumer */
> +		set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
> +	return err;
>  }
> 
>  static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> @@ -867,21 +869,14 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
>  	if (offset > i_size_read(inode))
>  		return -EINVAL;
> 
> -	ret = 0;
> +	ret = -ENOMEM;
>  	mutex_lock(uprobes_hash(inode));
>  	uprobe = alloc_uprobe(inode, offset);
> -
> -	if (!uprobe) {
> -		ret = -ENOMEM;
> -	} else if (!consumer_add(uprobe, uc)) {
> -		ret = __uprobe_register(uprobe);
> -		if (ret) {
> +	if (uprobe) {
> +		ret = __uprobe_register(uprobe, uc);
> +		if (ret)
>  			__uprobe_unregister(uprobe, uc);
> -		} else {
> -			set_bit(UPROBE_RUN_HANDLER, &uprobe->flags);
> -		}
>  	}
> -
>  	mutex_unlock(uprobes_hash(inode));
>  	if (uprobe)
>  		put_uprobe(uprobe);
> -- 
> 1.5.5.1
> 

Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-12-10 20:12:32]:

> On 12/10, Srikar Dronamraju wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:06]:
> >
> > > register/unregister verifies that inode/uc != NULL. For what?
> > > This really looks like "hide the potential problem", the caller
> > > should pass the valid data.
> > >
> >
> > Agree that users should pass valid data.
> > I do understand that we expect the users to be knowledge-able.
> > Also users are routed thro in-kernel api that does this check.
> >
> > However from an api perspective, if a user passes invalid data, do we
> > want the system to crash.
> >
> > Esp if kernel can identify that users has indeed passed wrong info. I do agree
> > that users can still pass invalid data that kernel maynot be able to
> > identify in most cases.
> 
> inode != NULL can't verify that it actually points to the valid inode,
> NULL is only one example of invalid data.
> 
> I agree, sometimes it makes sense to protect against the stupid mistakes,
> but if we want to check against NULL we should do
> 
> 	if (WARN_ON(!inode))
> 		return;
> 

agree, that warn_on is better than a simple check 

> Especially in uprobe_unregister(). The current code is really "hide
> the possible problem" and nothing more. It is better to crash imho
> than silently return.
> 
> > > register() also checks uc->next == NULL, probably to prevent the
> > > double-register but the caller can do other stupid/wrong things.
> >
> > Users can surely do more stupid things. But this is again something that
> > kernel can identify. By allowing a double-register of a consumer, thats
> > already registered, we might end up allowing circular loop of consumers.
> 
> I understand. But in this case we should document that uc->next must
> be cleared before uprobe_register(). Or add init_consumer().
> 
> And we should change uprobe_unregister() to clear uc->next as well.
> I think that the code like this
> 
> 	uprobe_register(uc);
> 	uprobe_unregister(uc);
> 
> 	uprobe_register(uc);
> 
> should work. Currently it doesn't because of this check.
> 

yes, these should work and makes a case to nullify ->next on unregister. 

However, what if someone tries

	uprobe_register(uc1);
	uprobe_register(uc2);
	uprobe_register(uc1);

i.e somebody tries to re-register uc1, while its active and has a valid
next. After the re-registration of uc1, the uprobe->consumers will no more reference uc2.

Should we leave this case as a fool shooting himself?
 
> So I still think these checks are pointless and (at least in unregister)
> even harmful.
> 

-- 
thanks and regards
Srikar

Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister

[Oleg Nesterov]

On 12/13, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-12-10 20:12:32]:
>
> > On 12/10, Srikar Dronamraju wrote:
> > >
> > > * Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:06]:
> > >
> > > > register/unregister verifies that inode/uc != NULL. For what?
> > > > This really looks like "hide the potential problem", the caller
> > > > should pass the valid data.
> > > >
> > >
> > > Agree that users should pass valid data.
> > > I do understand that we expect the users to be knowledge-able.
> > > Also users are routed thro in-kernel api that does this check.
> > >
> > > However from an api perspective, if a user passes invalid data, do we
> > > want the system to crash.
> > >
> > > Esp if kernel can identify that users has indeed passed wrong info. I do agree
> > > that users can still pass invalid data that kernel maynot be able to
> > > identify in most cases.
> >
> > inode != NULL can't verify that it actually points to the valid inode,
> > NULL is only one example of invalid data.
> >
> > I agree, sometimes it makes sense to protect against the stupid mistakes,
> > but if we want to check against NULL we should do
> >
> > 	if (WARN_ON(!inode))
> > 		return;
> >
>
> agree, that warn_on is better than a simple check

and this one

	if (WARN_ON(inode < PAGE_OFFSET))

is even better ;)

> > Especially in uprobe_unregister(). The current code is really "hide
> > the possible problem" and nothing more. It is better to crash imho
> > than silently return.
> >
> > > > register() also checks uc->next == NULL, probably to prevent the
> > > > double-register but the caller can do other stupid/wrong things.
> > >
> > > Users can surely do more stupid things. But this is again something that
> > > kernel can identify. By allowing a double-register of a consumer, thats
> > > already registered, we might end up allowing circular loop of consumers.
> >
> > I understand. But in this case we should document that uc->next must
> > be cleared before uprobe_register(). Or add init_consumer().
> >
> > And we should change uprobe_unregister() to clear uc->next as well.
> > I think that the code like this
> >
> > 	uprobe_register(uc);
> > 	uprobe_unregister(uc);
> >
> > 	uprobe_register(uc);
> >
> > should work. Currently it doesn't because of this check.
> >
>
> yes, these should work and makes a case to nullify ->next on unregister.
>
> However, what if someone tries
>
> 	uprobe_register(uc1);
> 	uprobe_register(uc2);
> 	uprobe_register(uc1);
>
> i.e somebody tries to re-register uc1, while its active and has a valid
> next. After the re-registration of uc1, the uprobe->consumers will no more reference uc2.

Yes. And even without uprobe_register(uc2) the result won't be good.
This is like list_add(node).

> Should we leave this case as a fool shooting himself?

IMHO yes, or we should create init_consumer() or at least document that
the private ->next member should be nullified.

But let me repeat,

> > So I still think these checks are pointless and (at least in unregister)
> > even harmful.

Yes, but I am not going to argue if you want to keep these checks.

Oleg.

Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister

[Srikar Dronamraju]

> > >
> > > I agree, sometimes it makes sense to protect against the stupid mistakes,
> > > but if we want to check against NULL we should do
> > >
> > > 	if (WARN_ON(!inode))
> > > 		return;
> > >
> >
> > agree, that warn_on is better than a simple check
> 
> and this one
> 
> 	if (WARN_ON(inode < PAGE_OFFSET))
> 
> is even better ;)

Okay.

> 
> > > Especially in uprobe_unregister(). The current code is really "hide
> > > the possible problem" and nothing more. It is better to crash imho
> > > than silently return.
> > >
> > > > > register() also checks uc->next == NULL, probably to prevent the
> > > > > double-register but the caller can do other stupid/wrong things.
> > > >
> > > > Users can surely do more stupid things. But this is again something that
> > > > kernel can identify. By allowing a double-register of a consumer, thats
> > > > already registered, we might end up allowing circular loop of consumers.
> > >
> > > I understand. But in this case we should document that uc->next must
> > > be cleared before uprobe_register(). Or add init_consumer().
> > >
> > > And we should change uprobe_unregister() to clear uc->next as well.
> > > I think that the code like this
> > >
> > > 	uprobe_register(uc);
> > > 	uprobe_unregister(uc);
> > >
> > > 	uprobe_register(uc);
> > >
> > > should work. Currently it doesn't because of this check.
> > >
> >
> > yes, these should work and makes a case to nullify ->next on unregister.
> >
> > However, what if someone tries
> >
> > 	uprobe_register(uc1);
> > 	uprobe_register(uc2);
> > 	uprobe_register(uc1);
> >
> > i.e somebody tries to re-register uc1, while its active and has a valid
> > next. After the re-registration of uc1, the uprobe->consumers will no more reference uc2.
> 
> Yes. And even without uprobe_register(uc2) the result won't be good.
> This is like list_add(node).
> 
> > Should we leave this case as a fool shooting himself?
> 
> IMHO yes, or we should create init_consumer() or at least document that
> the private ->next member should be nullified.
> 

Okay, Since we agree that its a user mistake. So lets document this and
continue with what you propose.

Re: [PATCH 3/7] uprobes: Kill the pointless inode/uc checks in register/unregister

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2012-11-23 21:28:06]:

> register/unregister verifies that inode/uc != NULL. For what?
> This really looks like "hide the potential problem", the caller
> should pass the valid data.
> 
> register() also checks uc->next == NULL, probably to prevent the
> double-register but the caller can do other stupid/wrong things.
> If we do this check, then we should document that uc->next should
> be cleared before register() and add BUG_ON().
> 
> Also add the small comment about the i_size_read() check.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

We might want to add a warn .. but that shouldnt stop this patch though

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/events/uprobes.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 13b247c..d8e930a 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -844,9 +844,7 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
>  	struct uprobe *uprobe;
>  	int ret;
> 
> -	if (!inode || !uc || uc->next)
> -		return -EINVAL;
> -
> +	/* Racy, just to catch the obvious mistakes */
>  	if (offset > i_size_read(inode))
>  		return -EINVAL;
> 
> @@ -883,9 +881,6 @@ void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consume
>  {
>  	struct uprobe *uprobe;
> 
> -	if (!inode || !uc)
> -		return;
> -
>  	uprobe = find_uprobe(inode, offset);
>  	if (!uprobe)
>  		return;
> -- 
> 1.5.5.1
>