LinuxLists.cc - [ 00/82] 3.4.2-stable review

2012-06-07 04:14:14

Hi,

On Thu, Jun 07, 2012 at 09:42:55AM -0400, Josh Boyer wrote:
> On Thu, Jun 7, 2012 at 12:03 AM, Greg KH <[email protected]> wrote:
> > 3.4-stable review patch. ?If anyone has any objections, please let me know.
> >
> > ------------------
> >
> > From: Andrea Arcangeli <[email protected]>
> >
> > commit 26c191788f18129af0eb32a358cdaea0c7479626 upstream.
> >
> > When holding the mmap_sem for reading, pmd_offset_map_lock should only
> > run on a pmd_t that has been read atomically from the pmdp pointer,
> > otherwise we may read only half of it leading to this crash.
>
> This one is important, but it can break Xen apparently:
>
> http://permalink.gmane.org/gmane.comp.emulators.xen.devel/132522
> https://bugzilla.redhat.com/show_bug.cgi?id=829016
>
> Not sure if you want to hold off on it or see if Andrea comes up with
> a follow up fix?

Not knowing exactly why Xen trips on the atomic64_read on a PAE 32bit
pmd on my side, I don't know what's the best direction to fix it yet.

I knew this fix has been tested and was working fine with Xen +
CONFIG_TRANSPARENT_HUGEPAGE=n + 32bit + x86 + PAE. And when THP=n I
could fix the problem without having to use a slightly more expensive
cmpxchg8b for every pmd read happening with the mmap_sem hold in read
mode.

It was totally unexpected to run into trouble with Xen +
CONFIG_TRANSPARENT_HUGEPAGE=y + 32bit + x86 + PAE, apologies.

>From the oops it looks like atomic64_read trips on a dangling pmdp
pointer, but if the problem doesn't happen with Xen then the pointer
value shouldn't be the problem, and in turn the lock cmpxchg8b used to
access the pointer is likely the problem.

I gave a few suggestions on how to fix it, that should work regardless
of why this is happening, but I'd prefer the Xen developers to comment
on that.

https://bugzilla.redhat.com/attachment.cgi?id=589620

<f0> 0f c7 09 c3 8d 76 0

f0 0f is the infamous opcode lock cmpxchg8b so it confirms it trips
exactly on the pmdp read.

ecx/edx = dcaea360 and "BUG: unable to handle kernel paging request at
dcaea360" are probably all right, my best guess is that the insn used
to read the pmd is unexpected.

2012-06-07 17:47:10

On Thu, 2012-06-07 at 13:04 +0900, Greg KH wrote:
> 3.4-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
>
> From: Eric Dumazet <[email protected]>
>
> [ Upstream commit 9dae31009b1a00d926c6fe032d5a88099620adc3 ]
>
> asix driver drops 8021Q full size frames because it doesn't take into
> account VLAN header size.
>
> Tested on AX88772 adapter.
[...]

