| Message ID | 20200403153050.20569-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: vsie: fixes and cleanups | expand |
On Fri, 3 Apr 2020 17:30:48 +0200 David Hildenbrand <david@redhat.com> wrote: > We have to properly retry again by returning -EINVAL immediately in > case somebody else instantiated the table concurrently. We missed to > add the goto in this function only. The code now matches the other, > similar shadowing functions. > > We are overwriting an existing region 2 table entry. All allocated > pages are added to the crst_list to be freed later, so they are not > lost forever. However, when unshadowing the region 2 table, we > wouldn't trigger unshadowing of the original shadowed region 3 table > that we replaced. It would get unshadowed when the original region 3 > table is modified. As it's not connected to the page table hierarchy > anymore, it's not going to get used anymore. However, for a limited > time, this page table will stick around, so it's in some sense a > temporary memory leak. > > Identified by manual code inspection. I don't think this classifies as > stable material. > > Fixes: 998f637cc4b9 ("s390/mm: avoid races on region/segment/page > table shadowing") Signed-off-by: David Hildenbrand <david@redhat.com> > --- > arch/s390/mm/gmap.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index b93dd54b234a..24ef30fb0833 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -1844,6 +1844,7 @@ int gmap_shadow_r3t(struct gmap *sg, unsigned > long saddr, unsigned long r3t, goto out_free; > } else if (*table & _REGION_ENTRY_ORIGIN) { > rc = -EAGAIN; /* Race with shadow */ > + goto out_free; > } > crst_table_init(s_r3t, _REGION3_ENTRY_EMPTY); > /* mark as invalid as long as the parent table is not > protected */ Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index b93dd54b234a..24ef30fb0833 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -1844,6 +1844,7 @@ int gmap_shadow_r3t(struct gmap *sg, unsigned long saddr, unsigned long r3t, goto out_free; } else if (*table & _REGION_ENTRY_ORIGIN) { rc = -EAGAIN; /* Race with shadow */ + goto out_free; } crst_table_init(s_r3t, _REGION3_ENTRY_EMPTY); /* mark as invalid as long as the parent table is not protected */
We have to properly retry again by returning -EINVAL immediately in case somebody else instantiated the table concurrently. We missed to add the goto in this function only. The code now matches the other, similar shadowing functions. We are overwriting an existing region 2 table entry. All allocated pages are added to the crst_list to be freed later, so they are not lost forever. However, when unshadowing the region 2 table, we wouldn't trigger unshadowing of the original shadowed region 3 table that we replaced. It would get unshadowed when the original region 3 table is modified. As it's not connected to the page table hierarchy anymore, it's not going to get used anymore. However, for a limited time, this page table will stick around, so it's in some sense a temporary memory leak. Identified by manual code inspection. I don't think this classifies as stable material. Fixes: 998f637cc4b9 ("s390/mm: avoid races on region/segment/page table shadowing") Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/s390/mm/gmap.c | 1 + 1 file changed, 1 insertion(+)