[PATCH -next] smb: client: remove redundant lstrp update in negotiate protocol

[PATCH -next] smb: client: remove redundant lstrp update in negotiate protocol

[Wang Zhaolong]

Commit 34331d7beed7 ("smb: client: fix first command failure during
re-negotiation") addressed a race condition by updating lstrp before
entering negotiate state. However, this approach may have some unintended
side effects.

The lstrp field is documented as "when we got last response from this
server", and updating it before actually receiving a server response
could potentially affect other mechanisms that rely on this timestamp.
For example, the SMB echo detection logic also uses lstrp as a reference
point. In scenarios with frequent user operations during reconnect states,
the repeated calls to cifs_negotiate_protocol() might continuously
update lstrp, which could interfere with the echo detection timing.

Additionally, commit 266b5d02e14f ("smb: client: fix race condition in
negotiate timeout by using more precise timing") introduced a dedicated
neg_start field specifically for tracking negotiate start time. This
provides a more precise solution for the original race condition while
preserving the intended semantics of lstrp.

Since the race condition is now properly handled by the neg_start
mechanism, the lstrp update in cifs_negotiate_protocol() is no longer
necessary and can be safely removed.

Fixes: 266b5d02e14f ("smb: client: fix race condition in negotiate timeout by using more precise timing")
Cc: stable@vger.kernel.org
Signed-off-by: Wang Zhaolong <wangzhaolong@huaweicloud.com>
---
 fs/smb/client/connect.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 205f547ca49e..a2c49683be66 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -4205,11 +4205,10 @@ cifs_negotiate_protocol(const unsigned int xid, struct cifs_ses *ses,
 	    server->tcpStatus == CifsGood) {
 		spin_unlock(&server->srv_lock);
 		return 0;
 	}
 
-	server->lstrp = jiffies;
 	server->tcpStatus = CifsInNegotiate;
 	server->neg_start = jiffies;
 	spin_unlock(&server->srv_lock);
 
 	rc = server->ops->negotiate(xid, ses, server);
-- 
2.34.3

Re: [PATCH -next] smb: client: remove redundant lstrp update in negotiate protocol

[Steve French]

I may have missed this one in the mailing list last week - any
thoughts on whether to add it to for-next

On Fri, Aug 1, 2025 at 4:07 AM Wang Zhaolong
<wangzhaolong@huaweicloud.com> wrote:
>
> Commit 34331d7beed7 ("smb: client: fix first command failure during
> re-negotiation") addressed a race condition by updating lstrp before
> entering negotiate state. However, this approach may have some unintended
> side effects.
>
> The lstrp field is documented as "when we got last response from this
> server", and updating it before actually receiving a server response
> could potentially affect other mechanisms that rely on this timestamp.
> For example, the SMB echo detection logic also uses lstrp as a reference
> point. In scenarios with frequent user operations during reconnect states,
> the repeated calls to cifs_negotiate_protocol() might continuously
> update lstrp, which could interfere with the echo detection timing.
>
> Additionally, commit 266b5d02e14f ("smb: client: fix race condition in
> negotiate timeout by using more precise timing") introduced a dedicated
> neg_start field specifically for tracking negotiate start time. This
> provides a more precise solution for the original race condition while
> preserving the intended semantics of lstrp.
>
> Since the race condition is now properly handled by the neg_start
> mechanism, the lstrp update in cifs_negotiate_protocol() is no longer
> necessary and can be safely removed.
>
> Fixes: 266b5d02e14f ("smb: client: fix race condition in negotiate timeout by using more precise timing")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wang Zhaolong <wangzhaolong@huaweicloud.com>
> ---
>  fs/smb/client/connect.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 205f547ca49e..a2c49683be66 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -4205,11 +4205,10 @@ cifs_negotiate_protocol(const unsigned int xid, struct cifs_ses *ses,
>             server->tcpStatus == CifsGood) {
>                 spin_unlock(&server->srv_lock);
>                 return 0;
>         }
>
> -       server->lstrp = jiffies;
>         server->tcpStatus = CifsInNegotiate;
>         server->neg_start = jiffies;
>         spin_unlock(&server->srv_lock);
>
>         rc = server->ops->negotiate(xid, ses, server);
> --
> 2.34.3
>
>


-- 
Thanks,

Steve

Re: [PATCH -next] smb: client: remove redundant lstrp update in negotiate protocol

[Steve French]

tentatively merged into cifs-2.6.git for-next pending more review and testing

On Fri, Aug 1, 2025 at 4:07 AM Wang Zhaolong
<wangzhaolong@huaweicloud.com> wrote:
>
> Commit 34331d7beed7 ("smb: client: fix first command failure during
> re-negotiation") addressed a race condition by updating lstrp before
> entering negotiate state. However, this approach may have some unintended
> side effects.
>
> The lstrp field is documented as "when we got last response from this
> server", and updating it before actually receiving a server response
> could potentially affect other mechanisms that rely on this timestamp.
> For example, the SMB echo detection logic also uses lstrp as a reference
> point. In scenarios with frequent user operations during reconnect states,
> the repeated calls to cifs_negotiate_protocol() might continuously
> update lstrp, which could interfere with the echo detection timing.
>
> Additionally, commit 266b5d02e14f ("smb: client: fix race condition in
> negotiate timeout by using more precise timing") introduced a dedicated
> neg_start field specifically for tracking negotiate start time. This
> provides a more precise solution for the original race condition while
> preserving the intended semantics of lstrp.
>
> Since the race condition is now properly handled by the neg_start
> mechanism, the lstrp update in cifs_negotiate_protocol() is no longer
> necessary and can be safely removed.
>
> Fixes: 266b5d02e14f ("smb: client: fix race condition in negotiate timeout by using more precise timing")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wang Zhaolong <wangzhaolong@huaweicloud.com>
> ---
>  fs/smb/client/connect.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 205f547ca49e..a2c49683be66 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -4205,11 +4205,10 @@ cifs_negotiate_protocol(const unsigned int xid, struct cifs_ses *ses,
>             server->tcpStatus == CifsGood) {
>                 spin_unlock(&server->srv_lock);
>                 return 0;
>         }
>
> -       server->lstrp = jiffies;
>         server->tcpStatus = CifsInNegotiate;
>         server->neg_start = jiffies;
>         spin_unlock(&server->srv_lock);
>
>         rc = server->ops->negotiate(xid, ses, server);
> --
> 2.34.3
>
>


-- 
Thanks,

Steve