uprobe: checking probe event include directory

uprobe: checking probe event include directory

[Jovi Zhang]

>From 16ed13ee9098ae01705e8456005d1ad6d9909128 Mon Sep 17 00:00:00 2001
From: Jovi Zhang <bookjovi@gmail.com>
Date: Wed, 18 Jul 2012 01:16:23 +0800
Subject: [PATCH] uprobe: checking probe event include directory

Currently below command run successful:
$ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events

this don't make sense, because /bin is a directory,
so make this checking earily, not report error untill probe enable.

Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
---
 kernel/trace/trace_uprobe.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..cf382de 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,11 @@ static int create_trace_uprobe(int argc, char **argv)
 		goto fail_address_parse;

 	inode = igrab(path.dentry->d_inode);
+	 if (S_ISDIR(inode->i_mode)) {
+		ret = -EINVAL;
+		pr_info("probe file cannot be directory.\n");
+		goto fail_address_parse;
+	}

 	argc -= 2;
 	argv += 2;
-- 
1.7.9.7

Re: uprobe: checking probe event include directory

[Frederic Weisbecker]

On Tue, Jul 17, 2012 at 06:12:28PM +0800, Jovi Zhang wrote:
> From 16ed13ee9098ae01705e8456005d1ad6d9909128 Mon Sep 17 00:00:00 2001
> From: Jovi Zhang <bookjovi@gmail.com>
> Date: Wed, 18 Jul 2012 01:16:23 +0800
> Subject: [PATCH] uprobe: checking probe event include directory
> 
> Currently below command run successful:
> $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
> 
> this don't make sense, because /bin is a directory,
> so make this checking earily, not report error untill probe enable.
> 
> Signed-off-by: Jovi Zhang <bookjovi@gmail.com>

Adding Srikar in Cc.

> ---
>  kernel/trace/trace_uprobe.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 85158fa..cf382de 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -259,6 +259,11 @@ static int create_trace_uprobe(int argc, char **argv)
>  		goto fail_address_parse;
> 
>  	inode = igrab(path.dentry->d_inode);
> +	 if (S_ISDIR(inode->i_mode)) {
> +		ret = -EINVAL;
> +		pr_info("probe file cannot be directory.\n");
> +		goto fail_address_parse;
> +	}
> 
>  	argc -= 2;
>  	argv += 2;
> -- 
> 1.7.9.7

Re: uprobe: checking probe event include directory

[Srikar Dronamraju]

* Frederic Weisbecker <fweisbec@gmail.com> [2012-07-17 12:59:39]:

> On Tue, Jul 17, 2012 at 06:12:28PM +0800, Jovi Zhang wrote:
> > From 16ed13ee9098ae01705e8456005d1ad6d9909128 Mon Sep 17 00:00:00 2001
> > From: Jovi Zhang <bookjovi@gmail.com>
> > Date: Wed, 18 Jul 2012 01:16:23 +0800
> > Subject: [PATCH] uprobe: checking probe event include directory
> > 
> > Currently below command run successful:
> > $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events

good catch.

> > 
> > this don't make sense, because /bin is a directory,
> > so make this checking earily, not report error untill probe enable.
> > 
> > Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
> 
> Adding Srikar in Cc.
> 
> > ---
> >  kernel/trace/trace_uprobe.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index 85158fa..cf382de 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -259,6 +259,11 @@ static int create_trace_uprobe(int argc, char **argv)
> >  		goto fail_address_parse;
> > 
> >  	inode = igrab(path.dentry->d_inode);
> > +	 if (S_ISDIR(inode->i_mode)) {

How about checking for regular files but not directory.
i.e it should avoid tracing special files 

Something like: 

	if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {

> > +		ret = -EINVAL;
> > +		pr_info("probe file cannot be directory.\n");

we could drop the pr_info line here, since we would any print we
failed to parse the address.

Probably you could change the last pr_info from 

pr_info("Failed to parse address.\n");

to 

pr_info("Failed to parse address or file.\n");


-- 
Thanks and Regards
Srikar
> > +		goto fail_address_parse;
> > +	}
> > 
> >  	argc -= 2;
> >  	argv += 2;
> > -- 
> > 1.7.9.7
> 

Re: uprobe: checking probe event include directory

[Jovi Zhang]

On Wed, Jul 18, 2012 at 1:27 AM, Srikar Dronamraju
<srikar@linux.vnet.ibm.com> wrote:
> * Frederic Weisbecker <fweisbec@gmail.com> [2012-07-17 12:59:39]:
>
>> On Tue, Jul 17, 2012 at 06:12:28PM +0800, Jovi Zhang wrote:
>> > From 16ed13ee9098ae01705e8456005d1ad6d9909128 Mon Sep 17 00:00:00 2001
>> > From: Jovi Zhang <bookjovi@gmail.com>
>> > Date: Wed, 18 Jul 2012 01:16:23 +0800
>> > Subject: [PATCH] uprobe: checking probe event include directory
>> >
>> > Currently below command run successful:
>> > $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
>
> good catch.
>
>> >
>> > this don't make sense, because /bin is a directory,
>> > so make this checking earily, not report error untill probe enable.
>> >
>> > Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
>>
>> Adding Srikar in Cc.
>>
>> > ---
>> >  kernel/trace/trace_uprobe.c |    5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> > index 85158fa..cf382de 100644
>> > --- a/kernel/trace/trace_uprobe.c
>> > +++ b/kernel/trace/trace_uprobe.c
>> > @@ -259,6 +259,11 @@ static int create_trace_uprobe(int argc, char **argv)
>> >             goto fail_address_parse;
>> >
>> >     inode = igrab(path.dentry->d_inode);
>> > +    if (S_ISDIR(inode->i_mode)) {
>
> How about checking for regular files but not directory.
> i.e it should avoid tracing special files
>
> Something like:
>
>         if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
>
>> > +           ret = -EINVAL;
>> > +           pr_info("probe file cannot be directory.\n");
>
> we could drop the pr_info line here, since we would any print we
> failed to parse the address.
>
> Probably you could change the last pr_info from
>
> pr_info("Failed to parse address.\n");
>
> to
>
> pr_info("Failed to parse address or file.\n");
>
>

Thanks srikar, try below patch:

>From dd37d3cc563e2619ec325d6c11d769a32def411b Mon Sep 17 00:00:00 2001
From: Jovi Zhang <bookjovi@gmail.com>
Date: Wed, 18 Jul 2012 18:16:44 +0800
Subject: [PATCH] uprobe: checking uprobe event inode valid

Currently below command run successful:
$ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events

this don't make sense, because /bin is a directory,
we need to check uprobe event inode valid more earily.

Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 kernel/trace/trace_uprobe.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..3b5f646 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
 		goto fail_address_parse;

 	inode = igrab(path.dentry->d_inode);
+	 if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
+		ret = -EINVAL;
+		goto fail_address_parse;
+	}

 	argc -= 2;
 	argv += 2;
@@ -358,7 +362,7 @@ fail_address_parse:
 	if (inode)
 		iput(inode);

-	pr_info("Failed to parse address.\n");
+	pr_info("Failed to parse address or file.\n");

 	return ret;
 }
-- 
1.7.9.7

Re: [offlist] uprobe: checking probe event include directory

[Jovi Zhang]

On Wed, Jul 18, 2012 at 7:07 PM, Srikar Dronamraju
<srikar@linux.vnet.ibm.com> wrote:
> The patch looks good,
>
> Can you modify the description a bit. However you are free to ignore
> these comments. After knowing your response,  I will ack the patch.
>
> I would probably put this as:
>
> The subject could be
> tracing: Verify target file before registering a uprobe event
>
> Description:
> Without this patch, we can register a uprobe event for a directory.
> Enabling such a uprobe event would anyway fail.
>
> Example:
>
> $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
>
> However directories cannot be valid targets for uprobe.
> Hence verify if the target is a regular file during the probe
> registration.

Thanks srikar, your description is more clear than mine.


>From fd5077196038cc271e2116e1fca359a0011e1669 Mon Sep 17 00:00:00 2001
From: Jovi Zhang <bookjovi@gmail.com>
Date: Wed, 18 Jul 2012 18:16:44 +0800
Subject: [PATCH] tracing: Verify target file before registering a uprobe
 event

