| Message ID | 20230626034257.2078391-2-wentao@uniontech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: fix session state checks to avoid use-after-free issues | expand |
On Mon, Jun 26, 2023 at 9:25 AM Winston Wen <wentao@uniontech.com> 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@uniontech.com> > --- > 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.
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@gmail.com> wrote: > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen <wentao@uniontech.com> 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@uniontech.com> > > --- > > 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
On Mon, Jun 26, 2023 at 10:54 AM Steve French <smfrench@gmail.com> 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@gmail.com> wrote: > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen <wentao@uniontech.com> 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@uniontech.com> > > > --- > > > 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.
On Mon, 26 Jun 2023 12:24:35 +0530 Shyam Prasad N <nspmangalore@gmail.com> wrote: > On Mon, Jun 26, 2023 at 10:54 AM Steve French <smfrench@gmail.com> > 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@gmail.com> wrote: > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > <wentao@uniontech.com> 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@uniontech.com> > > > > --- > > > > 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. So I gave up on this approach and did a small fix to make it work, but maybe I missed something elsewhere...
On Mon, Jun 26, 2023 at 2:09 PM Winston Wen <wentao@uniontech.com> wrote: > > On Mon, 26 Jun 2023 12:24:35 +0530 > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French <smfrench@gmail.com> > > 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@gmail.com> wrote: > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > <wentao@uniontech.com> 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@uniontech.com> > > > > > --- > > > > > 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. > > 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. The other two patches are not strictly necessary with this change, but don't hurt. -- Regards, Shyam
On Tue, 27 Jun 2023 12:24:04 +0530 Shyam Prasad N <nspmangalore@gmail.com> wrote: > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen <wentao@uniontech.com> > wrote: > > > > On Mon, 26 Jun 2023 12:24:35 +0530 > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French <smfrench@gmail.com> > > > 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@gmail.com> wrote: > > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > > <wentao@uniontech.com> 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@uniontech.com> > > > > > > --- > > > > > > 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. Thanks for your review and comments! > > -- > Regards, > Shyam
On Tue, Jun 27, 2023 at 1:04 PM Winston Wen <wentao@uniontech.com> wrote: > > On Tue, 27 Jun 2023 12:24:04 +0530 > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen <wentao@uniontech.com> > > wrote: > > > > > > On Mon, 26 Jun 2023 12:24:35 +0530 > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French <smfrench@gmail.com> > > > > 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@gmail.com> wrote: > > > > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > > > <wentao@uniontech.com> 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@uniontech.com> > > > > > > > --- > > > > > > > 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? > > Thanks for your review and comments! > > > > > -- > > Regards, > > Shyam > > > -- > Thanks, > Winston
On Tue, 27 Jun 2023 17:43:25 +0530 Shyam Prasad N <nspmangalore@gmail.com> wrote: > On Tue, Jun 27, 2023 at 1:04 PM Winston Wen <wentao@uniontech.com> > wrote: > > > > On Tue, 27 Jun 2023 12:24:04 +0530 > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen <wentao@uniontech.com> > > > wrote: > > > > > > > > On Mon, 26 Jun 2023 12:24:35 +0530 > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French > > > > > <smfrench@gmail.com> 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@gmail.com> wrote: > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > > > > <wentao@uniontech.com> 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@uniontech.com> > > > > > > > > --- > > > > > > > > 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 > > >
On Wed, 28 Jun 2023 08:43:33 +0800 Winston Wen <wentao@uniontech.com> wrote: > On Tue, 27 Jun 2023 17:43:25 +0530 > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > On Tue, Jun 27, 2023 at 1:04 PM Winston Wen <wentao@uniontech.com> > > wrote: > > > > > > On Tue, 27 Jun 2023 12:24:04 +0530 > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen > > > > <wentao@uniontech.com> wrote: > > > > > > > > > > On Mon, 26 Jun 2023 12:24:35 +0530 > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French > > > > > > <smfrench@gmail.com> 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@gmail.com> wrote: > > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > > > > > <wentao@uniontech.com> 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@uniontech.com> > > > > > > > > > --- > > > > > > > > > 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)
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. On Tue, Jun 27, 2023 at 8:55 PM Winston Wen <wentao@uniontech.com> wrote: > > On Wed, 28 Jun 2023 08:43:33 +0800 > Winston Wen <wentao@uniontech.com> wrote: > > > On Tue, 27 Jun 2023 17:43:25 +0530 > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > On Tue, Jun 27, 2023 at 1:04 PM Winston Wen <wentao@uniontech.com> > > > wrote: > > > > > > > > On Tue, 27 Jun 2023 12:24:04 +0530 > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen > > > > > <wentao@uniontech.com> wrote: > > > > > > > > > > > > On Mon, 26 Jun 2023 12:24:35 +0530 > > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French > > > > > > > <smfrench@gmail.com> 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@gmail.com> wrote: > > > > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > > > > > > <wentao@uniontech.com> 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@uniontech.com> > > > > > > > > > > --- > > > > > > > > > > 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
Hi Steve, On Wed, 28 Jun 2023 12:16:09 -0500 Steve French <smfrench@gmail.com> 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@uniontech.com> > wrote: > > > > On Wed, 28 Jun 2023 08:43:33 +0800 > > Winston Wen <wentao@uniontech.com> wrote: > > > > > On Tue, 27 Jun 2023 17:43:25 +0530 > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > On Tue, Jun 27, 2023 at 1:04 PM Winston Wen > > > > <wentao@uniontech.com> wrote: > > > > > > > > > > On Tue, 27 Jun 2023 12:24:04 +0530 > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen > > > > > > <wentao@uniontech.com> wrote: > > > > > > > > > > > > > > On Mon, 26 Jun 2023 12:24:35 +0530 > > > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French > > > > > > > > <smfrench@gmail.com> 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@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > > > > > > > <wentao@uniontech.com> 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@uniontech.com> > > > > > > > > > > > --- > > > > > > > > > > > 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 > > >
On Thu, Jun 29, 2023 at 6:45 AM Winston Wen <wentao@uniontech.com> wrote: > > Hi Steve, > > On Wed, 28 Jun 2023 12:16:09 -0500 > Steve French <smfrench@gmail.com> 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@uniontech.com> > > wrote: > > > > > > On Wed, 28 Jun 2023 08:43:33 +0800 > > > Winston Wen <wentao@uniontech.com> wrote: > > > > > > > On Tue, 27 Jun 2023 17:43:25 +0530 > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > On Tue, Jun 27, 2023 at 1:04 PM Winston Wen > > > > > <wentao@uniontech.com> wrote: > > > > > > > > > > > > On Tue, 27 Jun 2023 12:24:04 +0530 > > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen > > > > > > > <wentao@uniontech.com> wrote: > > > > > > > > > > > > > > > > On Mon, 26 Jun 2023 12:24:35 +0530 > > > > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French > > > > > > > > > <smfrench@gmail.com> 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@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > > > > > > > > <wentao@uniontech.com> 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@uniontech.com> > > > > > > > > > > > > --- > > > > > > > > > > > > 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 Hi Winston/Steve, It turns out that my patch above has a problem. For logoff to work, we need the session in the smb_ses_list. So I think Winston's patch series here make sense. @Winston Wen We may need similar checks for SES_EXITING in other places where we iterate smb_ses_list. Can you see if all the places are covered and send additional patches if needed? If you do not get to it in a few days, I can do it myself. Thanks for these patches. Keep them coming. :)
On Thu, 29 Jun 2023 21:50:18 +0530 Shyam Prasad N <nspmangalore@gmail.com> wrote: > On Thu, Jun 29, 2023 at 6:45 AM Winston Wen <wentao@uniontech.com> > wrote: > > > > Hi Steve, > > > > On Wed, 28 Jun 2023 12:16:09 -0500 > > Steve French <smfrench@gmail.com> 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@uniontech.com> > > > wrote: > > > > > > > > On Wed, 28 Jun 2023 08:43:33 +0800 > > > > Winston Wen <wentao@uniontech.com> wrote: > > > > > > > > > On Tue, 27 Jun 2023 17:43:25 +0530 > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > On Tue, Jun 27, 2023 at 1:04 PM Winston Wen > > > > > > <wentao@uniontech.com> wrote: > > > > > > > > > > > > > > On Tue, 27 Jun 2023 12:24:04 +0530 > > > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 2:09 PM Winston Wen > > > > > > > > <wentao@uniontech.com> wrote: > > > > > > > > > > > > > > > > > > On Mon, 26 Jun 2023 12:24:35 +0530 > > > > > > > > > Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 10:54 AM Steve French > > > > > > > > > > <smfrench@gmail.com> 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@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Jun 26, 2023 at 9:25 AM Winston Wen > > > > > > > > > > > > <wentao@uniontech.com> 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@uniontech.com> --- > > > > > > > > > > > > > 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 > > Hi Winston/Steve, > > It turns out that my patch above has a problem. > For logoff to work, we need the session in the smb_ses_list. > So I think Winston's patch series here make sense. > > @Winston Wen We may need similar checks for SES_EXITING in other > places where we iterate smb_ses_list. > Can you see if all the places are covered and send additional patches > if needed? If you do not get to it in a few days, I can do it myself. > > Thanks for these patches. Keep them coming. :) > Sorry for my late reply. I checked all the places where we iterate smb_ses_list and didn't see any real issues. But there is a dangerous pattern here which is very easy to misuse, and I don't know how to fix it... I think the rule when we iterate smb_ses_list is: we can't use a sesion which is exiting or may be exiting out of cifs_tcp_ses_lock. In some case, like smb2_reconnect_server() and cifs_find_smb_ses(), we can simply do the check and filter out exiting sessions. But in some other casees, like cifs_debug_files_proc_show(), smb2_check_message() and smb2_is_valid_oplock_break(), we may indeed need to handle all sessions, including those which are exiting. And we must be very careful to don't use the session we got after we release cifs_tcp_ses_lock. For example, the following change in smb2_check_message() will lead to potential use-after-free problem: smb2_check_message() { ... struct cifs_ses *ses = NULL; struct cifs_ses *iter; /* decrypt frame now that it is completely read in */ spin_lock(&cifs_tcp_ses_lock); list_for_each_entry(iter, &pserver->smb_ses_list, smb_ses_list) { if (iter->Suid == le64_to_cpu(thdr->SessionId)) { ses = iter; break; } } spin_unlock(&cifs_tcp_ses_lock); if (!ses) { cifs_dbg(VFS, "no decryption - session id not found\n"); return 1; } /* New code: */ cifs_dbg(FYI, "server inflight=%d\n", ses->server->in_flight); ... } Because once we realse cifs_tcp_ses_lock, we can no longer use the session. I'm not sure if this needs to be fixed and/or how to fix it, what do you think about it?
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);
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@uniontech.com> --- fs/smb/client/connect.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)