| 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 |
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 >
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.
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.
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"
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
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>
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 --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))