linux-kernel - Re: [RFC PATCH 2/8] perf: Helpers for alloc/init/fini PMU specific data

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: <20191202131646.GD2827@hirez.programming.kicks-ass.net>
Date:   Mon, 2 Dec 2019 14:16:46 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     kan.liang@...ux.intel.com
Cc:     mingo@...hat.com, acme@...nel.org, tglx@...utronix.de,
        bp@...en8.de, linux-kernel@...r.kernel.org, eranian@...gle.com,
        alexey.budankov@...ux.intel.com, vitaly.slobodskoy@...el.com,
        ak@...ux.intel.com
Subject: Re: [RFC PATCH 2/8] perf: Helpers for alloc/init/fini PMU specific
 data

On Thu, Nov 28, 2019 at 07:14:25AM -0800, kan.liang@...ux.intel.com wrote:

> +static int
> +__alloc_task_ctx_data_rcu(struct task_struct *task,
> +			  size_t ctx_size, gfp_t flags)
> +{
> +	struct perf_ctx_data *ctx_data = task->perf_ctx_data;
> +	int ret;
> +
> +	lockdep_assert_held_once(&task->perf_ctx_data_lock);
> +
> +	ret = alloc_perf_ctx_data(ctx_size, flags, &ctx_data);
> +	if (ret)
> +		return ret;
> +
> +	ctx_data->refcount = 1;
> +
> +	rcu_assign_pointer(task->perf_ctx_data, ctx_data);
> +
> +	return 0;
> +}

> +static int
> +__init_task_ctx_data_rcu(struct task_struct *task, size_t ctx_size, gfp_t flags)
> +{
> +	struct perf_ctx_data *ctx_data = task->perf_ctx_data;
> +
> +	lockdep_assert_held_once(&task->perf_ctx_data_lock);
> +
> +	if (ctx_data) {
> +		ctx_data->refcount++;
> +		return 0;
> +	}
> +
> +	return __alloc_task_ctx_data_rcu(task, ctx_size, flags);
> +}

> +/**
> + * Free perf_ctx_data RCU pointer for a task
> + * @task:        Target Task
> + * @force:       Unconditionally free perf_ctx_data
> + *
> + * If force is set, free perf_ctx_data unconditionally.
> + * Otherwise, free perf_ctx_data when there are no users.
> + * Lock is required to sync the writers of perf_ctx_data RCU pointer
> + * and refcount.
> + */
> +static void
> +fini_task_ctx_data_rcu(struct task_struct *task, bool force)
> +{
> +	struct perf_ctx_data *ctx_data;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&task->perf_ctx_data_lock, flags);
> +
> +	ctx_data = task->perf_ctx_data;
> +	if (!ctx_data)
> +		goto unlock;
> +
> +	if (!force && --ctx_data->refcount)
> +		goto unlock;
> +
> +	RCU_INIT_POINTER(task->perf_ctx_data, NULL);
> +	call_rcu(&ctx_data->rcu_head, free_perf_ctx_data);
> +
> +unlock:
> +	raw_spin_unlock_irqrestore(&task->perf_ctx_data_lock, flags);
> +}

All this refcount under lock is an anti-pattern. Also the naming is
insane.