RE: [PATCH] nvme-tcp: proper handling of tcp socket closing flows
From: Grupi, Elad <hidden>
Date: 2021-01-28 23:43:30
Release work might get invoked if nvmet_tcp_set_queue_sock is completed successfully and set sk_user_data, but sk_state_change is triggered by network stack before queue_work_on is invoked. That case - there is a race between release_work and accept_work. -----Original Message----- From: Sagi Grimberg <sagi@grimberg.me> Sent: Friday, 29 January 2021 1:34 To: Grupi, Elad; linux-nvme@lists.infradead.org Subject: Re: [PATCH] nvme-tcp: proper handling of tcp socket closing flows [EXTERNAL EMAIL]
That will work if sk_state_change is called under sk_callback_lock.
It is.
In addition, need to flush_work(&queue->port->accept_work) in nvmet_tcp_release_queue_work because accept_work is still using queue struct after unlocking sk_callback_lock.
But sk->sk_user_data was not set, so I don't see how the release work can invoke.
quoted hunk ↗ jump to hunk
What about this instead? --diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c index c41902f7ce39..6388d18ca7c2 100644 --- a/drivers/nvme/target/tcp.c +++ b/drivers/nvme/target/tcp.c@@ -1494,16 +1494,28 @@ static int nvmet_tcp_set_queue_sock(struct nvmet_tcp_queue *queue) ip_sock_set_tos(sock->sk, inet->rcv_tos); write_lock_bh(&sock->sk->sk_callback_lock); - sock->sk->sk_user_data = queue; - queue->data_ready = sock->sk->sk_data_ready; - sock->sk->sk_data_ready = nvmet_tcp_data_ready; - queue->state_change = sock->sk->sk_state_change; - sock->sk->sk_state_change = nvmet_tcp_state_change; - queue->write_space = sock->sk->sk_write_space; - sock->sk->sk_write_space = nvmet_tcp_write_space; + switch (sk->sk_state) { + case TCP_FIN_WAIT1: + case TCP_CLOSE_WAIT: + case TCP_CLOSE: + /* + * If the socket is already closing, don't even start + * consuming it + */ + ret = -ENOTCONN; + break; + default: + sock->sk->sk_user_data = queue; + queue->data_ready = sock->sk->sk_data_ready; + sock->sk->sk_data_ready = nvmet_tcp_data_ready; + queue->state_change = sock->sk->sk_state_change; + sock->sk->sk_state_change = nvmet_tcp_state_change; + queue->write_space = sock->sk->sk_write_space; + sock->sk->sk_write_space = nvmet_tcp_write_space; + } write_unlock_bh(&sock->sk->sk_callback_lock); - return 0; + return ret; } --
_______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme