linux-kernel - Re: [PATCH] cpufreq: convert the cpufreq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKohpon1QiCfJhWBi_f7U1EU410JMGHq+QnpEPJ+rdiUjsKsQA@mail.gmail.com>
Date:	Wed, 3 Apr 2013 21:02:27 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Nathan Zimmer <nzimmer@....com>
Cc:	rjw@...k.pl, cpufreq@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpufreq: convert the cpufreq_driver to use the rcu

Please always mention Version number and history. Not everybody
remembers what changed after last version.

On 3 April 2013 20:33, Nathan Zimmer <nzimmer@....com> wrote:
> We eventually would like to remove the rwlock cpufreq_driver_lock or convert
> it back to a spinlock and protect the read sections with RCU.  The first step in

Why do we want to convert it back to spinlock?

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c

>  bool have_governor_per_policy(void)
>  {
> -       return cpufreq_driver->have_governor_per_policy;
> +       bool have_governor;

Name it have_governor_per_policy, it looks wrong otherwise.

> +       rcu_read_lock();
> +       have_governor = rcu_dereference(cpufreq_driver)->have_governor_per_policy;
> +       rcu_read_unlock();
> +       return have_governor;
>  }

>  static ssize_t show_scaling_driver(struct cpufreq_policy *policy, char *buf)
>  {
> -       return scnprintf(buf, CPUFREQ_NAME_PLEN, "%s\n", cpufreq_driver->name);
> +       char *name;
> +       rcu_read_lock();
> +       name = rcu_dereference(cpufreq_driver)->name;
> +       rcu_read_unlock();
> +       return scnprintf(buf, CPUFREQ_NAME_PLEN, "%s\n", name);
>  }

This is the definition of struct cpufreq_driver:

struct cpufreq_driver {
	struct module           *owner;
	char			name[CPUFREQ_NAME_LEN];

       ...
};

Purpose of rcu read_lock/unlock are to define the rcu critical section
after which rcu layer is free to free the memory allocated to earlier
instance of cpufreq_driver.

So, after the unlock() call you _should_not_ use the memory allocated to
cpufreq_driver instance. And here, you are using memory allocated to name[]
after the unlock() call.

Which looks to be wrong... I left other parts of driver upto you to fix for this
"rule of thumb".

Sorry for not pointing this earlier but rcu is as new to me as it is
to you. I know
you must be frustrated with so many versions of this patch, and everytime we
get a new problem to you... Don't get disheartened with it.. Keep the good work
going :)

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/