[PATCH] scsi: target: iscsi: set memalloc_noio with loopback network connections

[PATCH] scsi: target: iscsi: set memalloc_noio with loopback network connections

[David Jeffery]

If an admin connects an iscsi initiator to an iscsi target on the same
system, the iscsi connection is vulnerable to deadlocks during memory
allocations. Memory allocations in the target task accepting the I/O from
the initiator can wait on the initiator's I/O when the system is under
memory pressure, causing a deadlock situation between the iscsi target and
initiator.

When in this configuration, the deadlock scenario can be avoided by use of
GFP_NOIO allocations. Rather than force all configurations to use NOIO,
memalloc_noio_save/restore can be used to force GFP_NOIO allocations only
when in this loopback configuration.

Signed-off-by: David Jeffery <djeffery@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index baf4da7bb3b4..a68e47e2cdf9 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -16,6 +16,7 @@
 #include <linux/vmalloc.h>
 #include <linux/idr.h>
 #include <linux/delay.h>
+#include <linux/sched/mm.h>
 #include <linux/sched/signal.h>
 #include <asm/unaligned.h>
 #include <linux/inet.h>
@@ -4168,7 +4169,10 @@ int iscsi_target_rx_thread(void *arg)
 {
 	int rc;
 	struct iscsit_conn *conn = arg;
+	struct dst_entry *dst;
 	bool conn_freed = false;
+	bool loopback = false;
+	unsigned int flags;
 
 	/*
 	 * Allow ourselves to be interrupted by SIGINT so that a
@@ -4186,8 +4190,25 @@ int iscsi_target_rx_thread(void *arg)
 	if (!conn->conn_transport->iscsit_get_rx_pdu)
 		return 0;
 
+	/*
+	 * If the iscsi connection is over a loopback device from using
+	 * iscsi and iscsit on the same system, we need to set memalloc_noio to
+	 * prevent memory allocation deadlocks between target and initiator.
+	 */
+	rcu_read_lock();
+	dst = rcu_dereference(conn->sock->sk->sk_dst_cache);
+	if (dst && dst->dev && dst->dev->flags & IFF_LOOPBACK)
+		loopback = true;
+	rcu_read_unlock();
+
+	if (loopback)
+		flags = memalloc_noio_save();
+
 	conn->conn_transport->iscsit_get_rx_pdu(conn);
 
+	if (loopback)
+		memalloc_noio_restore(flags);
+
 	if (!signal_pending(current))
 		atomic_set(&conn->transport_failed, 1);
 	iscsit_take_action_for_connection_exit(conn, &conn_freed);
-- 
2.39.1

Re: [PATCH] scsi: target: iscsi: set memalloc_noio with loopback network connections

[Laurence Oberman]

On Wed, 2023-02-08 at 15:09 -0500, David Jeffery wrote:
> If an admin connects an iscsi initiator to an iscsi target on the
> same
> system, the iscsi connection is vulnerable to deadlocks during memory
> allocations. Memory allocations in the target task accepting the I/O
> from
> the initiator can wait on the initiator's I/O when the system is
> under
> memory pressure, causing a deadlock situation between the iscsi
> target and
> initiator.
> 
> When in this configuration, the deadlock scenario can be avoided by
> use of
> GFP_NOIO allocations. Rather than force all configurations to use
> NOIO,
> memalloc_noio_save/restore can be used to force GFP_NOIO allocations
> only
> when in this loopback configuration.
> 
> Signed-off-by: David Jeffery <djeffery@redhat.com>
> ---
>  drivers/target/iscsi/iscsi_target.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/target/iscsi/iscsi_target.c
> b/drivers/target/iscsi/iscsi_target.c
> index baf4da7bb3b4..a68e47e2cdf9 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -16,6 +16,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/idr.h>
>  #include <linux/delay.h>
> +#include <linux/sched/mm.h>
>  #include <linux/sched/signal.h>
>  #include <asm/unaligned.h>
>  #include <linux/inet.h>
> @@ -4168,7 +4169,10 @@ int iscsi_target_rx_thread(void *arg)
>  {
>  	int rc;
>  	struct iscsit_conn *conn = arg;
> +	struct dst_entry *dst;
>  	bool conn_freed = false;
> +	bool loopback = false;
> +	unsigned int flags;
>  
>  	/*
>  	 * Allow ourselves to be interrupted by SIGINT so that a
> @@ -4186,8 +4190,25 @@ int iscsi_target_rx_thread(void *arg)
>  	if (!conn->conn_transport->iscsit_get_rx_pdu)
>  		return 0;
>  
> +	/*
> +	 * If the iscsi connection is over a loopback device from using
> +	 * iscsi and iscsit on the same system, we need to set
> memalloc_noio to
> +	 * prevent memory allocation deadlocks between target and
> initiator.
> +	 */
> +	rcu_read_lock();
> +	dst = rcu_dereference(conn->sock->sk->sk_dst_cache);
> +	if (dst && dst->dev && dst->dev->flags & IFF_LOOPBACK)
> +		loopback = true;
> +	rcu_read_unlock();
> +
> +	if (loopback)
> +		flags = memalloc_noio_save();
> +
>  	conn->conn_transport->iscsit_get_rx_pdu(conn);
>  
> +	if (loopback)
> +		memalloc_noio_restore(flags);
> +
>  	if (!signal_pending(current))
>  		atomic_set(&conn->transport_failed, 1);
>  	iscsit_take_action_for_connection_exit(conn, &conn_freed);


I had mentioned to Mike that this was already tested at a large
customer and in our labs and resolved the deadlocks .

Regards
Laurence Oberman

Re: [PATCH] scsi: target: iscsi: set memalloc_noio with loopback network connections

[Laurence Oberman]

On Wed, 2023-02-08 at 15:58 -0500, Laurence Oberman wrote:
> On Wed, 2023-02-08 at 15:09 -0500, David Jeffery wrote:
> > If an admin connects an iscsi initiator to an iscsi target on the
> > same
> > system, the iscsi connection is vulnerable to deadlocks during
> > memory
> > allocations. Memory allocations in the target task accepting the
> > I/O
> > from
> > the initiator can wait on the initiator's I/O when the system is
> > under
> > memory pressure, causing a deadlock situation between the iscsi
> > target and
> > initiator.
> > 
> > When in this configuration, the deadlock scenario can be avoided by
> > use of
> > GFP_NOIO allocations. Rather than force all configurations to use
> > NOIO,
> > memalloc_noio_save/restore can be used to force GFP_NOIO
> > allocations
> > only
> > when in this loopback configuration.
> > 
> > Signed-off-by: David Jeffery <djeffery@redhat.com>
> > ---
> >  drivers/target/iscsi/iscsi_target.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/target/iscsi/iscsi_target.c
> > b/drivers/target/iscsi/iscsi_target.c
> > index baf4da7bb3b4..a68e47e2cdf9 100644
> > --- a/drivers/target/iscsi/iscsi_target.c
> > +++ b/drivers/target/iscsi/iscsi_target.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/vmalloc.h>
> >  #include <linux/idr.h>
> >  #include <linux/delay.h>
> > +#include <linux/sched/mm.h>
> >  #include <linux/sched/signal.h>
> >  #include <asm/unaligned.h>
> >  #include <linux/inet.h>
> > @@ -4168,7 +4169,10 @@ int iscsi_target_rx_thread(void *arg)
> >  {
> >  	int rc;
> >  	struct iscsit_conn *conn = arg;
> > +	struct dst_entry *dst;
> >  	bool conn_freed = false;
> > +	bool loopback = false;
> > +	unsigned int flags;
> >  
> >  	/*
> >  	 * Allow ourselves to be interrupted by SIGINT so that a
> > @@ -4186,8 +4190,25 @@ int iscsi_target_rx_thread(void *arg)
> >  	if (!conn->conn_transport->iscsit_get_rx_pdu)
> >  		return 0;
> >  
> > +	/*
> > +	 * If the iscsi connection is over a loopback device from using
> > +	 * iscsi and iscsit on the same system, we need to set
> > memalloc_noio to
> > +	 * prevent memory allocation deadlocks between target and
> > initiator.
> > +	 */
> > +	rcu_read_lock();
> > +	dst = rcu_dereference(conn->sock->sk->sk_dst_cache);
> > +	if (dst && dst->dev && dst->dev->flags & IFF_LOOPBACK)
> > +		loopback = true;
> > +	rcu_read_unlock();
> > +
> > +	if (loopback)
> > +		flags = memalloc_noio_save();
> > +
> >  	conn->conn_transport->iscsit_get_rx_pdu(conn);
> >  
> > +	if (loopback)
> > +		memalloc_noio_restore(flags);
> > +
> >  	if (!signal_pending(current))
> >  		atomic_set(&conn->transport_failed, 1);
> >  	iscsit_take_action_for_connection_exit(conn, &conn_freed);
> 
> I had mentioned to Mike that this was already tested at a large
> customer and in our labs and resolved the deadlocks .
> 
> Regards
> Laurence Oberman
> 

Tested-by:   Laurence Oberman <loberman@redhat.com>
Reviewed-by: Laurence Oberman <loberman@redhat.com>

I hate  to nag here but we have a pressing customer issue and are keen
to get others to weigh in here.

Regards
Laurence



Thanks
Laurence

Re: [PATCH] scsi: target: iscsi: set memalloc_noio with loopback network connections

[Maurizio Lombardi]

st 8. 2. 2023 v 21:10 odesílatel David Jeffery <djeffery@redhat.com> napsal:
>
>
> +       /*
> +        * If the iscsi connection is over a loopback device from using
> +        * iscsi and iscsit on the same system, we need to set memalloc_noio to
> +        * prevent memory allocation deadlocks between target and initiator.
> +        */
> +       rcu_read_lock();
> +       dst = rcu_dereference(conn->sock->sk->sk_dst_cache);
> +       if (dst && dst->dev && dst->dev->flags & IFF_LOOPBACK)
> +               loopback = true;
> +       rcu_read_unlock();

Hi Mike,
I tested it, it works. The customer also confirmed that it fixes the
deadlock on his setup.

Maurizio

Re: [PATCH] scsi: target: iscsi: set memalloc_noio with loopback network connections

[Mike Christie]

On 2/13/23 5:59 AM, Maurizio Lombardi wrote:
> st 8. 2. 2023 v 21:10 odesílatel David Jeffery <djeffery@redhat.com> napsal:
>>
>>
>> +       /*
>> +        * If the iscsi connection is over a loopback device from using
>> +        * iscsi and iscsit on the same system, we need to set memalloc_noio to
>> +        * prevent memory allocation deadlocks between target and initiator.
>> +        */
>> +       rcu_read_lock();
>> +       dst = rcu_dereference(conn->sock->sk->sk_dst_cache);
>> +       if (dst && dst->dev && dst->dev->flags & IFF_LOOPBACK)
>> +               loopback = true;
>> +       rcu_read_unlock();
> 
> Hi Mike,
> I tested it, it works. The customer also confirmed that it fixes the
> deadlock on his setup.

You never responded about why/how it's used in production. Is it some sort
of clustering or container or what?

The login related code can still swing back on you if it's run for a relogin.
It would happen if we overqueue and a nop timesout because the iscsi recv thread
is waiting for backend resources like a request/queue slot, or if management tools
disable/enable the tpgt for reconfigs, etc.

Re: [PATCH] scsi: target: iscsi: set memalloc_noio with loopback network connections

[Laurence Oberman]

On Mon, 2023-02-13 at 10:22 -0600, Mike Christie wrote:
> On 2/13/23 5:59 AM, Maurizio Lombardi wrote:
> > st 8. 2. 2023 v 21:10 odesílatel David Jeffery <djeffery@redhat.com
> > > napsal:
> > > 
> > > +       /*
> > > +        * If the iscsi connection is over a loopback device from
> > > using
> > > +        * iscsi and iscsit on the same system, we need to set
> > > memalloc_noio to
> > > +        * prevent memory allocation deadlocks between target and
> > > initiator.
> > > +        */
> > > +       rcu_read_lock();
> > > +       dst = rcu_dereference(conn->sock->sk->sk_dst_cache);
> > > +       if (dst && dst->dev && dst->dev->flags & IFF_LOOPBACK)
> > > +               loopback = true;
> > > +       rcu_read_unlock();
> > 
> > Hi Mike,
> > I tested it, it works. The customer also confirmed that it fixes
> > the
> > deadlock on his setup.
> 
> You never responded about why/how it's used in production. Is it some
> sort
> of clustering or container or what?
> 
> The login related code can still swing back on you if it's run for a
> relogin.
> It would happen if we overqueue and a nop timesout because the iscsi
> recv thread
> is waiting for backend resources like a request/queue slot, or if
> management tools
> disable/enable the tpgt for reconfigs, etc.
> 

Hi Mike,
The use case described is as follows:


"This customer moved their on-premise system to the cloud.
Their on-premise system runs with two servers and one external storage
and uses data mirroring software to mirror data.

When moving to the cloud, customer wanted to implement a data mirror
using data mirror software with two instances to reduce the cost of
using the cloud infrastructure.
To build a system with two instances, we use iSCSI to mirror data
between a local disk on one instance and a local disk on the other
instance.
We coexist iSCSI initiator and target so that data mirroring software
can access each disk through a unified interface."

Thanks
Laurence