updated patch attached - rebased to work around Paulo's change and
removed the update to smb2inode.c compounding due to dependency on a
later patch, and tentatively merged to cifs-2.6.git for-next pending
testing
On Sat, Feb 10, 2024 at 10:42 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote:
>
> On Sat, Feb 10, 2024 at 11:52 AM Steve French <smfrench@xxxxxxxxx> wrote:
> >
> > It needed a small rebase to work around one of Paulo's patches which
> > it depended on (which is for 6.9-rc) but was wondering if you want
> > this patch in before 6.9 because its change to smb2_compound_op below
> > has a dependency on another patch - can that change be safely removed?
> >
> It will be good to have this in 6.8-rc4, if it will be accepted. It's
> the same small change made in several places.
> It avoids a possible handle leak on the server.
>
> > replay_again:
> > /* reinitialize for possible replay */
> > flags = 0;
> > oplock = SMB2_OPLOCK_LEVEL_NONE;
> > num_rqst = 0;
> > server = cifs_pick_channel(ses);
> > oparms->replay = !!(retries);
> >
> > On Fri, Feb 9, 2024 at 5:25 AM <nspmangalore@xxxxxxxxx> wrote:
> > >
> > > From: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
> > >
> > > File open requests made to the server contain a
> > > CreateGuid, which is used by the server to identify
> > > the open request. If the same request needs to be
> > > replayed, it needs to be sent with the same CreateGuid
> > > in the durable handle v2 context.
> > >
> > > Without doing so, we could end up leaking handles on
> > > the server when:
> > > 1. multichannel is used AND
> > > 2. connection goes down, but not for all channels
> > >
> > > This is because the replayed open request would have a
> > > new CreateGuid and the server will treat this as a new
> > > request and open a new handle.
> > >
> > > This change fixes this by reusing the existing create_guid
> > > stored in the cached fid struct.
> > >
> > > REF: MS-SMB2 4.9 Replay Create Request on an Alternate Channel
> > >
> > > Fixes: 4f1fffa23769 ("cifs: commands that are retried should have replay flag set")
> > > Signed-off-by: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
> > > ---
> > > fs/smb/client/cached_dir.c | 1 +
> > > fs/smb/client/cifsglob.h | 1 +
> > > fs/smb/client/smb2inode.c | 1 +
> > > fs/smb/client/smb2ops.c | 4 ++++
> > > fs/smb/client/smb2pdu.c | 10 ++++++++--
> > > 5 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
> > > index 1daeb5714faa..3de5047a7ff9 100644
> > > --- a/fs/smb/client/cached_dir.c
> > > +++ b/fs/smb/client/cached_dir.c
> > > @@ -242,6 +242,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
> > > .desired_access = FILE_READ_DATA | FILE_READ_ATTRIBUTES,
> > > .disposition = FILE_OPEN,
> > > .fid = pfid,
> > > + .replay = !!(retries),
> > > };
> > >
> > > rc = SMB2_open_init(tcon, server,
> > > diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> > > index cac10f8e17e4..efab4769de4e 100644
> > > --- a/fs/smb/client/cifsglob.h
> > > +++ b/fs/smb/client/cifsglob.h
> > > @@ -1373,6 +1373,7 @@ struct cifs_open_parms {
> > > struct cifs_fid *fid;
> > > umode_t mode;
> > > bool reconnect:1;
> > > + bool replay:1; /* indicates that this open is for a replay */
> > > struct kvec *ea_cctx;
> > > };
> > >
> > > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> > > index 63485078a6df..22bd01e7bf6e 100644
> > > --- a/fs/smb/client/smb2inode.c
> > > +++ b/fs/smb/client/smb2inode.c
> > > @@ -203,6 +203,7 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
> > > oplock = SMB2_OPLOCK_LEVEL_NONE;
> > > num_rqst = 0;
> > > server = cifs_pick_channel(ses);
> > > + oparms->replay = !!(retries);
> > >
> > > vars = kzalloc(sizeof(*vars), GFP_ATOMIC);
> > > if (vars == NULL)
> > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > > index 8d674aef8dd9..c0da1935b0bd 100644
> > > --- a/fs/smb/client/smb2ops.c
> > > +++ b/fs/smb/client/smb2ops.c
> > > @@ -1205,6 +1205,7 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
> > > .disposition = FILE_OPEN,
> > > .create_options = cifs_create_options(cifs_sb, 0),
> > > .fid = &fid,
> > > + .replay = !!(retries),
> > > };
> > >
> > > rc = SMB2_open_init(tcon, server,
> > > @@ -1570,6 +1571,7 @@ smb2_ioctl_query_info(const unsigned int xid,
> > > .disposition = FILE_OPEN,
> > > .create_options = cifs_create_options(cifs_sb, create_options),
> > > .fid = &fid,
> > > + .replay = !!(retries),
> > > };
> > >
> > > if (qi.flags & PASSTHRU_FSCTL) {
> > > @@ -2296,6 +2298,7 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon,
> > > .disposition = FILE_OPEN,
> > > .create_options = cifs_create_options(cifs_sb, 0),
> > > .fid = fid,
> > > + .replay = !!(retries),
> > > };
> > >
> > > rc = SMB2_open_init(tcon, server,
> > > @@ -2682,6 +2685,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon,
> > > .disposition = FILE_OPEN,
> > > .create_options = cifs_create_options(cifs_sb, 0),
> > > .fid = &fid,
> > > + .replay = !!(retries),
> > > };
> > >
> > > rc = SMB2_open_init(tcon, server,
> > > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > > index 2ecc5f210329..1ce9be3a7ca7 100644
> > > --- a/fs/smb/client/smb2pdu.c
> > > +++ b/fs/smb/client/smb2pdu.c
> > > @@ -2406,8 +2406,13 @@ create_durable_v2_buf(struct cifs_open_parms *oparms)
> > > */
> > > buf->dcontext.Timeout = cpu_to_le32(oparms->tcon->handle_timeout);
> > > buf->dcontext.Flags = cpu_to_le32(SMB2_DHANDLE_FLAG_PERSISTENT);
> > > - generate_random_uuid(buf->dcontext.CreateGuid);
> > > - memcpy(pfid->create_guid, buf->dcontext.CreateGuid, 16);
> > > +
> > > + /* for replay, we should not overwrite the existing create guid */
> > > + if (!oparms->replay) {
> > > + generate_random_uuid(buf->dcontext.CreateGuid);
> > > + memcpy(pfid->create_guid, buf->dcontext.CreateGuid, 16);
> > > + } else
> > > + memcpy(buf->dcontext.CreateGuid, pfid->create_guid, 16);
> > >
> > > /* SMB2_CREATE_DURABLE_HANDLE_REQUEST is "DH2Q" */
> > > buf->Name[0] = 'D';
> > > @@ -3156,6 +3161,7 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
> > > /* reinitialize for possible replay */
> > > flags = 0;
> > > server = cifs_pick_channel(ses);
> > > + oparms->replay = !!(retries);
> > >
> > > cifs_dbg(FYI, "create/open\n");
> > > if (!ses || !server)
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Regards,
> Shyam
--
Thanks,
Steve
From 8b39cfa4b970674a770f6f4c37e2d00c8359daeb Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Date: Fri, 9 Feb 2024 11:25:42 +0000
Subject: [PATCH] cifs: update the same create_guid on replay
File open requests made to the server contain a
CreateGuid, which is used by the server to identify
the open request. If the same request needs to be
replayed, it needs to be sent with the same CreateGuid
in the durable handle v2 context.
Without doing so, we could end up leaking handles on
the server when:
1. multichannel is used AND
2. connection goes down, but not for all channels
This is because the replayed open request would have a
new CreateGuid and the server will treat this as a new
request and open a new handle.
This change fixes this by reusing the existing create_guid
stored in the cached fid struct.
REF: MS-SMB2 4.9 Replay Create Request on an Alternate Channel
Fixes: 4f1fffa23769 ("cifs: commands that are retried should have replay flag set")
Signed-off-by: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
fs/smb/client/cached_dir.c | 1 +
fs/smb/client/cifsglob.h | 1 +
fs/smb/client/smb2inode.c | 1 +
fs/smb/client/smb2ops.c | 4 ++++
fs/smb/client/smb2pdu.c | 10 ++++++++--
5 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index 1daeb5714faa..3de5047a7ff9 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -242,6 +242,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
.desired_access = FILE_READ_DATA | FILE_READ_ATTRIBUTES,
.disposition = FILE_OPEN,
.fid = pfid,
+ .replay = !!(retries),
};
rc = SMB2_open_init(tcon, server,
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index c86a72c9d9ec..53c75cfb33ab 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1378,6 +1378,7 @@ struct cifs_open_parms {
struct cifs_fid *fid;
umode_t mode;
bool reconnect:1;
+ bool replay:1; /* indicates that this open is for a replay */
};
struct cifs_fid {
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 755f1c66b573..6b3c384ead0d 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -1204,6 +1204,7 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
.disposition = FILE_OPEN,
.create_options = cifs_create_options(cifs_sb, 0),
.fid = &fid,
+ .replay = !!(retries),
};
rc = SMB2_open_init(tcon, server,
@@ -1569,6 +1570,7 @@ smb2_ioctl_query_info(const unsigned int xid,
.disposition = FILE_OPEN,
.create_options = cifs_create_options(cifs_sb, create_options),
.fid = &fid,
+ .replay = !!(retries),
};
if (qi.flags & PASSTHRU_FSCTL) {
@@ -2295,6 +2297,7 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon,
.disposition = FILE_OPEN,
.create_options = cifs_create_options(cifs_sb, 0),
.fid = fid,
+ .replay = !!(retries),
};
rc = SMB2_open_init(tcon, server,
@@ -2681,6 +2684,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon,
.disposition = FILE_OPEN,
.create_options = cifs_create_options(cifs_sb, 0),
.fid = &fid,
+ .replay = !!(retries),
};
rc = SMB2_open_init(tcon, server,
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 4085ce27fd38..608ee05491e2 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -2404,8 +2404,13 @@ create_durable_v2_buf(struct cifs_open_parms *oparms)
*/
buf->dcontext.Timeout = cpu_to_le32(oparms->tcon->handle_timeout);
buf->dcontext.Flags = cpu_to_le32(SMB2_DHANDLE_FLAG_PERSISTENT);
- generate_random_uuid(buf->dcontext.CreateGuid);
- memcpy(pfid->create_guid, buf->dcontext.CreateGuid, 16);
+
+ /* for replay, we should not overwrite the existing create guid */
+ if (!oparms->replay) {
+ generate_random_uuid(buf->dcontext.CreateGuid);
+ memcpy(pfid->create_guid, buf->dcontext.CreateGuid, 16);
+ } else
+ memcpy(buf->dcontext.CreateGuid, pfid->create_guid, 16);
/* SMB2_CREATE_DURABLE_HANDLE_REQUEST is "DH2Q" */
buf->Name[0] = 'D';
@@ -3142,6 +3147,7 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
/* reinitialize for possible replay */
flags = 0;
server = cifs_pick_channel(ses);
+ oparms->replay = !!(retries);
cifs_dbg(FYI, "create/open\n");
if (!ses || !server)
--
2.40.1
