[PATCH V2] target: iscsi: use GFP_NOIO with loopback connections

[PATCH V2] target: iscsi: use GFP_NOIO with loopback connections

[Maurizio Lombardi]

When an iscsi initiator is connected to a target running on the
same machine, the system may trigger a deadlock when working
under memory pressure.
This may happen, for example, when the iscsi rx thread tries to
allocate memory and a memory reclaim is performed, the rx thread may
therefore end up waiting for the initiator to complete I/O operations,
causing a deadlock.

Fix the issue by using memalloc_noio_*() to enable implicit GFP_NOIO
in the vulnerable code paths, when the connection is in loopback.


v2: Check the IFF_LOOPBACK flag in the iscsit_accept_np() callback,
    where the conn->sock pointer is initialized.

Suggested-by: David Jeffery <djeffery@redhat.com>
Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c       | 19 ++++++++++++++++---
 drivers/target/iscsi/iscsi_target_login.c |  7 +++++++
 include/target/iscsi/iscsi_target_core.h  |  1 +
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index baf4da7bb3b4..4d997a049bf7 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3918,9 +3918,9 @@ static int iscsit_handle_response_queue(struct iscsit_conn *conn)
 
 int iscsi_target_tx_thread(void *arg)
 {
-	int ret = 0;
+	int ret = 0, flags;
 	struct iscsit_conn *conn = arg;
-	bool conn_freed = false;
+	bool conn_freed = false, loopback;
 
 	/*
 	 * Allow ourselves to be interrupted by SIGINT so that a
@@ -3928,6 +3928,10 @@ int iscsi_target_tx_thread(void *arg)
 	 */
 	allow_signal(SIGINT);
 
+	loopback = conn->loopback;
+	if (loopback)
+		flags = memalloc_noio_save();
+
 	while (!kthread_should_stop()) {
 		/*
 		 * Ensure that both TX and RX per connection kthreads
@@ -3966,6 +3970,9 @@ int iscsi_target_tx_thread(void *arg)
 	if (conn->conn_state != TARG_CONN_STATE_IN_LOGIN)
 		iscsit_take_action_for_connection_exit(conn, &conn_freed);
 out:
+	if (loopback)
+		memalloc_noio_restore(flags);
+
 	if (!conn_freed) {
 		while (!kthread_should_stop()) {
 			msleep(100);
@@ -4166,7 +4173,7 @@ static void iscsit_get_rx_pdu(struct iscsit_conn *conn)
 
 int iscsi_target_rx_thread(void *arg)
 {
-	int rc;
+	int rc, flags;
 	struct iscsit_conn *conn = arg;
 	bool conn_freed = false;
 
@@ -4186,8 +4193,14 @@ int iscsi_target_rx_thread(void *arg)
 	if (!conn->conn_transport->iscsit_get_rx_pdu)
 		return 0;
 
+	if (conn->loopback)
+		flags = memalloc_noio_save();
+
 	conn->conn_transport->iscsit_get_rx_pdu(conn);
 
+	if (conn->loopback)
+		memalloc_noio_restore(flags);
+
 	if (!signal_pending(current))
 		atomic_set(&conn->transport_failed, 1);
 	iscsit_take_action_for_connection_exit(conn, &conn_freed);
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 27e448c2d066..4a195e9e623e 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -17,6 +17,7 @@
 #include <linux/tcp.h>        /* TCP_NODELAY */
 #include <net/ip.h>
 #include <net/ipv6.h>         /* ipv6_addr_v4mapped() */
+#include <net/sock.h>
 #include <scsi/iscsi_proto.h>
 #include <target/target_core_base.h>
 #include <target/target_core_fabric.h>
@@ -960,6 +961,7 @@ int iscsi_target_setup_login_socket(
 int iscsit_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
 {
 	struct socket *new_sock, *sock = np->np_socket;
+	struct dst_entry *dst;
 	struct sockaddr_in sock_in;
 	struct sockaddr_in6 sock_in6;
 	int rc;
@@ -971,6 +973,11 @@ int iscsit_accept_np(struct iscsi_np *np, struct iscsit_conn *conn)
 	conn->sock = new_sock;
 	conn->login_family = np->np_sockaddr.ss_family;
 
+	dst = sk_dst_get(conn->sock->sk);
+	if (dst && dst->dev && dst->dev->flags & IFF_LOOPBACK)
+		conn->loopback = true;
+	dst_release(dst);
+
 	if (np->np_sockaddr.ss_family == AF_INET6) {
 		memset(&sock_in6, 0, sizeof(struct sockaddr_in6));
 
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 94d06ddfd80a..aa8d4026e32e 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -538,6 +538,7 @@ struct iscsit_conn {
 	struct sockaddr_storage local_sockaddr;
 	int			conn_usage_count;
 	int			conn_waiting_on_uc;
+	bool			loopback;
 	atomic_t		check_immediate_queue;
 	atomic_t		conn_logout_remove;
 	atomic_t		connection_exit;
-- 
2.31.1

Re: [PATCH V2] target: iscsi: use GFP_NOIO with loopback connections

[Mike Christie]

On 3/27/23 5:45 AM, Maurizio Lombardi wrote:
> When an iscsi initiator is connected to a target running on the
> same machine, the system may trigger a deadlock when working
> under memory pressure.
> This may happen, for example, when the iscsi rx thread tries to
> allocate memory and a memory reclaim is performed, the rx thread may
> therefore end up waiting for the initiator to complete I/O operations,
> causing a deadlock.
> 
> Fix the issue by using memalloc_noio_*() to enable implicit GFP_NOIO
> in the vulnerable code paths, when the connection is in loopback.
> 
> 
> v2: Check the IFF_LOOPBACK flag in the iscsit_accept_np() callback,
>     where the conn->sock pointer is initialized.
> 
> Suggested-by: David Jeffery <djeffery@redhat.com>
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/target/iscsi/iscsi_target.c       | 19 ++++++++++++++++---
>  drivers/target/iscsi/iscsi_target_login.c |  7 +++++++
>  include/target/iscsi/iscsi_target_core.h  |  1 +
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index baf4da7bb3b4..4d997a049bf7 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -3918,9 +3918,9 @@ static int iscsit_handle_response_queue(struct iscsit_conn *conn)
>  
>  int iscsi_target_tx_thread(void *arg)
>  {
> -	int ret = 0;
> +	int ret = 0, flags;
>  	struct iscsit_conn *conn = arg;
> -	bool conn_freed = false;
> +	bool conn_freed = false, loopback;
>  
>  	/*
>  	 * Allow ourselves to be interrupted by SIGINT so that a
> @@ -3928,6 +3928,10 @@ int iscsi_target_tx_thread(void *arg)
>  	 */
>  	allow_signal(SIGINT);
>  
> +	loopback = conn->loopback;
> +	if (loopback)
> +		flags = memalloc_noio_save();
> +
>  	while (!kthread_should_stop()) {
>  		/*
>  		 * Ensure that both TX and RX per connection kthreads
> @@ -3966,6 +3970,9 @@ int iscsi_target_tx_thread(void *arg)
>  	if (conn->conn_state != TARG_CONN_STATE_IN_LOGIN)
>  		iscsit_take_action_for_connection_exit(conn, &conn_freed);
>  out:
> +	if (loopback)
> +		memalloc_noio_restore(flags);
> +
>  	if (!conn_freed) {
>  		while (!kthread_should_stop()) {
>  			msleep(100);
> @@ -4166,7 +4173,7 @@ static void iscsit_get_rx_pdu(struct iscsit_conn *conn)
>  
>  int iscsi_target_rx_thread(void *arg)
>  {
> -	int rc;
> +	int rc, flags;
>  	struct iscsit_conn *conn = arg;
>  	bool conn_freed = false;
>  
> @@ -4186,8 +4193,14 @@ int iscsi_target_rx_thread(void *arg)
>  	if (!conn->conn_transport->iscsit_get_rx_pdu)
>  		return 0;
>  
> +	if (conn->loopback)
> +		flags = memalloc_noio_save();

Do you need to do sk_set_memalloc on the socket as well or do we do all
net allocations from the the rx/tx threads? I know there are some net
allocations that are not going to take place from those threads, but
did you not set the socket flags because they are small allocations or
just forgot, or something else?

Re: [PATCH V2] target: iscsi: use GFP_NOIO with loopback connections

[michael.christie]

On 3/27/23 5:45 AM, Maurizio Lombardi wrote:
> When an iscsi initiator is connected to a target running on the
> same machine, the system may trigger a deadlock when working
> under memory pressure.
> This may happen, for example, when the iscsi rx thread tries to
> allocate memory and a memory reclaim is performed, the rx thread may
> therefore end up waiting for the initiator to complete I/O operations,
> causing a deadlock.
> 

I'm still not sure this patch is the right way to go. Here are my issues:

1. We have a driver for local use, tcm loop. IMO, you should be using
that.

I don't really buy into the argument about it makes the tooling easier,
because the tools have to create a local LIO target. They can just as easily
make a loop one vs a iscsi one. For the iscsi discovery/login phase, you don't
need to run the iscsiadm command for the loop case so that saves you a step.

I would also not want to add another user that needs to dip in the memory
pools for skbs on the same system.


2. Block drivers in linux have to be able to make forward progress.

	2.A We use mempools, preallocate, etc to make sure we have the resources
	needed to be able to handle writes out to our device.

	For the iscsit side of things we use the memalloc related flags to
	dip into pools.

	For the LIO side of things, we don't have anything with or without
	this patch, so the iblock_req allocation could fail for example.

	2.B We can't swing back on our self in the main IO path, so we don't
	use GFP_KERNEL there.

	For the iscsit side of things and for iscsit's specific implementation
	this patch handles the LIO side issue. The drawback for the LIO side
	of things is that it relies on iscsit rx thread submitting and
	executing IO from that thread. If we ever do something like fix the
	issue where we can block in the submission path by deferring to the
	submission thread, then the patch does not work.

	For the general LIO side of things for loop the patch does not help.


I think if I'm wrong about #1, and we are going to support this then we:

1. need to handle the iscsit allocations using whatever we are supposed to use
like you are doing in this patch (memalloc_noio_save from the tx/rx thread and/or
sk_set_memalloc on the sock).

2. We also should fix the issue of LIO core using the wrong flags for this type
of setup so it works for loop and also for iscsit. For this, we should have a
gfp_t allocation field or flag on the se_cmd or se_session so LIO core and use
the right gfp_t.

3. For the second part of the problem of making forward progress in LIO
core/backends by using pools/preallocation then I'm not sure what to do.