[RFC 0/1] crypto: Avoid a sleepable operation when freeing a SCTP socket

* [RFC 0/1] crypto: Avoid a sleepable operation when freeing a SCTP socket
@ 2023-08-02 17:09 Florent Revest
  2023-08-02 17:09 ` [RFC 1/1] crypto: Defer transforms destruction to a worker function Florent Revest
  2023-08-03  9:59 ` crypto: api - Use work queue in crypto_destroy_instance Herbert Xu
  0 siblings, 2 replies; 6+ messages in thread
From: Florent Revest @ 2023-08-02 17:09 UTC (permalink / raw)
  To: linux-crypto, linux-kernel, linux-sctp
  Cc: herbert, davem, hillf.zj, marcelo.leitner, lucien.xin, Florent Revest

Hi!

I found that the following program reliably reproduces a "BUG: sleeping function
called from invalid context" backtrace in crypto code:

#include <sys/socket.h>
#include <linux/in.h>
#include <linux/if_alg.h>

int main(void)
{
	int fd1 = socket(AF_INET6, SOCK_STREAM, IPPROTO_SCTP);
	if (fd1 == -1)
		return 1;
	listen(fd1, 1);

	int fd2 = socket(AF_ALG, SOCK_SEQPACKET, 0);
	if (fd2 == -1)
		return 2;
	struct sockaddr_alg addr = {
		.salg_family = AF_ALG,
		.salg_type = "hash",
		.salg_name = "cryptd(md5-generic)",
	};
	bind(fd2, &addr, sizeof(addr));

	return 0;
}

The backtraces look like:

...
 __might_sleep+0x8f/0xe0 kernel/sched/core.c:7260
 down_write+0x78/0x180 kernel/locking/rwsem.c:1556
 crypto_drop_spawn+0x50/0x220 crypto/algapi.c:709
 shash_free_singlespawn_instance+0x19/0x30 crypto/shash.c:621
 crypto_shash_free_instance+0x35/0x40 crypto/shash.c:458
 crypto_free_instance crypto/algapi.c:68 [inline]
 crypto_destroy_instance+0x7d/0xb0 crypto/algapi.c:76
 crypto_alg_put crypto/internal.h:108 [inline]
 crypto_mod_put crypto/api.c:45 [inline]
 crypto_destroy_tfm+0x1f7/0x250 crypto/api.c:573
 crypto_free_shash include/crypto/hash.h:734 [inline]
 sctp_destruct_common net/sctp/socket.c:5003 [inline]
 sctp_v6_destruct_sock+0x40/0x50 net/sctp/socket.c:9436
 __sk_destruct+0x56/0x780 net/core/sock.c:1784
 sk_destruct net/core/sock.c:1829 [inline]
 __sk_free+0x36c/0x470 net/core/sock.c:1840
 sk_free+0x51/0x90 net/core/sock.c:1851
 sock_put include/net/sock.h:1815 [inline]
 sctp_endpoint_destroy_rcu+0xa6/0xf0 net/sctp/endpointola.c:193
 rcu_do_batch kernel/rcu/tree.c:2492 [inline]
 rcu_core+0x7cc/0x1260 kernel/rcu/tree.c:2733
 rcu_core_si+0x9/0x10 kernel/rcu/tree.c:2746
 __do_softirq+0x3dc/0x93b kernel/softirq.c:298
...

My analysis is that, when the process dies, the socket freeing is done in a RCU
callback, therefore under softirq context, therefore sleeping is disabled. As
part of freeing a SCTP socket, we free a cryptographical transform that frees a
"spawn" and this grabs a semaphore which triggers this BUG under
CONFIG_DEBUG_ATOMIC_SLEEP=y.

I believe that we could solve this problem by defering any part of this
backtrace into a worker function. Unfortunately, I have no clue about anything
SCTP nor anything crypto/ so I took a stab at defering... something. :) I marked
this as RFC to make it clear I don't hold strong opinions about what should be
defered exactly and expect this will probably change as a result of code review.

I believe that the same bug has been reported by syzbot twice in the past,
without reproducers and once with a slight twist:
- Once, upon freeing a sctp socket (the backtraces are very similar)
  https://lore.kernel.org/all/00000000000060f19905a74b6825@google.com/T/
  but as far as I can tell the conversation focused on the safety of kfree()
  rather than the semaphore protecting the spawns list (maybe I'm missing
  something here ?)
- Once, upon freeing a tipc socket:
  https://lore.kernel.org/all/000000000000c9257305c61c742c@google.com/T/
  Hillf proposed a fix but, as far as I can tell, it didn't get much attention
  and focused solely on addressing the bug for tipc sockets.
  My fix is inspired by this but further down the backtrace to make the fix work
  for both tipc and sctp (and potentially more ?) sockets freeing.

This patch should apply cleanly on v6.5-rc3.

Florent Revest (1):
  crypto: Defer transforms destruction to a worker function

 crypto/api.c           | 26 ++++++++++++++++++--------
 include/linux/crypto.h |  3 +++
 2 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.41.0.585.gd2178a4bd4-goog

^ permalink raw reply	[flat|nested] 6+ messages in thread