MIME-Version: 1.0
References: <Yb82O5i2DVcK9nAJ@zn.tnic>
In-Reply-To: <Yb82O5i2DVcK9nAJ@zn.tnic>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun, 19 Dec 2021 12:14:29 -0800
Message-ID: <CAHk-=wiNMghi=nZc432_Sj4QwG+BtxGUtovnpVQk-LpDj8r3ZA@mail.gmail.com>
Subject: Re: [GIT PULL] core/urgent for v5.16-rc6
To:     Borislav Petkov <bp@suse.de>,
        "Chang S. Bae" <chang.seok.bae@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>
Cc:     x86-ml <x86@kernel.org>, lkml <linux-kernel@vger.kernel.org>
Content-Type: multipart/mixed; boundary="00000000000058121f05d3856c61"
Precedence: bulk

--00000000000058121f05d3856c61
Content-Type: text/plain; charset="UTF-8"

On Sun, Dec 19, 2021 at 5:40 AM Borislav Petkov <bp@suse.de> wrote:
>
> Prevent lock contention on the new sigaltstack lock on the common-case
> path, when no changes have been made to the alternative signal stack.

I pulled this, but I think it's wrong.

It checks whether the new ss_size/ss_sp is the same as the old ones,
and skips the work if so.

But that check is bogus.

Why? Because it's comparing the wrong values.

It's comparing the values as they are *before* they are fixed up for
the SS_DISABLE case.

So if the new mode is SS_DISABLE, it's going to compare the values
against the old values for ss_size and ss_sp: but those old values
will have been reset to 0/NULL.

And the new values have not been reset yet before the comparison.

So the comparison wil easily fail when it shouldn't.

And that's all pointless anyway. If it's SS_DISABLE, there's no point
in doing *any* of this.

Now, I decided to keep the pull, because this bug only means that the
commit isn't actually as effective as it *should* be.

Honestly, that do_sigaltstack code is written just about pessimally,
and taking the sigaltstack_lock just for the limit checking is all
kinds of silly.

The SS_DISABLE case shouldn't take the lock at all.

And the actual modification of the values shouldn't need any locking
at all, since it's all thread-local.

I'm not convinced even the limit checking needs the lock, but
whatever. I think it could maybe just use "read_once()" or something.

I think the attached patch is an improvement, but I did *not* test
this, and I'm just throwing this out as a "maybe something like this".

Comments?

Note: I will throw this patch away after sending it out. If people agree,

                  Linus

--00000000000058121f05d3856c61
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_kxdonna10>
X-Attachment-Id: f_kxdonna10

