[PATCH] oprofile: disable write access to oprofilefs while profiler is running

[PATCH] oprofile: disable write access to oprofilefs while profiler is running

[Robert Richter]

Oprofile counters are setup when profiling is disabled. Thus, writing
to oprofilefs has no immediate effect. Changes are updated only after
oprofile is reenabled.

To keep userland and kernel states synchronized, we now allow
configuration of oprofile only if profiling is disabled.  In this case
it checks if the profiler is running and then disables write access to
oprofilefs by returning -EBUSY. The change should be backward
compatible with current oprofile userland daemon.

Cc: Maynard Johnson <maynardj@us.ibm.com>
Cc: William Cohen <wcohen@redhat.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 drivers/oprofile/oprof.c          |   19 +++++--------------
 drivers/oprofile/oprof.h          |    2 +-
 drivers/oprofile/oprofile_files.c |    7 +++++--
 drivers/oprofile/oprofilefs.c     |    8 ++++++--
 4 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
index b4a6857..a7ab09e 100644
--- a/drivers/oprofile/oprof.c
+++ b/drivers/oprofile/oprof.c
@@ -225,26 +225,17 @@ post_sync:
 	mutex_unlock(&start_mutex);
 }
 
-int oprofile_set_backtrace(unsigned long val)
+int oprofile_set_ulong(unsigned long *addr, unsigned long val)
 {
-	int err = 0;
+	int err = -EBUSY;
 
 	mutex_lock(&start_mutex);
-
 	if (oprofile_started) {
-		err = -EBUSY;
-		goto out;
-	}
-
-	if (!oprofile_ops.backtrace) {
-		err = -EINVAL;
-		goto out;
+		*addr = val;
+		err = 0;
 	}
-
-	oprofile_backtrace_depth = val;
-
-out:
 	mutex_unlock(&start_mutex);
+
 	return err;
 }
 
diff --git a/drivers/oprofile/oprof.h b/drivers/oprofile/oprof.h
index 47e12cb..177b73d 100644
--- a/drivers/oprofile/oprof.h
+++ b/drivers/oprofile/oprof.h
@@ -37,7 +37,7 @@ void oprofile_create_files(struct super_block *sb, struct dentry *root);
 int oprofile_timer_init(struct oprofile_operations *ops);
 void oprofile_timer_exit(void);
 
-int oprofile_set_backtrace(unsigned long depth);
+int oprofile_set_ulong(unsigned long *addr, unsigned long val);
 int oprofile_set_timeout(unsigned long time);
 
 #endif /* OPROF_H */
diff --git a/drivers/oprofile/oprofile_files.c b/drivers/oprofile/oprofile_files.c
index bbd7516..ccf099e 100644
--- a/drivers/oprofile/oprofile_files.c
+++ b/drivers/oprofile/oprofile_files.c
@@ -79,14 +79,17 @@ static ssize_t depth_write(struct file *file, char const __user *buf, size_t cou
 	if (*offset)
 		return -EINVAL;
 
+	if (!oprofile_ops.backtrace)
+		return -EINVAL;
+
 	retval = oprofilefs_ulong_from_user(&val, buf, count);
 	if (retval)
 		return retval;
 
-	retval = oprofile_set_backtrace(val);
-
+	retval = oprofile_set_ulong(&oprofile_backtrace_depth, val);
 	if (retval)
 		return retval;
+
 	return count;
 }
 
