Message ID | 20241030142829.234828-2-meetakshisetiyaoss@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/2] cifs: during remount, make sure passwords are in sync | expand |
meetakshisetiyaoss@gmail.com writes: > @@ -2245,6 +2269,7 @@ struct cifs_ses * > cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > { > int rc = 0; > + int retries = 0; > unsigned int xid; > struct cifs_ses *ses; > struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr; > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > cifs_dbg(FYI, "Session needs reconnect\n"); > > mutex_lock(&ses->session_mutex); > + > +retry_old_session: > rc = cifs_negotiate_protocol(xid, ses, server); > if (rc) { > mutex_unlock(&ses->session_mutex); > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > rc = cifs_setup_session(xid, ses, server, > ctx->local_nls); > if (rc) { > + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) || > + (rc == -EKEYREVOKED)) && !retries && ses->password2) { > + retries++; > + cifs_info("Session reconnect failed, retrying with alternate password\n"); Please don't add more noisy messages over reconnect. Remember that if SMB session doesn't get re-established, there will be flood enough on dmesg with "Send error in SessSetup = ..." messages on every 2s that already pisses off users and customers. > + swap(ses->password, ses->password2); > + goto retry_old_session; > + } > mutex_unlock(&ses->session_mutex); > /* problem -- put our reference */ > cifs_put_smb_ses(ses); > @@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > ses->chans_need_reconnect = 1; > spin_unlock(&ses->chan_lock); > > +retry_new_session: > mutex_lock(&ses->session_mutex); > rc = cifs_negotiate_protocol(xid, ses, server); > if (!rc) > @@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > sizeof(ses->smb3signingkey)); > spin_unlock(&ses->chan_lock); > > - if (rc) > - goto get_ses_fail; > + if (rc) { > + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) || > + (rc == -EKEYREVOKED)) && !retries && ses->password2) { > + retries++; > + cifs_info("Session setup failed, retrying with alternate password\n"); Ditto. > + swap(ses->password, ses->password2); > + goto retry_new_session; > + } else > + goto get_ses_fail; > + } > > /* > * success, put it on the list and add it as first channel > -- > 2.46.0.46.g406f326d27
On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote: > > meetakshisetiyaoss@gmail.com writes: > > > @@ -2245,6 +2269,7 @@ struct cifs_ses * > > cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > > { > > int rc = 0; > > + int retries = 0; > > unsigned int xid; > > struct cifs_ses *ses; > > struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr; > > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > > cifs_dbg(FYI, "Session needs reconnect\n"); > > > > mutex_lock(&ses->session_mutex); > > + > > +retry_old_session: > > rc = cifs_negotiate_protocol(xid, ses, server); > > if (rc) { > > mutex_unlock(&ses->session_mutex); > > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > > rc = cifs_setup_session(xid, ses, server, > > ctx->local_nls); > > if (rc) { > > + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) || > > + (rc == -EKEYREVOKED)) && !retries && ses->password2) { > > + retries++; > > + cifs_info("Session reconnect failed, retrying with alternate password\n"); > > Please don't add more noisy messages over reconnect. Remember that if > SMB session doesn't get re-established, there will be flood enough on > dmesg with "Send error in SessSetup = ..." messages on every 2s that > already pisses off users and customers. > Perhaps we could do a cifs_dbg instead of cifs_info. But Paulo, the problem here is that we retry every 2s. I think we should address that instead. One way is to do an exponential backoff every time we retry. I'd also want to understand why we need the reconnect work? Why not always do smb2_reconnect when someone does filesystem calls on the mount point? That way, we avoid unnecessary retries altogether. @Steve French your opinions? > > + swap(ses->password, ses->password2); > > + goto retry_old_session; > > + } > > mutex_unlock(&ses->session_mutex); > > /* problem -- put our reference */ > > cifs_put_smb_ses(ses); > > @@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > > ses->chans_need_reconnect = 1; > > spin_unlock(&ses->chan_lock); > > > > +retry_new_session: > > mutex_lock(&ses->session_mutex); > > rc = cifs_negotiate_protocol(xid, ses, server); > > if (!rc) > > @@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > > sizeof(ses->smb3signingkey)); > > spin_unlock(&ses->chan_lock); > > > > - if (rc) > > - goto get_ses_fail; > > + if (rc) { > > + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) || > > + (rc == -EKEYREVOKED)) && !retries && ses->password2) { > > + retries++; > > + cifs_info("Session setup failed, retrying with alternate password\n"); > > Ditto. > > > + swap(ses->password, ses->password2); > > + goto retry_new_session; > > + } else > > + goto get_ses_fail; > > + } > > > > /* > > * success, put it on the list and add it as first channel > > -- > > 2.46.0.46.g406f326d27
Shyam Prasad N <nspmangalore@gmail.com> writes: > On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote: >> >> meetakshisetiyaoss@gmail.com writes: >> >> > @@ -2245,6 +2269,7 @@ struct cifs_ses * >> > cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) >> > { >> > int rc = 0; >> > + int retries = 0; >> > unsigned int xid; >> > struct cifs_ses *ses; >> > struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr; >> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) >> > cifs_dbg(FYI, "Session needs reconnect\n"); >> > >> > mutex_lock(&ses->session_mutex); >> > + >> > +retry_old_session: >> > rc = cifs_negotiate_protocol(xid, ses, server); >> > if (rc) { >> > mutex_unlock(&ses->session_mutex); >> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) >> > rc = cifs_setup_session(xid, ses, server, >> > ctx->local_nls); >> > if (rc) { >> > + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) || >> > + (rc == -EKEYREVOKED)) && !retries && ses->password2) { >> > + retries++; >> > + cifs_info("Session reconnect failed, retrying with alternate password\n"); >> >> Please don't add more noisy messages over reconnect. Remember that if >> SMB session doesn't get re-established, there will be flood enough on >> dmesg with "Send error in SessSetup = ..." messages on every 2s that >> already pisses off users and customers. >> > Perhaps we could do a cifs_dbg instead of cifs_info. Yep, with FYI. > But Paulo, the problem here is that we retry every 2s. I think we > should address that instead. > One way is to do an exponential backoff every time we retry. Agreed, but that doesn't mean we should add more noisy messages. > I'd also want to understand why we need the reconnect work? I see it as an optimisation to allow next IOs to not take longer because it needs to reconnect SMB session. If there was no prior filesystem activity, why don't allow the client itself to reconnect the session? Besides, SMB2_IOCTL currently doesn't call smb2_reconnect(), so reconnect worker would be required to reconnect the session and then allow SMB2_IOCTL to work. We'd need to change that because with recent special file support, we need to avoid failures when creating or parsing reparse points because the SMB session isn't re-established yet. > Why not always do smb2_reconnect when someone does filesystem calls on > the mount point? We already do for most operations. SMB2_IOCTL and SMB2_TREE_CONNECT, for instance, can't call smb2_reconnect() as they would deadlock.
Thanks for the review Paulo, here is the updated patch that uses cifs_dbg (FYI) instead of cifs_info. Best Meetakshi On Fri, Nov 8, 2024 at 8:50 PM Paulo Alcantara <pc@manguebit.com> wrote: > > Shyam Prasad N <nspmangalore@gmail.com> writes: > > > On Fri, Nov 8, 2024 at 12:35 AM Paulo Alcantara <pc@manguebit.com> wrote: > >> > >> meetakshisetiyaoss@gmail.com writes: > >> > >> > @@ -2245,6 +2269,7 @@ struct cifs_ses * > >> > cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > >> > { > >> > int rc = 0; > >> > + int retries = 0; > >> > unsigned int xid; > >> > struct cifs_ses *ses; > >> > struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr; > >> > @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > >> > cifs_dbg(FYI, "Session needs reconnect\n"); > >> > > >> > mutex_lock(&ses->session_mutex); > >> > + > >> > +retry_old_session: > >> > rc = cifs_negotiate_protocol(xid, ses, server); > >> > if (rc) { > >> > mutex_unlock(&ses->session_mutex); > >> > @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) > >> > rc = cifs_setup_session(xid, ses, server, > >> > ctx->local_nls); > >> > if (rc) { > >> > + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) || > >> > + (rc == -EKEYREVOKED)) && !retries && ses->password2) { > >> > + retries++; > >> > + cifs_info("Session reconnect failed, retrying with alternate password\n"); > >> > >> Please don't add more noisy messages over reconnect. Remember that if > >> SMB session doesn't get re-established, there will be flood enough on > >> dmesg with "Send error in SessSetup = ..." messages on every 2s that > >> already pisses off users and customers. > >> > > Perhaps we could do a cifs_dbg instead of cifs_info. > > Yep, with FYI. > > > But Paulo, the problem here is that we retry every 2s. I think we > > should address that instead. > > One way is to do an exponential backoff every time we retry. > > Agreed, but that doesn't mean we should add more noisy messages. > > > I'd also want to understand why we need the reconnect work? > > I see it as an optimisation to allow next IOs to not take longer because > it needs to reconnect SMB session. If there was no prior filesystem > activity, why don't allow the client itself to reconnect the session? > > Besides, SMB2_IOCTL currently doesn't call smb2_reconnect(), so > reconnect worker would be required to reconnect the session and then > allow SMB2_IOCTL to work. We'd need to change that because with recent > special file support, we need to avoid failures when creating or parsing > reparse points because the SMB session isn't re-established yet. > > > Why not always do smb2_reconnect when someone does filesystem calls on > > the mount point? > > We already do for most operations. SMB2_IOCTL and SMB2_TREE_CONNECT, > for instance, can't call smb2_reconnect() as they would deadlock.
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c index 15d94ac4095e..71b305230e14 100644 --- a/fs/smb/client/connect.c +++ b/fs/smb/client/connect.c @@ -1898,11 +1898,35 @@ static int match_session(struct cifs_ses *ses, CIFS_MAX_USERNAME_LEN)) return 0; if ((ctx->username && strlen(ctx->username) != 0) && - ses->password != NULL && - strncmp(ses->password, - ctx->password ? ctx->password : "", - CIFS_MAX_PASSWORD_LEN)) - return 0; + ses->password != NULL) { + + /* New mount can only share sessions with an existing mount if: + * 1. Both password and password2 match, or + * 2. password2 of the old mount matches password of the new mount + * and password of the old mount matches password2 of the new + * mount + */ + if (ses->password2 != NULL && ctx->password2 != NULL) { + if (!((strncmp(ses->password, ctx->password ? + ctx->password : "", CIFS_MAX_PASSWORD_LEN) == 0 && + strncmp(ses->password2, ctx->password2, + CIFS_MAX_PASSWORD_LEN) == 0) || + (strncmp(ses->password, ctx->password2, + CIFS_MAX_PASSWORD_LEN) == 0 && + strncmp(ses->password2, ctx->password ? + ctx->password : "", CIFS_MAX_PASSWORD_LEN) == 0))) + return 0; + + } else if ((ses->password2 == NULL && ctx->password2 != NULL) || + (ses->password2 != NULL && ctx->password2 == NULL)) { + return 0; + + } else { + if (strncmp(ses->password, ctx->password ? + ctx->password : "", CIFS_MAX_PASSWORD_LEN)) + return 0; + } + } } if (strcmp(ctx->local_nls->charset, ses->local_nls->charset)) @@ -2245,6 +2269,7 @@ struct cifs_ses * cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) { int rc = 0; + int retries = 0; unsigned int xid; struct cifs_ses *ses; struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr; @@ -2263,6 +2288,8 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) cifs_dbg(FYI, "Session needs reconnect\n"); mutex_lock(&ses->session_mutex); + +retry_old_session: rc = cifs_negotiate_protocol(xid, ses, server); if (rc) { mutex_unlock(&ses->session_mutex); @@ -2275,6 +2302,13 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) rc = cifs_setup_session(xid, ses, server, ctx->local_nls); if (rc) { + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) || + (rc == -EKEYREVOKED)) && !retries && ses->password2) { + retries++; + cifs_info("Session reconnect failed, retrying with alternate password\n"); + swap(ses->password, ses->password2); + goto retry_old_session; + } mutex_unlock(&ses->session_mutex); /* problem -- put our reference */ cifs_put_smb_ses(ses); @@ -2350,6 +2384,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) ses->chans_need_reconnect = 1; spin_unlock(&ses->chan_lock); +retry_new_session: mutex_lock(&ses->session_mutex); rc = cifs_negotiate_protocol(xid, ses, server); if (!rc) @@ -2362,8 +2397,16 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx) sizeof(ses->smb3signingkey)); spin_unlock(&ses->chan_lock); - if (rc) - goto get_ses_fail; + if (rc) { + if (((rc == -EACCES) || (rc == -EKEYEXPIRED) || + (rc == -EKEYREVOKED)) && !retries && ses->password2) { + retries++; + cifs_info("Session setup failed, retrying with alternate password\n"); + swap(ses->password, ses->password2); + goto retry_new_session; + } else + goto get_ses_fail; + } /* * success, put it on the list and add it as first channel