[PATCH v2 1/2] nbd: mark the socket dead when a partial send times out

[PATCH v2 1/2] nbd: mark the socket dead when a partial send times out

[Joseph Qi]

nbd_pending_cmd_work() completes the request with BLK_STS_IOERR once its
retry loop passes req->deadline, but leaves nsock->pending set.  Nothing
else clears it: nbd_send_cmd() only does so at its out: label after a
complete send, and nbd_requeue_cmd() does not touch it.

Every later request on that socket then takes this branch in
nbd_handle_cmd():

	if (unlikely(nsock->pending && nsock->pending != req)) {
		nbd_requeue_cmd(cmd);

and is requeued again, forever, since the requeue path does not clear
->pending either.  If the tag is recycled first, ->pending matches the new
request instead and nbd_send_cmd() resumes it in place of the abandoned
one, skipping its header and leaving cmd_cookie alone.

Neither case leaves a usable socket.  The header went out and the rest of
the payload never will, so the stream no longer matches what the server
expects.  Mark the socket dead, which shuts it down and clears the partial
send state.  nbd_xmit_timeout() does the same for a timed out request and
only skips it here because NBD_CMD_PARTIAL_SEND makes it defer to this
work function.

Without this, a write issued after the deadline fires never completes and
the device has to be torn down to recover.

Reproduced by making the resumed send never progress, so the retry loop
runs out req->deadline.  With the socket marked dead a later request is
dispatched instead of requeued, and a reconnect afterwards does clean
O_DIRECT I/O with no oops or warning.

Fixes: 8337b029f788 ("nbd: fix partial sending")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 drivers/block/nbd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ffce519bf008..c2c3dbdd631f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -838,6 +838,14 @@ static void nbd_pending_cmd_work(struct work_struct *work)
 		/* don't bother timeout handler for partial sending */
 		if (READ_ONCE(jiffies) + msecs_to_jiffies(wait_ms) >= deadline) {
 			cmd->status = BLK_STS_IOERR;
+			/*
+			 * The header is on the wire but the rest of the payload
+			 * never will be, so the stream is out of sync with the
+			 * server.  Marking the socket dead also drops the stale
+			 * nsock->pending, which would otherwise make
+			 * nbd_handle_cmd() requeue every later request forever.
+			 */
+			nbd_mark_nsock_dead(nbd, nsock, 1);
 			blk_mq_complete_request(req);
 			break;
 		}
-- 
2.39.3

[PATCH v2 2/2] nbd: fix NULL pointer dereference in nbd_pending_cmd_work()

[Joseph Qi]

nbd_pending_cmd_work() turns nsock->pending into a nbd_cmd and an
nbd_device before it takes any lock:

	struct request *req = nsock->pending;
	struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
	struct nbd_device *nbd = cmd->nbd;

nbd_mark_nsock_dead() clears ->pending, and it can run between
schedule_work() in nbd_sched_pending_work() and the work function
starting: recv_work() on a receive error, sock_shutdown(), or
nbd_xmit_timeout() giving up on some other command, as it defers to
this work function for the one that is partial sending.  Since
blk_mq_rq_to_pdu() is rq + 1, a NULL ->pending becomes a bogus pointer
and reading cmd->nbd oopses:

  BUG: kernel NULL pointer dereference, address: 0000000000000088
  Workqueue: events nbd_pending_cmd_work [nbd]
  RIP: 0010:nbd_pending_cmd_work+0x1b/0x120 [nbd]

The race was forced by clearing ->pending immediately before
schedule_work().  The address is sizeof(struct request), since nbd is
the first member of struct nbd_cmd and blk_mq_rq_to_pdu() is rq + 1.

cancel_work_sync() cannot close the window.  Every nbd_mark_nsock_dead()
caller holds nsock->tx_lock, nbd_xmit_timeout() holds cmd->lock as well,
and the work function takes cmd->lock before tx_lock.  There is also no
teardown point that could cancel it, since the work holds a config_refs
reference and nbd_config_put() therefore never reaches the code that
frees the socket while the work is queued.

Track the request in a field of its own that teardown does not clear, and
fail the request if the socket is gone by the time the work runs.  Its
header is already on the wire, so it cannot complete successfully.

Complete the request only once cmd->lock has been dropped.
blk_mq_complete_request() runs nbd_complete_rq() inline whenever
blk_mq_complete_request_remote() decides not to redirect it, and
nbd_complete_rq() ends the request without taking cmd->lock, so holding
that lock does not keep cmd alive.  Anything the work function still
writes in cmd afterwards can land in a request whose tag has already
been recycled.  The deadline branch had the same problem.

Complete it before dropping the config reference, not after.  If this is
the last one, nbd_config_put() tears the device down and nbd_put() runs
nbd_dev_remove() inline unless NBD_DESTROY_ON_DISCONNECT is set, so
del_gendisk() would wait in blk_mq_freeze_queue_wait() for the
q_usage_counter that this request holds until it is completed.

Clear partial_req under tx_lock, where nbd_sched_pending_work() writes
it.  cmd->lock is per-command, so clearing it after tx_lock is dropped
would race with a store from another command and could hand the freshly
armed work a NULL request to dereference.

Also make re-arming from inside the work loop a no-op rather than a second
schedule_work().  test_and_set_bit() tells the two cases apart, and the
loop re-reads nsock->sent on its next pass, so the resumed send continues
from where it stopped.  This removes the WARN_ON_ONCE that fired on that
path and avoids taking a second config reference for a work item that is
already running.

Tested with the race forced as above: the request completes with EIO
instead of oopsing, and a reconnect afterwards does clean O_DIRECT
reads and writes.  Also re-ran the partial send happy path (64
requests, 64 work invocations, no requeues), the deadline path, a
reconnect with 256 MiB of O_DIRECT writes and reads, and rmmod, all
with no oops, warning or leaked config reference.

Fixes: 8337b029f788 ("nbd: fix partial sending")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
v2: Complete before nbd_config_put() to avoid latent deadlock, suggested
    by sashiko

 drivers/block/nbd.c | 58 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c2c3dbdd631f..9909774dbfbb 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -63,6 +63,7 @@ struct nbd_sock {
 	int fallback_index;
 	int cookie;
 	struct work_struct work;
+	struct request *partial_req;
 };
 
 struct recv_thread_args {
@@ -637,12 +638,24 @@ static void nbd_sched_pending_work(struct nbd_device *nbd,
 {
 	struct request *req = blk_mq_rq_from_pdu(cmd);
 
-	/* pending work should be scheduled only once */
-	WARN_ON_ONCE(test_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags));
-
 	nsock->pending = req;
 	nsock->sent = sent;
-	set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
+
+	/*
+	 * Already armed: this is nbd_pending_cmd_work() re-entering because the
+	 * resumed send was interrupted again.  Its work is still running, so
+	 * just refresh the resume point above and let its loop pick it up
+	 * instead of taking another config reference and requeueing the work.
+	 */
+	if (test_and_set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags))
+		return;
+
+	/*
+	 * nbd_mark_nsock_dead() clears ->pending, so the work function cannot
+	 * rely on it to find the request it owns.  Keep a copy that only it
+	 * clears.
+	 */
+	nsock->partial_req = req;
 	refcount_inc(&nbd->config_refs);
 	schedule_work(&nsock->work);
 }
@@ -817,11 +830,12 @@ static blk_status_t nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
 static void nbd_pending_cmd_work(struct work_struct *work)
 {
 	struct nbd_sock *nsock = container_of(work, struct nbd_sock, work);
-	struct request *req = nsock->pending;
+	struct request *req = nsock->partial_req;
 	struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
 	struct nbd_device *nbd = cmd->nbd;
 	unsigned long deadline = READ_ONCE(req->deadline);
 	unsigned int wait_ms = 2;
+	bool complete = false;
 
 	mutex_lock(&cmd->lock);
 
@@ -830,6 +844,18 @@ static void nbd_pending_cmd_work(struct work_struct *work)
 		goto out;
 
 	mutex_lock(&nsock->tx_lock);
+	/*
+	 * nbd_mark_nsock_dead() can tear the socket down between schedule_work()
+	 * and here, and it clears ->pending and ->sent.  The header is already
+	 * on the wire so this request can never be answered; fail it rather than
+	 * resuming a send on a socket that is gone.
+	 */
+	if (!nsock->pending) {
+		cmd->status = BLK_STS_IOERR;
+		__clear_bit(NBD_CMD_INFLIGHT, &cmd->flags);
+		complete = true;
+		goto unlock;
+	}
 	while (true) {
 		nbd_send_cmd(nbd, cmd, cmd->index);
 		if (!nsock->pending)
@@ -846,16 +872,36 @@ static void nbd_pending_cmd_work(struct work_struct *work)
 			 * nbd_handle_cmd() requeue every later request forever.
 			 */
 			nbd_mark_nsock_dead(nbd, nsock, 1);
-			blk_mq_complete_request(req);
+			complete = true;
 			break;
 		}
 		msleep(wait_ms);
 		wait_ms *= 2;
 	}
+unlock:
+	/*
+	 * nbd_sched_pending_work() writes partial_req under tx_lock, and
+	 * cmd->lock is per-command, so this has to be under tx_lock too.
+	 */
+	nsock->partial_req = NULL;
 	mutex_unlock(&nsock->tx_lock);
 	clear_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
 out:
 	mutex_unlock(&cmd->lock);
+
+	/*
+	 * Complete before nbd_config_put(): if this is the last config
+	 * reference, nbd_put() runs nbd_dev_remove() inline unless
+	 * NBD_DESTROY_ON_DISCONNECT is set, and del_gendisk() would then wait
+	 * in blk_mq_freeze_queue_wait() for the q_usage_counter that this
+	 * request holds until it is completed.  The config reference is what
+	 * keeps nbd itself alive across the completion, but cmd must not be
+	 * touched afterwards, since nbd_complete_rq() may run inline and ends
+	 * the request without taking cmd->lock.
+	 */
+	if (complete)
+		blk_mq_complete_request(req);
+
 	nbd_config_put(nbd);
 }
 
