Hi Steve,
On Wed, 28 Jun 2023 12:16:09 -0500
Steve French <smfrench@xxxxxxxxx> wrote:
> this didn't apply (merge conflict with Shyam's updated patch) - can
> you update it based on for-next and resend
>
> Also let me know if you see other patches missing.
With Shyam's updated patch, It is expected that no
existing session would be found in the list, so the check of session
state is no longer strictly necessary now, but don't hurt.
So we have 2 choices in scanning the list:
- do the check. (patch 2/3, merged)
- remove the check and add a warning. (this new patch)
If you find the latter to be better, you have the option to replace the
original two patches with this new one.
Alternatively, you can simply disregard the new patch and take no
action, as the patch 2/3 has already been merged into the for-next
branch. (This may be a better choice for now, but I don't have a strong
opinion on this, both are okay to me.)
Really sorry for my poor English and the lack of clarity in my
explanation.
>
> On Tue, Jun 27, 2023 at 8:55 PM Winston Wen <wentao@xxxxxxxxxxxxx>
> wrote:
> >
> > On Wed, 28 Jun 2023 08:43:33 +0800
> > Winston Wen <wentao@xxxxxxxxxxxxx> wrote:
> >
> > > On Tue, 27 Jun 2023 17:43:25 +0530
> > > Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
> > >
> > > > On Tue, Jun 27, 2023 at 1:04 PM Winston Wen
> > > > <wentao@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, 27 Jun 2023 12:24:04 +0530
> > > > > Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
> > > > >
> > > > > > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen
> > > > > > <wentao@xxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Mon, 26 Jun 2023 12:24:35 +0530
> > > > > > > Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
> > > > > > >
> > > > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French
> > > > > > > > <smfrench@xxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > Added Cc: stable and Shyam's RB and merged into
> > > > > > > > > cifs-2.6.git for-next
> > > > > > > > >
> > > > > > > > > On Mon, Jun 26, 2023 at 12:15 AM Shyam Prasad N
> > > > > > > > > <nspmangalore@xxxxxxxxx> wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen
> > > > > > > > > > <wentao@xxxxxxxxxxxxx> wrote:
> > > > > > > > > > >
> > > > > > > > > > > We switch session state to SES_EXITING without
> > > > > > > > > > > cifs_tcp_ses_lock now, it may lead to potential
> > > > > > > > > > > use-after-free issue.
> > > > > > > > > > >
> > > > > > > > > > > Consider the following execution processes:
> > > > > > > > > > >
> > > > > > > > > > > Thread 1:
> > > > > > > > > > > __cifs_put_smb_ses()
> > > > > > > > > > > spin_lock(&cifs_tcp_ses_lock)
> > > > > > > > > > > if (--ses->ses_count > 0)
> > > > > > > > > > > spin_unlock(&cifs_tcp_ses_lock)
> > > > > > > > > > > return
> > > > > > > > > > > spin_unlock(&cifs_tcp_ses_lock)
> > > > > > > > > > > ---> **GAP**
> > > > > > > > > > > spin_lock(&ses->ses_lock)
> > > > > > > > > > > if (ses->ses_status == SES_GOOD)
> > > > > > > > > > > ses->ses_status = SES_EXITING
> > > > > > > > > > > spin_unlock(&ses->ses_lock)
> > > > > > > > > > >
> > > > > > > > > > > Thread 2:
> > > > > > > > > > > cifs_find_smb_ses()
> > > > > > > > > > > spin_lock(&cifs_tcp_ses_lock)
> > > > > > > > > > > list_for_each_entry(ses, ...)
> > > > > > > > > > > spin_lock(&ses->ses_lock)
> > > > > > > > > > > if (ses->ses_status == SES_EXITING)
> > > > > > > > > > > spin_unlock(&ses->ses_lock)
> > > > > > > > > > > continue
> > > > > > > > > > > ...
> > > > > > > > > > > spin_unlock(&ses->ses_lock)
> > > > > > > > > > > if (ret)
> > > > > > > > > > > cifs_smb_ses_inc_refcount(ret)
> > > > > > > > > > > spin_unlock(&cifs_tcp_ses_lock)
> > > > > > > > > > >
> > > > > > > > > > > If thread 1 is preempted in the gap and thread 2
> > > > > > > > > > > start executing, thread 2 will get the session,
> > > > > > > > > > > and soon thread 1 will switch the session state
> > > > > > > > > > > to SES_EXITING and start releasing it, even
> > > > > > > > > > > though thread 1 had increased the session's
> > > > > > > > > > > refcount and still uses it.
> > > > > > > > > > >
> > > > > > > > > > > So switch session state under cifs_tcp_ses_lock to
> > > > > > > > > > > eliminate this gap.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Winston Wen <wentao@xxxxxxxxxxxxx>
> > > > > > > > > > > ---
> > > > > > > > > > > fs/smb/client/connect.c | 7 ++++---
> > > > > > > > > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/fs/smb/client/connect.c
> > > > > > > > > > > b/fs/smb/client/connect.c index
> > > > > > > > > > > 9d16626e7a66..165ecb222c19 100644 ---
> > > > > > > > > > > a/fs/smb/client/connect.c +++
> > > > > > > > > > > b/fs/smb/client/connect.c @@ -1963,15 +1963,16 @@
> > > > > > > > > > > void __cifs_put_smb_ses(struct cifs_ses *ses)
> > > > > > > > > > > spin_unlock(&cifs_tcp_ses_lock); return;
> > > > > > > > > > > }
> > > > > > > > > > > + spin_lock(&ses->ses_lock);
> > > > > > > > > > > + if (ses->ses_status == SES_GOOD)
> > > > > > > > > > > + ses->ses_status = SES_EXITING;
> > > > > > > > > > > + spin_unlock(&ses->ses_lock);
> > > > > > > > > > > spin_unlock(&cifs_tcp_ses_lock);
> > > > > > > > > > >
> > > > > > > > > > > /* ses_count can never go negative */
> > > > > > > > > > > WARN_ON(ses->ses_count < 0);
> > > > > > > > > > >
> > > > > > > > > > > spin_lock(&ses->ses_lock);
> > > > > > > > > > > - if (ses->ses_status == SES_GOOD)
> > > > > > > > > > > - ses->ses_status = SES_EXITING;
> > > > > > > > > > > -
> > > > > > > > > > > if (ses->ses_status == SES_EXITING &&
> > > > > > > > > > > server->ops->logoff) {
> > > > > > > > > > > spin_unlock(&ses->ses_lock); cifs_free_ipc(ses);
> > > > > > > > > > > --
> > > > > > > > > > > 2.40.1
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Good catch.
> > > > > > > > > > Looks good to me.
> > > > > > > > > > @Steve French Please CC stable for this one.
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Regards,
> > > > > > > > > > Shyam
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Thanks,
> > > > > > > > >
> > > > > > > > > Steve
> > > > > > > >
> > > > > > > > @Winston Wen I think the following change should be
> > > > > > > > sufficient to fix this issue:
> > > > > > > > diff --git a/fs/smb/client/connect.c
> > > > > > > > b/fs/smb/client/connect.c index
> > > > > > > > 9d16626e7a66..78874eb2537d 100644 ---
> > > > > > > > a/fs/smb/client/connect.c +++ b/fs/smb/client/connect.c
> > > > > > > > @@ -1963,10 +1963,11 @@ void __cifs_put_smb_ses(struct
> > > > > > > > cifs_ses *ses) spin_unlock(&cifs_tcp_ses_lock);
> > > > > > > > return;
> > > > > > > > }
> > > > > > > > - spin_unlock(&cifs_tcp_ses_lock);
> > > > > > > >
> > > > > > > > /* ses_count can never go negative */
> > > > > > > > WARN_ON(ses->ses_count < 0);
> > > > > > > > + list_del_init(&ses->smb_ses_list);
> > > > > > > > + spin_unlock(&cifs_tcp_ses_lock);
> > > > > > > >
> > > > > > > > spin_lock(&ses->ses_lock);
> > > > > > > > if (ses->ses_status == SES_GOOD)
> > > > > > > > @@ -1986,9 +1987,6 @@ void __cifs_put_smb_ses(struct
> > > > > > > > cifs_ses *ses) cifs_free_ipc(ses);
> > > > > > > > }
> > > > > > > >
> > > > > > > > - spin_lock(&cifs_tcp_ses_lock);
> > > > > > > > - list_del_init(&ses->smb_ses_list);
> > > > > > > > - spin_unlock(&cifs_tcp_ses_lock);
> > > > > > > >
> > > > > > > > chan_count = ses->chan_count;
> > > > > > > >
> > > > > > > > The bug was that the ses was kept in the smb ses list,
> > > > > > > > even after the ref count had reached 0.
> > > > > > > > With the above change, that should be fixed, and no one
> > > > > > > > should be able to get to the ses from that point.
> > > > > > > >
> > > > > > > > Please let me know if you see a problem with this.
> > > > > > > >
> > > > > > >
> > > > > > > Hi Shyam,
> > > > > > >
> > > > > > > Thanks for the comments! And sorry for my late reply...
> > > > > > >
> > > > > > > It make sense to me that maybe we should remove the
> > > > > > > session from the list once its refcount is reduced to 0
> > > > > > > to avoid any futher access. In fact, I did try to do this
> > > > > > > from the beginning. But I was not sure if we need to
> > > > > > > access the session from the list in the free process,
> > > > > > > such as the following:
> > > > > > >
> > > > > > > smb2_check_receive()
> > > > > > > smb2_verify_signature()
> > > > > > > server->ops->calc_signature()
> > > > > > > smb2_calc_signature()
> > > > > > > smb2_find_smb_ses()
> > > > > > > /* scan the list and find the session */
> > > > > > >
> > > > > > > Perhaps we need some refactoring here.
> > > > > >
> > > > > > Yes. The above ses finding is expected to fail during a
> > > > > > reconnect.
> > > > >
> > > > > Agreed.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > So I gave up on this approach and did a small fix to make
> > > > > > > it work, but maybe I missed something elsewhere...
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > > Winston
> > > > > >
> > > > > > Attaching the above change as a patch.
> > > > > > It replaces this particular patch in the series.
> > > > >
> > > > > I think this is a better way to fix the problem, the session
> > > > > really should not stay in the list and be found after it has
> > > > > been marked EXITING.
> > > > >
> > > > > >
> > > > > > The other two patches are not strictly necessary with this
> > > > > > change, but don't hurt.
> > > > > >
> > > > >
> > > > > Yes. Feel free to drop them if they are not necessary. And if
> > > > > that's the case, perhaps we should do some cleaning work on
> > > > > other paths to ensure consistency.
> > > >
> > > > I don't really have a strong opinion about this. Even if they
> > > > stay, I'm okay. But curious to know what you mean by the
> > > > cleaning work on other paths here. Do you still think there's
> > > > more cleanup needed around this?
> > >
> > > IIRC there are other paths that scan the list and do the
> > > check, like cifs_find_smb_ses(). So I think if they become
> > > unnecessary now after this fix patch, maybe we can also remove
> > > them at the same time to avoid make others confused.
> > >
> > > But I also don't have a strong opinion about this. I think we
> > > have the following options and all are okay to me. Which one do
> > > you prefer?
> > >
> > > - keep/add the check
> > > - remove all checks
> > > - remove all checks and add a WARNING
> > >
> > > (I think we shouldn't find a exiting session in the list now.)
> > >
> > > >
> > > > >
> > > > > Thanks for your review and comments!
> > > > >
> > > > > >
> > > > > > --
> > > > > > Regards,
> > > > > > Shyam
> > > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > > Winston
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> > Attaching the patch (remove all checks and add a warning)
> >
> > --
> > Thanks,
> > Winston
>
>
>
--
Thanks,
Winston