Without this patch, we can register a uprobe event for a directory.
Enabling such a uprobe event would anyway fail.

Example:
$ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events

However dirctories cannot be valid targets for uprobe.
Hence verify if the target is a regular file during the probe
registration.

Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 kernel/trace/trace_uprobe.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 85158fa..3b5f646 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
 		goto fail_address_parse;

 	inode = igrab(path.dentry->d_inode);
+	 if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
+		ret = -EINVAL;
+		goto fail_address_parse;
+	}

 	argc -= 2;
 	argv += 2;
@@ -358,7 +362,7 @@ fail_address_parse:
 	if (inode)
 		iput(inode);

-	pr_info("Failed to parse address.\n");
+	pr_info("Failed to parse address or file.\n");

 	return ret;
 }
-- 
1.7.9.7

Re: uprobe: checking probe event include directory

[Srikar Dronamraju]

* Jovi Zhang <bookjovi@gmail.com> [2012-07-18 19:38:27]:

> On Wed, Jul 18, 2012 at 7:07 PM, Srikar Dronamraju
> <srikar@linux.vnet.ibm.com> wrote:
> > The patch looks good,
> >
> > Can you modify the description a bit. However you are free to ignore
> > these comments. After knowing your response,  I will ack the patch.
> >
> > I would probably put this as:
> >
> > The subject could be
> > tracing: Verify target file before registering a uprobe event
> >
> > Description:
> > Without this patch, we can register a uprobe event for a directory.
> > Enabling such a uprobe event would anyway fail.
> >
> > Example:
> >
> > $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
> >
> > However directories cannot be valid targets for uprobe.
> > Hence verify if the target is a regular file during the probe
> > registration.
> 
> Thanks srikar, your description is more clear than mine.
> 
> 
> From fd5077196038cc271e2116e1fca359a0011e1669 Mon Sep 17 00:00:00 2001
> From: Jovi Zhang <bookjovi@gmail.com>
> Date: Wed, 18 Jul 2012 18:16:44 +0800
> Subject: [PATCH] tracing: Verify target file before registering a uprobe
>  event
> 
> Without this patch, we can register a uprobe event for a directory.
> Enabling such a uprobe event would anyway fail.
> 
> Example:
> $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
> 
> However dirctories cannot be valid targets for uprobe.
> Hence verify if the target is a regular file during the probe
> registration.
> 
> Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace_uprobe.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 85158fa..3b5f646 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
>  		goto fail_address_parse;
> 
>  	inode = igrab(path.dentry->d_inode);
> +	 if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
> +		ret = -EINVAL;
> +		goto fail_address_parse;
> +	}
> 
>  	argc -= 2;
>  	argv += 2;
> @@ -358,7 +362,7 @@ fail_address_parse:
>  	if (inode)
>  		iput(inode);
> 
> -	pr_info("Failed to parse address.\n");
> +	pr_info("Failed to parse address or file.\n");
> 
>  	return ret;
>  }

Looks good.

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

Re: uprobe: checking probe event include directory

[Jovi Zhang]

