[SRU][F/gke][PATCH 1/1] scsi: iscsi: iscsi_tcp: Avoid holding spinlock while calling getpeername()

[Khaled Elmously]

On 2021-04-01 10:33:06 , Stefan Bader wrote:
> On 01.04.21 10:07, Khalid Elmously wrote:
> > From: Mark Mielke <mark.mielke at gmail.com>
> > 
> > BugLink: https://bugs.launchpad.net/bugs/1921825
> > 
> > The kernel may fail to boot or devices may fail to come up when
> > initializing iscsi_tcp devices starting with Linux 5.8.
> > 
> > Commit a79af8a64d39 ("[SCSI] iscsi_tcp: use iscsi_conn_get_addr_param
> > libiscsi function") introduced getpeername() within the session spinlock.
> > 
> > Commit 1b66d253610c ("bpf: Add get{peer, sock}name attach types for
> > sock_addr") introduced BPF_CGROUP_RUN_SA_PROG_LOCK() within getpeername(),
> > which acquires a mutex and when used from iscsi_tcp devices can now lead to
> > "BUG: scheduling while atomic:" and subsequent damage.
> > 
> > Ensure that the spinlock is released before calling getpeername() or
> > getsockname(). sock_hold() and sock_put() are used to ensure that the
> > socket reference is preserved until after the getpeername() or
> > getsockname() complete.
> > 
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=1877345
> > Link: https://lkml.org/lkml/2020/7/28/1085
> > Link: https://lkml.org/lkml/2020/8/31/459
> > Link: https://lore.kernel.org/r/20200928043329.606781-1-mark.mielke@gmail.com
> > Fixes: a79af8a64d39 ("[SCSI] iscsi_tcp: use iscsi_conn_get_addr_param libiscsi function")
> > Fixes: 1b66d253610c ("bpf: Add get{peer, sock}name attach types for sock_addr")
> > Cc: stable at vger.kernel.org
> > Reported-by: Marc Dionne <marc.c.dionne at gmail.com>
> > Tested-by: Marc Dionne <marc.c.dionne at gmail.com>
> > Reviewed-by: Mike Christie <michael.christie at oracle.com>
> > Signed-off-by: Mark Mielke <mark.mielke at gmail.com>
> > Signed-off-by: Martin K. Petersen <martin.petersen at oracle.com>
> > (cherry picked from commit bcf3a2953d36bbfb9bd44ccb3db0897d935cc485)
> > Signed-off-by: Khalid Elmously <khalid.elmously at canonical.com>
> > ---
> 
> Just to clarify:
> - groovy:linux got the fix already
> - focal:linux has not backported the change introducing the problem
> - focal:linux-gcp (?) would that not also need this?

f/linux-gcp does not need this fix. bpf was updated to be on par with 5.8 in gke (and gkeop) only.

> - focal:linux-gke submitted for this
> - focal:linux-gkeop (?) was the bug introduced there, too?

Yes, it was. So gkeop needs the fix too. Thanks for pointing it out.
I will send a v2 with an updated subject.

> 
> -Stefan
> 
> >   drivers/scsi/iscsi_tcp.c | 22 +++++++++++++++-------
> >   1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> > index b5dd1caae5e9..d10efb66cf19 100644
> > --- a/drivers/scsi/iscsi_tcp.c
> > +++ b/drivers/scsi/iscsi_tcp.c
> > @@ -736,6 +736,7 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
> >   	struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
> >   	struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
> >   	struct sockaddr_in6 addr;
> > +	struct socket *sock;
> >   	int rc;
> >   	switch(param) {
> > @@ -747,13 +748,17 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
> >   			spin_unlock_bh(&conn->session->frwd_lock);
> >   			return -ENOTCONN;
> >   		}
> > +		sock = tcp_sw_conn->sock;
> > +		sock_hold(sock->sk);
> > +		spin_unlock_bh(&conn->session->frwd_lock);
> > +
> >   		if (param == ISCSI_PARAM_LOCAL_PORT)
> > -			rc = kernel_getsockname(tcp_sw_conn->sock,
> > +			rc = kernel_getsockname(sock,
> >   						(struct sockaddr *)&addr);
> >   		else
> > -			rc = kernel_getpeername(tcp_sw_conn->sock,
> > +			rc = kernel_getpeername(sock,
> >   						(struct sockaddr *)&addr);
> > -		spin_unlock_bh(&conn->session->frwd_lock);
> > +		sock_put(sock->sk);
> >   		if (rc < 0)
> >   			return rc;
> > @@ -775,6 +780,7 @@ static int iscsi_sw_tcp_host_get_param(struct Scsi_Host *shost,
> >   	struct iscsi_tcp_conn *tcp_conn;
> >   	struct iscsi_sw_tcp_conn *tcp_sw_conn;
> >   	struct sockaddr_in6 addr;
> > +	struct socket *sock;
> >   	int rc;
> >   	switch (param) {
> > @@ -789,16 +795,18 @@ static int iscsi_sw_tcp_host_get_param(struct Scsi_Host *shost,
> >   			return -ENOTCONN;
> >   		}
> >   		tcp_conn = conn->dd_data;
> > -
> >   		tcp_sw_conn = tcp_conn->dd_data;
> > -		if (!tcp_sw_conn->sock) {
> > +		sock = tcp_sw_conn->sock;
> > +		if (!sock) {
> >   			spin_unlock_bh(&session->frwd_lock);
> >   			return -ENOTCONN;
> >   		}
> > +		sock_hold(sock->sk);
> > +		spin_unlock_bh(&session->frwd_lock);
> > -		rc = kernel_getsockname(tcp_sw_conn->sock,
> > +		rc = kernel_getsockname(sock,
> >   					(struct sockaddr *)&addr);
> > -		spin_unlock_bh(&session->frwd_lock);
> > +		sock_put(sock->sk);
> >   		if (rc < 0)
> >   			return rc;
> > 
> 
> 




> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team