| Message ID | c2ec184226acd21a191ccc1aa46a1d7e43ca7104.1669036433.git.bcodding@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hmm. Having to set a flag to not accidentally corrupt per-task state seems a bit fragile. Wouldn't it make sense to find a way to opt into the feature only for sockets created from the syscall layer?
On 29 Nov 2022, at 9:02, Christoph Hellwig wrote: > Hmm. Having to set a flag to not accidentally corrupt per-task > state seems a bit fragile. Wouldn't it make sense to find a way to opt > into the feature only for sockets created from the syscall layer? It's totally fragile, and that's why it's currently broken in production. The fragile ship sailed when networking decided to depend on users setting the socket's GFP_ flags correctly to avoid corruption. Meantime, this problem needs fixing in a way that makes everyone happy. This fix doesn't make it less fragile, but it may (hopefully) address the previous criticisms enough that something gets done to fix it. Ben
On Tue, 2022-11-29 at 15:02 +0100, Christoph Hellwig wrote: > Hmm. Having to set a flag to not accidentally corrupt per-task > state seems a bit fragile. Wouldn't it make sense to find a way to opt > into the feature only for sockets created from the syscall layer? I agree that that would be cleaner. task_frag should have been an opt-in thing all along. That change regressed all of the in-kernel users of sockets. Where would be the right place to set that flag for only userland sockets? A lot of the in-kernel socket users hook into the socket API at a fairly high-level. 9P and CIFS, for instance, call __sock_create. We could set it in the syscall handlers (and maybe in iouring) I suppose, but that seems like the wrong thing to do too. In the absence of a clean place to do this, I think we're going to be stuck doing it the way Ben has proposed...
On Tue, Nov 29, 2022 at 03:02:42PM +0100, Christoph Hellwig wrote: > Hmm. Having to set a flag to not accidentally corrupt per-task > state seems a bit fragile. Wouldn't it make sense to find a way to opt > into the feature only for sockets created from the syscall layer? That's something I originally considered. But, as far as I can see, nbd needs this flag _and_ uses sockets created in user space. So it'd still need to opt out manually.
On Tue, Nov 29, 2022 at 11:47:47AM -0500, Benjamin Coddington wrote: > On 29 Nov 2022, at 9:02, Christoph Hellwig wrote: > > > Hmm. Having to set a flag to not accidentally corrupt per-task > > state seems a bit fragile. Wouldn't it make sense to find a way to opt > > into the feature only for sockets created from the syscall layer? > > It's totally fragile, and that's why it's currently broken in production. > The fragile ship sailed when networking decided to depend on users setting > the socket's GFP_ flags correctly to avoid corruption. > > Meantime, this problem needs fixing in a way that makes everyone happy. > This fix doesn't make it less fragile, but it may (hopefully) address the > previous criticisms enough that something gets done to fix it. Also, let's remember that while we're discussing how the kernel sould work in an ideal world, the reality is that production NFS systems crash randomly upon memory reclaim since commit a1231fda7e94 ("SUNRPC: Set memalloc_nofs_save() on all rpciod/xprtiod jobs"). Fixing that is just a matter of re-introducing GFP_NOFS on SUNRPC sockets (which has been proposed several times already). Then we'll have plenty of time to argue about how networking should use the per-task page_frag and how to remove GFP_NOFS in the long term.
On Mon, 2022-11-21 at 08:35 -0500, Benjamin Coddington wrote: > 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 <philipp.reisner@linbit.com> > CC: Lars Ellenberg <lars.ellenberg@linbit.com> > CC: "Christoph Böhmwalder" <christoph.boehmwalder@linbit.com> > CC: Jens Axboe <axboe@kernel.dk> > CC: Josef Bacik <josef@toxicpanda.com> > CC: Keith Busch <kbusch@kernel.org> > CC: Christoph Hellwig <hch@lst.de> > CC: Sagi Grimberg <sagi@grimberg.me> > CC: Lee Duncan <lduncan@suse.com> > CC: Chris Leech <cleech@redhat.com> > CC: Mike Christie <michael.christie@oracle.com> > CC: "James E.J. Bottomley" <jejb@linux.ibm.com> > CC: "Martin K. Petersen" <martin.petersen@oracle.com> > CC: Valentina Manea <valentina.manea.m@gmail.com> > CC: Shuah Khan <shuah@kernel.org> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > CC: David Howells <dhowells@redhat.com> > CC: Marc Dionne <marc.dionne@auristor.com> > CC: Steve French <sfrench@samba.org> > CC: Christine Caulfield <ccaulfie@redhat.com> > CC: David Teigland <teigland@redhat.com> > CC: Mark Fasheh <mark@fasheh.com> > CC: Joel Becker <jlbec@evilplan.org> > CC: Joseph Qi <joseph.qi@linux.alibaba.com> > CC: Eric Van Hensbergen <ericvh@gmail.com> > CC: Latchesar Ionkov <lucho@ionkov.net> > CC: Dominique Martinet <asmadeus@codewreck.org> > CC: "David S. Miller" <davem@davemloft.net> > CC: Eric Dumazet <edumazet@google.com> > CC: Jakub Kicinski <kuba@kernel.org> > CC: Paolo Abeni <pabeni@redhat.com> > CC: Ilya Dryomov <idryomov@gmail.com> > CC: Xiubo Li <xiubli@redhat.com> > CC: Chuck Lever <chuck.lever@oracle.com> > CC: Jeff Layton <jlayton@kernel.org> > CC: Trond Myklebust <trond.myklebust@hammerspace.com> > CC: Anna Schumaker <anna@kernel.org> > CC: drbd-dev@lists.linbit.com > CC: linux-block@vger.kernel.org > CC: linux-kernel@vger.kernel.org > CC: nbd@other.debian.org > CC: linux-nvme@lists.infradead.org > CC: open-iscsi@googlegroups.com > CC: linux-scsi@vger.kernel.org > CC: linux-usb@vger.kernel.org > CC: linux-afs@lists.infradead.org > CC: linux-cifs@vger.kernel.org > CC: samba-technical@lists.samba.org > CC: cluster-devel@redhat.com > CC: ocfs2-devel@oss.oracle.com > CC: v9fs-developer@lists.sourceforge.net > CC: netdev@vger.kernel.org > CC: ceph-devel@vger.kernel.org > CC: linux-nfs@vger.kernel.org > > Suggested-by: Guillaume Nault <gnault@redhat.com> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> I think this is the most feasible way out of the existing issue, and I think this patchset should go via the networking tree, targeting the Linux 6.2. If someone has disagreement with the above, please speak! Thanks, Paolo
On Fri, 09 Dec 2022 13:37:08 +0100 Paolo Abeni wrote: > I think this is the most feasible way out of the existing issue, and I > think this patchset should go via the networking tree, targeting the > Linux 6.2. FWIW some fields had been moved so this will not longer apply cleanly, see b534dc46c8ae016. But I think we can apply it to net since the merge window is upon us? Just a heads up.
On Fri, 2022-12-09 at 08:11 -0800, Jakub Kicinski wrote: > On Fri, 09 Dec 2022 13:37:08 +0100 Paolo Abeni wrote: > > I think this is the most feasible way out of the existing issue, and I > > think this patchset should go via the networking tree, targeting the > > Linux 6.2. > > FWIW some fields had been moved so this will not longer apply cleanly, > see b534dc46c8ae016. But I think we can apply it to net since the merge > window is upon us? Just a heads up. We will need an additional revision, see my reply to patch 3/3. I think the -net tree should be the appropriate target. Thanks, Paolo
diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c index af4c7d65490b..09ad8d82c200 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 2a709daefbc4..815ee631ed30 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -514,6 +514,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 d5871fd6f769..e01d78858cb4 100644 --- a/drivers/nvme/host/tcp.c +++ b/drivers/nvme/host/tcp.c @@ -1531,6 +1531,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, 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 29b1bd755afe..733e540d0abf 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -733,6 +733,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 2ab99244bc31..76bfc6e43881 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 7ae6f2c08153..c2b0d6f59f79 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2935,6 +2935,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 a4e84e8d94c8..4cf29ac3c428 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 e758978b44be..96f803499323 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -851,6 +851,7 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket) return -ENOMEM; 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 d3bb656308b4..cad8e0ca8432 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 e976007f4fd0..d3170b753dfc 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); @@ -2083,6 +2084,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); @@ -2250,6 +2252,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);
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 <philipp.reisner@linbit.com> CC: Lars Ellenberg <lars.ellenberg@linbit.com> CC: "Christoph Böhmwalder" <christoph.boehmwalder@linbit.com> CC: Jens Axboe <axboe@kernel.dk> CC: Josef Bacik <josef@toxicpanda.com> CC: Keith Busch <kbusch@kernel.org> CC: Christoph Hellwig <hch@lst.de> CC: Sagi Grimberg <sagi@grimberg.me> CC: Lee Duncan <lduncan@suse.com> CC: Chris Leech <cleech@redhat.com> CC: Mike Christie <michael.christie@oracle.com> CC: "James E.J. Bottomley" <jejb@linux.ibm.com> CC: "Martin K. Petersen" <martin.petersen@oracle.com> CC: Valentina Manea <valentina.manea.m@gmail.com> CC: Shuah Khan <shuah@kernel.org> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> CC: David Howells <dhowells@redhat.com> CC: Marc Dionne <marc.dionne@auristor.com> CC: Steve French <sfrench@samba.org> CC: Christine Caulfield <ccaulfie@redhat.com> CC: David Teigland <teigland@redhat.com> CC: Mark Fasheh <mark@fasheh.com> CC: Joel Becker <jlbec@evilplan.org> CC: Joseph Qi <joseph.qi@linux.alibaba.com> CC: Eric Van Hensbergen <ericvh@gmail.com> CC: Latchesar Ionkov <lucho@ionkov.net> CC: Dominique Martinet <asmadeus@codewreck.org> CC: "David S. Miller" <davem@davemloft.net> CC: Eric Dumazet <edumazet@google.com> CC: Jakub Kicinski <kuba@kernel.org> CC: Paolo Abeni <pabeni@redhat.com> CC: Ilya Dryomov <idryomov@gmail.com> CC: Xiubo Li <xiubli@redhat.com> CC: Chuck Lever <chuck.lever@oracle.com> CC: Jeff Layton <jlayton@kernel.org> CC: Trond Myklebust <trond.myklebust@hammerspace.com> CC: Anna Schumaker <anna@kernel.org> CC: drbd-dev@lists.linbit.com CC: linux-block@vger.kernel.org CC: linux-kernel@vger.kernel.org CC: nbd@other.debian.org CC: linux-nvme@lists.infradead.org CC: open-iscsi@googlegroups.com CC: linux-scsi@vger.kernel.org CC: linux-usb@vger.kernel.org CC: linux-afs@lists.infradead.org CC: linux-cifs@vger.kernel.org CC: samba-technical@lists.samba.org CC: cluster-devel@redhat.com CC: ocfs2-devel@oss.oracle.com CC: v9fs-developer@lists.sourceforge.net CC: netdev@vger.kernel.org CC: ceph-devel@vger.kernel.org CC: linux-nfs@vger.kernel.org Suggested-by: Guillaume Nault <gnault@redhat.com> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- 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 +++ 12 files changed, 17 insertions(+)