-- 
2.39.3

Re: [PATCH v2 2/2] nbd: fix NULL pointer dereference in nbd_pending_cmd_work()

[Bart Van Assche]

On 9/28/26 2:10 AM, Joseph Qi wrote:
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index c2c3dbdd631f..9909774dbfbb 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -63,6 +63,7 @@ struct nbd_sock {
>   	int fallback_index;
>   	int cookie;
>   	struct work_struct work;
> +	struct request *partial_req;
>   };
>   
>   struct recv_thread_args {
> @@ -637,12 +638,24 @@ static void nbd_sched_pending_work(struct nbd_device *nbd,
>   {
>   	struct request *req = blk_mq_rq_from_pdu(cmd);
>   
> -	/* pending work should be scheduled only once */
> -	WARN_ON_ONCE(test_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags));
> -
>   	nsock->pending = req;
>   	nsock->sent = sent;
> -	set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
> +
> +	/*
> +	 * Already armed: this is nbd_pending_cmd_work() re-entering because the
> +	 * resumed send was interrupted again.  Its work is still running, so
> +	 * just refresh the resume point above and let its loop pick it up
> +	 * instead of taking another config reference and requeueing the work.
> +	 */
> +	if (test_and_set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags))
> +		return;
> +
> +	/*
> +	 * nbd_mark_nsock_dead() clears ->pending, so the work function cannot
> +	 * rely on it to find the request it owns.  Keep a copy that only it
> +	 * clears.
> +	 */
> +	nsock->partial_req = req;
>   	refcount_inc(&nbd->config_refs);
>   	schedule_work(&nsock->work);
>   }
> @@ -817,11 +830,12 @@ static blk_status_t nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
>   static void nbd_pending_cmd_work(struct work_struct *work)
>   {
>   	struct nbd_sock *nsock = container_of(work, struct nbd_sock, work);
> -	struct request *req = nsock->pending;
> +	struct request *req = nsock->partial_req;
>   	struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
>   	struct nbd_device *nbd = cmd->nbd;
>   	unsigned long deadline = READ_ONCE(req->deadline);
>   	unsigned int wait_ms = 2;
> +	bool complete = false;
>   
>   	mutex_lock(&cmd->lock);
>   
> @@ -830,6 +844,18 @@ static void nbd_pending_cmd_work(struct work_struct *work)
>   		goto out;
>   
>   	mutex_lock(&nsock->tx_lock);
> +	/*
> +	 * nbd_mark_nsock_dead() can tear the socket down between schedule_work()
> +	 * and here, and it clears ->pending and ->sent.  The header is already
> +	 * on the wire so this request can never be answered; fail it rather than
> +	 * resuming a send on a socket that is gone.
> +	 */
> +	if (!nsock->pending) {
> +		cmd->status = BLK_STS_IOERR;
> +		__clear_bit(NBD_CMD_INFLIGHT, &cmd->flags);
> +		complete = true;
> +		goto unlock;
> +	}
>   	while (true) {
>   		nbd_send_cmd(nbd, cmd, cmd->index);
>   		if (!nsock->pending)
> @@ -846,16 +872,36 @@ static void nbd_pending_cmd_work(struct work_struct *work)
>   			 * nbd_handle_cmd() requeue every later request forever.
>   			 */
>   			nbd_mark_nsock_dead(nbd, nsock, 1);
> -			blk_mq_complete_request(req);
> +			complete = true;
>   			break;
>   		}
>   		msleep(wait_ms);
>   		wait_ms *= 2;
>   	}
> +unlock:
> +	/*
> +	 * nbd_sched_pending_work() writes partial_req under tx_lock, and
> +	 * cmd->lock is per-command, so this has to be under tx_lock too.
> +	 */
> +	nsock->partial_req = NULL;
>   	mutex_unlock(&nsock->tx_lock);
>   	clear_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
>   out:
>   	mutex_unlock(&cmd->lock);
> +
> +	/*
> +	 * Complete before nbd_config_put(): if this is the last config
> +	 * reference, nbd_put() runs nbd_dev_remove() inline unless
> +	 * NBD_DESTROY_ON_DISCONNECT is set, and del_gendisk() would then wait
> +	 * in blk_mq_freeze_queue_wait() for the q_usage_counter that this
> +	 * request holds until it is completed.  The config reference is what
> +	 * keeps nbd itself alive across the completion, but cmd must not be
> +	 * touched afterwards, since nbd_complete_rq() may run inline and ends
> +	 * the request without taking cmd->lock.
> +	 */
> +	if (complete)
> +		blk_mq_complete_request(req);
> +
>   	nbd_config_put(nbd);
>   }
>   

