Re: [PATCH 06/14] cifs: handle cases where a channel is closed

[Shyam Prasad N <nspmangalore@gmail.com>]

On Wed, Nov 1, 2023 at 8:39 AM Steve French <smfrench@gmail.com> wrote:
>
> This patch wouldn't merge due to a merge conflict here.  Am I missing
> an earlier patch of yours before the series?
>

I think the conflict is with:
0779365fc10d smb: client: remove extra @chan_count check in __cifs_put_smb_ses()

Let me resolve the conflict and give you an updated patch.

> I have these in for-next:
> d1ea8f36fa51 (HEAD -> for-next) cifs: force interface update before a
> fresh session setup
> 1e27cedcaf4e (origin/for-next) cifs: do not reset chan_max if
> multichannel is not supported at mount
> e257df806ae0 cifs: display the endpoint IP details in DebugData
> 9d95130c9f78 cifs: reconnect helper should set reconnect for the right channel
> 7ac6866076bd smb: client: fix use-after-free in smb2_query_info_compound()
> 0779365fc10d smb: client: remove extra @chan_count check in __cifs_put_smb_ses()
> 4cf6e1101a25 cifs: add xid to query server interface call
> 52768695d36a cifs: print server capabilities in DebugData
>
>
> --- fs/smb/client/connect.c
> +++ fs/smb/client/connect.c
> @@ -2037,7 +2041,9 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
>
> kref_put(&ses->chans[i].iface->refcount, release_iface);
>                                 ses->chans[i].iface = NULL;
>                         }
> -                       cifs_put_tcp_session(ses->chans[i].server, 0);
> +
> +                       if (ses->chans[i].server)
> +                               cifs_put_tcp_session(ses->chans[i].server, 0);
>                         ses->chans[i].server = NULL;
>                 }
>         }
>
> On Mon, Oct 30, 2023 at 6:00 AM <nspmangalore@gmail.com> wrote:
> >
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > So far, SMB multichannel could only scale up, but not
> > scale down the number of channels. In this series of
> > patch, we now allow the client to deal with the case
> > of multichannel disabled on the server when the share
> > is mounted. With that change, we now need the ability
> > to scale down the channels.
> >
> > This change allows the client to deal with cases of
> > missing channels more gracefully.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/cifs_debug.c    |  5 +++++
> >  fs/smb/client/cifsglob.h      |  1 +
> >  fs/smb/client/cifsproto.h     |  2 +-
> >  fs/smb/client/connect.c       | 10 ++++++++--
> >  fs/smb/client/sess.c          | 25 +++++++++++++++++++++----
> >  fs/smb/client/smb2transport.c |  8 +++++++-
> >  6 files changed, 43 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
> > index a9dfecc397a8..9fca09539728 100644
> > --- a/fs/smb/client/cifs_debug.c
> > +++ b/fs/smb/client/cifs_debug.c
> > @@ -136,6 +136,11 @@ cifs_dump_channel(struct seq_file *m, int i, struct cifs_chan *chan)
> >  {
> >         struct TCP_Server_Info *server = chan->server;
> >
> > +       if (!server) {
> > +               seq_printf(m, "\n\n\t\tChannel: %d DISABLED", i+1);
> > +               return;
> > +       }
> > +
> >         seq_printf(m, "\n\n\t\tChannel: %d ConnectionId: 0x%llx"
> >                    "\n\t\tNumber of credits: %d,%d,%d Dialect 0x%x"
> >                    "\n\t\tTCP status: %d Instance: %d"
> > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> > index 02082621d8e0..552ed441281a 100644
> > --- a/fs/smb/client/cifsglob.h
> > +++ b/fs/smb/client/cifsglob.h
> > @@ -1050,6 +1050,7 @@ struct cifs_ses {
> >         spinlock_t chan_lock;
> >         /* ========= begin: protected by chan_lock ======== */
> >  #define CIFS_MAX_CHANNELS 16
> > +#define CIFS_INVAL_CHAN_INDEX (-1)
> >  #define CIFS_ALL_CHANNELS_SET(ses)     \
> >         ((1UL << (ses)->chan_count) - 1)
> >  #define CIFS_ALL_CHANS_GOOD(ses)               \
> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> > index 0c37eefa18a5..65c84b3d1a65 100644
> > --- a/fs/smb/client/cifsproto.h
> > +++ b/fs/smb/client/cifsproto.h
> > @@ -616,7 +616,7 @@ bool is_server_using_iface(struct TCP_Server_Info *server,
> >  bool is_ses_using_iface(struct cifs_ses *ses, struct cifs_server_iface *iface);
> >  void cifs_ses_mark_for_reconnect(struct cifs_ses *ses);
> >
> > -unsigned int
> > +int
> >  cifs_ses_get_chan_index(struct cifs_ses *ses,
> >                         struct TCP_Server_Info *server);
> >  void
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 97c9a32cff36..8393977e21ee 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -173,8 +173,12 @@ cifs_signal_cifsd_for_reconnect(struct TCP_Server_Info *server,
> >         list_for_each_entry(ses, &pserver->smb_ses_list, smb_ses_list) {
> >                 spin_lock(&ses->chan_lock);
> >                 for (i = 0; i < ses->chan_count; i++) {
> > +                       if (!ses->chans[i].server)
> > +                               continue;
> > +
> >                         spin_lock(&ses->chans[i].server->srv_lock);
> > -                       ses->chans[i].server->tcpStatus = CifsNeedReconnect;
> > +                       if (ses->chans[i].server->tcpStatus != CifsExiting)
> > +                               ses->chans[i].server->tcpStatus = CifsNeedReconnect;
> >                         spin_unlock(&ses->chans[i].server->srv_lock);
> >                 }
> >                 spin_unlock(&ses->chan_lock);
> > @@ -2033,7 +2037,9 @@ void __cifs_put_smb_ses(struct cifs_ses *ses)
> >                                 kref_put(&ses->chans[i].iface->refcount, release_iface);
> >                                 ses->chans[i].iface = NULL;
> >                         }
> > -                       cifs_put_tcp_session(ses->chans[i].server, 0);
> > +
> > +                       if (ses->chans[i].server)
> > +                               cifs_put_tcp_session(ses->chans[i].server, 0);
> >                         ses->chans[i].server = NULL;
> >                 }
> >         }
> > diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
> > index c899b05c92f7..9d2228c2d7e5 100644
> > --- a/fs/smb/client/sess.c
> > +++ b/fs/smb/client/sess.c
> > @@ -69,7 +69,7 @@ bool is_ses_using_iface(struct cifs_ses *ses, struct cifs_server_iface *iface)
> >
> >  /* channel helper functions. assumed that chan_lock is held by caller. */
> >
> > -unsigned int
> > +int
> >  cifs_ses_get_chan_index(struct cifs_ses *ses,
> >                         struct TCP_Server_Info *server)
> >  {
> > @@ -85,14 +85,16 @@ cifs_ses_get_chan_index(struct cifs_ses *ses,
> >                 cifs_dbg(VFS, "unable to get chan index for server: 0x%llx",
> >                          server->conn_id);
> >         WARN_ON(1);
> > -       return 0;
> > +       return CIFS_INVAL_CHAN_INDEX;
> >  }
> >
> >  void
> >  cifs_chan_set_in_reconnect(struct cifs_ses *ses,
> >                              struct TCP_Server_Info *server)
> >  {
> > -       unsigned int chan_index = cifs_ses_get_chan_index(ses, server);
> > +       int chan_index = cifs_ses_get_chan_index(ses, server);
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX)
> > +               return;
> >
> >         ses->chans[chan_index].in_reconnect = true;
> >  }
> > @@ -102,6 +104,8 @@ cifs_chan_clear_in_reconnect(struct cifs_ses *ses,
> >                              struct TCP_Server_Info *server)
> >  {
> >         unsigned int chan_index = cifs_ses_get_chan_index(ses, server);
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX)
> > +               return;
> >
> >         ses->chans[chan_index].in_reconnect = false;
> >  }
> > @@ -111,6 +115,8 @@ cifs_chan_in_reconnect(struct cifs_ses *ses,
> >                           struct TCP_Server_Info *server)
> >  {
> >         unsigned int chan_index = cifs_ses_get_chan_index(ses, server);
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX)
> > +               return true;    /* err on the safer side */
> >
> >         return CIFS_CHAN_IN_RECONNECT(ses, chan_index);
> >  }
> > @@ -120,6 +126,8 @@ cifs_chan_set_need_reconnect(struct cifs_ses *ses,
> >                              struct TCP_Server_Info *server)
> >  {
> >         unsigned int chan_index = cifs_ses_get_chan_index(ses, server);
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX)
> > +               return;
> >
> >         set_bit(chan_index, &ses->chans_need_reconnect);
> >         cifs_dbg(FYI, "Set reconnect bitmask for chan %u; now 0x%lx\n",
> > @@ -131,6 +139,8 @@ cifs_chan_clear_need_reconnect(struct cifs_ses *ses,
> >                                struct TCP_Server_Info *server)
> >  {
> >         unsigned int chan_index = cifs_ses_get_chan_index(ses, server);
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX)
> > +               return;
> >
> >         clear_bit(chan_index, &ses->chans_need_reconnect);
> >         cifs_dbg(FYI, "Cleared reconnect bitmask for chan %u; now 0x%lx\n",
> > @@ -142,6 +152,8 @@ cifs_chan_needs_reconnect(struct cifs_ses *ses,
> >                           struct TCP_Server_Info *server)
> >  {
> >         unsigned int chan_index = cifs_ses_get_chan_index(ses, server);
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX)
> > +               return true;    /* err on the safer side */
> >
> >         return CIFS_CHAN_NEEDS_RECONNECT(ses, chan_index);
> >  }
> > @@ -151,6 +163,8 @@ cifs_chan_is_iface_active(struct cifs_ses *ses,
> >                           struct TCP_Server_Info *server)
> >  {
> >         unsigned int chan_index = cifs_ses_get_chan_index(ses, server);
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX)
> > +               return true;    /* err on the safer side */
> >
> >         return ses->chans[chan_index].iface &&
> >                 ses->chans[chan_index].iface->is_active;
> > @@ -269,7 +283,7 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
> >
> >         spin_lock(&ses->chan_lock);
> >         chan_index = cifs_ses_get_chan_index(ses, server);
> > -       if (!chan_index) {
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX) {
> >                 spin_unlock(&ses->chan_lock);
> >                 return 0;
> >         }
> > @@ -319,6 +333,9 @@ cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server)
> >
> >         spin_lock(&ses->chan_lock);
> >         chan_index = cifs_ses_get_chan_index(ses, server);
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX)
> > +               return 0;
> > +
> >         ses->chans[chan_index].iface = iface;
> >
> >         /* No iface is found. if secondary chan, drop connection */
> > diff --git a/fs/smb/client/smb2transport.c b/fs/smb/client/smb2transport.c
> > index 23c50ed7d4b5..84ea67301303 100644
> > --- a/fs/smb/client/smb2transport.c
> > +++ b/fs/smb/client/smb2transport.c
> > @@ -413,7 +413,13 @@ generate_smb3signingkey(struct cifs_ses *ses,
> >                       ses->ses_status == SES_GOOD);
> >
> >         chan_index = cifs_ses_get_chan_index(ses, server);
> > -       /* TODO: introduce ref counting for channels when the can be freed */
> > +       if (chan_index == CIFS_INVAL_CHAN_INDEX) {
> > +               spin_unlock(&ses->chan_lock);
> > +               spin_unlock(&ses->ses_lock);
> > +
> > +               return -EINVAL;
> > +       }
> > +
> >         spin_unlock(&ses->chan_lock);
> >         spin_unlock(&ses->ses_lock);
> >
> > --
> > 2.34.1
> >
>
>
> --
> Thanks,
>
> Steve



-- 
Regards,
Shyam