Message-ID: <6fc6c8c2a94aa46b99f276bf32650ec43d79328d.camel@redhat.com>
Subject: Re: [PATCH] scsi: target: iscsi: set memalloc_noio with loopback
 network connections
From:   Laurence Oberman <loberman@redhat.com>
To:     David Jeffery <djeffery@redhat.com>, target-devel@vger.kernel.org
Cc:     "Martin K . Petersen" <martin.petersen@oracle.com>,
        Mike Christie <michael.christie@oracle.com>,
        Maurizio Lombardi <mlombard@redhat.com>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Thu, 09 Feb 2023 14:26:44 -0500
In-Reply-To: <caff1add88d1e46c962ee52aae441769450c4dd9.camel@redhat.com>
References: <20230208200957.14073-1-djeffery@redhat.com>
         <caff1add88d1e46c962ee52aae441769450c4dd9.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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