[lvc-project] [PATCH 6.1 0847/1887] smb: client: let smbd_destroy() call disable_work_sync(&info->post_send_credits_work)

[Fedor Pchelkin]

Hi Greg, Sasha

please see below on what may be potentially improved in managing these
huge amounts of backports, many of which come with adaptations to old
stable kernels.

Greg Kroah-Hartman wrote:
> 6.1-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Stefan Metzmacher <metze at samba.org>
> 
> [ Upstream commit d9dcbbcf9145b68aa85c40947311a6907277e097 ]
> 
> In smbd_destroy() we may destroy the memory so we better
> wait until post_send_credits_work is no longer pending
> and will never be started again.
> 
> I actually just hit the case using rxe:
> 
> WARNING: CPU: 0 PID: 138 at drivers/infiniband/sw/rxe/rxe_verbs.c:1032 rxe_post_recv+0x1ee/0x480 [rdma_rxe]
> ...
> [ 5305.686979] [    T138]  smbd_post_recv+0x445/0xc10 [cifs]
> [ 5305.687135] [    T138]  ? srso_alias_return_thunk+0x5/0xfbef5
> [ 5305.687149] [    T138]  ? __kasan_check_write+0x14/0x30
> [ 5305.687185] [    T138]  ? __pfx_smbd_post_recv+0x10/0x10 [cifs]
> [ 5305.687329] [    T138]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
> [ 5305.687356] [    T138]  ? srso_alias_return_thunk+0x5/0xfbef5
> [ 5305.687368] [    T138]  ? srso_alias_return_thunk+0x5/0xfbef5
> [ 5305.687378] [    T138]  ? _raw_spin_unlock_irqrestore+0x11/0x60
> [ 5305.687389] [    T138]  ? srso_alias_return_thunk+0x5/0xfbef5
> [ 5305.687399] [    T138]  ? get_receive_buffer+0x168/0x210 [cifs]
> [ 5305.687555] [    T138]  smbd_post_send_credits+0x382/0x4b0 [cifs]
> [ 5305.687701] [    T138]  ? __pfx_smbd_post_send_credits+0x10/0x10 [cifs]
> [ 5305.687855] [    T138]  ? __pfx___schedule+0x10/0x10
> [ 5305.687865] [    T138]  ? __pfx__raw_spin_lock_irq+0x10/0x10
> [ 5305.687875] [    T138]  ? queue_delayed_work_on+0x8e/0xa0
> [ 5305.687889] [    T138]  process_one_work+0x629/0xf80
> [ 5305.687908] [    T138]  ? srso_alias_return_thunk+0x5/0xfbef5
> [ 5305.687917] [    T138]  ? __kasan_check_write+0x14/0x30
> [ 5305.687933] [    T138]  worker_thread+0x87f/0x1570
> ...
> 
> It means rxe_post_recv was called after rdma_destroy_qp().
> This happened because put_receive_buffer() was triggered
> by ib_drain_qp() and called:
> queue_work(info->workqueue, &info->post_send_credits_work);
> 
> Cc: Steve French <smfrench at gmail.com>
> Cc: Tom Talpey <tom at talpey.com>
> Cc: Long Li <longli at microsoft.com>
> Cc: Namjae Jeon <linkinjeon at kernel.org>
> Cc: linux-cifs at vger.kernel.org
> Cc: samba-technical at lists.samba.org
> Fixes: f198186aa9bb ("CIFS: SMBD: Establish SMB Direct connection")
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> Signed-off-by: Steve French <stfrench at microsoft.com>
> Signed-off-by: Sasha Levin <sashal at kernel.org>
> ---
>  fs/smb/client/smbdirect.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/smb/client/smbdirect.c b/fs/smb/client/smbdirect.c
> index 9b16840952c09..d2878973c1476 100644
> --- a/fs/smb/client/smbdirect.c
> +++ b/fs/smb/client/smbdirect.c
> @@ -1358,6 +1358,9 @@ void smbd_destroy(struct TCP_Server_Info *server)
>  			info->transport_status == SMBD_DISCONNECTED);
>  	}
>  
> +	log_rdma_event(INFO, "cancelling post_send_credits_work\n");
> +	cancel_work_sync(&info->post_send_credits_work);
> +
>  	log_rdma_event(INFO, "destroying qp\n");
>  	ib_drain_qp(info->id->qp);
>  	rdma_destroy_qp(info->id);

It would be good to see a small backporter's comment, you know, on the
reasons behind the performed backport adaptation.  Upstream commit adds a
call to disable_work_sync(), while the backport only does
cancel_work_sync() - and doesn't look to solve the problem completely.
The commit's message says the work can be queued again during ib_drain_qp()
processing a few lines below.  So canceling is not enough.

However, there is no info on why cancel_work_sync() was chosen still: to
reduce the existing race window as best effort, or maybe just calling
cancel_work_sync() is actually fine on 6.1 and fixes the bug properly?

I guess that adding a brief backporter's comment explaining the rationale
behind the adaptation won't be too resource-consuming on your side,
especially if major part is already done with AI stuff.  But it will make
things clearer on whether the backport fix is correct and complete.

--
Fedor