This should presumably go into earlier stable versions as well
(specifically requested for 3.2 in <http://bugs.debian.org/676545>).

Does the attached backport look reasonable?

Ben.

--
Ben Hutchings
I haven't lost my mind; it's backed up on tape somewhere.

Attachments:

asix-allow-full-size-8021q-frames-to-be-received.patch (1.24 kB)
signature.asc (828.00 B)
This is a digitally signed message part Download all attachments

2012-06-08 03:03:21

On Thu, 2012-06-07 at 23:11 -0400, Ted Ts'o wrote:
> On Fri, Jun 08, 2012 at 04:03:09AM +0100, Ben Hutchings wrote:
> > > ei->i_flags = flags;
> > [...]
> >
> > Shouldn't this last assignment have been deleted?
>
> Yes, I'm testing two patches to push to Linus. One of them deletes
> the last assignment.
>
> The testing cycle should be over by tomorrow morning; in the mean
> time, this commit is harmless in its current form (although it won't
> fix the problem it was intending to fix until we delete the last
> line).
[...]

It's not harmless because the user is no longer restricted to changing
EXT4_FL_USER_MODIFIABLE.

Ben.

--
Ben Hutchings
I haven't lost my mind; it's backed up on tape somewhere.

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2012-06-08 03:54:52

by David Miller

[permalink] [raw]

Subject: Re: [ 53/82] asix: allow full size 8021Q frames to be received

From: Ben Hutchings <[email protected]>
Date: Fri, 08 Jun 2012 03:27:52 +0100

> On Thu, 2012-06-07 at 13:04 +0900, Greg KH wrote:
>> 3.4-stable review patch. If anyone has any objections, please let me know.
>>
>> ------------------
>>
>>
>> From: Eric Dumazet <[email protected]>
>>
>> [ Upstream commit 9dae31009b1a00d926c6fe032d5a88099620adc3 ]
>>
>> asix driver drops 8021Q full size frames because it doesn't take into
>> account VLAN header size.
>>
>> Tested on AX88772 adapter.
> [...]
>
> This should presumably go into earlier stable versions as well
> (specifically requested for 3.2 in <http://bugs.debian.org/676545>).
>
> Does the attached backport look reasonable?

Yes, thanks Ben.

2012-06-08 08:05:07

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [ 08/82] mm: pmd_read_atomic: fix 32bit PAE pmd walk vs pmd_populate SMP race condition

On Thu, Jun 07, 2012 at 09:42:55AM -0400, Josh Boyer wrote:
> On Thu, Jun 7, 2012 at 12:03 AM, Greg KH <[email protected]> wrote:
> > 3.4-stable review patch. ?If anyone has any objections, please let me know.
> >
> > ------------------
> >
> > From: Andrea Arcangeli <[email protected]>
> >
> > commit 26c191788f18129af0eb32a358cdaea0c7479626 upstream.
> >
> > When holding the mmap_sem for reading, pmd_offset_map_lock should only
> > run on a pmd_t that has been read atomically from the pmdp pointer,
> > otherwise we may read only half of it leading to this crash.
>
> This one is important, but it can break Xen apparently:
>
> http://permalink.gmane.org/gmane.comp.emulators.xen.devel/132522
> https://bugzilla.redhat.com/show_bug.cgi?id=829016
>
> Not sure if you want to hold off on it or see if Andrea comes up with
> a follow up fix?

Ok, for now, I'll drop it.

When Andrea gets this fixed up, can someone let me know so I can apply
this one again, and the fixup?

thanks,

greg k-h

2012-06-08 20:06:01

by Theodore Ts'o

[permalink] [raw]

Subject: Re: [ 64/82] ext4: dont trash state flags in EXT4_IOC_SETFLAGS

On Fri, Jun 08, 2012 at 04:21:48AM +0100, Ben Hutchings wrote:
>
> It's not harmless because the user is no longer restricted to changing
> EXT4_FL_USER_MODIFIABLE.

Um, very good point. So we should either drop this commit and wait
until the next stable release, or also pull commit b22b1f178f6 in as
well. Basically, this commit and b22b1f178f6 should appear in the
same stable release.

- Ted

2012-06-08 23:01:27

On Fri, 2012-06-08 at 22:30 -0400, Ted Ts'o wrote:
> On Sat, Jun 09, 2012 at 12:01:16AM +0100, Ben Hutchings wrote:
> > > Um, very good point. So we should either drop this commit and wait
> > > until the next stable release, or also pull commit b22b1f178f6 in as
> > > well. Basically, this commit and b22b1f178f6 should appear in the
> > > same stable release.
> >
> > I've (re-)queued them both for 3.2.20.
>
> Hey Ben,
>
> If you haven't also queued b0dd6b70f0 (ext4: fix the free blocks
> calculation for ext3 file systems w/ uninit_bg) could you do me a
> favor and also include that in 3.2.20? I'd really like to get that
> into various distro kernels as quickly as possible. (i.e., Debian,
> et. al.)

Hmm, OK, that looks pretty serious.

Ben.

--
Ben Hutchings
73.46% of all statistics are made up.

Attachments:

signature.asc (828.00 B)
This is a digitally signed message part

2012-06-09 17:26:36

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [ 64/82] ext4: dont trash state flags in EXT4_IOC_SETFLAGS

On Fri, Jun 08, 2012 at 04:05:53PM -0400, Ted Ts'o wrote:
> On Fri, Jun 08, 2012 at 04:21:48AM +0100, Ben Hutchings wrote:
> >
> > It's not harmless because the user is no longer restricted to changing
> > EXT4_FL_USER_MODIFIABLE.
>
> Um, very good point. So we should either drop this commit and wait
> until the next stable release, or also pull commit b22b1f178f6 in as
> well. Basically, this commit and b22b1f178f6 should appear in the
> same stable release.

Ok, I've queued that one up as well.

thanks,

greg k-h

2012-06-10 02:03:41

by Konrad Rzeszutek Wilk

[permalink] [raw]

Subject: Re: [PATCH] thp: avoid atomic64_read in pmd_read_atomic for 32bit PAE\

On Thu, Jun 07, 2012 at 11:00:33PM +0200, Andrea Arcangeli wrote:
> In the x86 32bit PAE CONFIG_TRANSPARENT_HUGEPAGE=y case while holding
> the mmap_sem for reading, cmpxchg8b cannot be used to read pmd
> contents under Xen.
>
> So instead of dealing only with "consistent" pmdvals in
> pmd_none_or_trans_huge_or_clear_bad() (which would be conceptually
> simpler) we let pmd_none_or_trans_huge_or_clear_bad() deal with pmdvals
> where the low 32bit and high 32bit could be inconsistent (to avoid
> having to use cmpxchg8b).

<nods>
>
> The only guarantee we get from pmd_read_atomic is that if the low part
> of the pmd was found null, the high part will be null too (so the pmd
> will be considered unstable). And if the low part of the pmd is found
> "stable" later, then it means the whole pmd was read atomically
> (because after a pmd is stable, neither MADV_DONTNEED nor page faults
> can alter it anymore, and we read the high part after the low part).
>
> In the 32bit PAE x86 case, it is enough to read the low part of the
> pmdval atomically to declare the pmd as "stable" and that's true for
> THP and no THP, furthermore in the THP case we also have a barrier()
> that will prevent any inconsistent pmdvals to be cached by a later
> re-read of the *pmd.

Nice. Andrew, any chane you could test this patch on the affected
Xen hypervisors? Was it as easy to reproduce this on a RHEL5 (U1?)
hypervisor or is it really only on Linode and Amazon EC2?

>
> Signed-off-by: Andrea Arcangeli <[email protected]>
> ---
> arch/x86/include/asm/pgtable-3level.h | 30 +++++++++++++++++-------------
> include/asm-generic/pgtable.h | 10 ++++++++++
> 2 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/pgtable-3level.h b/arch/x86/include/asm/pgtable-3level.h
> index 43876f1..cb00ccc 100644
> --- a/arch/x86/include/asm/pgtable-3level.h
> +++ b/arch/x86/include/asm/pgtable-3level.h
> @@ -47,16 +47,26 @@ static inline void native_set_pte(pte_t *ptep, pte_t pte)
> * they can run pmd_offset_map_lock or pmd_trans_huge or other pmd
> * operations.
> *
> - * Without THP if the mmap_sem is hold for reading, the
> - * pmd can only transition from null to not null while pmd_read_atomic runs.
> - * So there's no need of literally reading it atomically.
> + * Without THP if the mmap_sem is hold for reading, the pmd can only
> + * transition from null to not null while pmd_read_atomic runs. So
> + * we can always return atomic pmd values with this function.
> *
> * With THP if the mmap_sem is hold for reading, the pmd can become
> - * THP or null or point to a pte (and in turn become "stable") at any
> - * time under pmd_read_atomic, so it's mandatory to read it atomically
> - * with cmpxchg8b.
> + * trans_huge or none or point to a pte (and in turn become "stable")
> + * at any time under pmd_read_atomic. We could read it really
> + * atomically here with a atomic64_read for the THP enabled case (and
> + * it would be a whole lot simpler), but to avoid using cmpxchg8b we
> + * only return an atomic pmdval if the low part of the pmdval is later
> + * found stable (i.e. pointing to a pte). And we're returning a none
> + * pmdval if the low part of the pmd is none. In some cases the high
> + * and low part of the pmdval returned may not be consistent if THP is
> + * enabled (the low part may point to previously mapped hugepage,
> + * while the high part may point to a more recently mapped hugepage),
> + * but pmd_none_or_trans_huge_or_clear_bad() only needs the low part
> + * of the pmd to be read atomically to decide if the pmd is unstable
> + * or not, with the only exception of when the low part of the pmd is
> + * zero in which case we return a none pmd.
> */
> -#ifndef CONFIG_TRANSPARENT_HUGEPAGE
> static inline pmd_t pmd_read_atomic(pmd_t *pmdp)
> {
> pmdval_t ret;
> @@ -74,12 +84,6 @@ static inline pmd_t pmd_read_atomic(pmd_t *pmdp)
>
> return (pmd_t) { ret };
> }
> -#else /* CONFIG_TRANSPARENT_HUGEPAGE */
> -static inline pmd_t pmd_read_atomic(pmd_t *pmdp)
> -{
> - return (pmd_t) { atomic64_read((atomic64_t *)pmdp) };
> -}
> -#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>
> static inline void native_set_pte_atomic(pte_t *ptep, pte_t pte)
> {
> diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
> index ae39c4b..0ff87ec 100644
> --- a/include/asm-generic/pgtable.h
> +++ b/include/asm-generic/pgtable.h
> @@ -484,6 +484,16 @@ static inline int pmd_none_or_trans_huge_or_clear_bad(pmd_t *pmd)
> /*
> * The barrier will stabilize the pmdval in a register or on
> * the stack so that it will stop changing under the code.
> + *
> + * When CONFIG_TRANSPARENT_HUGEPAGE=y on x86 32bit PAE,
> + * pmd_read_atomic is allowed to return a not atomic pmdval
> + * (for example pointing to an hugepage that has never been
> + * mapped in the pmd). The below checks will only care about
> + * the low part of the pmd with 32bit PAE x86 anyway, with the
> + * exception of pmd_none(). So the important thing is that if
> + * the low part of the pmd is found null, the high part will
> + * be also null or the pmd_none() check below would be
> + * confused.
> */
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> barrier();
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>

2012-06-11 10:35:29

by Andrew Jones

[permalink] [raw]

Subject: Re: [Xen-devel] [PATCH] thp: avoid atomic64_read in pmd_read_atomic for 32bit PAE\

----- Original Message -----
> On Thu, Jun 07, 2012 at 11:00:33PM +0200, Andrea Arcangeli wrote:
> > In the x86 32bit PAE CONFIG_TRANSPARENT_HUGEPAGE=y case while
> > holding
> > the mmap_sem for reading, cmpxchg8b cannot be used to read pmd
> > contents under Xen.
> >
> > So instead of dealing only with "consistent" pmdvals in
> > pmd_none_or_trans_huge_or_clear_bad() (which would be conceptually
> > simpler) we let pmd_none_or_trans_huge_or_clear_bad() deal with
> > pmdvals
> > where the low 32bit and high 32bit could be inconsistent (to avoid
> > having to use cmpxchg8b).
>
> <nods>
> >
> > The only guarantee we get from pmd_read_atomic is that if the low
> > part
> > of the pmd was found null, the high part will be null too (so the
> > pmd
> > will be considered unstable). And if the low part of the pmd is
> > found
> > "stable" later, then it means the whole pmd was read atomically
> > (because after a pmd is stable, neither MADV_DONTNEED nor page
> > faults
> > can alter it anymore, and we read the high part after the low
> > part).
> >
> > In the 32bit PAE x86 case, it is enough to read the low part of the
> > pmdval atomically to declare the pmd as "stable" and that's true
> > for
> > THP and no THP, furthermore in the THP case we also have a
> > barrier()
> > that will prevent any inconsistent pmdvals to be cached by a later
> > re-read of the *pmd.
>
> Nice. Andrew, any chane you could test this patch on the affected
> Xen hypervisors? Was it as easy to reproduce this on a RHEL5 (U1?)
> hypervisor or is it really only on Linode and Amazon EC2?
>

Originally, I was able to reproduce the issue easily with a RHEL5
host. Now, with this patch it's fixed.

Drew

2012-06-11 19:36:41

by Konrad Rzeszutek Wilk

[permalink] [raw]

Subject: Re: [Xen-devel] [PATCH] thp: avoid atomic64_read in pmd_read_atomic for 32bit PAE\

> > Nice. Andrew, any chane you could test this patch on the affected
> > Xen hypervisors? Was it as easy to reproduce this on a RHEL5 (U1?)
> > hypervisor or is it really only on Linode and Amazon EC2?
> >
>
> Originally, I was able to reproduce the issue easily with a RHEL5
> host. Now, with this patch it's fixed.

OK, so Tested-by: Andrew Jones..
and from my perspective it looks good - so Acked-by: Konrad Rzeszutek Wilk <[email protected]>

Andrea, any chance you can respin this patch and send it to Linus for 3.5 please?

2012-06-11 19:42:42

by Andrea Arcangeli

[permalink] [raw]

Subject: Re: [Xen-devel] [PATCH] thp: avoid atomic64_read in pmd_read_atomic for 32bit PAE\

Hi,

On Mon, Jun 11, 2012 at 03:27:38PM -0400, Konrad Rzeszutek Wilk wrote:
> > > Nice. Andrew, any chane you could test this patch on the affected
> > > Xen hypervisors? Was it as easy to reproduce this on a RHEL5 (U1?)
> > > hypervisor or is it really only on Linode and Amazon EC2?
> > >
> >
> > Originally, I was able to reproduce the issue easily with a RHEL5
> > host. Now, with this patch it's fixed.
>
> OK, so Tested-by: Andrew Jones..
> and from my perspective it looks good - so Acked-by: Konrad Rzeszutek Wilk <[email protected]>

Thanks for testing and reviews.

> Andrea, any chance you can respin this patch and send it to Linus for 3.5 please?

Andrew merged it in -mm last Friday, so I would expect it to go
upstream soon through the -mm flow (I assume everyone has been
rightfully waiting a bit of time for testing and reviews to be sure).

Andrea