svn commit: r336439 - in head: share/man/man9 sys/crypto/aesni sys/crypto/armv8 sys/crypto/blake2 sys/crypto/ccp sys/crypto/via sys/dev/cesa sys/dev/cxgbe/crypto sys/dev/hifn sys/dev/safe sys/dev/s...

[Conrad Meyer]

Please file a PR and we can track it there.

The first suggestion that comes to mind is that the XFORMS_LOCK
protects modification of the xforms list — not the lifetime of objects
in it — so drop the list lock over xf_init().  It does not appear that
xform_init can race with xform_detach with the lock dropped.

xform_init is called only by key_setsaval, which is called in two
places: key_newsav, and key_update.  In key_newsav, it is used on
sav's that are not yet linked in to a sah.  In key_update, it is on
larbal sav's only (i.e., linked in to savtree_larval list but not
savtree_alive list).

xform_detach -> key_delete_xform only enumerates over the sahtree
looking for sah's with sav's present in savtree_alive with matching
xform.  Since neither key_newsav nor key_update insert the sav into
the sah savtree_alive list until after setsaval -> xform_init, there
is no race between xform_init and xform_detach (protected by
SAHTREE_WLOCK).

I think this patch may be safe, and would remove the OOM-induced
deadlock condition:

--- netipsec/key.c      (revision 337955)
+++ netipsec/key.c      (working copy)
@@ -8676,11 +8676,13 @@
        XFORMS_LOCK();
        LIST_FOREACH(entry, &xforms, chain) {
            if (entry->xf_type == xftype) {
+                   XFORMS_UNLOCK();
                    ret = (*entry->xf_init)(sav, entry);
-                   break;
+                   goto out;
            }
        }
        XFORMS_UNLOCK();
+out:
        return (ret);
 }

Best,
Conrad




On Fri, Aug 17, 2018 at 6:01 AM, Andrey V. Elsukov <bu7cher at yandex.ru> wrote:
> On 18.07.2018 03:56, Conrad Meyer wrote:
>> Author: cem
>> Date: Wed Jul 18 00:56:25 2018
>> New Revision: 336439
>> URL: https://svnweb.freebsd.org/changeset/base/336439
>>
>> Log:
>>   OpenCrypto: Convert sessions to opaque handles instead of integers
>>
>>   Track session objects in the framework, and pass handles between the
>>   framework (OCF), consumers, and drivers.  Avoid redundancy and complexity in
>>   individual drivers by allocating session memory in the framework and
>>   providing it to drivers in ::newsession().
>
> Hi,
>
> this produces WITNESS warning, since crypto_newsession() allocates
> memory with M_WAITOK flag while xform_init() holds mutex:
>
> uma_zalloc_arg: zone "crypto_session" with the following non-sleepable
> locks held:
> exclusive sleep mutex xforms_list (IPsec transforms list) r = 0
> (0xffffffff81fdb840) locked @
> /home/devel/freebsd/base/head/sys/netipsec/key.c:8676
> stack backtrace:
> #0 0xffffffff80c01643 at witness_debugger+0x73
> #1 0xffffffff80c02a21 at witness_warn+0x461
> #2 0xffffffff80ed98a8 at uma_zalloc_arg+0x38
> #3 0xffffffff80e4a0ca at crypto_newsession+0x1ea
> #4 0xffffffff80e3994c at esp_init+0x37c
> #5 0xffffffff80e31e68 at key_setsaval+0x7f8
> #6 0xffffffff80e307e2 at key_newsav+0x302
> #7 0xffffffff80e2ba0d at key_add+0x53d
> #8 0xffffffff80e263f5 at key_parse+0xac5
> #9 0xffffffff80c34dc7 at sosend_generic+0x447
> #10 0xffffffff80c34ffd at sosend+0x6d
> #11 0xffffffff80c3c170 at kern_sendit+0x240
> #12 0xffffffff80c3c4be at sendit+0x19e
> #13 0xffffffff80c3c30d at sys_sendto+0x4d
> #14 0xffffffff8107e9c1 at amd64_syscall+0x281
> #15 0xffffffff8105846d at fast_syscall_common+0x101
>
> --
> WBR, Andrey V. Elsukov
>