These changes add significant complexity and hence make the NBD driver
harder to maintain. Has it been considered to increase the request
reference count while nsock->pending != NULL? See also 
req_ref_inc_not_zero() and blk_mq_put_rq_ref().

Thanks,

Bart.

Re: [PATCH v2 2/2] nbd: fix NULL pointer dereference in nbd_pending_cmd_work()

[Joseph Qi]

Hi Bart,

On 9/28/26 10:19 PM, Bart Van Assche wrote:
> On 9/28/26 2:10 AM, Joseph Qi wrote:
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index c2c3dbdd631f..9909774dbfbb 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -63,6 +63,7 @@ struct nbd_sock {
>>       int fallback_index;
>>       int cookie;
>>       struct work_struct work;
>> +    struct request *partial_req;
>>   };
>>     struct recv_thread_args {
>> @@ -637,12 +638,24 @@ static void nbd_sched_pending_work(struct nbd_device *nbd,
>>   {
>>       struct request *req = blk_mq_rq_from_pdu(cmd);
>>   -    /* pending work should be scheduled only once */
>> -    WARN_ON_ONCE(test_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags));
>> -
>>       nsock->pending = req;
>>       nsock->sent = sent;
>> -    set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
>> +
>> +    /*
>> +     * Already armed: this is nbd_pending_cmd_work() re-entering because the
>> +     * resumed send was interrupted again.  Its work is still running, so
>> +     * just refresh the resume point above and let its loop pick it up
>> +     * instead of taking another config reference and requeueing the work.
>> +     */
>> +    if (test_and_set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags))
>> +        return;
>> +
>> +    /*
>> +     * nbd_mark_nsock_dead() clears ->pending, so the work function cannot
>> +     * rely on it to find the request it owns.  Keep a copy that only it
>> +     * clears.
>> +     */
>> +    nsock->partial_req = req;
>>       refcount_inc(&nbd->config_refs);
>>       schedule_work(&nsock->work);
>>   }
>> @@ -817,11 +830,12 @@ static blk_status_t nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
>>   static void nbd_pending_cmd_work(struct work_struct *work)
>>   {
>>       struct nbd_sock *nsock = container_of(work, struct nbd_sock, work);
>> -    struct request *req = nsock->pending;
>> +    struct request *req = nsock->partial_req;
>>       struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
>>       struct nbd_device *nbd = cmd->nbd;
>>       unsigned long deadline = READ_ONCE(req->deadline);
>>       unsigned int wait_ms = 2;
>> +    bool complete = false;
>>         mutex_lock(&cmd->lock);
>>   @@ -830,6 +844,18 @@ static void nbd_pending_cmd_work(struct work_struct *work)
>>           goto out;
>>         mutex_lock(&nsock->tx_lock);
>> +    /*
>> +     * nbd_mark_nsock_dead() can tear the socket down between schedule_work()
>> +     * and here, and it clears ->pending and ->sent.  The header is already
>> +     * on the wire so this request can never be answered; fail it rather than
>> +     * resuming a send on a socket that is gone.
>> +     */
>> +    if (!nsock->pending) {
>> +        cmd->status = BLK_STS_IOERR;
>> +        __clear_bit(NBD_CMD_INFLIGHT, &cmd->flags);
>> +        complete = true;
>> +        goto unlock;
>> +    }
>>       while (true) {
>>           nbd_send_cmd(nbd, cmd, cmd->index);
>>           if (!nsock->pending)
>> @@ -846,16 +872,36 @@ static void nbd_pending_cmd_work(struct work_struct *work)
>>                * nbd_handle_cmd() requeue every later request forever.
>>                */
>>               nbd_mark_nsock_dead(nbd, nsock, 1);
>> -            blk_mq_complete_request(req);
>> +            complete = true;
>>               break;
>>           }
>>           msleep(wait_ms);
>>           wait_ms *= 2;
>>       }
>> +unlock:
>> +    /*
>> +     * nbd_sched_pending_work() writes partial_req under tx_lock, and
>> +     * cmd->lock is per-command, so this has to be under tx_lock too.
>> +     */
>> +    nsock->partial_req = NULL;
>>       mutex_unlock(&nsock->tx_lock);
>>       clear_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
>>   out:
>>       mutex_unlock(&cmd->lock);
>> +
>> +    /*
>> +     * Complete before nbd_config_put(): if this is the last config
>> +     * reference, nbd_put() runs nbd_dev_remove() inline unless
>> +     * NBD_DESTROY_ON_DISCONNECT is set, and del_gendisk() would then wait
>> +     * in blk_mq_freeze_queue_wait() for the q_usage_counter that this
>> +     * request holds until it is completed.  The config reference is what
>> +     * keeps nbd itself alive across the completion, but cmd must not be
>> +     * touched afterwards, since nbd_complete_rq() may run inline and ends
>> +     * the request without taking cmd->lock.
>> +     */
>> +    if (complete)
>> +        blk_mq_complete_request(req);
>> +
>>       nbd_config_put(nbd);
>>   }
>>   
> 
> These changes add significant complexity and hence make the NBD driver
> harder to maintain. Has it been considered to increase the request
> reference count while nsock->pending != NULL? See also req_ref_inc_not_zero() and blk_mq_put_rq_ref().
> 

