Re: [PATCH 09/14] cifs: add a back pointer to cifs_sb from tcon

Shyam Prasad N
2023-11-06T17:04:44Z

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

On Mon, Nov 6, 2023 at 9:42 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Sat, Nov 4, 2023 at 2:33 AM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > nspmangalore@gmail.com writes:
> >
> > > From: Shyam Prasad N <sprasad@microsoft.com>
> > >
> > > Today, we have no way to access the cifs_sb when we
> > > just have pointers to struct tcon. This is very
> > > limiting as many functions deal with cifs_sb, and
> > > these calls do not directly originate from VFS.
> > >
> > > This change introduces a new cifs_sb field in cifs_tcon
> > > that points to the cifs_sb for the tcon. The assumption
> > > here is that a tcon will always map to this cifs_sb and
> > > will never change.
> > >
> > > Also, refcounting should not be necessary, since cifs_sb
> > > will never be freed before tcon.
> > >
> > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > ---
> > > fs/smb/client/cifsglob.h | 1 +
> > > fs/smb/client/connect.c | 2 ++
> > > 2 files changed, 3 insertions(+)
> >
> > This is wrong as a single tcon may be shared among different
> > superblocks. You can, however, map those superblocks to a tcon by using
> > the cifs_sb_master_tcon() helper.
> >
> > If you do something like this
> >
> > mount.cifs //srv/share /mnt/1 -o ...
> > mount.cifs //srv/share /mnt/1 -o ... -> -EBUSY
> >
> > tcon->cifs_sb will end up with the already freed superblock pointer that
> > was compared to the existing one. So, you'll get an use-after-free when
> > you dereference tcon->cifs_sb as in patch 11/14.
>
> Hi Steve,
> I discussed this one with Paulo separately. You can drop this patch.
> I'll have another patch in place of this one. And then send you an
> updated patch for the patch which depends on it.
>
> --
> Regards,
> Shyam

Here's the replacement patch for "cifs: add a back pointer to cifs_sb from tcon"

--
Regards,
Shyam

[-- Attachment #2: 0004-cifs-do-not-pass-cifs_sb-when-trying-to-add-channels.patch --]
[-- Type: application/octet-stream, Size: 4004 bytes --]

From 833e5471212056e107757f1df908e0980ccd9112 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Mon, 6 Nov 2023 16:22:11 +0000
Subject: [PATCH 4/9] cifs: do not pass cifs_sb when trying to add channels

The only reason why cifs_sb gets passed today to cifs_try_adding_channels
is to pass the local_nls field for the new channels and binding session.
However, the ses struct already has local_nls field that is setup during
the first cifs_setup_session. So there is no need to pass cifs_sb.

This change removes cifs_sb from the arg list for this and the functions
that it calls and uses ses->local_nls instead.

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

diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index c1f71c6be7e3..eed8dbb6b2fb 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -610,7 +610,7 @@ void cifs_free_hash(struct shash_desc **sdesc);

struct cifs_chan *
cifs_ses_find_chan(struct cifs_ses *ses, struct TCP_Server_Info *server);
-int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
+int cifs_try_adding_channels(struct cifs_ses *ses);
bool is_server_using_iface(struct TCP_Server_Info *server,
struct cifs_server_iface *iface);
bool is_ses_using_iface(struct cifs_ses *ses, struct cifs_server_iface *iface);
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 3ff82f0aa00e..947e3c362beb 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -3564,7 +3564,7 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
ctx->prepath = NULL;

out:
- cifs_try_adding_channels(cifs_sb, mnt_ctx.ses);
+ cifs_try_adding_channels(mnt_ctx.ses);
rc = mount_setup_tlink(cifs_sb, mnt_ctx.ses, mnt_ctx.tcon);
if (rc)
goto error;
diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
index f454ee1de9ef..b4e60dcf3a94 100644
--- a/fs/smb/client/sess.c
+++ b/fs/smb/client/sess.c
@@ -24,7 +24,7 @@
#include "fs_context.h"

static int
-cifs_ses_add_channel(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses,
+cifs_ses_add_channel(struct cifs_ses *ses,
struct cifs_server_iface *iface);

bool
@@ -171,7 +171,7 @@ cifs_chan_is_iface_active(struct cifs_ses *ses,
}

/* returns number of channels added */
-int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses)
+int cifs_try_adding_channels(struct cifs_ses *ses)
{
struct TCP_Server_Info *server = ses->server;
int old_chan_count, new_chan_count;
@@ -253,7 +253,7 @@ int cifs_try_adding_channels(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses)
kref_get(&iface->refcount);

spin_unlock(&ses->iface_lock);
- rc = cifs_ses_add_channel(cifs_sb, ses, iface);
+ rc = cifs_ses_add_channel(ses, iface);
spin_lock(&ses->iface_lock);

if (rc) {
@@ -449,7 +449,7 @@ cifs_ses_find_chan(struct cifs_ses *ses, struct TCP_Server_Info *server)
}

static int
-cifs_ses_add_channel(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses,
+cifs_ses_add_channel(struct cifs_ses *ses,
struct cifs_server_iface *iface)
{
struct TCP_Server_Info *chan_server;
@@ -528,7 +528,7 @@ cifs_ses_add_channel(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses,
* This will be used for encoding/decoding user/domain/pw
* during sess setup auth.
*/
- ctx->local_nls = cifs_sb->local_nls;
+ ctx->local_nls = ses->local_nls;

/* Use RDMA if possible */
ctx->rdma = iface->rdma_capable;
@@ -574,7 +574,7 @@ cifs_ses_add_channel(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses,

rc = cifs_negotiate_protocol(xid, ses, chan->server);
if (!rc)
- rc = cifs_setup_session(xid, ses, chan->server, cifs_sb->local_nls);
+ rc = cifs_setup_session(xid, ses, chan->server, ses->local_nls);

mutex_unlock(&ses->session_mutex);

--
2.34.1