Re: [PATCH] locking/atomics: don't alias ____ptr

[Mark Rutland]

On Wed, Jun 28, 2017 at 03:54:42PM +0200, Thomas Gleixner wrote:
> On Wed, 28 Jun 2017, Thomas Gleixner wrote:
> > On Wed, 28 Jun 2017, Sebastian Andrzej Siewior wrote:
> > > On 2017-06-28 14:15:18 [+0300], Andrey Ryabinin wrote:
> > > > The main problem here is that arch_cmpxchg64_local() calls cmpxhg_local() instead of using arch_cmpxchg_local().
> > > > 
> > > > So, the patch bellow should fix the problem, also this will fix double instrumentation of cmpcxchg64[_local]().
> > > > But I haven't tested this patch yet.
> > > 
> > > tested, works. Next step?
> > 
> > Check all other implementations in every architecture whether there is a
> > similar problem .....

FWIW, as x86 is the only user of atomic-instrumented.h, any similar
issues are unrelated to this series.

That's not to say they don't exist, just that they're orthognal to this.

I've been reworking things for arm64 [1], but there's more cleanup
needed first.

> > But this really want's a proper cleanup unless we want to waste the time
> > over and over again with the next hard to figure out macro expansion fail.
> > 
> > First of all, cmpxchg64[_local]() can be implemented as inlines right away.
> > 
> > For cmpxchg*(), the situation is slightly different, but the sizeof()
> > evaluation should be done at the top most level, even if we do it further
> > down in the low level arch/asm-generic implementation once more.
> > 
> > Something along the lines of:
> > 
> > static inline unsigned long cmpxchg_varsize(void *ptr, unsigned long old,
> > 					    unsigned long new, int size)
> > {
> > 	switch (size) {
> > 	case 1:
> > 	case 2:
> > 	case 4:
> > 		break;
> > 	case 8:
> > 		if (sizeof(unsigned long) == 8)
> > 			break;
> > 	default:
> > 		BUILD_BUG_ON(1);
> > 	}
> > 	kasan_check(ptr, size);
> > 	return arch_cmpxchg(ptr, old, new);
> > }

This'll need to re-cast things before the call to arch_cmpxchg(), and we
can move the check above the switch, as in [2].

> > #define cmpxchg(ptr, o, n)						\
> > ({									\
> > 	((__typeof__(*(ptr)))cmpxchg_varsize((ptr), (unsigned long)(o), \
> > 			     (unsigned long)(n), sizeof(*(ptr))));	\
> > })
> > 
> > That's the first step to cure the actual mess.
> > 
> > Ideally we get rid of that whole macro maze and convert everything to
> > proper inlines with actual cmpxchg8/16/32/64() variants, but that's going
> > to take some time. As an intermediate step we can at least propagate 'size'
> > to arch_cmpxchg(), which is not that much of an effort.
> 
> And to be honest. That should have be done in the first place _BEFORE_
> adding that atomic-instrumented stuff. I'm tempted to revert that mess
> instead of 'fixing' it half arsed.

Sure.

Let's figure out what this *should* look like first.

If that's sufficiently different to what we have now, we revert this and
clean things up first.

> As a side note, we have files (aside of x86/asm/atomic.h) which include
> asm/cmpxchg.h ...
> 
> net/sunrpc/xprtmultipath.c:#include <asm/cmpxchg.h>
> arch/x86/kvm/mmu.c:#include <asm/cmpxchg.h>
> arch/x86/um/asm/barrier.h:#include <asm/cmpxchg.h>

Ugh. I'd sent out a patch [3] for the first of these a while back, as I
spotted that when experimenting with arm64, but tht got dropped on the
floor.

I can resend that, if you like?

I guess it'd also make sense to fix the x86 bits at the same time, so
I'm fine with tahat being folded with other fixes.

> I'm really tired of all this featuritis crammed into the code without much
> thought. Dammit, can we please stop this and clean up the existing mess
> first before duct taping more mess on top of it.

Sorry for adding to the mess here.

Thanks,
Mark.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/kasan-atomic
[2] 20170628124552.GG5981@leverpostej">https://lkml.kernel.org/r/20170628124552.GG5981@leverpostej
[3] http://lkml.kernel.org/r/1489574142-20856-1-git-send-email-mark.rutland@xxxxxxx