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

[Khalid Elmously]

The bug was introduced in ebpf in mainline 5.8. gke and gkeop have ebpf backported from 5.8 so they are affected. Regular 5.4 did not get the updated ebpf so is unaffected by the bug.


On 2021-04-05 06:46:42 , Tim Gardner wrote:
> Why isn't this patch destined for focal:linux from which linux-gke and
> linux-gkeop are derived ?
> 
> rtg
> 
> On 4/1/21 8:10 PM, 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>
> > ---
> >   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;
> > 
> 
> -- 
> -----------
> Tim Gardner
> Canonical, Inc