diff --git a/drivers/oprofile/oprofilefs.c b/drivers/oprofile/oprofilefs.c
index 789a1a8..1944621 100644
--- a/drivers/oprofile/oprofilefs.c
+++ b/drivers/oprofile/oprofilefs.c
@@ -91,16 +91,20 @@ static ssize_t ulong_read_file(struct file *file, char __user *buf, size_t count
 
 static ssize_t ulong_write_file(struct file *file, char const __user *buf, size_t count, loff_t *offset)
 {
-	unsigned long *value = file->private_data;
+	unsigned long value;
 	int retval;
 
 	if (*offset)
 		return -EINVAL;
 
-	retval = oprofilefs_ulong_from_user(value, buf, count);
+	retval = oprofilefs_ulong_from_user(&value, buf, count);
+	if (retval)
+		return retval;
 
+	retval = oprofile_set_ulong(file->private_data, value);
 	if (retval)
 		return retval;
+
 	return count;
 }
 
-- 
1.7.2.2

Re: [PATCH] oprofile: disable write access to oprofilefs while profiler is running

[Robert Richter]

On 05.10.10 06:36:57, Robert Richter wrote:
> Oprofile counters are setup when profiling is disabled. Thus, writing
> to oprofilefs has no immediate effect. Changes are updated only after
> oprofile is reenabled.
> 
> To keep userland and kernel states synchronized, we now allow
> configuration of oprofile only if profiling is disabled.  In this case
> it checks if the profiler is running and then disables write access to
> oprofilefs by returning -EBUSY. The change should be backward
> compatible with current oprofile userland daemon.
> 
> Cc: Maynard Johnson <maynardj@us.ibm.com>
> Cc: William Cohen <wcohen@redhat.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Robert Richter <robert.richter@amd.com>

Maynard, please ack if you are fine with it.

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[PATCH] oprofile: disable write access to oprofilefs while profiler is running

[Robert Richter]

On 05.10.10 06:36:57, Robert Richter wrote:
> Oprofile counters are setup when profiling is disabled. Thus, writing
> to oprofilefs has no immediate effect. Changes are updated only after
> oprofile is reenabled.
> 
> To keep userland and kernel states synchronized, we now allow
> configuration of oprofile only if profiling is disabled.  In this case
> it checks if the profiler is running and then disables write access to
> oprofilefs by returning -EBUSY. The change should be backward
> compatible with current oprofile userland daemon.
> 
> Cc: Maynard Johnson <maynardj@us.ibm.com>
> Cc: William Cohen <wcohen@redhat.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  drivers/oprofile/oprof.c          |   19 +++++--------------
>  drivers/oprofile/oprof.h          |    2 +-
>  drivers/oprofile/oprofile_files.c |    7 +++++--
>  drivers/oprofile/oprofilefs.c     |    8 ++++++--
>  4 files changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
> index b4a6857..a7ab09e 100644
> --- a/drivers/oprofile/oprof.c
> +++ b/drivers/oprofile/oprof.c
> @@ -225,26 +225,17 @@ post_sync:
>  	mutex_unlock(&start_mutex);
>  }
>  
> -int oprofile_set_backtrace(unsigned long val)
> +int oprofile_set_ulong(unsigned long *addr, unsigned long val)
>  {
> -	int err = 0;
> +	int err = -EBUSY;
>  
>  	mutex_lock(&start_mutex);
> -
>  	if (oprofile_started) {

I did a late change which I never should do. It should be:

	if (!oprofile_started) ...

The concept remains the same.

Updated patch below.

Sorry,

-Robert

> -		err = -EBUSY;
> -		goto out;
> -	}
> -
> -	if (!oprofile_ops.backtrace) {
> -		err = -EINVAL;
> -		goto out;
> +		*addr = val;
> +		err = 0;
>  	}
> -
> -	oprofile_backtrace_depth = val;
> -
> -out:
>  	mutex_unlock(&start_mutex);
> +
>  	return err;
>  }

--

>From 2d5710a2850eec24c54a1338ae5986963928cf8a Mon Sep 17 00:00:00 2001
From: Robert Richter <robert.richter@amd.com>
Date: Mon, 4 Oct 2010 21:09:36 +0200
Subject: [PATCH] oprofile: disable write access to oprofilefs while profiler is running

Oprofile counters are setup when profiling is disabled. Thus, writing
to oprofilefs has no immediate effect. Changes are updated only after
oprofile is reenabled.

To keep userland and kernel states synchronized, we now allow
configuration of oprofile only if profiling is disabled.  In this case
it checks if the profiler is running and then disables write access to
oprofilefs by returning -EBUSY. The change should be backward
compatible with current oprofile userland daemon.

Cc: Maynard Johnson <maynardj@us.ibm.com>
Cc: William Cohen <wcohen@redhat.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 drivers/oprofile/oprof.c          |   21 ++++++---------------
 drivers/oprofile/oprof.h          |    2 +-
 drivers/oprofile/oprofile_files.c |    7 +++++--
 drivers/oprofile/oprofilefs.c     |    8 ++++++--
 4 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
