Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 23 Jul 2023 08:40:22 +0200
From:   Heiko Carstens <hca@linux.ibm.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Alexander Gordeev <agordeev@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, Matthew Wilcox <willy@infradead.org>
Subject: Re: [GIT PULL] s390 fixes for 6.5-rc3
Message-ID: <20230723064022.11906-A-hca@linux.ibm.com>
References: <20230722160215.7417-A-hca@linux.ibm.com>
 <CAHk-=wj2g7kDTKPawbhOKXFsAF+Zayygmp1f64oerQktc_LCYw@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wj2g7kDTKPawbhOKXFsAF+Zayygmp1f64oerQktc_LCYw@mail.gmail.com>
MIME-Version: 1.0
Precedence: bulk

On Sat, Jul 22, 2023 at 11:52:22AM -0700, Linus Torvalds wrote:
> On Sat, 22 Jul 2023 at 09:02, Heiko Carstens <hca@linux.ibm.com> wrote:
> >
> > - Fix per vma lock fault handling: add missing !(fault & VM_FAULT_ERROR)
> >   check to fault handler to prevent error handling for return values that
> >   don't indicate an error
> 
> Hmm. The s390 code / people seems to still be a bit confused about the
> VM_FAULT flags.
> 
> The commit comment says "With per-vma locks, handle_mm_fault() may
> return non-fatal error flags".
> 
> That's actively misleading.
...
> Anyway, I have pulled this, since it clearly fixes a problem. But I do
> think that the *deeper* problem is that s390 treats those bits as
> errors in the first place, when they really aren't. Yes, the error
> bits are *common*, but that field really shouldn't be seen as just
> errors, and I really think that the deeper problem is that
> 
>         if (unlikely(fault))
>                 do_fault_error(regs, fault);
> 
> logic. It's simply wrong.
> 
> Of course, it looks like the reason you found this is that the s390
> do_fault_error() then does a BUG() on any bits it doesn't understand.
> You have that nonsensical "clear flags" in other places too. So it's
> not like this work-around is new. But it's a workaround, and a sign of
> confusion, I feel.
> 
> Maybe the extra s390 fault conditions should be added to the generic
> list and added to the VM_FAULT_ERROR mask. I dunno.

Thanks for looking a bit deeper into the code. Our "special" private
VM_FAULT flags came already to attention a couple of months ago [1]. Most
of the flags are historic - I'll try to get rid of all of them, since for
other architectures it is also possible without having private flags.

Our fault handling code needs some refactoring anyway. Hopefully the result
will be easier to maintain, and makes it a bit more difficult to add bugs
like I recently introduced with this per-vma lock architecture backend.

[1] https://lore.kernel.org/all/Y+CLpdnOGFg28uMJ@casper.infradead.org/