linux-kernel - Re: [RFC/REFACT] Refactoring and significantly reducing code complexity

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]

Message-ID: <x7jww3a7lebhjbwfxwabtypqzq2s752nh6xo4vpchuz2dnsefo@45aftcl4wlyx>
Date:   Wed, 25 Oct 2023 14:12:31 -0400
From:   Daniel Jordan <daniel.m.jordan@...cle.com>
To:     Wang Jinchao <wangjinchao@...sion.com>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        stone.xulei@...sion.com
Subject: Re: [RFC/REFACT] Refactoring and significantly reducing code
 complexity

On Thu, Sep 28, 2023 at 04:53:38PM +0800, Wang Jinchao wrote:
> -/**
> - * struct parallel_data - Internal control structure, covers everything
> - * that depends on the cpumask in use.
> - *
> - * @ps: padata_shell object.
> - * @reorder_list: percpu reorder lists
> - * @squeue: percpu padata queues used for serialuzation.
> - * @refcnt: Number of objects holding a reference on this parallel_data.
> - * @seq_nr: Sequence number of the parallelized data object.
> - * @processed: Number of already processed objects.
> - * @cpu: Next CPU to be processed.
> - * @cpumask: The cpumasks in use for parallel and serial workers.
> - * @reorder_work: work struct for reordering.
> - * @lock: Reorder lock.
> - */
> -struct parallel_data {
> -	struct padata_shell		*ps;
> -	struct padata_list		__percpu *reorder_list;
> -	struct padata_serial_queue	__percpu *squeue;
> -	refcount_t			refcnt;
> -	unsigned int			seq_nr;
> -	unsigned int			processed;
> -	int				cpu;
> -	struct padata_cpumask		cpumask;
> -	struct work_struct		reorder_work;
> -	spinlock_t                      ____cacheline_aligned lock;
> -};

reorder_list used to serialize one sequence of objects per padata_shell,
but now serial_wq serializes all sequences of objects in one list of
work_structs.  That works in theory, since a total order can maintain
each sequence's order, but it's possible (not sure yet, need to think
more) that this could lead to deadlocks or other issues in odd cases
such as the one that padata_shell was introduced for in bbefa1dd6a6d
("crypto: pcrypt - Avoid deadlock by using per-instance padata queues").

The reproducer in that commit produces this splat when testing the RFC.
Not sure if it's related to the above though.

[   40.084146] alg: aead: pcrypt(pcrypt(rfc4106-gcm-aesni)) encryption test failed (wrong result) on test vector 1, cfg="in-place (one sglist)"
[   40.087192] alg: self-tests for rfc4106(gcm(aes)) using pcrypt(pcrypt(rfc4106-gcm-aesni)) failed (rc=-22)
[   40.087195] ------------[ cut here ]------------
[   40.090296] alg: self-tests for rfc4106(gcm(aes)) using pcrypt(pcrypt(rfc4106-gcm-aesni)) failed (rc=-22)
[   40.090313] WARNING: CPU: 4 PID: 321 at crypto/testmgr.c:5936 alg_test+0x404/0x510
[   40.094020] Modules linked in: pcrypt
[   40.094882] CPU: 4 PID: 321 Comm: cryptomgr_test Not tainted 6.6.0-rc6-padata-refact-rfc-v1-test+ #3
[   40.096856] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.2-2-2 04/01/2014
[   40.098838] RIP: 0010:alg_test+0x404/0x510
[   40.099664] Code: e9 4a fd ff ff c6 44 24 17 01 41 bd ff ff ff ff e9 d1 fc ff ff 44 89 e9 48 89 da 4c 89 e6 48 c7 c7 a0 59 1d 82 e8 1c 45 c4 ff <0f> 0b e9 aa fe ff ff 0f 0b 48 c7 c7 80 58 1d 82 e8 e7 c6 cc ff e9
[   40.103180] RSP: 0018:ffffc90000343e10 EFLAGS: 00010286
[   40.104322] RAX: 0000000000000000 RBX: ffff88800421cc00 RCX: 0000000000000000
[   40.105875] RDX: 0000000000000002 RSI: ffffc90000343cc0 RDI: 00000000ffffffff
[   40.107467] RBP: 0000000000000883 R08: 00000000ffffdfff R09: 0000000000000001
[   40.109249] R10: 00000000ffffdfff R11: ffffffff824722a0 R12: ffff88800421cc80
[   40.110862] R13: 00000000ffffffea R14: 00000000ffffffff R15: 0000000000000000
[   40.112354] FS:  0000000000000000(0000) GS:ffff88807dd00000(0000) knlGS:0000000000000000
[   40.114132] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   40.115130] CR2: 00007f1808faba38 CR3: 000000000243e002 CR4: 0000000000170ea0
[   40.115959] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   40.116630] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   40.117261] Call Trace:
[   40.117559]  <TASK>
[   40.117811]  ? __warn+0x7d/0x140
[   40.118165]  ? alg_test+0x404/0x510
[   40.118452]  ? report_bug+0x18d/0x1c0
[   40.118743]  ? handle_bug+0x3a/0x70
[   40.118998]  ? exc_invalid_op+0x13/0x60
[   40.119265]  ? asm_exc_invalid_op+0x16/0x20
[   40.119597]  ? alg_test+0x404/0x510
[   40.119918]  ? _raw_spin_unlock_irqrestore+0x1b/0x40
[   40.120390]  ? try_to_wake_up+0x9a/0x6a0
[   40.120749]  ? preempt_count_add+0x6a/0xa0
[   40.121121]  ? __pfx_cryptomgr_test+0x10/0x10
[   40.121494]  cryptomgr_test+0x20/0x40
[   40.121751]  kthread+0x100/0x130
[   40.121979]  ? __pfx_kthread+0x10/0x10
[   40.122247]  ret_from_fork+0x30/0x50
[   40.122511]  ? __pfx_kthread+0x10/0x10
[   40.122775]  ret_from_fork_asm+0x1b/0x30
[   40.123050]  </TASK>
[   40.123207] ---[ end trace 0000000000000000 ]---