Stop corrupting socket's task_frag
Subsystems (19)
v3 cover Stop corrupting socket's task_frag
The networking code uses flags in sk_allocation to determine if it can use current->task_frag, however in-kernel users of sockets may stop setting sk_allocation when they convert to the preferred memalloc_nofs_save/restore, as SUNRPC has done in commit a1231fda7e94 ("SUNRPC: Set memalloc_nofs_save() on all rpciod/xprtiod jobs").
This will cause corruption in current->task_frag when recursing into the network layer for those subsystems during page fault or reclaim. The corruption is difficult to diagnose because stack traces may not contain the offending subsystem at all. The corruption is unlikely to show up in testing because it requires memory pressure, and so subsystems that convert to memalloc_nofs_save/restore are likely to continue to run into this issue.
Previous reports and proposed fixes:
https://lore.kernel.org/netdev/96a18bd00cbc6cb554603cc0d6ef1c551965b078.1663762494.git.gnault@redhat.com/
https://lore.kernel.org/netdev/b4d8cb09c913d3e34f853736f3f5628abfd7f4b6.1656699567.git.gnault@redhat.com/
https://lore.kernel.org/linux-nfs/de6d99321d1dcaa2ad456b92b3680aa77c07a747.1665401788.git.gnault@redhat.com/
Guilluame Nault has done all of the hard work tracking this problem down and finding the best fix for this issue. I'm just taking a turn posting another fix.
Changes on v2:
- rebased on -net
- set sk_use_task_frag = false for xfrm/espintcp.c
Changes on v3:
- fixup comments in sock.h for kernel-doc
Benjamin Coddington (2):
Treewide: Stop corrupting socket's task_frag
net: simplify sk_page_frag
Guillaume Nault (1):
net: Introduce sk_use_task_frag in struct sock.
drivers/block/drbd/drbd_receiver.c | 3 +++ drivers/block/nbd.c | 1 + drivers/nvme/host/tcp.c | 1 + drivers/scsi/iscsi_tcp.c | 1 + drivers/usb/usbip/usbip_common.c | 1 + fs/afs/rxrpc.c | 1 + fs/cifs/connect.c | 1 + fs/dlm/lowcomms.c | 2 ++ fs/ocfs2/cluster/tcp.c | 1 + include/net/sock.h | 10 ++++++---- net/9p/trans_fd.c | 1 + net/ceph/messenger.c | 1 + net/core/sock.c | 1 + net/sunrpc/xprtsock.c | 3 +++ net/xfrm/espintcp.c | 1 + 15 files changed, 25 insertions(+), 4 deletions(-)
Signature · 1 line
2.31.1
PATCH v3 1/3 net: Introduce sk_use_task_frag in struct sock.
From: Guillaume Nault <[email protected]>
Sockets that can be used while recursing into memory reclaim, like those used by network block devices and file systems, mustn't use current->task_frag: if the current process is already using it, then the inner memory reclaim call would corrupt the task_frag structure.
To avoid this, sk_page_frag() uses ->sk_allocation to detect sockets that mustn't use current->task_frag, assuming that those used during memory reclaim had their allocation constraints reflected in ->sk_allocation.
This unfortunately doesn't cover all cases: in an attempt to remove all usage of GFP_NOFS and GFP_NOIO, sunrpc stopped setting these flags in ->sk_allocation, and used memalloc_nofs critical sections instead. This breaks the sk_page_frag() heuristic since the allocation constraints are now stored in current->flags, which sk_page_frag() can't read without risking triggering a cache miss and slowing down TCP's fast path.
This patch creates a new field in struct sock, named sk_use_task_frag, which sockets with memory reclaim constraints can set to false if they can't safely use current->task_frag. In such cases, sk_page_frag() now always returns the socket's page_frag (->sk_frag). The first user is sunrpc, which needs to avoid using current->task_frag but can keep ->sk_allocation set to GFP_KERNEL otherwise.
Eventually, it might be possible to simplify sk_page_frag() by only testing ->sk_use_task_frag and avoid relying on the ->sk_allocation heuristic entirely (assuming other sockets will set ->sk_use_task_frag according to their constraints in the future).
The new ->sk_use_task_frag field is placed in a hole in struct sock and belongs to a cache line shared with ->sk_shutdown. Therefore it should be hot and shouldn't have negative performance impacts on TCP's fast path (sk_shutdown is tested just before the while() loop in tcp_sendmsg_locked()).
--- include/net/sock.h | 11 +++++++++-- net/core/sock.c | 1 + 2 files changed, 10 insertions(+), 2 deletions(-)
include/net/sock.h +9 -2
@@ -318,6 +318,9 @@ struct sk_filter;* @sk_stamp: time stamp of last packet received * @sk_stamp_seq: lock for accessing sk_stamp on 32 bit architectures only * @sk_tsflags: SO_TIMESTAMPING flags +* @sk_use_task_frag: allow sk_page_frag() to use current->task_frag. +* Sockets that can be used under memory reclaim should +* set this to false. * @sk_bind_phc: SO_TIMESTAMPING bind PHC index of PTP virtual clock * for timestamping * @sk_tskey: counter to disambiguate concurrent tstamp requests
@@ -505,6 +508,7 @@ struct sock {#endif u16 sk_tsflags;u8 sk_shutdown; +bool sk_use_task_frag;atomic_t sk_tskey;atomic_t sk_zckey;
@@ -2561,14 +2565,17 @@ static inline void sk_stream_moderate_sndbuf(struct sock *sk)* socket operations and end up recursing into sk_page_frag() * while it's already in use: explicitly avoid task page_frag * usage if the caller is potentially doing any of them. -* This assumes that page fault handlers use the GFP_NOFS flags. +* This assumes that page fault handlers use the GFP_NOFS flags or +* explicitly disable sk_use_task_frag. * * Return: a per task page_frag if context allows that, * otherwise a per socket one. */ static inline struct page_frag *sk_page_frag (struct sock *sk) { -if ((sk->sk_allocation & (__GFP_DIRECT_RECLAIM | __GFP_MEMALLOC | __GFP_FS)) == +if (sk->sk_use_task_frag && + (sk->sk_allocation & (__GFP_DIRECT_RECLAIM | __GFP_MEMALLOC | + __GFP_FS)) == (__GFP_DIRECT_RECLAIM | __GFP_FS))return ¤t->task_frag;
net/core/sock.c +1 -0
@@ -3368,6 +3368,7 @@ void sock_init_data(struct socket *sock, struct sock *sk) sk->sk_rcvbuf =READ_ONCE (sysctl_rmem_default); sk->sk_sndbuf =READ_ONCE (sysctl_wmem_default); sk->sk_state =TCP_CLOSE ; + sk->sk_use_task_frag = true;sk_set_socket (sk, sock);sock_set_flag (sk,SOCK_ZAPPED );
Signature · 1 line
2.31.1
Paolo Abeni
maintainerRe: [PATCH net v3 1/3] net: Introduce sk_use_task_frag in struct sock.
Quoted 61 lines · Benjamin Coddington
From: Guillaume Nault <[email protected]>
Sockets that can be used while recursing into memory reclaim, like those used by network block devices and file systems, mustn't use
current->task_frag: if the current process is already using it, then the inner memory reclaim call would corrupt thetask_fragstructure.To avoid this,
sk_page_frag()uses ->sk_allocation to detect sockets that mustn't usecurrent->task_frag, assuming that those used during memory reclaim had their allocation constraints reflected in ->sk_allocation.This unfortunately doesn't cover all cases: in an attempt to remove all usage of
GFP_NOFSandGFP_NOIO, sunrpc stopped setting these flags in ->sk_allocation, and usedmemalloc_nofscritical sections instead. This breaks thesk_page_frag()heuristic since the allocation constraints are now stored incurrent->flags, whichsk_page_frag()can't read without risking triggering a cache miss and slowing down TCP's fast path.This patch creates a new field in struct sock, named
sk_use_task_frag, which sockets with memory reclaim constraints can set to false if they can't safely usecurrent->task_frag. In such cases,sk_page_frag()now always returns the socket'spage_frag(->sk_frag). The first user is sunrpc, which needs to avoid usingcurrent->task_fragbut can keep ->sk_allocation set toGFP_KERNELotherwise.Eventually, it might be possible to simplify
sk_page_frag()by only testing ->sk_use_task_frag and avoid relying on the ->sk_allocation heuristic entirely (assuming other sockets will set ->sk_use_task_frag according to their constraints in the future).The new ->sk_use_task_frag field is placed in a hole in struct sock and belongs to a cache line shared with ->sk_shutdown. Therefore it should be hot and shouldn't have negative performance impacts on TCP's fast path (
sk_shutdownis tested just before thewhile()loop intcp_sendmsg_locked()).Link https://lore.kernel.org/netdev/b4d8cb09c913d3e34f853736f3f5628abfd7f4b6.1656699567.git.gnault@redhat.com/ open hereSigned-off-by Guillaume Nault <[email protected]>Reviewed-by Benjamin Coddington <[email protected]>--- include/net/sock.h | 11 +++++++++-- net/core/sock.c | 1 + 2 files changed, 10 insertions(+), 2 deletions(-)include/net/sock.h +3
-0@@ -318,6 +318,9 @@ struct sk_filter;* @sk_stamp: time stamp of last packet received * @sk_stamp_seq: lock for accessing sk_stamp on 32 bit architectures only * @sk_tsflags: SO_TIMESTAMPING flags +* @sk_use_task_frag: allow sk_page_frag() to use current->task_frag. +* Sockets that can be used under memory reclaim should +* set this to false. * @sk_bind_phc: SO_TIMESTAMPING bind PHC index of PTP virtual clock * for timestamping * @sk_tskey: counter to disambiguate concurrent tstamp requests
include/net/sock.h struct sock { · line 513
Quoted 6 lines
include/net/sock.h +1
-0@@ -505,6 +508,7 @@ struct sock {#endif u16 sk_tsflags;u8 sk_shutdown; +bool sk_use_task_frag;atomic_t sk_tskey;atomic_t sk_zckey;
I'm sorry, but after the post PR -net -> net-next merge, this does not apply cleanly any-more, you need to rebase it once more.
Note that commit b534dc46c8ae ("net_tstamp: add SOF_TIMESTAMPING_OPT_ID_TCP") moved the surronding fields a bit but there is still one byte hole after sk_txtime_unused, before sk_socket.
Thanks!
Paolo
PATCH v3 2/3 Treewide: Stop corrupting socket's task_frag
In reply to Benjamin CoddingtonSince moving to memalloc_nofs_save/restore, SUNRPC has stopped setting the GFP_NOIO flag on sk_allocation which the networking system uses to decide when it is safe to use current->task_frag. The results of this are unexpected corruption in task_frag when SUNRPC is involved in memory reclaim.
The corruption can be seen in crashes, but the root cause is often difficult to ascertain as a crashing machine's stack trace will have no evidence of being near NFS or SUNRPC code. I believe this problem to be much more pervasive than reports to the community may indicate.
Fix this by having kernel users of sockets that may corrupt task_frag due to reclaim set sk_use_task_frag = false. Preemptively correcting this situation for users that still set sk_allocation allows them to convert to memalloc_nofs_save/restore without the same unexpected corruptions that are sure to follow, unlikely to show up in testing, and difficult to bisect.
--- drivers/block/drbd/drbd_receiver.c | 3 +++ drivers/block/nbd.c | 1 + drivers/nvme/host/tcp.c | 1 + drivers/scsi/iscsi_tcp.c | 1 + drivers/usb/usbip/usbip_common.c | 1 + fs/afs/rxrpc.c | 1 + fs/cifs/connect.c | 1 + fs/dlm/lowcomms.c | 2 ++ fs/ocfs2/cluster/tcp.c | 1 + net/9p/trans_fd.c | 1 + net/ceph/messenger.c | 1 + net/sunrpc/xprtsock.c | 3 +++ net/xfrm/espintcp.c | 1 + 13 files changed, 18 insertions(+)
drivers/block/drbd/drbd_receiver.c +3 -0
@@ -1030,6 +1030,9 @@ static int conn_connect(struct drbd_connection *connection) sock.socket->sk->sk_allocation =GFP_NOIO ; msock.socket->sk->sk_allocation =GFP_NOIO ; + sock.socket->sk->sk_use_task_frag = false; + msock.socket->sk->sk_use_task_frag = false; + sock.socket->sk->sk_priority =TC_PRIO_INTERACTIVE_BULK ; msock.socket->sk->sk_priority =TC_PRIO_INTERACTIVE ;
drivers/block/nbd.c +1 -0
@@ -512,6 +512,7 @@ static int sock_xmit(struct nbd_device *nbd, int index, int send, noreclaim_flag =memalloc_noreclaim_save (); do { sock->sk->sk_allocation = GFP_NOIO | __GFP_MEMALLOC; + sock->sk->sk_use_task_frag = false; msg.msg_name = NULL; msg.msg_namelen = 0; msg.msg_control = NULL;
drivers/nvme/host/tcp.c +1 -0
@@ -1537,6 +1537,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid) queue->sock->sk->sk_rcvtimeo =10 *HZ ; queue->sock->sk->sk_allocation =GFP_ATOMIC ; + queue->sock->sk->sk_use_task_frag = false;nvme_tcp_set_queue_io_cpu (queue); queue->request =NULL ; queue->data_remaining =0 ;
drivers/scsi/iscsi_tcp.c +1 -0
@@ -738,6 +738,7 @@ iscsi_sw_tcp_conn_bind(struct iscsi_cls_session *cls_session, sk->sk_reuse =SK_CAN_REUSE ; sk->sk_sndtimeo =15 *HZ ;/* FIXME: make it configurable */ sk->sk_allocation =GFP_ATOMIC ; + sk->sk_use_task_frag = false;sk_set_memalloc (sk);sock_no_linger (sk);
drivers/usb/usbip/usbip_common.c +1 -0
@@ -315,6 +315,7 @@ int usbip_recv(struct socket *sock, void *buf, int size) do { sock->sk->sk_allocation = GFP_NOIO; + sock->sk->sk_use_task_frag = false; result = sock_recvmsg(sock, &msg, MSG_WAITALL); if (result <= 0)
fs/afs/rxrpc.c +1 -0
@@ -46,6 +46,7 @@ int afs_open_socket(struct afs_net *net) goto error_1; socket->sk->sk_allocation =GFP_NOFS ; + socket->sk->sk_use_task_frag = false;/* bind the callback manager's address to make this a server socket */ memset (&srx,0 ,sizeof (srx));
fs/cifs/connect.c +1 -0
@@ -2944,6 +2944,7 @@ generic_ip_connect(struct TCP_Server_Info *server)cifs_dbg (FYI ,"Socket created\n" ); server->ssocket = socket; socket->sk->sk_allocation =GFP_NOFS ; + socket->sk->sk_use_task_frag = false;if (sfamily ==AF_INET6 )cifs_reclassify_socket6 (socket);else
fs/dlm/lowcomms.c +2 -0
@@ -699,6 +699,7 @@ static void add_listen_sock(struct socket *sock, struct listen_connection *con) sk->sk_user_data = con; sk->sk_allocation =GFP_NOFS ; + sk->sk_use_task_frag = false;/* Install a data_ready callback */ sk->sk_data_ready = lowcomms_listen_data_ready;release_sock (sk);
@@ -718,6 +719,7 @@ static void add_sock(struct socket *sock, struct connection *con) sk->sk_write_space = lowcomms_write_space; sk->sk_state_change = lowcomms_state_change; sk->sk_allocation =GFP_NOFS ; + sk->sk_use_task_frag = false; sk->sk_error_report = lowcomms_error_report;release_sock (sk); }
fs/ocfs2/cluster/tcp.c +1 -0
@@ -1604,6 +1604,7 @@ static void o2net_start_connect(struct work_struct *work) sc->sc_sock = sock;/* freed by sc_kref_release */ sock->sk->sk_allocation =GFP_ATOMIC ; + sock->sk->sk_use_task_frag = false; myaddr.sin_family =AF_INET ; myaddr.sin_addr.s_addr = mynode->nd_ipv4_address;
net/9p/trans_fd.c +1 -0
@@ -868,6 +868,7 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket) } csocket->sk->sk_allocation =GFP_NOIO ; + csocket->sk->sk_use_task_frag = false; file =sock_alloc_file (csocket,0 ,NULL );if (IS_ERR (file)) { pr_err("%s (%d): failed to map fd\n",
net/ceph/messenger.c +1 -0
@@ -446,6 +446,7 @@ int ceph_tcp_connect(struct ceph_connection *con)if (ret)return ret; sock->sk->sk_allocation =GFP_NOFS ; + sock->sk->sk_use_task_frag = false;#ifdef CONFIG_LOCKDEP lockdep_set_class (&sock->sk->sk_lock, &socket_class);
net/sunrpc/xprtsock.c +3 -0
@@ -1882,6 +1882,7 @@ static int xs_local_finish_connecting(struct rpc_xprt *xprt, sk->sk_write_space = xs_udp_write_space; sk->sk_state_change = xs_local_state_change; sk->sk_error_report = xs_error_report; + sk->sk_use_task_frag = false;xprt_clear_connected (xprt);
@@ -2082,6 +2083,7 @@ static void xs_udp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) sk->sk_user_data = xprt; sk->sk_data_ready = xs_data_ready; sk->sk_write_space = xs_udp_write_space; + sk->sk_use_task_frag = false;xprt_set_connected (xprt);
@@ -2249,6 +2251,7 @@ static int xs_tcp_finish_connecting(struct rpc_xprt *xprt, struct socket *sock) sk->sk_state_change = xs_tcp_state_change; sk->sk_write_space = xs_tcp_write_space; sk->sk_error_report = xs_error_report; + sk->sk_use_task_frag = false;/* socket options */ sock_reset_flag (sk,SOCK_LINGER );
net/xfrm/espintcp.c +1 -0
@@ -489,6 +489,7 @@ static int espintcp_init_sk(struct sock *sk)/* avoid using task_frag */ sk->sk_allocation =GFP_ATOMIC ; + sk->sk_use_task_frag = false;return 0 ;
Signature · 1 line
2.31.1
David Howells
maintainerRe: [PATCH net v3 2/3] Treewide: Stop corrupting socket's task_frag
fs/afs/rxrpc.c int afs_open_socket(struct afs_net *net) · line 52
Quoted 12 lines · Benjamin Coddington
fs/afs/rxrpc.c +1
-0@@ -46,6 +46,7 @@ int afs_open_socket(struct afs_net *net) goto error_1; socket->sk->sk_allocation =GFP_NOFS ; + socket->sk->sk_use_task_frag = false;/* bind the callback manager's address to make this a server socket */ memset (&srx,0 ,sizeof (srx));
Possibly this should be done in net/rxrpc/local_object.c too? Or maybe in udp_sock_create() or sock_create_kern()?
David
Re: [PATCH net v3 2/3] Treewide: Stop corrupting socket's task_frag
Quoted 10 lines · David Howells
Quoted 7 lines · Benjamin Coddington
fs/afs/rxrpc.c +0
-0@@ -46,6 +46,7 @@ int afs_open_socket(struct afs_net *net) goto error_1;
Quoted 8 lines
fs/afs/rxrpc.c int afs_open_socket(struct afs_net *net) · line 52
Quoted 5 lines
fs/afs/rxrpc.c +1
-0socket->sk->sk_allocation =GFP_NOFS ; + socket->sk->sk_use_task_frag = false;/* bind the callback manager's address to make this a server socket */ memset (&srx,0 ,sizeof (srx));Possibly this should be done in
net/rxrpc/local_object.ctoo? Or maybe inudp_sock_create()orsock_create_kern()?
UDP tunnels typically don't need to set sk_use_task_frag, as they don't call sk_page_frag(). One exception would be if they called ip_append_data() (or ip6_append_data()), but none of them seem to do that (and I can't see any reason why they would).
And net/rxrpc/local_object.c doesn't seems very different in this regard.
Maybe setting sk_use_task_frag in fs/afs/rxrpc.c was overzealous but I'm not familiar enough with the AF_RXRPC family to tell. If AF_RXRPC sockets can't call sk_page_frag() and have no reason to do so in the future, then it should be safe to drop this chunk.
Quoted 2 lines
David
David Howells
maintainerRe: [PATCH net v3 2/3] Treewide: Stop corrupting socket's task_frag
Quoted 4 lines · Guillaume Nault
Maybe setting
sk_use_task_fraginfs/afs/rxrpc.cwas overzealous but I'm not familiar enough with theAF_RXRPCfamily to tell. IfAF_RXRPCsockets can't callsk_page_frag()and have no reason to do so in the future, then it should be safe to drop this chunk.
As of this merge window, AF_RXRPC doesn't actually allocate sk_buffs apart from when it calls skb_unshare(). It does steal the incoming sk_buffs from the UDP socket it uses as a transport, but they're allocated in the IP/IP6 stack somewhere.
The UDP transport socket, on the other hand, will allocate sk_buffs for transmission, but rxrpc sends an entire UDP packet at a time, each with a single sendmsg call.
Further, this mostly now moved such that the UDP sendmsg calls are performed inside an I/O thread. The application thread does not interact directly with the UDP transport socket.
David
Re: [PATCH net v3 2/3] Treewide: Stop corrupting socket's task_frag
Quoted 17 lines · David Howells
Quoted 4 lines · Guillaume Nault
Maybe setting
sk_use_task_fraginfs/afs/rxrpc.cwas overzealous but I'm not familiar enough with theAF_RXRPCfamily to tell. IfAF_RXRPCsockets can't callsk_page_frag()and have no reason to do so in the future, then it should be safe to drop this chunk.As of this merge window,
AF_RXRPCdoesn't actually allocatesk_buffsapart from when it callsskb_unshare(). It does steal the incomingsk_buffsfrom the UDP socket it uses as a transport, but they're allocated in the IP/IP6 stack somewhere.The UDP transport socket, on the other hand, will allocate
sk_buffsfor transmission, but rxrpc sends an entire UDP packet at a time, each with a single sendmsg call.
Quoted 5 lines
Further, this mostly now moved such that the UDP sendmsg calls are performed inside an I/O thread. The application thread does not interact directly with the UDP transport socket.
David
Thanks for the explanations. Looks like we could drop the fs/afs/rxrpc.c chunk then.
PATCH v3 3/3 net: simplify sk_page_frag
In reply to Benjamin CoddingtonNow that in-kernel socket users that may recurse during reclaim have benn converted to sk_use_task_frag = false, we can have sk_page_frag() simply check that value.
--- include/net/sock.h | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
include/net/sock.h +2 -7
@@ -2564,19 +2564,14 @@ static inline void sk_stream_moderate_sndbuf(struct sock *sk)* Both direct reclaim and page faults can nest inside other * socket operations and end up recursing into sk_page_frag() * while it's already in use: explicitly avoid task page_frag -* usage if the caller is potentially doing any of them. -* This assumes that page fault handlers use the GFP_NOFS flags or -* explicitly disable sk_use_task_frag. +* when users disable sk_use_task_frag. * * Return: a per task page_frag if context allows that, * otherwise a per socket one. */ static inline struct page_frag *sk_page_frag (struct sock *sk) { -if (sk->sk_use_task_frag && - (sk->sk_allocation & (__GFP_DIRECT_RECLAIM | __GFP_MEMALLOC | - __GFP_FS)) == - (__GFP_DIRECT_RECLAIM | __GFP_FS)) +if (sk->sk_use_task_frag)return ¤t->task_frag;return &sk->sk_frag;
Signature · 1 line
2.31.1