Re: [PATCH mptcp-next 1/2] nbd: allow MPTCP sockets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] nbd: missing SOCK_MEMALLOC and sk_allocation propagation to MPTCP subflows
- [High] nbd: lockdep circular dependency and reclaim deadlock with MPTCP sockets
--

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

nbd: allow MPTCP sockets

This commit allows MPTCP sockets to be used with 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 ffce519bf0087..2aeb22b1ad40e 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c

[ ... ]

> @@ -1221,8 +1222,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]
When an MPTCP socket is attached to NBD, does SOCK_MEMALLOC and
sk_allocation get propagated to the underlying TCP subflow sockets (ssk)?

In drivers/block/nbd.c, nbd_start_device() and
nbd_genl_reconnect_sock_cb() call sk_set_memalloc(sock->sk), and
__sock_xmit() configures sock->sk->sk_allocation:

drivers/block/nbd.c:__sock_xmit()
    sock->sk->sk_allocation = GFP_NOIO | __GFP_MEMALLOC;
    sock->sk->sk_use_task_frag = false;

For an MPTCP socket, sock->sk is the master struct mptcp_sock (msk), while
actual wire segments are sent and received on the underlying TCP subflow
sockets (ssk). Calling sk_set_memalloc() on msk enables the global
memalloc_socks_key static key, but neither NBD nor MPTCP (such as
sync_socket_options() in net/mptcp/sockopt.c or
mptcp_subflow_create_socket() in net/mptcp/subflow.c) propagates
SOCK_MEMALLOC or sk_allocation to existing or newly created subflows.

Under memory pressure (for example, swap or writeback over NBD), incoming
packets allocated from emergency reserves have skb_pfmemalloc(skb) set.
When tcp_v4_rcv() or tcp_v6_rcv() looks up the subflow socket ssk, both
sk_filter_trim_cap() in net/core/filter.c:

    if (skb_pfmemalloc(skb) && !sock_flag(sk, SOCK_MEMALLOC)) {
        NET_INC_STATS(sock_net(sk), LINUX_MIB_PFMEMALLOCDROP);
        return SKB_DROP_REASON_PFMEMALLOC;
    }

and sk_add_backlog() in include/net/sock.h:

    if (skb_pfmemalloc(skb) && !sock_flag(sk, SOCK_MEMALLOC))
        return -ENOMEM;

check ssk rather than msk, dropping all incoming pfmemalloc packets. In
addition, subflow TX allocations in __tcp_push_pending_frames() and
__tcp_send_ack() use ssk->sk_allocation (which remains GFP_KERNEL without
__GFP_MEMALLOC). Won't this stall NBD I/O completion and cause a memory
reclaim deadlock under memory pressure?

[Severity: High]
Could attaching an MPTCP socket to NBD also trigger a lockdep circular
locking dependency splat and a reclaim deadlock via subflow socket locks
and mptcp_worker()?

When nbd_add_socket() attaches a socket, nbd_reclassify_socket() in
drivers/block/nbd.c only reclassifies the top-level sock->sk (msk) to
sk_lock-AF_INET-NBD or sk_lock-AF_INET6-NBD. The underlying TCP subflow
sockets created in mptcp_subflow_create_socket() via sock_create_kern()
retain the generic k-sk_lock-AF_INET or k-sk_lock-AF_INET6 lock class,
under which GFP_KERNEL allocations are performed:

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");

This establishes k-sk_lock-AF_INET -> fs_reclaim. Meanwhile, on the NBD I/O
dispatch path, nbd_queue_rq() -> nbd_handle_cmd() -> nbd_send_cmd() ->
sock_xmit() -> __sock_xmit() -> sock_sendmsg() -> mptcp_sendmsg() ->
__mptcp_push_pending() runs under set->srcu (which is chained after
fs_reclaim -> &q->q_usage_counter(io)) and acquires lock_sock(ssk) in
net/mptcp/protocol.c:

net/mptcp/protocol.c:__mptcp_push_pending()
    lock_sock(ssk);

completing a circular dependency chain between k-sk_lock-AF_INET and
fs_reclaim.

Furthermore, mptcp_worker() in net/mptcp/protocol.c runs in workqueue
context without GFP_NOIO / PF_MEMALLOC, acquires lock_sock(sk) on msk
(sk_lock-AF_INET-NBD), and calls mptcp_pm_worker() and
__mptcp_close_subflow(), which perform GFP_KERNEL allocations while holding
msk->sk_lock. Can this cause a lockdep splat and deadlocks if
mptcp_worker() enters direct reclaim waiting on NBD writeback while
nbd_queue_rq() or recv_work() waits for lock_sock(msk)?

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