Re: [PATCH 1/2] smb: client: hold server ref while queueing reconnect work

[Joseph Qi <joseph.qi@linux.alibaba.com>]

Hi Paulo,

Thanks for reviewing it.

On 11/8/26 1:28 AM, Paulo Alcantara wrote:
> Joseph Qi <joseph.qi@linux.alibaba.com> writes:
> 
>> When query_server_interfaces() returns -EOPNOTSUPP on a secondary
>> channel, smb2_reconnect() calls smb3_update_ses_channels() which for a
>> channel runs cifs_chan_skip_or_disable().  That clears the channel's
>> slot in the session and calls cifs_put_tcp_session(), which can drop
>> the last reference of the channel and tear it down.
>>
>> smb2_reconnect() then falls through to skip_add_channels and queues
>> reconnect work on that server, using a server that may already be
>> freed.  The send path obtained the channel from cifs_pick_channel(),
>> which does not take a reference, so the session's channel reference is
>> the only one.
>>
>> Take our own reference on the channel before calling
>> smb3_update_ses_channels() and drop it after the reconnect work has
>> been queued, so the channel cannot be freed while it is still used.
>>
>> Fixes: f591062bdbf4 ("cifs: handle servers that still advertise multichannel after disabling")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> 
> Please check sashiko comments
> 
>         https://sashiko.dev/#/patchset/20261103091558.2342761-1-joseph.qi%40linux.alibaba.com

These points are about the multichannel disable path as a whole rather
than about this change, so let me try to answer them together.

What this patch does is narrow.  It holds a reference on the channel
across smb3_update_ses_channels() so that the final cifs_put_tcp_session()
runs after cifs_queue_server_reconn() instead of before it.  Previously
the last reference was dropped inside cifs_chan_skip_or_disable(), which
cancelled server->reconnect, and skip_add_channels then re-armed it on a
server whose srv_count was already zero.

Queueing reconnect work on a just-terminated channel is indeed
pointless, but that comes from f591062bdbf4 ("cifs: handle servers that
still advertise multichannel after disabling"), which added the goto
skip_add_channels into a label that already ended with
mod_delayed_work(cifsiod_wq, &server->reconnect, 0).  With the reference
held, the work is either cancelled by the final put or runs and bails on
server->terminate in smb2_reconnect_server(), before it reaches
smb2_reconnect() and ses->session_mutex.  The send path holds no locks
there, so neither ordering can deadlock.

The srv_count == 0 race is reachable, and smb3_reconfigure() is the
clearer path into cifs_decrease_secondary_channels().  But it is older
than this patch and I don't think it can be closed here.
cifs_pick_channel() returns a channel without a reference, so once
srv_count can drop to zero under us the pointer is already stale on
entry to smb2_reconnect().  Reading server->srv_count to decide whether
to take a reference would itself be the use-after-free.

The guarded increment is the same idiom smb2_reconnect_server() already
uses, and it narrows that window rather than widening it.  Refcounting
the channel selection looks like the real fix, which is a much larger
change.  There is at least no double put, since cifs_ses_get_chan_index()
returns CIFS_INVAL_CHAN_INDEX once terminate is set and
cifs_chan_skip_or_disable() then returns -EHOSTDOWN without putting
again.

So what's your opinion?

Thanks,
Joseph