| Message ID | 20240523222128.786137-2-khazhy@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] iscsi_tcp: do not bind sockets that already have extra callbacks | expand |
On 5/23/24 5:21 PM, Khazhismel Kumykov wrote: > This fixes issue where misbehaving userspace initiator could bind the > same connection multiple times, which would leak the old connection > socket without cleaning it up. > > For iscsi_tcp, it calls iscsi_suspend_tx directly in stop_conn. Update > this to iscsi_conn_unbind, which matches the lifecycle of other drivers, > and clears the CONN_FLAG_BOUND. > > Suggested-by: Mike Christie <michael.christie@oracle.com> > Signed-off-by: Khazhismel Kumykov <khazhy@google.com> > --- > drivers/scsi/iscsi_tcp.c | 2 +- > drivers/scsi/libiscsi.c | 6 ++++++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > index deb9252e02e6..1d93404515ae 100644 > --- a/drivers/scsi/iscsi_tcp.c > +++ b/drivers/scsi/iscsi_tcp.c > @@ -696,7 +696,7 @@ static void iscsi_sw_tcp_conn_stop(struct iscsi_cls_conn *cls_conn, int flag) > wake_up_interruptible(sk_sleep(sock->sk)); > > /* stop xmit side */ > - iscsi_suspend_tx(conn); > + iscsi_conn_unbind(cls_conn, true); > > /* stop recv side and release socket */ > iscsi_sw_tcp_release_conn(conn); > diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c > index 0fda8905eabd..0fb98eb53584 100644 > --- a/drivers/scsi/libiscsi.c > +++ b/drivers/scsi/libiscsi.c > @@ -3453,6 +3453,12 @@ int iscsi_conn_bind(struct iscsi_cls_session *cls_session, > struct iscsi_conn *conn = cls_conn->dd_data; > > spin_lock_bh(&session->frwd_lock); > + if (test_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags)) { > + spin_unlock_bh(&session->frwd_lock); > + return -EBUSY; > + } > + If some of the driver's bind_conn callout fails then it could leave the ISCSI_CONN_FLAG_BOUND bit set, and this will fail on a retry. It's ok to call iscsi_conn_bind at the end of the bind_conn callout like how iscsi_tcp and cxgbi* do. So for iscsi_iser.c, be_iscsi.c, bnx2i_iscsi.c, and qedi_iscsi.c you need to move their iscsi_conn_bind to the end of their bind_conn callout. You could also keep the the existing flow but add unwind/goto error handling to those drivers, but I think just moving the iscsi_conn_bind call to the end of the function is easier and it syncs all the drivers behavior which is nice.
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index deb9252e02e6..1d93404515ae 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -696,7 +696,7 @@ static void iscsi_sw_tcp_conn_stop(struct iscsi_cls_conn *cls_conn, int flag) wake_up_interruptible(sk_sleep(sock->sk)); /* stop xmit side */ - iscsi_suspend_tx(conn); + iscsi_conn_unbind(cls_conn, true); /* stop recv side and release socket */ iscsi_sw_tcp_release_conn(conn); diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index 0fda8905eabd..0fb98eb53584 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -3453,6 +3453,12 @@ int iscsi_conn_bind(struct iscsi_cls_session *cls_session, struct iscsi_conn *conn = cls_conn->dd_data; spin_lock_bh(&session->frwd_lock); + if (test_bit(ISCSI_CONN_FLAG_BOUND, &conn->flags)) { + spin_unlock_bh(&session->frwd_lock); + return -EBUSY; + } + + if (is_leading) session->leadconn = conn;
This fixes issue where misbehaving userspace initiator could bind the same connection multiple times, which would leak the old connection socket without cleaning it up. For iscsi_tcp, it calls iscsi_suspend_tx directly in stop_conn. Update this to iscsi_conn_unbind, which matches the lifecycle of other drivers, and clears the CONN_FLAG_BOUND. Suggested-by: Mike Christie <michael.christie@oracle.com> Signed-off-by: Khazhismel Kumykov <khazhy@google.com> --- drivers/scsi/iscsi_tcp.c | 2 +- drivers/scsi/libiscsi.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)