diff mbox series

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

Message ID 20231030110020.45627-9-sprasad@microsoft.com (mailing list archive)
State New, archived
Headers show
Series [01/14] cifs: print server capabilities in DebugData | expand

Commit Message

Shyam Prasad N Oct. 30, 2023, 11 a.m. UTC
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(+)

Comments

Steve French Nov. 1, 2023, 3:30 a.m. UTC | #1
merged into cifs-2.6.git for-next pending more testing

added cc:stable

On Mon, Oct 30, 2023 at 6:00 AM <nspmangalore@gmail.com> wrote:
>
> 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(+)
>
> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 81e7a45f413d..cdbc2cd207dc 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1134,6 +1134,7 @@ struct cifs_tcon {
>         int tc_count;
>         struct list_head rlist; /* reconnect list */
>         spinlock_t tc_lock;  /* protect anything here that is not protected */
> +       struct cifs_sb_info *cifs_sb; /* back pointer to cifs super block */
>         atomic_t num_local_opens;  /* num of all opens including disconnected */
>         atomic_t num_remote_opens; /* num of all network opens on server */
>         struct list_head openFileList;
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index 8393977e21ee..184075da5c6e 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -3355,6 +3355,7 @@ int cifs_mount_get_tcon(struct cifs_mount_ctx *mnt_ctx)
>                 tcon = NULL;
>                 goto out;
>         }
> +       tcon->cifs_sb = cifs_sb;
>
>         /* if new SMB3.11 POSIX extensions are supported do not remap / and \ */
>         if (tcon->posix_extensions)
> @@ -3986,6 +3987,7 @@ cifs_construct_tcon(struct cifs_sb_info *cifs_sb, kuid_t fsuid)
>                 cifs_put_smb_ses(ses);
>                 goto out;
>         }
> +       tcon->cifs_sb = cifs_sb;
>
>  #ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
>         if (cap_unix(ses))
> --
> 2.34.1
>
Paulo Alcantara Nov. 3, 2023, 9:03 p.m. UTC | #2
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.
Shyam Prasad N Nov. 6, 2023, 4:12 p.m. UTC | #3
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.
Shyam Prasad N Nov. 6, 2023, 5:04 p.m. UTC | #4
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"
Steve French Nov. 6, 2023, 7:36 p.m. UTC | #5
resending to list

---------- Forwarded message ---------
From: Steve French <smfrench@gmail.com>
Date: Mon, Nov 6, 2023 at 1:13 PM
Subject: Re: [PATCH 09/14] cifs: add a back pointer to cifs_sb from tcon
To: Shyam Prasad N <nspmangalore@gmail.com>
Cc: Paulo Alcantara <pc@manguebit.com>, <bharathsm.hsk@gmail.com>,
<linux-cifs@vger.kernel.org>, Shyam Prasad N <sprasad@microsoft.com>


Updated patches again to address compile/sparse warning and pushed to
for-next pending more testing and review

Attached are updated patches 1 and 2

On Mon, Nov 6, 2023 at 12:45 PM Steve French <smfrench@gmail.com> wrote:
>
> lightly updated patch 1 and patch 2 in your series to fix checkpatch warnings
>
> Was thinking about patch 2 though and whether it could hurt cases where there is little parallelism - we could randomly pick channels to send things on so we could overuse channels with higher latency rather than preferring the first two channels which are the "faster" ones (and presumably lower latencies).   Was wondering if we end up picking e.g. 4 channels, 1 of which is "fast" and 3 are slower - we could end up with cases where no traffic on channel 1 but we end up sending on a longer latency channlel - ie the round robin approach could end up using channel 2 or 3 or 4 which have longer latencies even if no traffic on the "fastest" channel ...
>
>
> On Mon, Nov 6, 2023 at 11:04 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>>
>> 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
>
>
>
> --
> Thanks,
>
> Steve
Paulo Alcantara Nov. 8, 2023, 3:24 p.m. UTC | #6
Shyam Prasad N <nspmangalore@gmail.com> writes:

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

Looks good,

Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
Steve French Nov. 8, 2023, 4:11 p.m. UTC | #7
added RB for you for this and for "cifs: Fix encryption of cleared,
but unset rq_iter ..."

Let me know if any more RB or Acked-by for the other 11 in for-next

fce2374a6ce1 (HEAD -> for-next) smb3: fix caching of ctime on setxattr
8cc431e2afba smb3: more minor cleanups for session handling routines
2828c39da33f smb3: minor cleanup of session handling code
e8c286d1fb82 cifs: update internal module version number for cifs.ko
a25ebba8a4f2 smb3: minor RDMA cleanup
091578169aa4 cifs: handle when server stops supporting multichannel
a340e7145da5 cifs: handle when server starts supporting multichannel
693358495264 cifs: reconnect work should have reference on server struct
e935ee282ccf cifs: do not pass cifs_sb when trying to add channels
4266585b23f4 cifs: account for primary channel in the interface list
6dbc7a50c5e4 cifs: distribute channels across interfaces based on speed
44a65e388107 cifs: handle cases where a channel is closed
37de5a80e932 cifs: Fix encryption of cleared, but unset rq_iter data buffers

Am trying to work on a few patches today (e.g. caching root handle
across mount when no dir leases support to reduce open and close)
and how we might save off an "altpassword" when servers do "key
rotation" (where you move from one password to another but don't want
to risk sessions going down for a while for that user).  Let me know
if any patches/features for you in next few days of merge window

On Wed, Nov 8, 2023 at 9:24 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
>
> > Here's the replacement patch for "cifs: add a back pointer to cifs_sb from tcon"
>
> Looks good,
>
> Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
diff mbox series

Patch

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 81e7a45f413d..cdbc2cd207dc 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1134,6 +1134,7 @@  struct cifs_tcon {
 	int tc_count;
 	struct list_head rlist; /* reconnect list */
 	spinlock_t tc_lock;  /* protect anything here that is not protected */
+	struct cifs_sb_info *cifs_sb; /* back pointer to cifs super block */
 	atomic_t num_local_opens;  /* num of all opens including disconnected */
 	atomic_t num_remote_opens; /* num of all network opens on server */
 	struct list_head openFileList;
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 8393977e21ee..184075da5c6e 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -3355,6 +3355,7 @@  int cifs_mount_get_tcon(struct cifs_mount_ctx *mnt_ctx)
 		tcon = NULL;
 		goto out;
 	}
+	tcon->cifs_sb = cifs_sb;
 
 	/* if new SMB3.11 POSIX extensions are supported do not remap / and \ */
 	if (tcon->posix_extensions)
@@ -3986,6 +3987,7 @@  cifs_construct_tcon(struct cifs_sb_info *cifs_sb, kuid_t fsuid)
 		cifs_put_smb_ses(ses);
 		goto out;
 	}
+	tcon->cifs_sb = cifs_sb;
 
 #ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
 	if (cap_unix(ses))