[net,v3,2/3] Treewide: Stop corrupting socket's task_frag

Commit Message

Benjamin Coddington Dec. 13, 2022, 11:10 a.m. UTC

Since 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.

CC: Philipp Reisner <[e-mail]>
CC: Lars Ellenberg <[e-mail]>
CC: "Christoph Böhmwalder" <[e-mail]>
CC: Jens Axboe <[e-mail]>
CC: Josef Bacik <[e-mail]>
CC: Keith Busch <[e-mail]>
CC: Christoph Hellwig <[e-mail]>
CC: Sagi Grimberg <[e-mail]>
CC: Lee Duncan <[e-mail]>
CC: Chris Leech <[e-mail]>
CC: Mike Christie <[e-mail]>
CC: "James E.J. Bottomley" <[e-mail]>
CC: "Martin K. Petersen" <[e-mail]>
CC: Valentina Manea <[e-mail]>
CC: Shuah Khan <[e-mail]>
CC: Greg Kroah-Hartman <[e-mail]>
CC: David Howells <[e-mail]>
CC: Marc Dionne <[e-mail]>
CC: Steve French <[e-mail]>
CC: Christine Caulfield <[e-mail]>
CC: David Teigland <[e-mail]>
CC: Mark Fasheh <[e-mail]>
CC: Joel Becker <[e-mail]>
CC: Joseph Qi <[e-mail]>
CC: Eric Van Hensbergen <[e-mail]>
CC: Latchesar Ionkov <[e-mail]>
CC: Dominique Martinet <[e-mail]>
CC: "David S. Miller" <[e-mail]>
CC: Eric Dumazet <[e-mail]>
CC: Jakub Kicinski <[e-mail]>
CC: Paolo Abeni <[e-mail]>
CC: Ilya Dryomov <[e-mail]>
CC: Xiubo Li <[e-mail]>
CC: Chuck Lever <[e-mail]>
CC: Jeff Layton <[e-mail]>
CC: Trond Myklebust <[e-mail]>
CC: Anna Schumaker <[e-mail]>
CC: Steffen Klassert <[e-mail]>
CC: Herbert Xu <[e-mail]>
CC: [e-mail]

Suggested-by: Guillaume Nault <[e-mail]>
Signed-off-by: Benjamin Coddington <[e-mail]>
Reviewed-by: Guillaume Nault <[e-mail]>
---
 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(+)

Comments

David Howells Dec. 15, 2022, 12:12 p.m. UTC | #1

Benjamin Coddington <[e-mail]> wrote:

> diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> index eccc3cd0cb70..ac75ad18db83 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c
> @@ -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

Guillaume Nault Dec. 15, 2022, 1:59 p.m. UTC | #2

On Thu, Dec 15, 2022 at 12:12:42PM +0000, David Howells wrote:
> 
> Benjamin Coddington <[e-mail]> wrote:
> 
> > diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> > index eccc3cd0cb70..ac75ad18db83 100644
> > --- a/fs/afs/rxrpc.c
> > +++ b/fs/afs/rxrpc.c
> > @@ -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()?

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.

> David
>

David Howells Dec. 15, 2022, 2:36 p.m. UTC | #3

Guillaume Nault <[e-mail]> wrote:

> 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.

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

Guillaume Nault Dec. 15, 2022, 3:37 p.m. UTC | #4

On Thu, Dec 15, 2022 at 02:36:52PM +0000, David Howells wrote:
> 
> Guillaume Nault <[e-mail]> wrote:
> 
> > 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.
> 
> 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

Thanks for the explanations. Looks like we could drop the fs/afs/rxrpc.c
chunk then.

Patch

diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
index ee69d50ba4fd..0d3f910ae347 100644
--- a/drivers/block/drbd/drbd_receiver.c
+++ b/drivers/block/drbd/drbd_receiver.c
@@ -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;
 
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5cffd96ef2d7..3a46b776354d 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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;
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 9b47dcb2a7d9..fe772d6c4c96 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -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;
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 5fb1f364e815..1d1cf641937c 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -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);
 
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 053a2bca4c47..e15ae6ca95ea 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -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)
diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
index eccc3cd0cb70..ac75ad18db83 100644
--- a/fs/afs/rxrpc.c
+++ b/fs/afs/rxrpc.c
@@ -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));
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9db9527c61cf..d84f1660cacb 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -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
diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 59f64c596233..120be782edbc 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -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);
 }
diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
index f660c0dbdb63..3eaafa5e5ec4 100644
--- a/fs/ocfs2/cluster/tcp.c
+++ b/fs/ocfs2/cluster/tcp.c
@@ -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;
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 07db2f436d44..d9120f14684b 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -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",
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index dfa237fbd5a3..1d06e114ba3f 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -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);
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 915b9902f673..41ffc2169743 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -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);
diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
index 29a540dcb5a7..4ca2c5927ace 100644
--- a/net/xfrm/espintcp.c
+++ b/net/xfrm/espintcp.c
@@ -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;