| Message ID | 20231229111618.38887-1-sprasad@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] cifs: cifs_chan_is_iface_active should be called with chan_lock held | expand |
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
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);