Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Heiko Carstens]

> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Not all architectures implement futex_atomic_cmpxchg_inatomic().  The default
> implementation returns -ENOSYS, which is currently not handled inside of the
> futex guts.
> 
> Futex PI calls and robust list exits with a held futex result in an endless
> loop in the futex code on architectures which have no support.
> 
> Fixing up every place where futex_atomic_cmpxchg_inatomic() is called would
> add a fair amount of extra if/else constructs to the already complex code.  It
> is also not possible to disable the robust feature before user space tries to
> register robust lists.
> 
> Compile time disabling is not a good idea either, as there are already
> architectures with runtime detection of futex_atomic_cmpxchg_inatomic support.
> 
> Detect the functionality at runtime instead by calling
> cmpxchg_futex_value_locked() with a NULL pointer from the futex initialization
> code.  This is guaranteed to fail, but the call of
> futex_atomic_cmpxchg_inatomic() happens with pagefaults disabled.
> 
> On architectures, which use the asm-generic implementation or have a runtime
> CPU feature detection, a -ENOSYS return value disables the PI/robust features.
> 
> On architectures with a working implementation the call returns -EFAULT and
> the PI/robust features are enabled.
> 
> The relevant syscalls return -ENOSYS and the robust list exit code is blocked,
> when the detection fails.
> 
> Fixes http://lkml.org/lkml/2008/2/11/149
> Originally reported by: Lennart Buytenhek

[...]

>  static int __init init(void)
>  {
> +	u32 curval;
>  	int i;
> 
> +	/*
> +	 * This will fail and we want it. Some arch implementations do
> +	 * runtime detection of the futex_atomic_cmpxchg_inatomic()
> +	 * functionality. We want to know that before we call in any
> +	 * of the complex code paths. Also we want to prevent
> +	 * registration of robust lists in that case. NULL is
> +	 * guaranteed to fault and we get -EFAULT on functional
> +	 * implementation, the non functional ones will return
> +	 * -ENOSYS.
> +	 */
> +	curval = cmpxchg_futex_value_locked(NULL, 0, 0);
> +	if (curval == -EFAULT)
> +		futex_cmpxchg_enabled = 1;
> +

Why should that fail? You're accessing a kernel space address here and no
user space address.
Indeed it does fail with an Oops on s390 since we enable low address
protection in the kernel so we get an exception if something within the
kernel writes to the first 512 bytes of the kernel address space.
Otherwise it would have silently passed the test...

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Thomas Gleixner]

On Sat, 16 Feb 2008, Heiko Carstens wrote:
> > +	/*
> > +	 * This will fail and we want it. Some arch implementations do
> > +	 * runtime detection of the futex_atomic_cmpxchg_inatomic()
> > +	 * functionality. We want to know that before we call in any
> > +	 * of the complex code paths. Also we want to prevent
> > +	 * registration of robust lists in that case. NULL is
> > +	 * guaranteed to fault and we get -EFAULT on functional
> > +	 * implementation, the non functional ones will return
> > +	 * -ENOSYS.
> > +	 */
> > +	curval = cmpxchg_futex_value_locked(NULL, 0, 0);
> > +	if (curval == -EFAULT)
> > +		futex_cmpxchg_enabled = 1;
> > +
> 
> Why should that fail? You're accessing a kernel space address here and no
> user space address.

Well, NULL pointer dereferencing is supposed to fail, isn't it ?

> Indeed it does fail with an Oops on s390 since we enable low address
> protection in the kernel so we get an exception if something within the
> kernel writes to the first 512 bytes of the kernel address space.
> Otherwise it would have silently passed the test...

NULL pointer dereferencing faults on all architectures, at least it
should, but we explicitely disable pagefaults and recover via the
extable fixup, which is in S390 as well. That returns -EFAULT and
signals that there is a working implementation, while those which have
no support return -ENOSYS, which keeps the robust/pi stuff disabled.

Thanks,

	tglx

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Heiko Carstens]

On Sat, Feb 16, 2008 at 02:05:13PM +0100, Thomas Gleixner wrote:
> On Sat, 16 Feb 2008, Heiko Carstens wrote:
> > > +	/*
> > > +	 * This will fail and we want it. Some arch implementations do
> > > +	 * runtime detection of the futex_atomic_cmpxchg_inatomic()
> > > +	 * functionality. We want to know that before we call in any
> > > +	 * of the complex code paths. Also we want to prevent
> > > +	 * registration of robust lists in that case. NULL is
> > > +	 * guaranteed to fault and we get -EFAULT on functional
> > > +	 * implementation, the non functional ones will return
> > > +	 * -ENOSYS.
> > > +	 */
> > > +	curval = cmpxchg_futex_value_locked(NULL, 0, 0);
> > > +	if (curval == -EFAULT)
> > > +		futex_cmpxchg_enabled = 1;
> > > +
> > 
> > Why should that fail? You're accessing a kernel space address here and no
> > user space address.
> 
> Well, NULL pointer dereferencing is supposed to fail, isn't it ?

