Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Elliott, Robert (Servers)" <elliott@hpe.com>
To:     "Elliott, Robert (Servers)" <elliott@hpe.com>,
        Herbert Xu <herbert@gondor.apana.org.au>
CC:     Ard Biesheuvel <ardb@kernel.org>, Taehee Yoo <ap420073@gmail.com>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "x86@kernel.org" <x86@kernel.org>, "hpa@zytor.com" <hpa@zytor.com>,
        "ebiggers@google.com" <ebiggers@google.com>
Subject: RE: [PATCH] crypto: x86: Do not acquire fpu context for too long
Thread-Topic: [PATCH] crypto: x86: Do not acquire fpu context for too long
Thread-Index: AQHY16zBFmM7SwmJd0SQI42PUTup5q39qv2AgAATNYCAAJL7sIAFLlCAgAFSKxCAAM1MgIAA4KgwgALKpsA=
Date:   Tue, 11 Oct 2022 15:16:12 +0000
Message-ID: <MW5PR84MB1842A321C785BC0DEF490A31AB239@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
References: <20221004044912.24770-1-ap420073@gmail.com>
 <Yzu8Kd2botr3eegj@gondor.apana.org.au>
 <f7c52ed1-8061-8147-f676-86190118cc56@gmail.com>
 <MW5PR84MB18420D6E1A31D9C765EF6ED4AB5A9@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
 <CAMj1kXGmKunh-OCvGFf8T6KJJXSHRYzacjSojBD3__u0o-3D1w@mail.gmail.com>
 <MW5PR84MB1842762F8B2ABC27A1A13614AB5E9@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
 <Y0JoDWx0Q5BmO/wR@gondor.apana.org.au>
 <MW5PR84MB1842FD77B90B1553367235CEAB219@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