Thanks for taking a look.

It seems a request reference can't replace the extra pointer, since the
two answer different questions.

A reference keeps the request object alive, but it doesn't stop
nsock->pending from being cleared, which is what breaks the worker:

  nbd_mark_nsock_dead()
    nsock->dead = true;
    nsock->pending = NULL;
    nsock->sent = 0;

  nbd_pending_cmd_work()
    struct request *req = nsock->pending;
    struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
    struct nbd_device *nbd = cmd->nbd;

Nothing between schedule_work() and the worker starting takes a lock the
teardown path also takes, so pinning the request still leaves the worker
reading NULL from ->pending and faulting on cmd->nbd.

Thanks,
Joseph

Re: [PATCH v2 2/2] nbd: fix NULL pointer dereference in nbd_pending_cmd_work()

[Joseph Qi]

Hi,

On 9/29/26 9:02 AM, Joseph Qi wrote:
> Hi Bart,
> 
> On 9/28/26 10:19 PM, Bart Van Assche wrote:
>> On 9/28/26 2:10 AM, Joseph Qi wrote:
>>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>>> index c2c3dbdd631f..9909774dbfbb 100644
>>> --- a/drivers/block/nbd.c
>>> +++ b/drivers/block/nbd.c
>>> @@ -63,6 +63,7 @@ struct nbd_sock {
>>>       int fallback_index;
>>>       int cookie;
>>>       struct work_struct work;
>>> +    struct request *partial_req;
>>>   };
>>>     struct recv_thread_args {
>>> @@ -637,12 +638,24 @@ static void nbd_sched_pending_work(struct nbd_device *nbd,
>>>   {
>>>       struct request *req = blk_mq_rq_from_pdu(cmd);
>>>   -    /* pending work should be scheduled only once */
>>> -    WARN_ON_ONCE(test_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags));
>>> -
>>>       nsock->pending = req;
>>>       nsock->sent = sent;
>>> -    set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
>>> +
>>> +    /*
>>> +     * Already armed: this is nbd_pending_cmd_work() re-entering because the
>>> +     * resumed send was interrupted again.  Its work is still running, so
>>> +     * just refresh the resume point above and let its loop pick it up
>>> +     * instead of taking another config reference and requeueing the work.
>>> +     */
>>> +    if (test_and_set_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags))
>>> +        return;
>>> +
>>> +    /*
>>> +     * nbd_mark_nsock_dead() clears ->pending, so the work function cannot
>>> +     * rely on it to find the request it owns.  Keep a copy that only it
>>> +     * clears.
>>> +     */
>>> +    nsock->partial_req = req;
>>>       refcount_inc(&nbd->config_refs);
>>>       schedule_work(&nsock->work);
>>>   }
>>> @@ -817,11 +830,12 @@ static blk_status_t nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
>>>   static void nbd_pending_cmd_work(struct work_struct *work)
>>>   {
>>>       struct nbd_sock *nsock = container_of(work, struct nbd_sock, work);
>>> -    struct request *req = nsock->pending;
>>> +    struct request *req = nsock->partial_req;
>>>       struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
>>>       struct nbd_device *nbd = cmd->nbd;
>>>       unsigned long deadline = READ_ONCE(req->deadline);
>>>       unsigned int wait_ms = 2;
>>> +    bool complete = false;
>>>         mutex_lock(&cmd->lock);
>>>   @@ -830,6 +844,18 @@ static void nbd_pending_cmd_work(struct work_struct *work)
>>>           goto out;
>>>         mutex_lock(&nsock->tx_lock);
>>> +    /*
>>> +     * nbd_mark_nsock_dead() can tear the socket down between schedule_work()
>>> +     * and here, and it clears ->pending and ->sent.  The header is already
>>> +     * on the wire so this request can never be answered; fail it rather than
>>> +     * resuming a send on a socket that is gone.
>>> +     */
>>> +    if (!nsock->pending) {
>>> +        cmd->status = BLK_STS_IOERR;
>>> +        __clear_bit(NBD_CMD_INFLIGHT, &cmd->flags);
>>> +        complete = true;
>>> +        goto unlock;
>>> +    }
>>>       while (true) {
>>>           nbd_send_cmd(nbd, cmd, cmd->index);
>>>           if (!nsock->pending)
>>> @@ -846,16 +872,36 @@ static void nbd_pending_cmd_work(struct work_struct *work)
>>>                * nbd_handle_cmd() requeue every later request forever.
>>>                */
>>>               nbd_mark_nsock_dead(nbd, nsock, 1);
>>> -            blk_mq_complete_request(req);
>>> +            complete = true;
>>>               break;
>>>           }
>>>           msleep(wait_ms);
>>>           wait_ms *= 2;
>>>       }
>>> +unlock:
>>> +    /*
>>> +     * nbd_sched_pending_work() writes partial_req under tx_lock, and
>>> +     * cmd->lock is per-command, so this has to be under tx_lock too.
>>> +     */
>>> +    nsock->partial_req = NULL;
>>>       mutex_unlock(&nsock->tx_lock);
>>>       clear_bit(NBD_CMD_PARTIAL_SEND, &cmd->flags);
>>>   out:
>>>       mutex_unlock(&cmd->lock);
>>> +
>>> +    /*
>>> +     * Complete before nbd_config_put(): if this is the last config
>>> +     * reference, nbd_put() runs nbd_dev_remove() inline unless
>>> +     * NBD_DESTROY_ON_DISCONNECT is set, and del_gendisk() would then wait
>>> +     * in blk_mq_freeze_queue_wait() for the q_usage_counter that this
>>> +     * request holds until it is completed.  The config reference is what
>>> +     * keeps nbd itself alive across the completion, but cmd must not be
>>> +     * touched afterwards, since nbd_complete_rq() may run inline and ends
>>> +     * the request without taking cmd->lock.
>>> +     */
>>> +    if (complete)
>>> +        blk_mq_complete_request(req);
>>> +
>>>       nbd_config_put(nbd);
>>>   }
>>>   
>>
>> These changes add significant complexity and hence make the NBD driver
>> harder to maintain. Has it been considered to increase the request
>> reference count while nsock->pending != NULL? See also req_ref_inc_not_zero() and blk_mq_put_rq_ref().
>>
> 
> Thanks for taking a look.
> 
> It seems a request reference can't replace the extra pointer, since the
> two answer different questions.
> 
> A reference keeps the request object alive, but it doesn't stop
> nsock->pending from being cleared, which is what breaks the worker:
> 
>   nbd_mark_nsock_dead()
>     nsock->dead = true;
>     nsock->pending = NULL;
>     nsock->sent = 0;
> 
>   nbd_pending_cmd_work()
>     struct request *req = nsock->pending;
>     struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
>     struct nbd_device *nbd = cmd->nbd;
> 
> Nothing between schedule_work() and the worker starting takes a lock the
> teardown path also takes, so pinning the request still leaves the worker
> reading NULL from ->pending and faulting on cmd->nbd.
> 