On Wed, Jul 18, 2012 at 7:45 PM, Srikar Dronamraju
<srikar@linux.vnet.ibm.com> wrote:
> * Jovi Zhang <bookjovi@gmail.com> [2012-07-18 19:38:27]:
>
>> On Wed, Jul 18, 2012 at 7:07 PM, Srikar Dronamraju
>> <srikar@linux.vnet.ibm.com> wrote:
>> > The patch looks good,
>> >
>> > Can you modify the description a bit. However you are free to ignore
>> > these comments. After knowing your response,  I will ack the patch.
>> >
>> > I would probably put this as:
>> >
>> > The subject could be
>> > tracing: Verify target file before registering a uprobe event
>> >
>> > Description:
>> > Without this patch, we can register a uprobe event for a directory.
>> > Enabling such a uprobe event would anyway fail.
>> >
>> > Example:
>> >
>> > $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
>> >
>> > However directories cannot be valid targets for uprobe.
>> > Hence verify if the target is a regular file during the probe
>> > registration.
>>
>> Thanks srikar, your description is more clear than mine.
>>
>>
>> From fd5077196038cc271e2116e1fca359a0011e1669 Mon Sep 17 00:00:00 2001
>> From: Jovi Zhang <bookjovi@gmail.com>
>> Date: Wed, 18 Jul 2012 18:16:44 +0800
>> Subject: [PATCH] tracing: Verify target file before registering a uprobe
>>  event
>>
>> Without this patch, we can register a uprobe event for a directory.
>> Enabling such a uprobe event would anyway fail.
>>
>> Example:
>> $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
>>
>> However dirctories cannot be valid targets for uprobe.
>> Hence verify if the target is a regular file during the probe
>> registration.
>>
>> Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
>> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> ---
>>  kernel/trace/trace_uprobe.c |    6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> index 85158fa..3b5f646 100644
>> --- a/kernel/trace/trace_uprobe.c
>> +++ b/kernel/trace/trace_uprobe.c
>> @@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
>>               goto fail_address_parse;
>>
>>       inode = igrab(path.dentry->d_inode);
>> +      if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
>> +             ret = -EINVAL;
>> +             goto fail_address_parse;
>> +     }
>>
>>       argc -= 2;
>>       argv += 2;
>> @@ -358,7 +362,7 @@ fail_address_parse:
>>       if (inode)
>>               iput(inode);
>>
>> -     pr_info("Failed to parse address.\n");
>> +     pr_info("Failed to parse address or file.\n");
>>
>>       return ret;
>>  }
>
> Looks good.
>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>

Hi Andrew,
Is this patch ok to go through your tree?

.jovi

Re: uprobe: checking probe event include directory

[Steven Rostedt]

I've been cleaning out my inbox and found this patch hasn't made it into
mainline yet.

There were a couple of versions of the patch in this thread. Please
re-submit the final version with the associated acks. And not within
this thread.

Patches that are added to replies in threads seldom get applied.

-- Steve

On Wed, 2012-07-18 at 19:38 +0800, Jovi Zhang wrote:
> On Wed, Jul 18, 2012 at 7:07 PM, Srikar Dronamraju
> <srikar@linux.vnet.ibm.com> wrote:
> > The patch looks good,
> >
> > Can you modify the description a bit. However you are free to ignore
> > these comments. After knowing your response,  I will ack the patch.
> >
> > I would probably put this as:
> >
> > The subject could be
> > tracing: Verify target file before registering a uprobe event
> >
> > Description:
> > Without this patch, we can register a uprobe event for a directory.
> > Enabling such a uprobe event would anyway fail.
> >
> > Example:
> >
> > $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
> >
> > However directories cannot be valid targets for uprobe.
> > Hence verify if the target is a regular file during the probe
> > registration.
> 
> Thanks srikar, your description is more clear than mine.
> 
> 
> >From fd5077196038cc271e2116e1fca359a0011e1669 Mon Sep 17 00:00:00 2001
> From: Jovi Zhang <bookjovi@gmail.com>
> Date: Wed, 18 Jul 2012 18:16:44 +0800
> Subject: [PATCH] tracing: Verify target file before registering a uprobe
>  event
> 
> Without this patch, we can register a uprobe event for a directory.
> Enabling such a uprobe event would anyway fail.
> 
> Example:
> $ echo 'p /bin:0x4245c0' > /sys/kernel/debug/tracing/uprobe_events
> 
> However dirctories cannot be valid targets for uprobe.
> Hence verify if the target is a regular file during the probe
> registration.
> 
> Signed-off-by: Jovi Zhang <bookjovi@gmail.com>
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  kernel/trace/trace_uprobe.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 85158fa..3b5f646 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -259,6 +259,10 @@ static int create_trace_uprobe(int argc, char **argv)
>  		goto fail_address_parse;
> 
>  	inode = igrab(path.dentry->d_inode);
> +	 if (!S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)) {
> +		ret = -EINVAL;
> +		goto fail_address_parse;
> +	}
> 
>  	argc -= 2;
>  	argv += 2;
> @@ -358,7 +362,7 @@ fail_address_parse:
>  	if (inode)
>  		iput(inode);
> 
> -	pr_info("Failed to parse address.\n");
> +	pr_info("Failed to parse address or file.\n");
> 
>  	return ret;
>  }