"name" is a poor name for a file-global variable. It was used in three
different functions, with no mutual exclusion. But it's just a tiny,
temporary string; let's just move it onto the stack in the functions that
need it. Also use snprintf() just in case.
Signed-off-by: Jonathan Corbet <[email protected]>
Cc: Mark Gross <[email protected]>
Reviewed-by: Frederic Weisbecker <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index d96b83e..3db49b9 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -343,18 +343,18 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
}
EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
-#define PID_NAME_LEN sizeof("process_1234567890")
-static char name[PID_NAME_LEN];
+#define PID_NAME_LEN 32
static int pm_qos_power_open(struct inode *inode, struct file *filp)
{
int ret;
long pm_qos_class;
+ char name[PID_NAME_LEN];
pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
if (pm_qos_class >= 0) {
filp->private_data = (void *)pm_qos_class;
- sprintf(name, "process_%d", current->pid);
+ snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
ret = pm_qos_add_requirement(pm_qos_class, name,
PM_QOS_DEFAULT_VALUE);
if (ret >= 0)
@@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
static int pm_qos_power_release(struct inode *inode, struct file *filp)
{
int pm_qos_class;
+ char name[PID_NAME_LEN];
pm_qos_class = (long)filp->private_data;
- sprintf(name, "process_%d", current->pid);
+ snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
pm_qos_remove_requirement(pm_qos_class, name);
return 0;
@@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
{
s32 value;
int pm_qos_class;
+ char name[PID_NAME_LEN];
pm_qos_class = (long)filp->private_data;
if (count != sizeof(s32))
return -EINVAL;
if (copy_from_user(&value, buf, sizeof(s32)))
return -EFAULT;
- sprintf(name, "process_%d", current->pid);
+ snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
pm_qos_update_requirement(pm_qos_class, name, value);
return sizeof(s32);
On Sat, 10 Oct 2009, Thomas Gleixner wrote:
> "name" is a poor name for a file-global variable. It was used in three
> different functions, with no mutual exclusion. But it's just a tiny,
> temporary string; let's just move it onto the stack in the functions that
> need it. Also use snprintf() just in case.
>
> Signed-off-by: Jonathan Corbet <[email protected]>
> Cc: Mark Gross <[email protected]>
> Reviewed-by: Frederic Weisbecker <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
>
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index d96b83e..3db49b9 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -343,18 +343,18 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
> }
> EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
>
> -#define PID_NAME_LEN sizeof("process_1234567890")
> -static char name[PID_NAME_LEN];
> +#define PID_NAME_LEN 32
Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890"
which is 19, an attempt to show what the maximum string size would be. If
a system were configured to enlarge the maximum PID from 32767 to 4194303
that would still only be 7 digits, so "process_1234567" - which is 16
digits with the newline would enough.
So, I suggest you change that to
#define PID_NAME_LEN sizeof("process_1234567")
Other than that, Reviewed-by: John Kacur <[email protected]>
>
> static int pm_qos_power_open(struct inode *inode, struct file *filp)
> {
> int ret;
> long pm_qos_class;
> + char name[PID_NAME_LEN];
>
> pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
> if (pm_qos_class >= 0) {
> filp->private_data = (void *)pm_qos_class;
> - sprintf(name, "process_%d", current->pid);
> + snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
> ret = pm_qos_add_requirement(pm_qos_class, name,
> PM_QOS_DEFAULT_VALUE);
> if (ret >= 0)
> @@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
> static int pm_qos_power_release(struct inode *inode, struct file *filp)
> {
> int pm_qos_class;
> + char name[PID_NAME_LEN];
>
> pm_qos_class = (long)filp->private_data;
> - sprintf(name, "process_%d", current->pid);
> + snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
> pm_qos_remove_requirement(pm_qos_class, name);
>
> return 0;
> @@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> {
> s32 value;
> int pm_qos_class;
> + char name[PID_NAME_LEN];
>
> pm_qos_class = (long)filp->private_data;
> if (count != sizeof(s32))
> return -EINVAL;
> if (copy_from_user(&value, buf, sizeof(s32)))
> return -EFAULT;
> - sprintf(name, "process_%d", current->pid);
> + snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
> pm_qos_update_requirement(pm_qos_class, name, value);
>
> return sizeof(s32);
>
>
>
On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
John Kacur <[email protected]> wrote:
> Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890"
> which is 19, an attempt to show what the maximum string size would be. If
> a system were configured to enlarge the maximum PID from 32767 to 4194303
> that would still only be 7 digits, so "process_1234567" - which is 16
> digits with the newline would enough.
>
> So, I suggest you change that to
> #define PID_NAME_LEN sizeof("process_1234567")
...which works great until somebody enables 64-bit process IDs...:)
We're talking about 20 bytes of stack space in an almost-never-called
function. I honestly don't think it's worth worrying about, but if
somebody wants to tweak it, I'll not complain.
(Thanks for looking at the patch).
jon
On Sat, 2009-10-10 at 14:03 -0600, Jonathan Corbet wrote:
> On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
> John Kacur <[email protected]> wrote:
>
> > Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890"
> > which is 19, an attempt to show what the maximum string size would be. If
> > a system were configured to enlarge the maximum PID from 32767 to 4194303
> > that would still only be 7 digits, so "process_1234567" - which is 16
> > digits with the newline would enough.
> >
> > So, I suggest you change that to
> > #define PID_NAME_LEN sizeof("process_1234567")
>
> ....which works great until somebody enables 64-bit process IDs...:)
PID/TIDs are limited to 2^29, raising it above that will break things
like futexes. Raising it above 2^32 will break heaps of userspace.
That said, 512M tasks still seems like a lot, but if history is
something to go by we'll eventually run out...
On Sat, 10 Oct 2009, Jonathan Corbet wrote:
> On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
> John Kacur <[email protected]> wrote:
>
> > Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890"
> > which is 19, an attempt to show what the maximum string size would be. If
> > a system were configured to enlarge the maximum PID from 32767 to 4194303
> > that would still only be 7 digits, so "process_1234567" - which is 16
> > digits with the newline would enough.
> >
> > So, I suggest you change that to
> > #define PID_NAME_LEN sizeof("process_1234567")
>
> ...which works great until somebody enables 64-bit process IDs...:)
>
> We're talking about 20 bytes of stack space in an almost-never-called
> function. I honestly don't think it's worth worrying about, but if
> somebody wants to tweak it, I'll not complain.
>
> (Thanks for looking at the patch).
>
It was a minor nit at best! My point was less about the stack space than
the readability - which you could argue is a personal style choice here.
There is nothing else to criticize in these patches. :)
Thomas, I reviewed all 28 patches and applied them.
Thanks
Commit-ID: 1a6deaea3584fd7af1cad492b1fe0867060b45db
Gitweb: http://git.kernel.org/tip/1a6deaea3584fd7af1cad492b1fe0867060b45db
Author: Jonathan Corbet <[email protected]>
AuthorDate: Thu, 6 Aug 2009 13:35:44 -0600
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 14 Oct 2009 15:31:10 +0200
pm_qos: clean up racy global "name" variable
"name" is a poor name for a file-global variable. It was used in three
different functions, with no mutual exclusion. But it's just a tiny,
temporary string; let's just move it onto the stack in the functions that
need it. Also use snprintf() just in case.
Signed-off-by: Jonathan Corbet <[email protected]>
LKML-Reference: <[email protected]>
Acked-by: Mark Gross <[email protected]>
Reviewed-by: Frederic Weisbecker <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/pm_qos_params.c | 12 +++++++-----
1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index d96b83e..3db49b9 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -343,18 +343,18 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
}
EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
-#define PID_NAME_LEN sizeof("process_1234567890")
-static char name[PID_NAME_LEN];
+#define PID_NAME_LEN 32
static int pm_qos_power_open(struct inode *inode, struct file *filp)
{
int ret;
long pm_qos_class;
+ char name[PID_NAME_LEN];
pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
if (pm_qos_class >= 0) {
filp->private_data = (void *)pm_qos_class;
- sprintf(name, "process_%d", current->pid);
+ snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
ret = pm_qos_add_requirement(pm_qos_class, name,
PM_QOS_DEFAULT_VALUE);
if (ret >= 0)
@@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
static int pm_qos_power_release(struct inode *inode, struct file *filp)
{
int pm_qos_class;
+ char name[PID_NAME_LEN];
pm_qos_class = (long)filp->private_data;
- sprintf(name, "process_%d", current->pid);
+ snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
pm_qos_remove_requirement(pm_qos_class, name);
return 0;
@@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
{
s32 value;
int pm_qos_class;
+ char name[PID_NAME_LEN];
pm_qos_class = (long)filp->private_data;
if (count != sizeof(s32))
return -EINVAL;
if (copy_from_user(&value, buf, sizeof(s32)))
return -EFAULT;
- sprintf(name, "process_%d", current->pid);
+ snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
pm_qos_update_requirement(pm_qos_class, name, value);
return sizeof(s32);