[PATCH] smb: client: fix first failure in negotiation after server reboot

[PATCH] smb: client: fix first failure in negotiation after server reboot

[zhangjian (CG)]

after fabc4ed200f9, server_unresponsive add a condition to check whether
 client need to reconnect depending on server->lstrp. When client failed 
to reconnect in 180s, client will abort connection and update server-
>lstrp for the last time. In the following scene, server->lstrp is too 
old, which may cause failure for the first negotiation.

client                                         | server
-----------------------------------------------+-----------
mount to cifs server                           |
ls                                             |
                                               | reboot
    stuck for 180s and return EHOSTDOWN        |
    abort connection and update server->lstrp  |
                                               | service smb restart
ls                                             |
    smb_negotiate                              |
        server_unresponsive is true [in cifsd] |
        cifs_sync_mid_result return EAGAIN     |
    smb_negotiate return EHOSTDOWN             |
ls failed                                      |

we update server->lstrp before last switching into CifsInNegotiate state 
to avoid this failure.

Fixes: fabc4ed200f9 ("smb: client: fix hang in wait_for_response() for 
negproto")
Signed-off-by: zhangjian <zhangjian496@huawei.com>
---
 fs/smb/client/connect.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 28bc33496..f9aef60f1 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -4193,6 +4193,7 @@ cifs_negotiate_protocol(const unsigned int xid, struct cifs_ses *ses,
 		return 0;
 	}
 
+	server->lstrp = jiffies;
 	server->tcpStatus = CifsInNegotiate;
 	spin_unlock(&server->srv_lock);
 
-- 
2.33.0

[PATCH] smb: client: fix first failure in negotiation after server reboot

[zhangjian (CG)]

After fabc4ed200f9, server_unresponsive add a condition to check whether 
client need to reconnect depending on server->lstrp. When client failed 
to reconnect in 180s, client will abort connection and update server->lstrp 
for the last time. In the following scene, server->lstrp is too 
old, which may cause failure for the first negotiation.

client                                                 | server
-------------------------------------------------------+------------------
mount to cifs server                                   |
ls                                                     |
                                                       | reboot
    stuck for 180s and return EHOSTDOWN                |
    abort connection and update server->lstrp          |
                                                       | sleep 21s
                                                       | service smb restart
ls                                                     |
    smb_negotiate                                      |
        server_unresponsive cause reconnect [in cifsd] |
        ( tcpStatus == CifsInNegotiate &&              |
	            jiffies > server->lstrp + 20s )        |
        cifs_sync_mid_result return EAGAIN             |
    smb_negotiate return EHOSTDOWN                     |
ls failed                                              |

The condition (tcpStatus == CifsInNegotiate && jiffies > server->lstrp + 20s)
expect client stay in CifsInNegotiate state for more than 20s. So we update 
server->lstrp before last switching into CifsInNegotiate state to avoid 
this failure.

Fixes: fabc4ed200f9 ("smb: client: fix hang in wait_for_response() for 
negproto")
Signed-off-by: zhangjian <zhangjian496@huawei.com>
---
 fs/smb/client/connect.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 28bc33496..f9aef60f1 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -4193,6 +4193,7 @@ cifs_negotiate_protocol(const unsigned int xid, struct cifs_ses *ses,
 		return 0;
 	}
 
+	server->lstrp = jiffies;
 	server->tcpStatus = CifsInNegotiate;
 	spin_unlock(&server->srv_lock);
 
-- 
2.33.0

Re: [EXTERNAL] Re: [PATCH] smb: client: fix first failure in negotiation after server reboot

[zhangjian (CG)]

In addition, If negotiation received no response, there are two possible
actions:
1. server_unresponsive may trigger reconnecting again and return true.
2. server_unresponsive may return false and client falls back to
CifsNeedNegotiate state and trigger reconnecting in SMB2_echo.

There two conditions are similar to the stage when first mounting to
cifs server.

On 2025/6/16 10:01, zhangjian (CG) wrote:
> 
> 
> 
> 
> On 2025/6/15 21:08, Shyam Prasad (Azure Files) wrote:
>> Can we have a situation where we just got the sock_recvmsg just timed out, and before we loop back to server_unresponsive, if another parallel negotiate updates lstrp?
> 
> Negotiation only comes when connection is touchable. Client will send a
> negotiation message to server. If we just got the sock_recvmsg timeout
> and loop back to server_unresponsive, it will return false. Client calls
> sock_recvmsg again and wait for negotiation response. Everything is Ok.
> 
>> That will cause us to not detect the server unresponsive situation, even if that did happen.
>> server->lstrp is meant to store the last "response" time from the server.
> 
> server->lstrp is also updated during setting up and aborting connection
> even when there is no response. These can be regarded as initial value
> for server->lstrp.
> I think server->lstrp needs an initial value before negotiation rather
> than connection.
> 
>>
>>
>> Regards,
>>
>> Shyam
>>
>>
>>
>>
>> ________________________________
>> From: Steve French <smfrench@gmail.com>
>> Sent: Friday, June 13, 2025 20:53
>> To: zhangjian (CG) <zhangjian496@huawei.com>
>> Cc: Shyam Prasad (Azure Files) <Shyam.Prasad@microsoft.com>; Paulo Alcantara <pc@manguebit.com>
>> Subject: [EXTERNAL] Re: [PATCH] smb: client: fix first failure in negotiation after server reboot
>>
>> Could you clarify the reproduction scenario? It was a little hard to read
>>
>> On Fri, Jun 13, 2025 at 5:44 AM zhangjian (CG) <zhangjian496@huawei.com> wrote:
>>>
>>> After fabc4ed200f9, server_unresponsive add a condition to check whether
>>> client need to reconnect depending on server->lstrp. When client failed
>>> to reconnect in 180s, client will abort connection and update server->lstrp
>>> for the last time. In the following scene, server->lstrp is too
>>> old, which may cause failure for the first negotiation.
>>>
>>> client                                                 | server
>>> -------------------------------------------------------+------------------
>>> mount to cifs server                                   |
>>> ls                                                     |
>>>                                                        | reboot
>>>     stuck for 180s and return EHOSTDOWN                |
>>>     abort connection and update server->lstrp          |
>>>                                                        | sleep 21s
>>>                                                        | service smb restart
>>> ls                                                     |
>>>     smb_negotiate                                      |
>>>         server_unresponsive cause reconnect [in cifsd] |
>>>         ( tcpStatus == CifsInNegotiate &&              |
>>>                     jiffies > server->lstrp + 20s )        |
>>>         cifs_sync_mid_result return EAGAIN             |
>>>     smb_negotiate return EHOSTDOWN                     |
>>> ls failed                                              |
>>>
>>> The condition (tcpStatus == CifsInNegotiate && jiffies > server->lstrp + 20s)
>>> expect client stay in CifsInNegotiate state for more than 20s. So we update
>>> server->lstrp before last switching into CifsInNegotiate state to avoid
>>> this failure.
>>>
>>> Fixes: fabc4ed200f9 ("smb: client: fix hang in wait_for_response() for
>>> negproto")
>>> Signed-off-by: zhangjian <zhangjian496@huawei.com>
>>> ---
>>>  fs/smb/client/connect.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
>>> index 28bc33496..f9aef60f1 100644
>>> --- a/fs/smb/client/connect.c
>>> +++ b/fs/smb/client/connect.c
>>> @@ -4193,6 +4193,7 @@ cifs_negotiate_protocol(const unsigned int xid, struct cifs_ses *ses,
>>>                 return 0;
>>>         }
>>>
>>> +       server->lstrp = jiffies;
>>>         server->tcpStatus = CifsInNegotiate;
>>>         spin_unlock(&server->srv_lock);
>>>
>>> --
>>> 2.33.0
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
> 

[Question] nfsacl: why deny owner mode when deny user

[zhangjian (CG)]

When user read bit is denied by nfs4_setfacl, owner read bit is also
denied.
Example:

[root@localhost ~]# nfs4_getfacl test/a
# file: test/a
A::OWNER@:rwatTcCy
A::1000:rwatcy
A::GROUP@:rtcy
A::EVERYONE@:rtcy

[root@localhost ~]# nfs4_setfacl -a D::1000:r test/a
[root@localhost ~]# nfs4_getfacl test/a
# file: test/a
D::OWNER@:r
A::OWNER@:watTcCy
D::1000:r
A::1000:watcy
A::GROUP@:rtcy
A::EVERYONE@:rtcy

In function process_one_v4_ace, I see read bit is denied for owner:
case ACL_USER:
	i = find_uid(state, state->users, ace->who);
	if (ace->type == NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE) {
		allow_bits(&state->users->aces[i].perms, mask);
	} else {
		deny_bits(&state->users->aces[i].perms, mask);
		mask = state->users->aces[i].perms.deny;
		deny_bits(&state->owner, mask);
	}
This change is commit in 09229ed. But I wonder why it is implemented
like this.

Re: [Question] nfsacl: why deny owner mode when deny user

[Chuck Lever]

[ bfields@redhat.com dropped because it's a dead address ]

On Mon, Dec 8, 2025, at 4:56 AM, zhangjian (CG) wrote:
> When user read bit is denied by nfs4_setfacl, owner read bit is also
> denied.
> Example:
>
> [root@localhost ~]# nfs4_getfacl test/a
> # file: test/a
> A::OWNER@:rwatTcCy
> A::1000:rwatcy
> A::GROUP@:rtcy
> A::EVERYONE@:rtcy
>
> [root@localhost ~]# nfs4_setfacl -a D::1000:r test/a
> [root@localhost ~]# nfs4_getfacl test/a
> # file: test/a
> D::OWNER@:r
> A::OWNER@:watTcCy
> D::1000:r
> A::1000:watcy
> A::GROUP@:rtcy
> A::EVERYONE@:rtcy
>
> In function process_one_v4_ace, I see read bit is denied for owner:
> case ACL_USER:
> 	i = find_uid(state, state->users, ace->who);
> 	if (ace->type == NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE) {
> 		allow_bits(&state->users->aces[i].perms, mask);
> 	} else {
> 		deny_bits(&state->users->aces[i].perms, mask);
> 		mask = state->users->aces[i].perms.deny;
> 		deny_bits(&state->owner, mask);
> 	}
> This change is commit in 09229ed. But I wonder why it is implemented
> like this.

I'm not an ACL expert. But here's a stab at an answer.

NFSD must translate incoming NFSv4 ACLs to POSIX ACLs before
they can be stored by local POSIX file systems. The root issue
is the semantic mismatch between NFSv4 ACLs and POSIX ACLs. 

In particular, NFSv4 ACL evaluation is strictly ordered
top-to-bottom. A DENY ACE early in the list blocks access even
if a later ALLOW would grant it.

However, POSIX ACL evaluation uses a fixed priority:
1. If user is the file owner -> use ACL_USER_OBJ entry
2. Else if user matches a named user -> use that ACL_USER
   entry (masked)
3. Else if user's groups match -> use group entries (masked)
4. Else -> use ACL_OTHER

For example:
  DENY uid=1000: READ
  ALLOW OWNER@: READ

Under NFSv4 semantics, if the file owner IS uid=1000, they are
denied READ (the DENY comes first in the ordered evaluation).

But in POSIX ACL semantics, the owner entry (ACL_USER_OBJ) is
checked before any named user entries. If the code only denied
READ to the named user entry without also denying it to the
owner entry, then when uid=1000 owns the file, they would get
READ through the owner check -- bypassing the intended denial.

Commit 09229edb68a3 explicitly states:
> errs on the side of restricting permissions...the posix acl
> produced should be the most permissive acl that is not more
> permissive than the given nfsv4 acl.

The algorithm doesn't know at ACL-setting time whether the
owner might match a named user (or might later via chown), so
it defensively denies to both. This guarantees the POSIX ACL
is never more permissive than the NFSv4 ACL was intended to be.

The downside is that when the owner is NOT the named user, the
owner's permissions are unnecessarily restricted. This is the
cost of the conservative approach: it may deny more than
strictly necessary, but it will never allow more than intended.

A "perfect" translation isn't possible because the semantic
models differ fundamentally. The algorithm  chose correctness
over permissiveness.


-- 
Chuck Lever