[1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held

From: Shyam Prasad N <sprasad@microsoft.com>

cifs_chan_is_iface_active checks the channels of a session to see
if the associated iface is active. This should always happen
with chan_lock held. However, these two callers of this function
were missing this locking.

This change makes sure the function calls are protected with
proper locking.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/connect.c | 7 +++++--
 fs/smb/client/smb2ops.c | 7 ++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> cifs_chan_is_iface_active checks the channels of a session to see
> if the associated iface is active. This should always happen
> with chan_lock held. However, these two callers of this function
> were missing this locking.
>
> This change makes sure the function calls are protected with
> proper locking.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/connect.c | 7 +++++--
>  fs/smb/client/smb2ops.c | 7 ++++++-
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 8b7cffba1ad5..3052a208c6ca 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -232,10 +232,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
>         spin_lock(&cifs_tcp_ses_lock);
>         list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
>                 /* check if iface is still active */
> -               if (!cifs_chan_is_iface_active(ses, server))
> +               spin_lock(&ses->chan_lock);
> +               if (!cifs_chan_is_iface_active(ses, server)) {
> +                       spin_unlock(&ses->chan_lock);
>                         cifs_chan_update_iface(ses, server);
> +                       spin_lock(&ses->chan_lock);
> +               }
>
> -               spin_lock(&ses->chan_lock);
>                 if (!mark_smb_session && cifs_chan_needs_reconnect(ses, server)) {
>                         spin_unlock(&ses->chan_lock);
>                         continue;
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 441d144bd712..104c58df0368 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -784,9 +784,14 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
>                 goto out;
>
>         /* check if iface is still active */
> +       spin_lock(&ses->chan_lock);
>         pserver = ses->chans[0].server;
> -       if (pserver && !cifs_chan_is_iface_active(ses, pserver))
> +       if (pserver && !cifs_chan_is_iface_active(ses, pserver)) {
> +               spin_unlock(&ses->chan_lock);
>                 cifs_chan_update_iface(ses, pserver);
> +               spin_lock(&ses->chan_lock);
> +       }
> +       spin_unlock(&ses->chan_lock);
>
>  out:
>         kfree(out_buf);
> --
> 2.34.1
>

This one fixes two changes. Not sure if it's valid to have two Fixes tag.
Fixes: b54034a73baf ("cifs: during reconnect, update interface if necessary")
Fixes: fa1d0508bdd4 ("cifs: account for primary channel in the interface list")

Should also CC stable.

updated patch attached (to fix minor merge conflict)

On Fri, Dec 29, 2023 at 5:25 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
> >
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > cifs_chan_is_iface_active checks the channels of a session to see
> > if the associated iface is active. This should always happen
> > with chan_lock held. However, these two callers of this function
> > were missing this locking.
> >
> > This change makes sure the function calls are protected with
> > proper locking.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/connect.c | 7 +++++--
> >  fs/smb/client/smb2ops.c | 7 ++++++-
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 8b7cffba1ad5..3052a208c6ca 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -232,10 +232,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
> >         spin_lock(&cifs_tcp_ses_lock);
> >         list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
> >                 /* check if iface is still active */
> > -               if (!cifs_chan_is_iface_active(ses, server))
> > +               spin_lock(&ses->chan_lock);
> > +               if (!cifs_chan_is_iface_active(ses, server)) {
> > +                       spin_unlock(&ses->chan_lock);
> >                         cifs_chan_update_iface(ses, server);
> > +                       spin_lock(&ses->chan_lock);
> > +               }
> >
> > -               spin_lock(&ses->chan_lock);
> >                 if (!mark_smb_session && cifs_chan_needs_reconnect(ses, server)) {
> >                         spin_unlock(&ses->chan_lock);
> >                         continue;
> > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > index 441d144bd712..104c58df0368 100644
> > --- a/fs/smb/client/smb2ops.c
> > +++ b/fs/smb/client/smb2ops.c
> > @@ -784,9 +784,14 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
> >                 goto out;
> >
> >         /* check if iface is still active */
> > +       spin_lock(&ses->chan_lock);
> >         pserver = ses->chans[0].server;
> > -       if (pserver && !cifs_chan_is_iface_active(ses, pserver))
> > +       if (pserver && !cifs_chan_is_iface_active(ses, pserver)) {
> > +               spin_unlock(&ses->chan_lock);
> >                 cifs_chan_update_iface(ses, pserver);
> > +               spin_lock(&ses->chan_lock);
> > +       }
> > +       spin_unlock(&ses->chan_lock);
> >
> >  out:
> >         kfree(out_buf);
> > --
> > 2.34.1
> >
>
> This one fixes two changes. Not sure if it's valid to have two Fixes tag.

Yes - that is ok (two Fixes tags)

ignore my rebased patch. I applied his series in wrong order

On Fri, Dec 29, 2023 at 8:57 AM Steve French <smfrench@gmail.com> wrote:
>
> updated patch attached (to fix minor merge conflict)
>
> On Fri, Dec 29, 2023 at 5:25 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > On Fri, Dec 29, 2023 at 4:46 PM <nspmangalore@gmail.com> wrote:
> > >
> > > From: Shyam Prasad N <sprasad@microsoft.com>
> > >
> > > cifs_chan_is_iface_active checks the channels of a session to see
> > > if the associated iface is active. This should always happen
> > > with chan_lock held. However, these two callers of this function
> > > were missing this locking.
> > >
> > > This change makes sure the function calls are protected with
> > > proper locking.
> > >
> > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > ---
> > >  fs/smb/client/connect.c | 7 +++++--
> > >  fs/smb/client/smb2ops.c | 7 ++++++-
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > > index 8b7cffba1ad5..3052a208c6ca 100644
> > > --- a/fs/smb/client/connect.c
> > > +++ b/fs/smb/client/connect.c
> > > @@ -232,10 +232,13 @@ cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
> > >         spin_lock(&cifs_tcp_ses_lock);
> > >         list_for_each_entry_safe(ses, nses, &pserver->smb_ses_list, smb_ses_list) {
> > >                 /* check if iface is still active */
> > > -               if (!cifs_chan_is_iface_active(ses, server))
> > > +               spin_lock(&ses->chan_lock);
> > > +               if (!cifs_chan_is_iface_active(ses, server)) {
> > > +                       spin_unlock(&ses->chan_lock);
> > >                         cifs_chan_update_iface(ses, server);
> > > +                       spin_lock(&ses->chan_lock);
> > > +               }
> > >
> > > -               spin_lock(&ses->chan_lock);
> > >                 if (!mark_smb_session && cifs_chan_needs_reconnect(ses, server)) {
> > >                         spin_unlock(&ses->chan_lock);
> > >                         continue;
> > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > > index 441d144bd712..104c58df0368 100644
> > > --- a/fs/smb/client/smb2ops.c
> > > +++ b/fs/smb/client/smb2ops.c
> > > @@ -784,9 +784,14 @@ SMB3_request_interfaces(const unsigned int xid, struct cifs_tcon *tcon, bool in_
> > >                 goto out;
> > >
> > >         /* check if iface is still active */
> > > +       spin_lock(&ses->chan_lock);
> > >         pserver = ses->chans[0].server;
> > > -       if (pserver && !cifs_chan_is_iface_active(ses, pserver))
> > > +       if (pserver && !cifs_chan_is_iface_active(ses, pserver)) {
> > > +               spin_unlock(&ses->chan_lock);
> > >                 cifs_chan_update_iface(ses, pserver);
> > > +               spin_lock(&ses->chan_lock);
> > > +       }
> > > +       spin_unlock(&ses->chan_lock);
> > >
> > >  out:
> > >         kfree(out_buf);
> > > --
> > > 2.34.1
> > >
> >
> > This one fixes two changes. Not sure if it's valid to have two Fixes tag.
>
> Yes - that is ok (two Fixes tags)
> --
> Thanks,
>
> Steve