Re: [PATCH RESEND] iscsi: Don't destroy session if there are outstanding connections

[Lee Duncan <LDuncan@suse.com>]

On 1/13/20 2:36 PM, Gabriel Krisman Bertazi wrote:
> Gabriel Krisman Bertazi <krisman@collabora.com> writes:
> 
>> From: Nick Black <nlb@google.com>
>>
>> Hi,
>>
>> I thought this was already committed for some reason, until it bit me
>> again today.  Any opposition to this one?
> 
> Hi,
> 
> Pinging this patch.  Any oposion?
> 
>>> 8
>>
>> A faulty userspace that calls destroy_session() before destroying the
>> connections can trigger the failure.  This patch prevents the
>> issue by refusing to destroy the session if there are outstanding
>> connections.
>>
>> ------------[ cut here ]------------
>> kernel BUG at mm/slub.c:306!
>> invalid opcode: 0000 [#1] SMP PTI
>> CPU: 1 PID: 1224 Comm: iscsid Not tainted 5.4.0-rc2.iscsi+ #7
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
>> RIP: 0010:__slab_free+0x181/0x350
>> [...]
>> [ 1209.686056] RSP: 0018:ffffa93d4074fae0 EFLAGS: 00010246
>> [ 1209.686694] RAX: ffff934efa5ad800 RBX: 000000008010000a RCX: ffff934efa5ad800
>> [ 1209.687651] RDX: ffff934efa5ad800 RSI: ffffeb4041e96b00 RDI: ffff934efd402c40
>> [ 1209.688582] RBP: ffffa93d4074fb80 R08: 0000000000000001 R09: ffffffffbb5dfa26
>> [ 1209.689425] R10: ffff934efa5ad800 R11: 0000000000000001 R12: ffffeb4041e96b00
>> [ 1209.690285] R13: ffff934efa5ad800 R14: ffff934efd402c40 R15: 0000000000000000
>> [ 1209.691213] FS:  00007f7945dfb540(0000) GS:ffff934efda80000(0000) knlGS:0000000000000000
>> [ 1209.692316] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 1209.693013] CR2: 000055877fd3da80 CR3: 0000000077384000 CR4: 00000000000006e0
>> [ 1209.693897] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> [ 1209.694773] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> [ 1209.695631] Call Trace:
>> [ 1209.695957]  ? __wake_up_common_lock+0x8a/0xc0
>> [ 1209.696712]  iscsi_pool_free+0x26/0x40
>> [ 1209.697263]  iscsi_session_teardown+0x2f/0xf0
>> [ 1209.698117]  iscsi_sw_tcp_session_destroy+0x45/0x60
>> [ 1209.698831]  iscsi_if_rx+0xd88/0x14e0
>> [ 1209.699370]  netlink_unicast+0x16f/0x200
>> [ 1209.699932]  netlink_sendmsg+0x21a/0x3e0
>> [ 1209.700446]  sock_sendmsg+0x4f/0x60
>> [ 1209.700902]  ___sys_sendmsg+0x2ae/0x320
>> [ 1209.701451]  ? cp_new_stat+0x150/0x180
>> [ 1209.701922]  __sys_sendmsg+0x59/0xa0
>> [ 1209.702357]  do_syscall_64+0x52/0x160
>> [ 1209.702812]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 1209.703419] RIP: 0033:0x7f7946433914
>> [...]
>> [ 1209.706084] RSP: 002b:00007fffb99f2378 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>> [ 1209.706994] RAX: ffffffffffffffda RBX: 000055bc869eac20 RCX: 00007f7946433914
>> [ 1209.708082] RDX: 0000000000000000 RSI: 00007fffb99f2390 RDI: 0000000000000005
>> [ 1209.709120] RBP: 00007fffb99f2390 R08: 000055bc84fe9320 R09: 00007fffb99f1f07
>> [ 1209.710110] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000038
>> [ 1209.711085] R13: 000055bc8502306e R14: 0000000000000000 R15: 0000000000000000
>>  Modules linked in:
>>  ---[ end trace a2d933ede7f730d8 ]---
>>
>> Co-developed-by: Salman Qazi <sqazi@google.com>
>> Signed-off-by: Salman Qazi <sqazi@google.com>
>> Co-developed-by: Junho Ryu <jayr@google.com>
>> Signed-off-by: Junho Ryu <jayr@google.com>
>> Co-developed-by: Khazhismel Kumykov <khazhy@google.com>
>> Signed-off-by: Khazhismel Kumykov <khazhy@google.com>
>> Signed-off-by: Nick Black <nlb@google.com>
>> Co-developed-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>> ---
>>  drivers/scsi/iscsi_tcp.c            |  4 ++++
>>  drivers/scsi/scsi_transport_iscsi.c | 26 +++++++++++++++++++++++---
>>  2 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
>> index 0bc63a7ab41c..b5dd1caae5e9 100644
>> --- a/drivers/scsi/iscsi_tcp.c
>> +++ b/drivers/scsi/iscsi_tcp.c
>> @@ -887,6 +887,10 @@ iscsi_sw_tcp_session_create(struct iscsi_endpoint *ep, uint16_t cmds_max,
>>  static void iscsi_sw_tcp_session_destroy(struct iscsi_cls_session *cls_session)
>>  {
>>  	struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
>> +	struct iscsi_session *session = cls_session->dd_data;
>> +
>> +	if (WARN_ON_ONCE(session->leadconn))
>> +		return;
>>  
>>  	iscsi_tcp_r2tpool_free(cls_session->dd_data);
>>  	iscsi_session_teardown(cls_session);
>> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
>> index ed8d9709b9b9..271afea654e2 100644
>> --- a/drivers/scsi/scsi_transport_iscsi.c
>> +++ b/drivers/scsi/scsi_transport_iscsi.c
>> @@ -2947,6 +2947,24 @@ iscsi_set_path(struct iscsi_transport *transport, struct iscsi_uevent *ev)
>>  	return err;
>>  }
>>  
>> +static int iscsi_session_has_conns(int sid)
>> +{
>> +	struct iscsi_cls_conn *conn;
>> +	unsigned long flags;
>> +	int found = 0;
>> +
>> +	spin_lock_irqsave(&connlock, flags);
>> +	list_for_each_entry(conn, &connlist, conn_list) {
>> +		if (iscsi_conn_get_sid(conn) == sid) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&connlock, flags);
>> +
>> +	return found;
>> +}
>> +
>>  static int
>>  iscsi_set_iface_params(struct iscsi_transport *transport,
>>  		       struct iscsi_uevent *ev, uint32_t len)
>> @@ -3524,10 +3542,12 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
>>  		break;
>>  	case ISCSI_UEVENT_DESTROY_SESSION:
>>  		session = iscsi_session_lookup(ev->u.d_session.sid);
>> -		if (session)
>> -			transport->destroy_session(session);
>> -		else
>> +		if (!session)
>>  			err = -EINVAL;
>> +		else if (iscsi_session_has_conns(ev->u.d_session.sid))
>> +			err = -EBUSY;
>> +		else
>> +			transport->destroy_session(session);
>>  		break;
>>  	case ISCSI_UEVENT_UNBIND_SESSION:
>>  		session = iscsi_session_lookup(ev->u.d_session.sid);
>> -- 
>> 2.24.1
> 

Please add my:

Reviewed-by: Lee Duncan <lduncan@suse.com>