Re: [PATCH 2/2] cifs: support mounting with alternate password to allow password rotation

[Paulo Alcantara <pc@manguebit.com>]

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.