Re: [PATCH mptcp-next v2 3/4] nbd: allow MPTCP sockets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] nbd: per-I/O sk_set_memalloc() in __sock_xmit() leaks static key and locks
- [High] nbd: unreclassified MPTCP subflows and GFP_KERNEL in mptcp_worker() deadlock
- [High] mptcp: new subflows miss SOCK_MEMALLOC sync in mptcp_sockopt_sync_locked()
--

commit 1d164aaf19b3c6998b999ca3f8b551a6fe753173
Author: Geliang Tang <geliang@kernel.org>

nbd: allow MPTCP sockets

This commit allows MPTCP sockets to be attached to NBD devices by checking
msk_is_mptcp(sock->sk) in nbd_get_socket().

> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index c5e26fc98d1aa..9cf89d53839a1 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
[ ... ]
> @@ -1222,8 +1223,9 @@ static struct socket *nbd_get_socket(struct nbd_device *nbd, unsigned long fd,
>  		return NULL;
>  
>  	if (!sk_is_tcp(sock->sk) &&
> +	    !msk_is_mptcp(sock->sk) &&
>  	    !sk_is_stream_unix(sock->sk)) {

[Severity: High]
With MPTCP sockets enabled here in drivers/block/nbd.c:nbd_get_socket(),
an earlier patch in this series (commit b144942ec73c, "sock: add
set_memalloc protocol callback") added a call to sk_set_memalloc(sock->sk)
inside the per-segment loop of drivers/block/nbd.c:__sock_xmit():

drivers/block/nbd.c:__sock_xmit() {
	...
		do {
			sock->sk->sk_allocation = GFP_NOIO | __GFP_MEMALLOC;
			sock->sk->sk_use_task_frag = false;
			sk_set_memalloc(sock->sk);
			msg.msg_flags = msg_flags | MSG_NOSIGNAL;
}

Looking at net/core/sock.c:sk_set_memalloc():

void sk_set_memalloc(struct sock *sk)
{
	sock_set_flag(sk, SOCK_MEMALLOC);
	sk->sk_allocation |= __GFP_MEMALLOC;
	static_branch_inc(&memalloc_socks_key);

	/* Call protocol-specific callback if available */
	if (sk->sk_prot->set_memalloc)
		sk->sk_prot->set_memalloc(sk);
}

Since NBD already calls sk_set_memalloc() at device setup in
nbd_start_device() and nbd_genl_reconnect_sock_cb() (and never calls
sk_clear_memalloc() on teardown), won't calling sk_set_memalloc() on every
send and receive iteration in __sock_xmit() cause several regressions?

1. Unbounded static key refcount leak and overflow:
   static_branch_inc(&memalloc_socks_key) is called unconditionally on
   every request header, write bvec, reply header, and read bvec until
   memalloc_socks_key.key.enabled saturates at INT_MAX, at which point
   kernel/jump_label.c:static_key_slow_inc_cpuslocked() takes
   jump_label_mutex and triggers
   WARN_ON_ONCE(!static_key_fast_inc_not_disabled(key)) on every I/O.

2. Lock hierarchy inversion in the block I/O path:
   static_branch_inc() calls static_key_slow_inc(), which acquires
   cpus_read_lock() (cpu_hotplug_lock) inside __sock_xmit() while
   nbd_queue_rq() -> nbd_handle_cmd() -> nbd_send_cmd() holds
   &q->q_usage_counter(io), cmd->lock, and nsock->tx_lock under
   memalloc_noreclaim_save().

3. Unlocked non-atomic race on sk->sk_flags:
   __sock_xmit() runs concurrently in the send path (under nsock->tx_lock)
   and the receive path (recv_work, without nsock->tx_lock or
   lock_sock(sk)), so sock_set_flag(sk, SOCK_MEMALLOC) performs an
   unlocked non-atomic __set_bit() racing with sk->sk_flags updates under
   lock_sock(sk) in sock_sendmsg() and sock_recvmsg().

4. Per-segment subflow lock contention:
   For MPTCP sockets, sk_set_memalloc() calls
   net/mptcp/protocol.c:mptcp_set_memalloc(), which acquires lock_sock(sk)
   and lock_sock_nested(ssk, SINGLE_DEPTH_NESTING) across all subflows
   before every sock_sendmsg() and sock_recvmsg() call.

[Severity: High]
Can attaching an MPTCP socket via nbd_add_socket() -> nbd_get_socket() ->
nbd_reclassify_socket() cause lockdep circular locking dependency warnings
and memory-reclaim deadlocks with MPTCP subflows and mptcp_worker()?

In drivers/block/nbd.c:nbd_reclassify_socket():

static void nbd_reclassify_socket(struct socket *sock)
{
	struct sock *sk = sock->sk;

	if (!sock_allow_reclassification(sk))
		return;

	switch (sk->sk_family) {
	case AF_INET:
		sock_lock_init_class_and_name(sk, "slock-AF_INET-NBD",
					      &nbd_slock_key[0],
					      "sk_lock-AF_INET-NBD",
					      &nbd_key[0]);
		break;

This only reclassifies the outer MPTCP master socket (sock->sk), while its
underlying TCP subflow sockets (ssk) created via sock_create_kern() in
net/mptcp/subflow.c:mptcp_subflow_create_socket() remain in the generic
kernel socket lock class (k-sk_lock-AF_INET / k-sk_lock-AF_INET6):

net/mptcp/subflow.c:mptcp_subflow_create_socket() {
	err = sock_create_kern(net, family, SOCK_STREAM, IPPROTO_TCP, &sf);
	if (err)
		return err;

	lock_sock_nested(sf->sk, SINGLE_DEPTH_NESTING);
	...
	sk_net_refcnt_upgrade(sf->sk);
	err = tcp_set_ulp(sf->sk, "mptcp");
}

Here mptcp_subflow_create_socket() holds sf->sk's lock (k-sk_lock-AF_INET)
while calling sk_net_refcnt_upgrade() (which calls get_net_track() with
GFP_KERNEL) and tcp_set_ulp() -> subflow_ulp_init() -> subflow_create_ctx()
with GFP_KERNEL, establishing k-sk_lock-AF_INET -> fs_reclaim.

When NBD submits I/O via nbd_queue_rq() -> nbd_handle_cmd() ->
nbd_send_cmd() -> sock_xmit() -> __sock_xmit(), it holds
&q->q_usage_counter(io), cmd->lock, and nsock->tx_lock and calls into
mptcp_set_memalloc() and mptcp_sendmsg() -> __mptcp_push_pending(), which
acquire lock_sock(sk) (sk_lock-AF_INET-NBD) and lock_sock(ssk)
(k-sk_lock-AF_INET). Since blk_alloc_queue() establishes
fs_reclaim -> &q->q_usage_counter(io), doesn't this create a circular lock
dependency chain:

  k-sk_lock-AF_INET -> fs_reclaim -> &q->q_usage_counter(io) ->
  cmd->lock -> nsock->tx_lock -> sk_lock-AF_INET-NBD -> k-sk_lock-AF_INET

Additionally, in net/mptcp/protocol.c:mptcp_worker():

static void mptcp_worker(struct work_struct *work)
{
	struct mptcp_sock *msk = container_of(work, struct mptcp_sock, work);
	struct sock *sk = (struct sock *)msk;
	unsigned long fail_tout;
	int state;

	lock_sock(sk);
	...
	mptcp_pm_worker(msk);

mptcp_worker() acquires lock_sock(sk) (sk_lock-AF_INET-NBD) without
memalloc_noreclaim_save() or memalloc_noio_save() and runs
mptcp_pm_worker() -> __mptcp_subflow_connect() ->
mptcp_subflow_create_socket() and kernel_connect() (as well as
__mptcp_close_ssk() -> mptcp_event(..., GFP_KERNEL)), performing GFP_KERNEL
allocations under lock_sock(sk). If memory reclaim recurses into NBD I/O,
won't __sock_xmit() deadlock waiting for lock_sock(sk) held by
mptcp_worker()?

[Severity: High]
How do newly created or passively joined MPTCP subflows inherit
SOCK_MEMALLOC, sk_allocation, and sk_use_task_frag when an MPTCP socket is
used by NBD?

When the MPTCP path manager creates an additional subflow in
mptcp_worker() -> mptcp_pm_worker() -> __mptcp_subflow_connect() ->
mptcp_subflow_create_socket(), or finishes a passive MP_JOIN in
__mptcp_finish_join(), socket options are synchronized from the master
socket to the subflow via net/mptcp/sockopt.c:mptcp_sockopt_sync_locked():

void mptcp_sockopt_sync_locked(struct mptcp_sock *msk, struct sock *ssk)
{
	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);

	msk_owned_by_me(msk);

	ssk->sk_rcvlowat = 0;

	/* subflows must ignore any latency-related settings: will not affect
	 * the user-space - only the msk is relevant - but will foul the
	 * mptcp scheduler
	 */
	tcp_sk(ssk)->notsent_lowat = UINT_MAX;

	if (READ_ONCE(subflow->setsockopt_seq) != msk->setsockopt_seq) {
		sync_socket_options(msk, ssk);

		subflow->setsockopt_seq = msk->setsockopt_seq;
	}
}

Neither mptcp_sockopt_sync_locked() nor sync_socket_options() propagates
SOCK_MEMALLOC, sk->sk_allocation, or sk->sk_use_task_frag from msk to ssk.
Meanwhile, net/mptcp/protocol.c:mptcp_set_memalloc() only iterates over
subflows already in msk->conn_list when sk_set_memalloc() is called (which
also misses subflows still on msk->join_list waiting for mptcp_release_cb(),
and in nbd_start_device() and nbd_genl_reconnect_sock_cb() is called before
sk_allocation = GFP_NOIO | __GFP_MEMALLOC and sk_use_task_frag = false are
assigned).

Won't this leave new subflows with ssk->sk_allocation == GFP_KERNEL during
kernel_connect() and without SOCK_MEMALLOC set (for instance, while
recv_work is already blocked in mptcp_recvmsg()), causing GFP_KERNEL
allocations during connection establishment and causing incoming pfmemalloc
packets on the new subflow to be dropped by sk_filter_trim_cap() under
memory pressure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793010744.git.tanggeliang@kylinos.cn?part=3