[PATCH] uprobes: Use file_inode()

[PATCH] uprobes: Use file_inode()

[Oleg Nesterov]

Cleanup. Now that we have f_inode/file_inode() we can use it
instead of ->f_mapping->host.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2bcd08e..5c273b3 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -758,7 +758,7 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
 		down_write(&mm->mmap_sem);
 		vma = find_vma(mm, info->vaddr);
 		if (!vma || !valid_vma(vma, is_register) ||
-		    vma->vm_file->f_mapping->host != uprobe->inode)
+		    file_inode(vma->vm_file) != uprobe->inode)
 			goto unlock;
 
 		if (vma->vm_start > info->vaddr ||
@@ -917,7 +917,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
 		loff_t offset;
 
 		if (!valid_vma(vma, false) ||
-		    vma->vm_file->f_mapping->host != uprobe->inode)
+		    file_inode(vma->vm_file) != uprobe->inode)
 			continue;
 
 		offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
@@ -1010,7 +1010,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	if (no_uprobe_events() || !valid_vma(vma, true))
 		return 0;
 
-	inode = vma->vm_file->f_mapping->host;
+	inode = file_inode(vma->vm_file);
 	if (!inode)
 		return 0;
 
@@ -1041,7 +1041,7 @@ vma_has_uprobes(struct vm_area_struct *vma, unsigned long start, unsigned long e
 	struct inode *inode;
 	struct rb_node *n;
 
-	inode = vma->vm_file->f_mapping->host;
+	inode = file_inode(vma->vm_file);
 
 	min = vaddr_to_offset(vma, start);
 	max = min + (end - start) - 1;
@@ -1465,7 +1465,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	vma = find_vma(mm, bp_vaddr);
 	if (vma && vma->vm_start <= bp_vaddr) {
 		if (valid_vma(vma, false)) {
-			struct inode *inode = vma->vm_file->f_mapping->host;
+			struct inode *inode = file_inode(vma->vm_file);
 			loff_t offset = vaddr_to_offset(vma, bp_vaddr);
 
 			uprobe = find_uprobe(inode, offset);
-- 
1.5.5.1

Re: [PATCH] uprobes: Use file_inode()

[Al Viro]

On Sun, Mar 17, 2013 at 07:00:36PM +0100, Oleg Nesterov wrote:
> Cleanup. Now that we have f_inode/file_inode() we can use it
> instead of ->f_mapping->host.

No.  This is *not* guaranteed to be the same thing in general; note that
e.g. for block devices ->f_mapping->host is *not* equal to file_inode().
It probably is valid in this particular case, but at the very least you
need to explain that in commit message, or soon we'll get the Knights of
Holy Commit Count(tm) crapping all over the tree, breaking stuff.

Re: [PATCH] uprobes: Use file_inode()

[Oleg Nesterov]

On 03/17, Al Viro wrote:
>
> On Sun, Mar 17, 2013 at 07:00:36PM +0100, Oleg Nesterov wrote:
> > Cleanup. Now that we have f_inode/file_inode() we can use it
> > instead of ->f_mapping->host.
>
> No.  This is *not* guaranteed to be the same thing in general; note that
> e.g. for block devices ->f_mapping->host is *not* equal to file_inode().

Yes,

> It probably is valid in this particular case,

And yes (I think). In fact I think ->f_mode is "more correct" in this case.
Say, if this uprobe was created by create_trace_uprobe() we use d_inode,
and uprobe_mmap/etc uses file_inode() only to compare this pointer with
uprobe->inode.

But I'll try to recheck, and:

> but at the very least you
> need to explain that in commit message,

OK. Will do, thanks.

Oleg.

[PATCH v2] uprobes: Use file_inode()

[Oleg Nesterov]

Cleanup. Now that we have f_inode/file_inode() we can use it instead
of ->f_mapping->host.

This should not make any difference for uprobes, but in theory this
change is more correct. We use this inode as a key, to compare it
with uprobe->inode set by uprobe_register(inode), and the caller uses
d_inode.

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

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 2bcd08e..5c273b3 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -758,7 +758,7 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
 		down_write(&mm->mmap_sem);
 		vma = find_vma(mm, info->vaddr);
 		if (!vma || !valid_vma(vma, is_register) ||
-		    vma->vm_file->f_mapping->host != uprobe->inode)
+		    file_inode(vma->vm_file) != uprobe->inode)
 			goto unlock;
 
 		if (vma->vm_start > info->vaddr ||
@@ -917,7 +917,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
 		loff_t offset;
 
 		if (!valid_vma(vma, false) ||
-		    vma->vm_file->f_mapping->host != uprobe->inode)
+		    file_inode(vma->vm_file) != uprobe->inode)
 			continue;
 
 		offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
@@ -1010,7 +1010,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
 	if (no_uprobe_events() || !valid_vma(vma, true))
 		return 0;
 
-	inode = vma->vm_file->f_mapping->host;
+	inode = file_inode(vma->vm_file);
 	if (!inode)
 		return 0;
 
@@ -1041,7 +1041,7 @@ vma_has_uprobes(struct vm_area_struct *vma, unsigned long start, unsigned long e
 	struct inode *inode;
 	struct rb_node *n;
 
-	inode = vma->vm_file->f_mapping->host;
+	inode = file_inode(vma->vm_file);
 
 	min = vaddr_to_offset(vma, start);
 	max = min + (end - start) - 1;
@@ -1465,7 +1465,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
 	vma = find_vma(mm, bp_vaddr);
 	if (vma && vma->vm_start <= bp_vaddr) {
 		if (valid_vma(vma, false)) {
-			struct inode *inode = vma->vm_file->f_mapping->host;
+			struct inode *inode = file_inode(vma->vm_file);
 			loff_t offset = vaddr_to_offset(vma, bp_vaddr);
 
 			uprobe = find_uprobe(inode, offset);
-- 
1.5.5.1

Re: [PATCH v2] uprobes: Use file_inode()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-03-18 19:43:17]:

> Cleanup. Now that we have f_inode/file_inode() we can use it instead
> of ->f_mapping->host.
> 
> This should not make any difference for uprobes, but in theory this
> change is more correct. We use this inode as a key, to compare it
> with uprobe->inode set by uprobe_register(inode), and the caller uses
> d_inode.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Looks good to me.

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

> ---
>  kernel/events/uprobes.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 2bcd08e..5c273b3 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -758,7 +758,7 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
>  		down_write(&mm->mmap_sem);
>  		vma = find_vma(mm, info->vaddr);
>  		if (!vma || !valid_vma(vma, is_register) ||
> -		    vma->vm_file->f_mapping->host != uprobe->inode)
> +		    file_inode(vma->vm_file) != uprobe->inode)
>  			goto unlock;
> 
>  		if (vma->vm_start > info->vaddr ||
> @@ -917,7 +917,7 @@ static int unapply_uprobe(struct uprobe *uprobe, struct mm_struct *mm)
>  		loff_t offset;
> 
>  		if (!valid_vma(vma, false) ||
> -		    vma->vm_file->f_mapping->host != uprobe->inode)
> +		    file_inode(vma->vm_file) != uprobe->inode)
>  			continue;
> 
>  		offset = (loff_t)vma->vm_pgoff << PAGE_SHIFT;
> @@ -1010,7 +1010,7 @@ int uprobe_mmap(struct vm_area_struct *vma)
>  	if (no_uprobe_events() || !valid_vma(vma, true))
>  		return 0;
> 

> -	inode = vma->vm_file->f_mapping->host;
> +	inode = file_inode(vma->vm_file);
>  	if (!inode)
>  		return 0;
> 
> @@ -1041,7 +1041,7 @@ vma_has_uprobes(struct vm_area_struct *vma, unsigned long start, unsigned long e
>  	struct inode *inode;
>  	struct rb_node *n;
> 
> -	inode = vma->vm_file->f_mapping->host;
> +	inode = file_inode(vma->vm_file);
> 
>  	min = vaddr_to_offset(vma, start);
>  	max = min + (end - start) - 1;
> @@ -1465,7 +1465,7 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
>  	vma = find_vma(mm, bp_vaddr);
>  	if (vma && vma->vm_start <= bp_vaddr) {
>  		if (valid_vma(vma, false)) {
> -			struct inode *inode = vma->vm_file->f_mapping->host;
> +			struct inode *inode = file_inode(vma->vm_file);
>  			loff_t offset = vaddr_to_offset(vma, bp_vaddr);
> 
>  			uprobe = find_uprobe(inode, offset);
> -- 
> 1.5.5.1
> 
> 

-- 
Thanks and Regards
Srikar Dronamraju