linux-kernel - Re: [PATCH RFC] s390x/vmem: get rid of memory segment list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Jun 2020 20:46:21 +0200
From:   Gerald Schaefer <gerald.schaefer@...ibm.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
        linux-mm@...ck.org, Heiko Carstens <heiko.carstens@...ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH RFC] s390x/vmem: get rid of memory segment list

On Fri, 26 Jun 2020 19:22:53 +0200
Gerald Schaefer <gerald.schaefer@...ibm.com> wrote:

> On Thu, 25 Jun 2020 17:00:29 +0200
> David Hildenbrand <david@...hat.com> wrote:
> 
> > I can't come up with a satisfying reason why we still need the memory
> > segment list. We used to represent in the list:
> > - boot memory
> > - standby memory added via add_memory()
> > - loaded dcss segments
> > 
> > When loading/unloading dcss segments, we already track them in a
> > separate list and check for overlaps
> > (arch/s390/mm/extmem.c:segment_overlaps_others()) when loading segments.
> > 
> > The overlap check was introduced for some segments in
> > commit b2300b9efe1b ("[S390] dcssblk: add >2G DCSSs support and stacked
> > contiguous DCSSs support.")
> > and was extended to cover all dcss segments in
> > commit ca57114609d1 ("s390/extmem: remove code for 31 bit addressing
> > mode").
> > 
> > Although I doubt that overlaps with boot memory and standby memory
> > are relevant, let's reshuffle the checks in load_segment() to request
> > the resource first. This will bail out in case we have overlaps with
> > other resources (esp. boot memory and standby memory). The order
> > is now different compared to segment_unload() and segment_unload(), but
> > that should not matter.
> 
> You are right that this is ancient, but I think "overlaps with boot
> memory and standby memory" were very relevant, that might actually
> have been the initial reason for this in ancient times (but I do not
> really remember).
> 
> With DCSS you can define a fixed start address where the segment will
> be loaded into guest address space. The current code queries this address
> and directly gives it to vmem_add_mapping(), at least if there is no
> overlap with other DCSS segments. If there would be an overlap with
> boot memory, and not checked / rejected in vmem_add_mapping(), the
> following dcss_diag() would probably fail because AFAIR z/VM will
> not allow loading a DCSS with guest memory overlap. So far, so good,
> but the vmem_remove_mapping() on the exit path would then remove
> (part of) boot memory.
> 
> That being said, I think your patch prevents this by moving
> request_resource() up, so we should not call vmem_add_mapping()
> for such overlaps. I still want to give it some test, but need
> to find / setup systems with that ancient technology first :-)
> 

Verified with DCSS overlapping boot and standby memory, works fine.
As expected, the error message changes, but I don't think that is a
problem, as long as you also remove the old -ENOSPC case / comment
in arch/s390/mm/extmem.c. It is actually more correct now I guess,
-ENOSPC doesn't look like the correct return value anyway.

Thanks for cleaning up! Looks good to me, and removes > 100 LOC,
unless Heiko remembers some other issues from ancient times.

Reviewed-by: Gerald Schaefer <gerald.schaefer@...ibm.com>
Tested-by: Gerald Schaefer <gerald.schaefer@...ibm.com> [DCSS]