I wasn't sure that this is true for all architectures, but...

> > Indeed it does fail with an Oops on s390 since we enable low address
> > protection in the kernel so we get an exception if something within the
> > kernel writes to the first 512 bytes of the kernel address space.
> > Otherwise it would have silently passed the test...
> 
> NULL pointer dereferencing faults on all architectures, at least it
> should, but we explicitely disable pagefaults and recover via the
> extable fixup, which is in S390 as well. That returns -EFAULT and
> signals that there is a working implementation, while those which have
> no support return -ENOSYS, which keeps the robust/pi stuff disabled.

...one of our exception table entries has an off-by-one bug.
Never mind, I'll go and fix our own stuff instead ;)

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Thomas Gleixner]

On Sat, 16 Feb 2008, Heiko Carstens wrote:

> > Well, NULL pointer dereferencing is supposed to fail, isn't it ?
>
> I wasn't sure that this is true for all architectures, but...

It's an requirement for futex support.

> > > Indeed it does fail with an Oops on s390 since we enable low address
> > > protection in the kernel so we get an exception if something within the
> > > kernel writes to the first 512 bytes of the kernel address space.
> > > Otherwise it would have silently passed the test...
> > 
> > NULL pointer dereferencing faults on all architectures, at least it
> > should, but we explicitely disable pagefaults and recover via the
> > extable fixup, which is in S390 as well. That returns -EFAULT and
> > signals that there is a working implementation, while those which have
> > no support return -ENOSYS, which keeps the robust/pi stuff disabled.
> 
> ...one of our exception table entries has an off-by-one bug.
> Never mind, I'll go and fix our own stuff instead ;)

Maybe we should do such tests on all exception table protected
assembler constructs :)

Thanks,

	tglx

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Heiko Carstens]

On Sat, Feb 16, 2008 at 02:48:17PM +0100, Thomas Gleixner wrote:
> On Sat, 16 Feb 2008, Heiko Carstens wrote:
> 
> > > Well, NULL pointer dereferencing is supposed to fail, isn't it ?
> >
> > I wasn't sure that this is true for all architectures, but...
> 
> It's an requirement for futex support.

To be more precise: dereferencing alone won't cause an exception for
NULL pointers on s390. Only writes will do so.
That is very architecture specific since we cannot unmap page 0,
it contains per-cpu data -- like exception pointers and all such stuff
that the cpu needs.
Just in case there is any code that relies on the fact that also reads
via a NULL pointer are supposed to failed.

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Thomas Gleixner]

On Sat, 16 Feb 2008, Heiko Carstens wrote:

> On Sat, Feb 16, 2008 at 02:48:17PM +0100, Thomas Gleixner wrote:
> > On Sat, 16 Feb 2008, Heiko Carstens wrote:
> > 
> > > > Well, NULL pointer dereferencing is supposed to fail, isn't it ?
> > >
> > > I wasn't sure that this is true for all architectures, but...
> > 
> > It's an requirement for futex support.
> 
> To be more precise: dereferencing alone won't cause an exception for
> NULL pointers on s390. Only writes will do so.
> That is very architecture specific since we cannot unmap page 0,
> it contains per-cpu data -- like exception pointers and all such stuff
> that the cpu needs.
> Just in case there is any code that relies on the fact that also reads
> via a NULL pointer are supposed to failed.

Hmm, not sure whether there is such code, but then it would be not too bad
to add

   if (!p)
      return -EFAULT;

to the S390 implementations which only read data and have an exception
fixup.

Thanks,

	tglx

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Andrew Morton]

On Sat, 16 Feb 2008 13:42:48 +0100 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> To: linux-kernel@vger.kernel.org
> Cc: mm-commits@vger.kernel.org, tglx@linutronix.de, buytenh@wantstofly.org, mingo@elte.hu, riku.voipio@movial.fi, stable@kernel.org, schwidefsky@de.ibm.com

Hrm, people have suddenly started removing me from the cc's on mm-commits
replies.  Please don't, else I'll end up sending dud patches into mainline.

More than usual, that is.

Thanks.

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Heiko Carstens]