index b4a6857..f9bda64 100644
--- a/drivers/oprofile/oprof.c
+++ b/drivers/oprofile/oprof.c
@@ -225,26 +225,17 @@ post_sync:
 	mutex_unlock(&start_mutex);
 }
 
-int oprofile_set_backtrace(unsigned long val)
+int oprofile_set_ulong(unsigned long *addr, unsigned long val)
 {
-	int err = 0;
+	int err = -EBUSY;
 
 	mutex_lock(&start_mutex);
-
-	if (oprofile_started) {
-		err = -EBUSY;
-		goto out;
+	if (!oprofile_started) {
+		*addr = val;
+		err = 0;
 	}
-
-	if (!oprofile_ops.backtrace) {
-		err = -EINVAL;
-		goto out;
-	}
-
-	oprofile_backtrace_depth = val;
-
-out:
 	mutex_unlock(&start_mutex);
+
 	return err;
 }
 
diff --git a/drivers/oprofile/oprof.h b/drivers/oprofile/oprof.h
index 47e12cb..177b73d 100644
--- a/drivers/oprofile/oprof.h
+++ b/drivers/oprofile/oprof.h
@@ -37,7 +37,7 @@ void oprofile_create_files(struct super_block *sb, struct dentry *root);
 int oprofile_timer_init(struct oprofile_operations *ops);
 void oprofile_timer_exit(void);
 
-int oprofile_set_backtrace(unsigned long depth);
+int oprofile_set_ulong(unsigned long *addr, unsigned long val);
 int oprofile_set_timeout(unsigned long time);
 
 #endif /* OPROF_H */
diff --git a/drivers/oprofile/oprofile_files.c b/drivers/oprofile/oprofile_files.c
index bbd7516..ccf099e 100644
--- a/drivers/oprofile/oprofile_files.c
+++ b/drivers/oprofile/oprofile_files.c
@@ -79,14 +79,17 @@ static ssize_t depth_write(struct file *file, char const __user *buf, size_t cou
 	if (*offset)
 		return -EINVAL;
 
+	if (!oprofile_ops.backtrace)
+		return -EINVAL;
+
 	retval = oprofilefs_ulong_from_user(&val, buf, count);
 	if (retval)
 		return retval;
 
-	retval = oprofile_set_backtrace(val);
-
+	retval = oprofile_set_ulong(&oprofile_backtrace_depth, val);
 	if (retval)
 		return retval;
+
 	return count;
 }
 
diff --git a/drivers/oprofile/oprofilefs.c b/drivers/oprofile/oprofilefs.c
index 789a1a8..1944621 100644
--- a/drivers/oprofile/oprofilefs.c
+++ b/drivers/oprofile/oprofilefs.c
@@ -91,16 +91,20 @@ static ssize_t ulong_read_file(struct file *file, char __user *buf, size_t count
 
 static ssize_t ulong_write_file(struct file *file, char const __user *buf, size_t count, loff_t *offset)
 {
-	unsigned long *value = file->private_data;
+	unsigned long value;
 	int retval;
 
 	if (*offset)
 		return -EINVAL;
 
-	retval = oprofilefs_ulong_from_user(value, buf, count);
+	retval = oprofilefs_ulong_from_user(&value, buf, count);
+	if (retval)
+		return retval;
 
+	retval = oprofile_set_ulong(file->private_data, value);
 	if (retval)
 		return retval;
+
 	return count;
 }
 
-- 
1.7.2.2



-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] oprofile: disable write access to oprofilefs while profiler is running

[Maynard Johnson]

On 10/05/2010 11:33 AM, Robert Richter wrote:
> On 05.10.10 06:36:57, Robert Richter wrote:
>> Oprofile counters are setup when profiling is disabled. Thus, writing
>> to oprofilefs has no immediate effect. Changes are updated only after
>> oprofile is reenabled.
Robert, I don't have any issue with this patch except I'm not sure what problem 
it's fixing.  From my read of oprofile userland code, I don't see where 
profiling parameters are ever written to oprofilefs when profiling is active. 
Is your patch intended to guard against potential non-intentional cases 
occurring in userland code?  Or is there some case existing right now?

-Maynard
>>
>> To keep userland and kernel states synchronized, we now allow
>> configuration of oprofile only if profiling is disabled.  In this case
>> it checks if the profiler is running and then disables write access to
>> oprofilefs by returning -EBUSY. The change should be backward
>> compatible with current oprofile userland daemon.
>>
>> Cc: Maynard Johnson<maynardj@us.ibm.com>
>> Cc: William Cohen<wcohen@redhat.com>
>> Cc: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> Signed-off-by: Robert Richter<robert.richter@amd.com>
>> ---
>>   drivers/oprofile/oprof.c          |   19 +++++--------------
>>   drivers/oprofile/oprof.h          |    2 +-
>>   drivers/oprofile/oprofile_files.c |    7 +++++--
>>   drivers/oprofile/oprofilefs.c     |    8 ++++++--
>>   4 files changed, 17 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
>> index b4a6857..a7ab09e 100644
>> --- a/drivers/oprofile/oprof.c
>> +++ b/drivers/oprofile/oprof.c
>> @@ -225,26 +225,17 @@ post_sync:
>>   	mutex_unlock(&start_mutex);
>>   }
>>
>> -int oprofile_set_backtrace(unsigned long val)
>> +int oprofile_set_ulong(unsigned long *addr, unsigned long val)
>>   {
>> -	int err = 0;
>> +	int err = -EBUSY;
>>
>>   	mutex_lock(&start_mutex);
>> -
>>   	if (oprofile_started) {
>
> I did a late change which I never should do. It should be:
>
> 	if (!oprofile_started) ...
>
> The concept remains the same.
>
> Updated patch below.
>
> Sorry,
>
> -Robert
>
>> -		err = -EBUSY;
>> -		goto out;
>> -	}
>> -
>> -	if (!oprofile_ops.backtrace) {
>> -		err = -EINVAL;
>> -		goto out;
>> +		*addr = val;
>> +		err = 0;
>>   	}
>> -
>> -	oprofile_backtrace_depth = val;
>> -
>> -out:
>>   	mutex_unlock(&start_mutex);
>> +
>>   	return err;
>>   }
>
> --
>
>  From 2d5710a2850eec24c54a1338ae5986963928cf8a Mon Sep 17 00:00:00 2001
> From: Robert Richter<robert.richter@amd.com>
> Date: Mon, 4 Oct 2010 21:09:36 +0200
> Subject: [PATCH] oprofile: disable write access to oprofilefs while profiler is running
>
> Oprofile counters are setup when profiling is disabled. Thus, writing
> to oprofilefs has no immediate effect. Changes are updated only after
> oprofile is reenabled.
>
> To keep userland and kernel states synchronized, we now allow
> configuration of oprofile only if profiling is disabled.  In this case
> it checks if the profiler is running and then disables write access to
> oprofilefs by returning -EBUSY. The change should be backward
> compatible with current oprofile userland daemon.
>
> Cc: Maynard Johnson<maynardj@us.ibm.com>
> Cc: William Cohen<wcohen@redhat.com>
> Cc: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
> Signed-off-by: Robert Richter<robert.richter@amd.com>
> ---
>   drivers/oprofile/oprof.c          |   21 ++++++---------------
>   drivers/oprofile/oprof.h          |    2 +-
>   drivers/oprofile/oprofile_files.c |    7 +++++--
>   drivers/oprofile/oprofilefs.c     |    8 ++++++--
>   4 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
> index b4a6857..f9bda64 100644
> --- a/drivers/oprofile/oprof.c
> +++ b/drivers/oprofile/oprof.c
> @@ -225,26 +225,17 @@ post_sync:
>   	mutex_unlock(&start_mutex);
>   }
>
> -int oprofile_set_backtrace(unsigned long val)
> +int oprofile_set_ulong(unsigned long *addr, unsigned long val)
>   {
> -	int err = 0;
> +	int err = -EBUSY;
>
>   	mutex_lock(&start_mutex);
> -
> -	if (oprofile_started) {
> -		err = -EBUSY;
> -		goto out;
> +	if (!oprofile_started) {
> +		*addr = val;
> +		err = 0;
>   	}
> -
> -	if (!oprofile_ops.backtrace) {
> -		err = -EINVAL;
> -		goto out;
> -	}
> -
> -	oprofile_backtrace_depth = val;
> -
> -out:
>   	mutex_unlock(&start_mutex);
> +
>   	return err;
>   }
>
> diff --git a/drivers/oprofile/oprof.h b/drivers/oprofile/oprof.h
> index 47e12cb..177b73d 100644
> --- a/drivers/oprofile/oprof.h
> +++ b/drivers/oprofile/oprof.h
> @@ -37,7 +37,7 @@ void oprofile_create_files(struct super_block *sb, struct dentry *root);
>   int oprofile_timer_init(struct oprofile_operations *ops);
>   void oprofile_timer_exit(void);
>
> -int oprofile_set_backtrace(unsigned long depth);
> +int oprofile_set_ulong(unsigned long *addr, unsigned long val);
>   int oprofile_set_timeout(unsigned long time);
>
>   #endif /* OPROF_H */
> diff --git a/drivers/oprofile/oprofile_files.c b/drivers/oprofile/oprofile_files.c
> index bbd7516..ccf099e 100644
> --- a/drivers/oprofile/oprofile_files.c
> +++ b/drivers/oprofile/oprofile_files.c
> @@ -79,14 +79,17 @@ static ssize_t depth_write(struct file *file, char const __user *buf, size_t cou
>   	if (*offset)
>   		return -EINVAL;
>
> +	if (!oprofile_ops.backtrace)
> +		return -EINVAL;
> +
>   	retval = oprofilefs_ulong_from_user(&val, buf, count);
>   	if (retval)
>   		return retval;
>
> -	retval = oprofile_set_backtrace(val);
> -
> +	retval = oprofile_set_ulong(&oprofile_backtrace_depth, val);
>   	if (retval)
>   		return retval;
> +
>   	return count;
>   }
>
> diff --git a/drivers/oprofile/oprofilefs.c b/drivers/oprofile/oprofilefs.c
> index 789a1a8..1944621 100644
> --- a/drivers/oprofile/oprofilefs.c
> +++ b/drivers/oprofile/oprofilefs.c
> @@ -91,16 +91,20 @@ static ssize_t ulong_read_file(struct file *file, char __user *buf, size_t count
>
>   static ssize_t ulong_write_file(struct file *file, char const __user *buf, size_t count, loff_t *offset)
>   {
> -	unsigned long *value = file->private_data;
> +	unsigned long value;
>   	int retval;
>
>   	if (*offset)
>   		return -EINVAL;
>
> -	retval = oprofilefs_ulong_from_user(value, buf, count);
> +	retval = oprofilefs_ulong_from_user(&value, buf, count);
> +	if (retval)
> +		return retval;
>
> +	retval = oprofile_set_ulong(file->private_data, value);
>   	if (retval)
>   		return retval;
> +
>   	return count;
>   }
>

Re: [PATCH] oprofile: disable write access to oprofilefs while profiler is running

[Robert Richter]

On 11.10.10 09:48:02, Maynard Johnson wrote:
> Robert, I don't have any issue with this patch except I'm not sure what problem 
> it's fixing.  From my read of oprofile userland code, I don't see where 
> profiling parameters are ever written to oprofilefs when profiling is active. 
> Is your patch intended to guard against potential non-intentional cases 
> occurring in userland code?  Or is there some case existing right now?

The kernel updates the counter configuration during profiling only
when the counters are started. Since sampling data and its
representation depends on the counter configuration, we do not want to
change the configuration in-flight. Otherwise the daemon may not know
which configuration was used for which sample.

Instead, userland is required to restart profiling which also flushes
cpu buffers. Thus, the daemon always is aware of the configuration
which was used when the kernel wrote a sample.

This may happen e.g. for IBS samples, where certain options may be
enabled using oprofilefs. We want to know which options are set while
a sample is written and make sure, the options do not change. There is
another case, the latest patch I sent that implements IBS branch
target address reporting extends the IBS sample size if its option is
set. To always proper parse the sampling data by the daemon, we must
ensure the sampling data is consistent over one profiling session and
sample size does not change.

As we rely on not writing to the configuration while profiling and
since this works with the current daemon, I added the check.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH] oprofile: disable write access to oprofilefs while profiler is running

[Maynard Johnson]

Robert Richter wrote:
> On 05.10.10 06:36:57, Robert Richter wrote:
>> Oprofile counters are setup when profiling is disabled. Thus, writing
>> to oprofilefs has no immediate effect. Changes are updated only after
>> oprofile is reenabled.
>>
>> To keep userland and kernel states synchronized, we now allow
>> configuration of oprofile only if profiling is disabled.  In this case
>> it checks if the profiler is running and then disables write access to
>> oprofilefs by returning -EBUSY. The change should be backward
>> compatible with current oprofile userland daemon.
>>
>> Cc: Maynard Johnson <maynardj@us.ibm.com>
Ack.  This patch looks fine, and tests OK on ppc64.  A little paranoia in the kernel can't hurt.

-Maynard
>> Cc: William Cohen <wcohen@redhat.com>
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Robert Richter <robert.richter@amd.com>
>> ---
>>  drivers/oprofile/oprof.c          |   19 +++++--------------
>>  drivers/oprofile/oprof.h          |    2 +-
>>  drivers/oprofile/oprofile_files.c |    7 +++++--
>>  drivers/oprofile/oprofilefs.c     |    8 ++++++--
>>  4 files changed, 17 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
>> index b4a6857..a7ab09e 100644
>> --- a/drivers/oprofile/oprof.c
>> +++ b/drivers/oprofile/oprof.c
>> @@ -225,26 +225,17 @@ post_sync:
>>  	mutex_unlock(&start_mutex);
>>  }
>>  
>> -int oprofile_set_backtrace(unsigned long val)
>> +int oprofile_set_ulong(unsigned long *addr, unsigned long val)
>>  {
>> -	int err = 0;
>> +	int err = -EBUSY;
>>  
>>  	mutex_lock(&start_mutex);
>> -
>>  	if (oprofile_started) {
> 
> I did a late change which I never should do. It should be:
> 
> 	if (!oprofile_started) ...
> 
> The concept remains the same.
> 
> Updated patch below.
> 
> Sorry,
> 
> -Robert
> 
>> -		err = -EBUSY;
>> -		goto out;
>> -	}
>> -
>> -	if (!oprofile_ops.backtrace) {
>> -		err = -EINVAL;
>> -		goto out;
>> +		*addr = val;
>> +		err = 0;
>>  	}
>> -
>> -	oprofile_backtrace_depth = val;
>> -
>> -out:
>>  	mutex_unlock(&start_mutex);
>> +
>>  	return err;
>>  }
> 
> --
> 
> From 2d5710a2850eec24c54a1338ae5986963928cf8a Mon Sep 17 00:00:00 2001
> From: Robert Richter <robert.richter@amd.com>
> Date: Mon, 4 Oct 2010 21:09:36 +0200
> Subject: [PATCH] oprofile: disable write access to oprofilefs while profiler is running
> 
> Oprofile counters are setup when profiling is disabled. Thus, writing
> to oprofilefs has no immediate effect. Changes are updated only after
> oprofile is reenabled.
> 
> To keep userland and kernel states synchronized, we now allow
> configuration of oprofile only if profiling is disabled.  In this case
> it checks if the profiler is running and then disables write access to
> oprofilefs by returning -EBUSY. The change should be backward
> compatible with current oprofile userland daemon.
> 
> Cc: Maynard Johnson <maynardj@us.ibm.com>
> Cc: William Cohen <wcohen@redhat.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  drivers/oprofile/oprof.c          |   21 ++++++---------------
>  drivers/oprofile/oprof.h          |    2 +-
>  drivers/oprofile/oprofile_files.c |    7 +++++--
>  drivers/oprofile/oprofilefs.c     |    8 ++++++--
>  4 files changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/oprofile/oprof.c b/drivers/oprofile/oprof.c
> index b4a6857..f9bda64 100644
> --- a/drivers/oprofile/oprof.c
> +++ b/drivers/oprofile/oprof.c
> @@ -225,26 +225,17 @@ post_sync:
>  	mutex_unlock(&start_mutex);
>  }
> 
> -int oprofile_set_backtrace(unsigned long val)
> +int oprofile_set_ulong(unsigned long *addr, unsigned long val)
>  {
> -	int err = 0;
> +	int err = -EBUSY;
> 
>  	mutex_lock(&start_mutex);
> -
> -	if (oprofile_started) {
> -		err = -EBUSY;
> -		goto out;
> +	if (!oprofile_started) {
> +		*addr = val;
> +		err = 0;
>  	}
> -
> -	if (!oprofile_ops.backtrace) {
> -		err = -EINVAL;
> -		goto out;
> -	}
> -
> -	oprofile_backtrace_depth = val;
> -
> -out:
>  	mutex_unlock(&start_mutex);
> +
>  	return err;
>  }
> 
> diff --git a/drivers/oprofile/oprof.h b/drivers/oprofile/oprof.h
> index 47e12cb..177b73d 100644
> --- a/drivers/oprofile/oprof.h
> +++ b/drivers/oprofile/oprof.h
> @@ -37,7 +37,7 @@ void oprofile_create_files(struct super_block *sb, struct dentry *root);
>  int oprofile_timer_init(struct oprofile_operations *ops);
>  void oprofile_timer_exit(void);
> 
> -int oprofile_set_backtrace(unsigned long depth);
> +int oprofile_set_ulong(unsigned long *addr, unsigned long val);
>  int oprofile_set_timeout(unsigned long time);
> 
>  #endif /* OPROF_H */
> diff --git a/drivers/oprofile/oprofile_files.c b/drivers/oprofile/oprofile_files.c
> index bbd7516..ccf099e 100644
> --- a/drivers/oprofile/oprofile_files.c
> +++ b/drivers/oprofile/oprofile_files.c
> @@ -79,14 +79,17 @@ static ssize_t depth_write(struct file *file, char const __user *buf, size_t cou
>  	if (*offset)
>  		return -EINVAL;
> 
> +	if (!oprofile_ops.backtrace)
> +		return -EINVAL;
> +
>  	retval = oprofilefs_ulong_from_user(&val, buf, count);
>  	if (retval)
>  		return retval;
> 
> -	retval = oprofile_set_backtrace(val);
> -
> +	retval = oprofile_set_ulong(&oprofile_backtrace_depth, val);
>  	if (retval)
>  		return retval;
> +
>  	return count;
>  }
> 
> diff --git a/drivers/oprofile/oprofilefs.c b/drivers/oprofile/oprofilefs.c
> index 789a1a8..1944621 100644
> --- a/drivers/oprofile/oprofilefs.c
> +++ b/drivers/oprofile/oprofilefs.c
> @@ -91,16 +91,20 @@ static ssize_t ulong_read_file(struct file *file, char __user *buf, size_t count
> 
>  static ssize_t ulong_write_file(struct file *file, char const __user *buf, size_t count, loff_t *offset)
>  {
> -	unsigned long *value = file->private_data;
> +	unsigned long value;
>  	int retval;
> 
>  	if (*offset)
>  		return -EINVAL;
> 
> -	retval = oprofilefs_ulong_from_user(value, buf, count);
> +	retval = oprofilefs_ulong_from_user(&value, buf, count);
> +	if (retval)
> +		return retval;
> 
> +	retval = oprofile_set_ulong(file->private_data, value);
>  	if (retval)
>  		return retval;
> +
>  	return count;
>  }
> 

Re: [PATCH] oprofile: disable write access to oprofilefs while profiler is running

[Robert Richter]

On 05.10.10 06:36:57, Robert Richter wrote:
> Oprofile counters are setup when profiling is disabled. Thus, writing
> to oprofilefs has no immediate effect. Changes are updated only after
> oprofile is reenabled.
> 
> To keep userland and kernel states synchronized, we now allow
> configuration of oprofile only if profiling is disabled.  In this case
> it checks if the profiler is running and then disables write access to
> oprofilefs by returning -EBUSY. The change should be backward
> compatible with current oprofile userland daemon.
> 
> Cc: Maynard Johnson <maynardj@us.ibm.com>
> Cc: William Cohen <wcohen@redhat.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  drivers/oprofile/oprof.c          |   19 +++++--------------
>  drivers/oprofile/oprof.h          |    2 +-
>  drivers/oprofile/oprofile_files.c |    7 +++++--
>  drivers/oprofile/oprofilefs.c     |    8 ++++++--
>  4 files changed, 17 insertions(+), 19 deletions(-)

I applied the patch to oprofile/core. Maynard, thanks for review.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center