Any more comments on this? Or am I missing something?

Thanks,
Joseph

Re: [PATCH v2 1/2] nbd: mark the socket dead when a partial send times out

[Bart Van Assche]

On 9/28/26 2:10 AM, Joseph Qi wrote:
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index ffce519bf008..c2c3dbdd631f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -838,6 +838,14 @@ static void nbd_pending_cmd_work(struct work_struct *work)
>   		/* don't bother timeout handler for partial sending */
>   		if (READ_ONCE(jiffies) + msecs_to_jiffies(wait_ms) >= deadline) {
>   			cmd->status = BLK_STS_IOERR;
> +			/*
> +			 * The header is on the wire but the rest of the payload
> +			 * never will be, so the stream is out of sync with the
> +			 * server.  Marking the socket dead also drops the stale
> +			 * nsock->pending, which would otherwise make
> +			 * nbd_handle_cmd() requeue every later request forever.
> +			 */
> +			nbd_mark_nsock_dead(nbd, nsock, 1);
>   			blk_mq_complete_request(req);
>   			break;
>   		}

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 1/2] nbd: mark the socket dead when a partial send times out

[Ming Lei]

On Mon, Sep 28, 2026 at 05:10:20PM +0800, Joseph Qi wrote:
> nbd_pending_cmd_work() completes the request with BLK_STS_IOERR once its
> retry loop passes req->deadline, but leaves nsock->pending set.  Nothing
> else clears it: nbd_send_cmd() only does so at its out: label after a
> complete send, and nbd_requeue_cmd() does not touch it.
> 
> Every later request on that socket then takes this branch in
> nbd_handle_cmd():
> 
> 	if (unlikely(nsock->pending && nsock->pending != req)) {
> 		nbd_requeue_cmd(cmd);
> 
> and is requeued again, forever, since the requeue path does not clear
> ->pending either.  If the tag is recycled first, ->pending matches the new
> request instead and nbd_send_cmd() resumes it in place of the abandoned
> one, skipping its header and leaving cmd_cookie alone.
> 
> Neither case leaves a usable socket.  The header went out and the rest of
> the payload never will, so the stream no longer matches what the server
> expects.  Mark the socket dead, which shuts it down and clears the partial
> send state.  nbd_xmit_timeout() does the same for a timed out request and
> only skips it here because NBD_CMD_PARTIAL_SEND makes it defer to this
> work function.
> 
> Without this, a write issued after the deadline fires never completes and
> the device has to be torn down to recover.
> 
> Reproduced by making the resumed send never progress, so the retry loop
> runs out req->deadline.  With the socket marked dead a later request is
> dispatched instead of requeued, and a reconnect afterwards does clean
> O_DIRECT I/O with no oops or warning.
> 
> Fixes: 8337b029f788 ("nbd: fix partial sending")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
>  drivers/block/nbd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index ffce519bf008..c2c3dbdd631f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -838,6 +838,14 @@ static void nbd_pending_cmd_work(struct work_struct *work)
>  		/* don't bother timeout handler for partial sending */
>  		if (READ_ONCE(jiffies) + msecs_to_jiffies(wait_ms) >= deadline) {
>  			cmd->status = BLK_STS_IOERR;
> +			/*
> +			 * The header is on the wire but the rest of the payload
> +			 * never will be, so the stream is out of sync with the
> +			 * server.  Marking the socket dead also drops the stale
> +			 * nsock->pending, which would otherwise make
> +			 * nbd_handle_cmd() requeue every later request forever.
> +			 */
> +			nbd_mark_nsock_dead(nbd, nsock, 1);

Reviewed-by: Ming Lei <tom.leiming@gmail.com>


Thanks,
Ming