In-Reply-To: <MW5PR84MB1842FD77B90B1553367235CEAB219@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?P89Ykz5bntIHo3zoXCLNMyY+vz89rE5JfH0OlJMXTOAyXtI1dTE05QPRKLyB?=
 =?us-ascii?Q?DULymV+KydNJw4oZ5pCAlZKrzXOOcXNz/Pryeu/4cEyi3ZVEf+BWiqQdX1KG?=
 =?us-ascii?Q?gROZ6OGx5BMHAmcRkH4ydFsXq527zqgrCdsYOXjy5gl7aVU8Q7dlmGHTq9aa?=
 =?us-ascii?Q?RsTIYI8bTt0LXBb6ecR8LuR/a6kR7Z6YqmfOsTDw1ldd7dhBNYgcWuFc6aC2?=
 =?us-ascii?Q?V1yH+wphdXE+IPMv6NpD9j+QpwGrKuybNqHhzDKd90j3sj2r0Xqc5hZFIyKm?=
 =?us-ascii?Q?Ms6vsFgLgJpEkZD8B3LFfiREX+wztD6WDgVB5RWJZ2b0rmJYOGWS7pRLIlnF?=
 =?us-ascii?Q?Aux16dbm9ZbM7IlSAQZCFlPp2xKEjAXul60gfDwoGOtKVBt1WGnCDvznl+R0?=
 =?us-ascii?Q?ewn/lA6HWKRmkAgfp0NXiGufBQNtF6wxsxkVKu2cMSnZJuYev8tGeYrpaI/Z?=
 =?us-ascii?Q?31Cfu2aB9A8lPh6h58n5IOUCFGM9dY0s45o39wEoHzhxyhC8qt62UQ0ldAui?=
 =?us-ascii?Q?apoGxQNa4odhBy4YnpWTLkize7YQ3Qqp3Qu8M1XQiYHGS9FKHlSIcPNc9Jh4?=
 =?us-ascii?Q?FWdmesZjKihgsXYDA7JqOZOhbx0mHUjG8lhRdWAYBc439k/bf79MisQAI6i2?=
 =?us-ascii?Q?MBmRcqqBrrE5MQEeKAoG/jIKR1KbH+GZS12S8FHYoUP+eKZI2YbcdfaestIy?=
 =?us-ascii?Q?HKH3WoGJ0AzKS4YZ1vqSyUJ/37Evamn6p0UsnMOv2S9q1mNaFw+4XEPXP0qx?=
 =?us-ascii?Q?Z+LX19DSRXmUOUGxp4DaoYTLSp8vC0a2iIg51d78MROQQLhGc2pO1+w1fQNP?=
 =?us-ascii?Q?MprXpwY7zdSYJKgo9y9muec59oKKokdoWjSwGVP0XM+WqFIoaRZZM3GdBbue?=
 =?us-ascii?Q?oMdv4qQKo/BSLipBx3paluIe5RFw7RZhjofvE6XoB2qi1AAhxeCNf/PGfg1w?=
 =?us-ascii?Q?9S5Eyg2wQ/FsA13whIZlMpLv8E+NihACCrSbEI8OHbviA5r6yvfmb8D5PwQZ?=
 =?us-ascii?Q?Cxr8z/O7FvmQpj8vwSI0R4q5a4WnatuKOuT4nS8X2jTvtFD7SFQ/zKWUKLg9?=
 =?us-ascii?Q?UHsZSOw0mKiHG+4Ph5ocmsctemAodoZd8k40hE7wyESFQ2TQSvScN3kLxWOi?=
 =?us-ascii?Q?VPfzqhDiDd4ELuGEq6p5NuhOcP8xKNGClSOnpadNuqDqpuO2ESJipNPqVLbt?=
 =?us-ascii?Q?KPPj5gBfaozXf5U2Lg2zQGwS2oMH3fjHLZWKb09A8nDcK3GqkUeMpFY+820G?=
 =?us-ascii?Q?xH+TQR6gIUFqGvx6CpDqM9ZKAyHIx+lEqNbHH3IcXtYDtb7ylNjAPG4V9WI6?=
 =?us-ascii?Q?zMYPwYEBU3p36AZcxueOZUQPIXD8oWgX8u7qQoTW+/xVqwOVsIWlTzGnCZVv?=
 =?us-ascii?Q?g7YOgOfBO6vBAX17l9bNAerkxb+3E9wWUz36PxbMmv21ScnQ0b1KFq2Fx+ug?=
 =?us-ascii?Q?c9kX2vmEjhQKPh3ePNrySiZ+3izQfddSFOdEvdTsd/qnfAZKfpGiTF8XoyjU?=
 =?us-ascii?Q?MjXPpfrFZBteogTqo2qCJJAfIreq2XyiMfkbE4GkYnFEWuscr92/Y8zpE7ne?=
 =?us-ascii?Q?x0UEbnITi8WWSlZZ79g=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-Network-Message-Id: 94f15750-9e82-41c9-397e-08daab9b88bf
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Oct 2022 15:16:12.8704
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: nrQb9FOBcK1M0//vetTx1ncmUaFMDl3OByT3o/fI7CySgAwt9wx53sa0sWSK2Cmw
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR84MB1700
X-OriginatorOrg: hpe.com
X-Proofpoint-GUID: krmPqojRjyXqzy7bYZfPCPSD1QtulGYa
X-Proofpoint-ORIG-GUID: krmPqojRjyXqzy7bYZfPCPSD1QtulGYa
X-HPE-SCL: -1
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-11_08,2022-10-11_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0
 priorityscore=1501 bulkscore=0 impostorscore=0 mlxscore=0 suspectscore=0
 spamscore=0 mlxlogscore=999 malwarescore=0 clxscore=1015 adultscore=0
 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2210110087
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org


> > From: Herbert Xu <herbert@gondor.apana.org.au>
...
> > On Sat, Oct 08, 2022 at 07:48:07PM +0000, Elliott, Robert (Servers) wro=
te:
> > >
> > > Perhaps the cycles mode needs to call cond_resched() too?
> >
> > Yes, just make the cond_resched unconditional.  Having a few too many
> > rescheds shouldn't be an issue.
>=20
> This looks promising. I was able to trigger a lot of rcu stalls by settin=
g:
>   echo 2 > /sys/module/rcupdate/parameters/rcu_cpu_stall_timeout
>   echo 200 > /sys/module/rcupdate/parameters/rcu_exp_cpu_stall_timeout
>=20
> and running these concurrently:
>   watch -n 0 modprobe tcrypt=3D200
>   watch -n 0 module tcrypt=3D0 through 999
>=20
> I am getting miscompares from the extended self-test for crc32 and
> crct10dif, and will investigate those further.

The assembly functions for those two do not handle small sizes (unlike
crc32c, which handles all sizes), so the new do/while loop needs to
use larger steps and call the generic function for any leftover bytes.
I've corrected that patch.

> After changing tcrypt to call cond_resched in both cases, I don't see any
> more rcu stalls.

This ran cleanly with a few nights of testing with=20
   CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=3Dy

which compares the accelerated implementation to the generic
implementation under randomly generated test vectors - a very
helpful test suite. It only works with data sizes up to
2 * PAGE_SIZE, though.

With the cond_resched changes, rcu stalls are gone with one exception.
If printing of Call Traces is triggered for any reason, either by a
test failing (e.g., because of the CRC loop bugs) or by using a SysRq
trigger like:
   echo l > /proc/sysrq-trigger

then the very act of generating and printing the Call Traces can
trigger rcu stall messages. Examples:

  Oct 09 14:56:39 kernel: cryptomgr: alg: shash: crct10dif-pclmul test fail=
ed (wrong result) on test vector "random: psize=3D4303 ksize=3D0", cfg=3D"r=
andom: use_final src_divs=3D[<flush>95.43%@+4, <flush>4.57%@+6]"
  Oct 09 14:56:39 kernel: cryptomgr: alg: self-tests for crct10dif using cr=
ct10dif failed (rc=3D-22)
  Oct 09 14:56:39 kernel: ------------[ cut here ]------------
  Oct 09 14:56:39 kernel: alg: self-tests for crct10dif using crct10dif fai=
led (rc=3D-22)
  Oct 09 14:56:39 kernel: WARNING: CPU: 55 PID: 10553 at crypto/testmgr.c:5=
837 alg_test.cold+0x3e/0x124
  ...
  Oct 09 14:56:42 kernel: R10: 0000000000000003 R11: 0000000000000246 R12: =
0000000000040000
  Oct 09 14:56:42 kernel: rcu: INFO: rcu_preempt detected stalls on CPUs/ta=
sks:

and:
  Oct 10 19:45:02 kernel: sysrq: Show backtrace of all active CPUs
  ...
  Oct 10 19:45:03 kernel:  </TASK>
  ...
  Oct 10 19:45:06 kernel: rcu: INFO: rcu_preempt detected stalls on CPUs/ta=
sks:

If there are no WARNs or call traces, then there are no rcu stalls.

So, we should be aware that commit 09a5ef9644bc0e1 ("crypto: testmgr - WARN
on test failure") might introduce additional errors.

> BTW, the way tcrypt always refuses to load leads to an ever-growing list =
in
> the Call Traces:
>=20
> kernel: Unloaded tainted modules: tcrypt():1 tcrypt():1 tcrypt():1 tcrypt=
():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt(=
):1
> tcrypt():1 t
> crypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt()=
:1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt(=
):1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt(=
):1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt(=
):1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
>  tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt=
():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt(=
):1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():
> 1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcryp=
t():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt(=
):1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt()
> :1 tcrypt():1 tcrypt():1 tcrypt():1

That was already noticed and a fix has already been posted for 6.1
as commit 47cc75aa9283 ("module: tracking: Keep a record of tainted unloade=
d
modules only")