| Message ID | 20220413000217.1609615-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: verify that tcon is valid before dereference in cifs_kill_sb | expand |
On Wed, Apr 13, 2022 at 7:06 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > On umount, cifs_sb->tlink_tree might contain entries that do not represent > a valid tcon. > Check the tcon for error before we dereference it. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cifsfs.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index aba0783a8f09..25719b70564a 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -266,10 +266,11 @@ static void cifs_kill_sb(struct super_block *sb) > * before we kill the sb. > */ > if (cifs_sb->root) { > - node = rb_first(root); > - while (node != NULL) { > + for(node = rb_first(root); node; node = rb_next(node)) { > tlink = rb_entry(node, struct tcon_link, tl_rbnode); > tcon = tlink_tcon(tlink); > + if (IS_ERR(tcon)) > + continue; > cfid = &tcon->crfid; > mutex_lock(&cfid->fid_mutex); > if (cfid->dentry) { > @@ -277,7 +278,6 @@ static void cifs_kill_sb(struct super_block *sb) > cfid->dentry = NULL; > } > mutex_unlock(&cfid->fid_mutex); > - node = rb_next(node); > } > > /* finally release root dentry */ > -- > 2.30.2 > The changes look good to me. Reviewed-by: Shyam Prasad N <sprasad@microsoft.com> Does this cover all the mount failure scenarios, Ronnie?
On Thu, Apr 14, 2022 at 11:33 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Wed, Apr 13, 2022 at 7:06 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > On umount, cifs_sb->tlink_tree might contain entries that do not represent > > a valid tcon. > > Check the tcon for error before we dereference it. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/cifsfs.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > index aba0783a8f09..25719b70564a 100644 > > --- a/fs/cifs/cifsfs.c > > +++ b/fs/cifs/cifsfs.c > > @@ -266,10 +266,11 @@ static void cifs_kill_sb(struct super_block *sb) > > * before we kill the sb. > > */ > > if (cifs_sb->root) { > > - node = rb_first(root); > > - while (node != NULL) { > > + for(node = rb_first(root); node; node = rb_next(node)) { > > tlink = rb_entry(node, struct tcon_link, tl_rbnode); > > tcon = tlink_tcon(tlink); > > + if (IS_ERR(tcon)) > > + continue; > > cfid = &tcon->crfid; > > mutex_lock(&cfid->fid_mutex); > > if (cfid->dentry) { > > @@ -277,7 +278,6 @@ static void cifs_kill_sb(struct super_block *sb) > > cfid->dentry = NULL; > > } > > mutex_unlock(&cfid->fid_mutex); > > - node = rb_next(node); > > } > > > > /* finally release root dentry */ > > -- > > 2.30.2 > > > > The changes look good to me. > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com> > > Does this cover all the mount failure scenarios, Ronnie? It should. It should also fix similar issues at umount if an idle tcon has been reaped, or say a tcon failed to initialize during multiuser. For example, mount multiuser, then try to access the share with a different user without credentials. That second tcon will fail and then when we umount we would oops. > > -- > Regards, > Shyam
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index aba0783a8f09..25719b70564a 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -266,10 +266,11 @@ static void cifs_kill_sb(struct super_block *sb) * before we kill the sb. */ if (cifs_sb->root) { - node = rb_first(root); - while (node != NULL) { + for(node = rb_first(root); node; node = rb_next(node)) { tlink = rb_entry(node, struct tcon_link, tl_rbnode); tcon = tlink_tcon(tlink); + if (IS_ERR(tcon)) + continue; cfid = &tcon->crfid; mutex_lock(&cfid->fid_mutex); if (cfid->dentry) { @@ -277,7 +278,6 @@ static void cifs_kill_sb(struct super_block *sb) cfid->dentry = NULL; } mutex_unlock(&cfid->fid_mutex); - node = rb_next(node); } /* finally release root dentry */
On umount, cifs_sb->tlink_tree might contain entries that do not represent a valid tcon. Check the tcon for error before we dereference it. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cifsfs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)