linux-kernel - Re: [RFC][PATCH 0/5] Introduce checks for preemptable code for this_cpu

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: <alpine.LFD.2.02.1109211828080.2723@ionos>
Date:	Wed, 21 Sep 2011 18:32:17 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Christoph Lameter <cl@...two.org>
cc:	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC][PATCH 0/5] Introduce checks for preemptable code for
 this_cpu_read/write()

On Wed, 21 Sep 2011, Christoph Lameter wrote:
> On Tue, 20 Sep 2011, Steven Rostedt wrote:
> 
> > BTW, Can you explain to me where the this_cpu_*() ops were designed to
> > be used? The only places where "this_cpu_*()" is used in slub.c and
> > page_alloc.c have irqs disabled on their use. I thought this was for
> > slub and page_alloc?
> 
> Initially these were used for statistics that used per cpu counters. The
> slub thing was an outgrow of this.
> 
> > Is this_cpu() made just for statistics? I see it used in the inode code
> > for that, and some accounting in the namespace.c code.
> 
> That is the main use case yes.
> 
> > Note and there's places all over the kernel that uses this_cpu_read()
> > and thinks preemption should be disabled. Just look at
> > arch/x86/mm/tlb.c:
> >
> > 	/* Caller has disabled preemption */
> > 	sender = this_cpu_read(tlb_vector_offset);
> >
> > Why the comment?
> >
> > My argument is that this_cpu_* is just confusing. Rename your use case
> > and keep this_cpu_*() as what you want __this_cpu_*() to be.
> 
> Thought about this a bit last night. I think the main issue are these
> this_cpu_read() and this_cpu_write() operations since people use those
> irresponsibly. It usually does not make sense to read a value from a
> random cpu nor does writing make sense. The situation is different for
> per cpu counter increments where it does not matter which cpus counter is
> incremented since we sum them up later anyways.
> 
> How about getting rid of this_cpu_read() and this_cpu_write() entirely?
> 
> Only allow __this_cpu_read and __this_cpu_write. There we check that the
> caller has disabled preemption.
> 
> For the rare special cases (are there any?) that are legitimate use cases
> for this_cpu_read/write we can use manual determination of per cpu
> pointers and then just do a load via the pointer?
> 
> Or alternatively give this_cpu_read and write special names that make
> their dangerousness clear.
> 
> In the case of slub there are only some this_cpu_write() things that can
> be __this_cpu_write without a problem.
> 
> The __this_cpu_ptr() can become this_cpu_ptr as far as I can tell. This
> should make it consistent so that we can check for disabled preemption for
> all __this_cpu thingies.
 
The problem I have with that approach is that this_cpu_inc/dec still
look too close to __this_cpu_*.

It would really be nice to rename this_cpu_inc/dec to something which
makes it clear that this is statistics and does not care a whit about
the actual cpu on which this happens.

Thanks,

	tglx
--
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/