On Mon, Feb 18, 2008 at 05:00:30AM -0800, Andrew Morton wrote:
> On Sat, 16 Feb 2008 13:42:48 +0100 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> 
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > To: linux-kernel@vger.kernel.org
> > Cc: mm-commits@vger.kernel.org, tglx@linutronix.de, buytenh@wantstofly.org, mingo@elte.hu, riku.voipio@movial.fi, stable@kernel.org, schwidefsky@de.ibm.com
> 
> Hrm, people have suddenly started removing me from the cc's on mm-commits
> replies.  Please don't, else I'll end up sending dud patches into mainline.

Sorry.. I didn't remove you at all. That seems to be a bug in my mail client,
for some reason it removes you from the To:/Cc: list when I hit "g" for
reply-to-all on all mails that come via mm-commits.
Strange.

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Heiko Carstens]

On Mon, Feb 18, 2008 at 05:00:30AM -0800, Andrew Morton wrote:
> On Sat, 16 Feb 2008 13:42:48 +0100 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> 
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > To: linux-kernel@vger.kernel.org
> > Cc: mm-commits@vger.kernel.org, tglx@linutronix.de, buytenh@wantstofly.org, mingo@elte.hu, riku.voipio@movial.fi, stable@kernel.org, schwidefsky@de.ibm.com
> 
> Hrm, people have suddenly started removing me from the cc's on mm-commits
> replies.  Please don't, else I'll end up sending dud patches into mainline.

Ah.. not a bug but a feature: your mails sent via mm-commit have you as
sender but also

Reply-to: linux-kernel@vger.kernel.org

in the mail header. That's why this happens.

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Benjamin Herrenschmidt]

> Hmm, not sure whether there is such code, but then it would be not too bad
> to add
> 
>    if (!p)
>       return -EFAULT;
> 
> to the S390 implementations which only read data and have an exception
> fixup.

I have a different problem on some embedded powerpc's where the TLB miss
code isn't checking for the cached pgdir pointer being NULL (which
happens because we are early at boot and haven't activated an mm). So we
end up either taking recursive faults or going into lalaland walking
the page tables.

It happens on some unreleased code, I'll verify if it happens on
8xx/4xx/etc... in a minute, and will cook a patch if it does.

Cheers,
Ben.

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Benjamin Herrenschmidt]

On Thu, 2008-03-27 at 14:32 +1100, Benjamin Herrenschmidt wrote:
> > Hmm, not sure whether there is such code, but then it would be not too bad
> > to add
> > 
> >    if (!p)
> >       return -EFAULT;
> > 
> > to the S390 implementations which only read data and have an exception
> > fixup.
> 
> I have a different problem on some embedded powerpc's where the TLB miss
> code isn't checking for the cached pgdir pointer being NULL (which
> happens because we are early at boot and haven't activated an mm). So we
> end up either taking recursive faults or going into lalaland walking
> the page tables.
> 
> It happens on some unreleased code, I'll verify if it happens on
> 8xx/4xx/etc... in a minute, and will cook a patch if it does.

Ok, so everything released seems to be fine. It will use swapper_pg_dir
which on 32 bits will do the right thing. Pfiew ! So only some stuff I'm
still working on breaks, I'll fix it.

Cheers,
Ben.

Re: + futex-runtime-enable-pi-and-robust-functionality.patch added to -mm tree

[Josh Boyer]

On Thu, 2008-03-27 at 14:48 +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2008-03-27 at 14:32 +1100, Benjamin Herrenschmidt wrote:
> > > Hmm, not sure whether there is such code, but then it would be not too bad
> > > to add
> > > 
> > >    if (!p)
> > >       return -EFAULT;
> > > 
> > > to the S390 implementations which only read data and have an exception
> > > fixup.
> > 
> > I have a different problem on some embedded powerpc's where the TLB miss
> > code isn't checking for the cached pgdir pointer being NULL (which
> > happens because we are early at boot and haven't activated an mm). So we
> > end up either taking recursive faults or going into lalaland walking
> > the page tables.
> > 
> > It happens on some unreleased code, I'll verify if it happens on
> > 8xx/4xx/etc... in a minute, and will cook a patch if it does.
> 
> Ok, so everything released seems to be fine. It will use swapper_pg_dir
> which on 32 bits will do the right thing. Pfiew ! So only some stuff I'm
> still working on breaks, I'll fix it.

I try to test at least the -rc releases on 4xx as they come out, if not
a daily git update, and this never bit on 4xx for the reasons you
stated.  Glad we have similar results.

josh