IGtlcm5lbC9zaWduYWwuYyB8IDM2ICsrKysrKysrKysrKysrKysrKystLS0tLS0tLS0tLS0tLS0t
LQogMSBmaWxlIGNoYW5nZWQsIDE5IGluc2VydGlvbnMoKyksIDE3IGRlbGV0aW9ucygtKQoKZGlm
ZiAtLWdpdCBhL2tlcm5lbC9zaWduYWwuYyBiL2tlcm5lbC9zaWduYWwuYwppbmRleCBkZmNlZTM4
ODhiMDAuLmY1OGYxZDU3NDkzMSAxMDA2NDQKLS0tIGEva2VybmVsL3NpZ25hbC5jCisrKyBiL2tl
cm5lbC9zaWduYWwuYwpAQCAtNDE2MSw3ICs0MTYxLDYgQEAgZG9fc2lnYWx0c3RhY2sgKGNvbnN0
IHN0YWNrX3QgKnNzLCBzdGFja190ICpvc3MsIHVuc2lnbmVkIGxvbmcgc3AsCiAJCXNpemVfdCBt
aW5fc3Nfc2l6ZSkKIHsKIAlzdHJ1Y3QgdGFza19zdHJ1Y3QgKnQgPSBjdXJyZW50OwotCWludCBy
ZXQgPSAwOwogCiAJaWYgKG9zcykgewogCQltZW1zZXQob3NzLCAwLCBzaXplb2Yoc3RhY2tfdCkp
OwpAQCAtNDE4MSw4ICs0MTgwLDE1IEBAIGRvX3NpZ2FsdHN0YWNrIChjb25zdCBzdGFja190ICpz
cywgc3RhY2tfdCAqb3NzLCB1bnNpZ25lZCBsb25nIHNwLAogCQkJcmV0dXJuIC1FUEVSTTsKIAog
CQlzc19tb2RlID0gc3NfZmxhZ3MgJiB+U1NfRkxBR19CSVRTOwotCQlpZiAodW5saWtlbHkoc3Nf
bW9kZSAhPSBTU19ESVNBQkxFICYmIHNzX21vZGUgIT0gU1NfT05TVEFDSyAmJgotCQkJCXNzX21v
ZGUgIT0gMCkpCisKKwkJaWYgKHNzX21vZGUgPT0gU1NfRElTQUJMRSkgeworCQkJdC0+c2FzX3Nz
X3NwID0gMDsKKwkJCXQtPnNhc19zc19zaXplID0gMDsKKwkJCXQtPnNhc19zc19mbGFncyA9IHNz
X2ZsYWdzOworCQkJcmV0dXJuIDA7CisJCX0KKworCQlpZiAodW5saWtlbHkoc3NfbW9kZSAhPSBT
U19PTlNUQUNLICYmIHNzX21vZGUgIT0gMCkpCiAJCQlyZXR1cm4gLUVJTlZBTDsKIAogCQkvKgpA
QCAtNDE5NCwyNCArNDIwMCwyMCBAQCBkb19zaWdhbHRzdGFjayAoY29uc3Qgc3RhY2tfdCAqc3Ms
IHN0YWNrX3QgKm9zcywgdW5zaWduZWQgbG9uZyBzcCwKIAkJICAgIHQtPnNhc19zc19mbGFncyA9
PSBzc19mbGFncykKIAkJCXJldHVybiAwOwogCisJCS8qIElzIHRoaXMgbG9jayByZWFsbHkgd29y
dGggaXQ/ICovCiAJCXNpZ2FsdHN0YWNrX2xvY2soKTsKLQkJaWYgKHNzX21vZGUgPT0gU1NfRElT
QUJMRSkgewotCQkJc3Nfc2l6ZSA9IDA7Ci0JCQlzc19zcCA9IE5VTEw7Ci0JCX0gZWxzZSB7Ci0J
CQlpZiAodW5saWtlbHkoc3Nfc2l6ZSA8IG1pbl9zc19zaXplKSkKLQkJCQlyZXQgPSAtRU5PTUVN
OwotCQkJaWYgKCFzaWdhbHRzdGFja19zaXplX3ZhbGlkKHNzX3NpemUpKQotCQkJCXJldCA9IC1F
Tk9NRU07Ci0JCX0KLQkJaWYgKCFyZXQpIHsKLQkJCXQtPnNhc19zc19zcCA9ICh1bnNpZ25lZCBs
b25nKSBzc19zcDsKLQkJCXQtPnNhc19zc19zaXplID0gc3Nfc2l6ZTsKLQkJCXQtPnNhc19zc19m
bGFncyA9IHNzX2ZsYWdzOworCQlpZiAodW5saWtlbHkoc3Nfc2l6ZSA8IG1pbl9zc19zaXplKSB8
fAorCQkgICAgdW5saWtlbHkoc2lnYWx0c3RhY2tfc2l6ZV92YWxpZChzc19zaXplKSkpIHsKKwkJ
CXNpZ2FsdHN0YWNrX3VubG9jaygpOworCQkJcmV0dXJuIC1FTk9NRU07CiAJCX0KIAkJc2lnYWx0
c3RhY2tfdW5sb2NrKCk7CisKKwkJdC0+c2FzX3NzX3NwID0gKHVuc2lnbmVkIGxvbmcpIHNzX3Nw
OworCQl0LT5zYXNfc3Nfc2l6ZSA9IHNzX3NpemU7CisJCXQtPnNhc19zc19mbGFncyA9IHNzX2Zs
YWdzOwogCX0KLQlyZXR1cm4gcmV0OworCXJldHVybiAwOwogfQogCiBTWVNDQUxMX0RFRklORTIo
c2lnYWx0c3RhY2ssY29uc3Qgc3RhY2tfdCBfX3VzZXIgKix1c3MsIHN0YWNrX3QgX191c2VyICos
dW9zcykK
--00000000000058121f05d3856c61--