| Message ID | 20200402184819.34215-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: vsie: fixes and cleanups. | expand |
On 02.04.20 20:48, David Hildenbrand wrote: > In case we have a region 1 ASCE, our shadow/g3 address can have any value. > Unfortunately, (-1UL << 64) is undefined and triggers sometimes, > rejecting valid shadow addresses when trying to walk our shadow table > hierarchy. > > The result is that the prefix cannot get mapped and will loop basically > forever trying to map it (-EAGAIN loop). > > After all, the broken check is only a sanity check, our table shadowing > code in kvm_s390_shadow_tables() already checks these conditions, injecting > proper translation exceptions. Turn it into a WARN_ON_ONCE(). > > Fixes: 4be130a08420 ("s390/mm: add shadow gmap support") > Cc: <stable@vger.kernel.org> # v4.8+ > Reported-by: Janosch Frank <frankja@linux.ibm.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > arch/s390/mm/gmap.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index 2fbece47ef6f..f3dbc5bdde50 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -787,14 +787,18 @@ static void gmap_call_notifier(struct gmap *gmap, unsigned long start, > static inline unsigned long *gmap_table_walk(struct gmap *gmap, > unsigned long gaddr, int level) > { > + const int asce_type = gmap->asce & _ASCE_TYPE_MASK; > unsigned long *table; > > if ((gmap->asce & _ASCE_TYPE_MASK) + 4 < (level * 4)) > return NULL; > if (gmap_is_shadow(gmap) && gmap->removed) > return NULL; > - if (gaddr & (-1UL << (31 + ((gmap->asce & _ASCE_TYPE_MASK) >> 2)*11))) > + > + if (WARN_ON_ONCE(asce_type != _ASCE_TYPE_REGION1) && > + gaddr & (-1UL << (31 + (asce_type >> 2) * 11))) > return NULL; This has to be if (WARN_ON_ONCE(asce_type != _ASCE_TYPE_REGION1 && gaddr & (-1UL << (31 + (asce_type >> 2) * 11)))) otherwise we'll get wrong warnings
On 4/2/20 8:48 PM, David Hildenbrand wrote: > In case we have a region 1 ASCE, our shadow/g3 address can have any value. > Unfortunately, (-1UL << 64) is undefined and triggers sometimes, > rejecting valid shadow addresses when trying to walk our shadow table > hierarchy. > > The result is that the prefix cannot get mapped and will loop basically > forever trying to map it (-EAGAIN loop). > > After all, the broken check is only a sanity check, our table shadowing > code in kvm_s390_shadow_tables() already checks these conditions, injecting > proper translation exceptions. Turn it into a WARN_ON_ONCE(). > > Fixes: 4be130a08420 ("s390/mm: add shadow gmap support") > Cc: <stable@vger.kernel.org> # v4.8+ > Reported-by: Janosch Frank <frankja@linux.ibm.com> > Signed-off-by: David Hildenbrand <david@redhat.com> With the WARN_ON_ONCE fix applied I don't run into stalls or warnings anymore, so: Tested-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/mm/gmap.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index 2fbece47ef6f..f3dbc5bdde50 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -787,14 +787,18 @@ static void gmap_call_notifier(struct gmap *gmap, unsigned long start, > static inline unsigned long *gmap_table_walk(struct gmap *gmap, > unsigned long gaddr, int level) > { > + const int asce_type = gmap->asce & _ASCE_TYPE_MASK; > unsigned long *table; > > if ((gmap->asce & _ASCE_TYPE_MASK) + 4 < (level * 4)) > return NULL; > if (gmap_is_shadow(gmap) && gmap->removed) > return NULL; > - if (gaddr & (-1UL << (31 + ((gmap->asce & _ASCE_TYPE_MASK) >> 2)*11))) > + > + if (WARN_ON_ONCE(asce_type != _ASCE_TYPE_REGION1) && > + gaddr & (-1UL << (31 + (asce_type >> 2) * 11))) > return NULL; > + > table = gmap->table; > switch (gmap->asce & _ASCE_TYPE_MASK) { > case _ASCE_TYPE_REGION1: >
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index 2fbece47ef6f..f3dbc5bdde50 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -787,14 +787,18 @@ static void gmap_call_notifier(struct gmap *gmap, unsigned long start, static inline unsigned long *gmap_table_walk(struct gmap *gmap, unsigned long gaddr, int level) { + const int asce_type = gmap->asce & _ASCE_TYPE_MASK; unsigned long *table; if ((gmap->asce & _ASCE_TYPE_MASK) + 4 < (level * 4)) return NULL; if (gmap_is_shadow(gmap) && gmap->removed) return NULL; - if (gaddr & (-1UL << (31 + ((gmap->asce & _ASCE_TYPE_MASK) >> 2)*11))) + + if (WARN_ON_ONCE(asce_type != _ASCE_TYPE_REGION1) && + gaddr & (-1UL << (31 + (asce_type >> 2) * 11))) return NULL; + table = gmap->table; switch (gmap->asce & _ASCE_TYPE_MASK) { case _ASCE_TYPE_REGION1:
In case we have a region 1 ASCE, our shadow/g3 address can have any value. Unfortunately, (-1UL << 64) is undefined and triggers sometimes, rejecting valid shadow addresses when trying to walk our shadow table hierarchy. The result is that the prefix cannot get mapped and will loop basically forever trying to map it (-EAGAIN loop). After all, the broken check is only a sanity check, our table shadowing code in kvm_s390_shadow_tables() already checks these conditions, injecting proper translation exceptions. Turn it into a WARN_ON_ONCE(). Fixes: 4be130a08420 ("s390/mm: add shadow gmap support") Cc: <stable@vger.kernel.org> # v4.8+ Reported-by: Janosch Frank <frankja@linux.ibm.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/s390/mm/gmap.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)