| Message ID | 20220321002007.26903-1-pc@cjr.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: fix bad fids sent over wire | expand |
On 3/20/2022 8:20 PM, Paulo Alcantara wrote: > The client used to partially convert the fids to le64, while storing > or sending them by using host endianness. This broke the client on > big-endian machines. Instead of converting them to le64, store them > verbatim and then avoid byteswapping when sending them over wire. > > Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > --- > fs/cifs/smb2misc.c | 4 ++-- > fs/cifs/smb2ops.c | 8 +++---- > fs/cifs/smb2pdu.c | 53 ++++++++++++++++++++-------------------------- > 3 files changed, 29 insertions(+), 36 deletions(-) > > diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > index b25623e3fe3d..3b7c636be377 100644 > --- a/fs/cifs/smb2misc.c > +++ b/fs/cifs/smb2misc.c > @@ -832,8 +832,8 @@ smb2_handle_cancelled_mid(struct mid_q_entry *mid, struct TCP_Server_Info *serve > rc = __smb2_handle_cancelled_cmd(tcon, > le16_to_cpu(hdr->Command), > le64_to_cpu(hdr->MessageId), > - le64_to_cpu(rsp->PersistentFileId), > - le64_to_cpu(rsp->VolatileFileId)); > + rsp->PersistentFileId, > + rsp->VolatileFileId); This conflicts with the statement "store them verbatim". Because the rsp->{Persistent,Volatile}FileId fields are u64 (integer) types, they are not being stored verbatim, they are being manipulated by the CPU load/store instructions. Storing them into a u8[8] array is more to the point. If course, if the rsp structure is purely private to the code, then the structure element type is similarly private. But a debugger, or a future structure reference, may once again get it wrong Are you rejecting the idea of using a byte array? > if (rc) > cifs_put_tcon(tcon); > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 0ecd6e1832a1..c122530e5043 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -900,8 +900,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > atomic_inc(&tcon->num_remote_opens); > > o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > - oparms.fid->persistent_fid = le64_to_cpu(o_rsp->PersistentFileId); > - oparms.fid->volatile_fid = le64_to_cpu(o_rsp->VolatileFileId); > + oparms.fid->persistent_fid = o_rsp->PersistentFileId; > + oparms.fid->volatile_fid = o_rsp->VolatileFileId; > #ifdef CONFIG_CIFS_DEBUG2 > oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId); > #endif /* CIFS_DEBUG2 */ > @@ -2410,8 +2410,8 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon, > cifs_dbg(FYI, "query_dir_first: open failed rc=%d\n", rc); > goto qdf_free; > } > - fid->persistent_fid = le64_to_cpu(op_rsp->PersistentFileId); > - fid->volatile_fid = le64_to_cpu(op_rsp->VolatileFileId); > + fid->persistent_fid = op_rsp->PersistentFileId; > + fid->volatile_fid = op_rsp->VolatileFileId; > > /* Anything else than ENODATA means a genuine error */ > if (rc && rc != -ENODATA) { > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 7e7909b1ae11..178af70331f8 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -2734,13 +2734,10 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode, > goto err_free_req; > } > > - trace_smb3_posix_mkdir_done(xid, le64_to_cpu(rsp->PersistentFileId), > - tcon->tid, > - ses->Suid, CREATE_NOT_FILE, > - FILE_WRITE_ATTRIBUTES); > + trace_smb3_posix_mkdir_done(xid, rsp->PersistentFileId, tcon->tid, ses->Suid, > + CREATE_NOT_FILE, FILE_WRITE_ATTRIBUTES); > > - SMB2_close(xid, tcon, le64_to_cpu(rsp->PersistentFileId), > - le64_to_cpu(rsp->VolatileFileId)); > + SMB2_close(xid, tcon, rsp->PersistentFileId, rsp->VolatileFileId); > > /* Eventually save off posix specific response info and timestaps */ > > @@ -3009,14 +3006,12 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path, > } else if (rsp == NULL) /* unlikely to happen, but safer to check */ > goto creat_exit; > else > - trace_smb3_open_done(xid, le64_to_cpu(rsp->PersistentFileId), > - tcon->tid, > - ses->Suid, oparms->create_options, > - oparms->desired_access); > + trace_smb3_open_done(xid, rsp->PersistentFileId, tcon->tid, ses->Suid, > + oparms->create_options, oparms->desired_access); > > atomic_inc(&tcon->num_remote_opens); > - oparms->fid->persistent_fid = le64_to_cpu(rsp->PersistentFileId); > - oparms->fid->volatile_fid = le64_to_cpu(rsp->VolatileFileId); > + oparms->fid->persistent_fid = rsp->PersistentFileId; > + oparms->fid->volatile_fid = rsp->VolatileFileId; > oparms->fid->access = oparms->desired_access; > #ifdef CONFIG_CIFS_DEBUG2 > oparms->fid->mid = le64_to_cpu(rsp->hdr.MessageId); > @@ -3313,8 +3308,8 @@ SMB2_close_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server, > if (rc) > return rc; > > - req->PersistentFileId = cpu_to_le64(persistent_fid); > - req->VolatileFileId = cpu_to_le64(volatile_fid); > + req->PersistentFileId = persistent_fid; > + req->VolatileFileId = volatile_fid; > if (query_attrs) > req->Flags = SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB; > else > @@ -3677,8 +3672,8 @@ SMB2_notify_init(const unsigned int xid, struct smb_rqst *rqst, > if (rc) > return rc; > > - req->PersistentFileId = cpu_to_le64(persistent_fid); > - req->VolatileFileId = cpu_to_le64(volatile_fid); > + req->PersistentFileId = persistent_fid; > + req->VolatileFileId = volatile_fid; > /* See note 354 of MS-SMB2, 64K max */ > req->OutputBufferLength = > cpu_to_le32(SMB2_MAX_BUFFER_SIZE - MAX_SMB2_HDR_SIZE); > @@ -3951,8 +3946,8 @@ SMB2_flush_init(const unsigned int xid, struct smb_rqst *rqst, > if (rc) > return rc; > > - req->PersistentFileId = cpu_to_le64(persistent_fid); > - req->VolatileFileId = cpu_to_le64(volatile_fid); > + req->PersistentFileId = persistent_fid; > + req->VolatileFileId = volatile_fid; > > iov[0].iov_base = (char *)req; > iov[0].iov_len = total_len; > @@ -4033,8 +4028,8 @@ smb2_new_read_req(void **buf, unsigned int *total_len, > shdr = &req->hdr; > shdr->Id.SyncId.ProcessId = cpu_to_le32(io_parms->pid); > > - req->PersistentFileId = cpu_to_le64(io_parms->persistent_fid); > - req->VolatileFileId = cpu_to_le64(io_parms->volatile_fid); > + req->PersistentFileId = io_parms->persistent_fid; > + req->VolatileFileId = io_parms->volatile_fid; > req->ReadChannelInfoOffset = 0; /* reserved */ > req->ReadChannelInfoLength = 0; /* reserved */ > req->Channel = 0; /* reserved */ > @@ -4094,8 +4089,8 @@ smb2_new_read_req(void **buf, unsigned int *total_len, > */ > shdr->SessionId = cpu_to_le64(0xFFFFFFFFFFFFFFFF); > shdr->Id.SyncId.TreeId = cpu_to_le32(0xFFFFFFFF); > - req->PersistentFileId = cpu_to_le64(0xFFFFFFFFFFFFFFFF); > - req->VolatileFileId = cpu_to_le64(0xFFFFFFFFFFFFFFFF); > + req->PersistentFileId = (u64)-1; > + req->VolatileFileId = (u64)-1; > } > } > if (remaining_bytes > io_parms->length) > @@ -4312,10 +4307,8 @@ SMB2_read(const unsigned int xid, struct cifs_io_parms *io_parms, > io_parms->offset, io_parms->length, > rc); > } else > - trace_smb3_read_done(xid, > - le64_to_cpu(req->PersistentFileId), > - io_parms->tcon->tid, ses->Suid, > - io_parms->offset, 0); > + trace_smb3_read_done(xid, req->PersistentFileId, io_parms->tcon->tid, > + ses->Suid, io_parms->offset, 0); > free_rsp_buf(resp_buftype, rsp_iov.iov_base); > cifs_small_buf_release(req); > return rc == -ENODATA ? 0 : rc; > @@ -4463,8 +4456,8 @@ smb2_async_writev(struct cifs_writedata *wdata, > shdr = (struct smb2_hdr *)req; > shdr->Id.SyncId.ProcessId = cpu_to_le32(wdata->cfile->pid); > > - req->PersistentFileId = cpu_to_le64(wdata->cfile->fid.persistent_fid); > - req->VolatileFileId = cpu_to_le64(wdata->cfile->fid.volatile_fid); > + req->PersistentFileId = wdata->cfile->fid.persistent_fid; > + req->VolatileFileId = wdata->cfile->fid.volatile_fid; > req->WriteChannelInfoOffset = 0; > req->WriteChannelInfoLength = 0; > req->Channel = 0; > @@ -4615,8 +4608,8 @@ SMB2_write(const unsigned int xid, struct cifs_io_parms *io_parms, > > req->hdr.Id.SyncId.ProcessId = cpu_to_le32(io_parms->pid); > > - req->PersistentFileId = cpu_to_le64(io_parms->persistent_fid); > - req->VolatileFileId = cpu_to_le64(io_parms->volatile_fid); > + req->PersistentFileId = io_parms->persistent_fid; > + req->VolatileFileId = io_parms->volatile_fid; > req->WriteChannelInfoOffset = 0; > req->WriteChannelInfoLength = 0; > req->Channel = 0;
Tom Talpey <tom@talpey.com> writes: > On 3/20/2022 8:20 PM, Paulo Alcantara wrote: >> The client used to partially convert the fids to le64, while storing >> or sending them by using host endianness. This broke the client on >> big-endian machines. Instead of converting them to le64, store them >> verbatim and then avoid byteswapping when sending them over wire. >> >> Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> >> --- >> fs/cifs/smb2misc.c | 4 ++-- >> fs/cifs/smb2ops.c | 8 +++---- >> fs/cifs/smb2pdu.c | 53 ++++++++++++++++++++-------------------------- >> 3 files changed, 29 insertions(+), 36 deletions(-) >> >> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c >> index b25623e3fe3d..3b7c636be377 100644 >> --- a/fs/cifs/smb2misc.c >> +++ b/fs/cifs/smb2misc.c >> @@ -832,8 +832,8 @@ smb2_handle_cancelled_mid(struct mid_q_entry *mid, struct TCP_Server_Info *serve >> rc = __smb2_handle_cancelled_cmd(tcon, >> le16_to_cpu(hdr->Command), >> le64_to_cpu(hdr->MessageId), >> - le64_to_cpu(rsp->PersistentFileId), >> - le64_to_cpu(rsp->VolatileFileId)); >> + rsp->PersistentFileId, >> + rsp->VolatileFileId); > > This conflicts with the statement "store them verbatim". Because the > rsp->{Persistent,Volatile}FileId fields are u64 (integer) types, > they are not being stored verbatim, they are being manipulated > by the CPU load/store instructions. Storing them into a u8[8] > array is more to the point. Yes, makes sense. > If course, if the rsp structure is purely private to the code, then > the structure element type is similarly private. But a debugger, or > a future structure reference, may once again get it wrong > > Are you rejecting the idea of using a byte array? No. That would work, too. I was just trying to avoid changing a lot of places and eventually making it harder to backport. I'll go with the byte array then.
On 3/21/2022 8:30 AM, Paulo Alcantara wrote: > Tom Talpey <tom@talpey.com> writes: > >> On 3/20/2022 8:20 PM, Paulo Alcantara wrote: >>> The client used to partially convert the fids to le64, while storing >>> or sending them by using host endianness. This broke the client on >>> big-endian machines. Instead of converting them to le64, store them >>> verbatim and then avoid byteswapping when sending them over wire. >>> >>> Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> >>> --- >>> fs/cifs/smb2misc.c | 4 ++-- >>> fs/cifs/smb2ops.c | 8 +++---- >>> fs/cifs/smb2pdu.c | 53 ++++++++++++++++++++-------------------------- >>> 3 files changed, 29 insertions(+), 36 deletions(-) >>> >>> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c >>> index b25623e3fe3d..3b7c636be377 100644 >>> --- a/fs/cifs/smb2misc.c >>> +++ b/fs/cifs/smb2misc.c >>> @@ -832,8 +832,8 @@ smb2_handle_cancelled_mid(struct mid_q_entry *mid, struct TCP_Server_Info *serve >>> rc = __smb2_handle_cancelled_cmd(tcon, >>> le16_to_cpu(hdr->Command), >>> le64_to_cpu(hdr->MessageId), >>> - le64_to_cpu(rsp->PersistentFileId), >>> - le64_to_cpu(rsp->VolatileFileId)); >>> + rsp->PersistentFileId, >>> + rsp->VolatileFileId); >> >> This conflicts with the statement "store them verbatim". Because the >> rsp->{Persistent,Volatile}FileId fields are u64 (integer) types, >> they are not being stored verbatim, they are being manipulated >> by the CPU load/store instructions. Storing them into a u8[8] >> array is more to the point. > > Yes, makes sense. > >> If course, if the rsp structure is purely private to the code, then >> the structure element type is similarly private. But a debugger, or >> a future structure reference, may once again get it wrong >> >> Are you rejecting the idea of using a byte array? > > No. That would work, too. I was just trying to avoid changing a lot of > places and eventually making it harder to backport. > > I'll go with the byte array then. If you want to reduce a bit of code change, you could let the compiler generate the loads and stores via memcpy, with (perhaps) a struct { u8[8] } instead of the bare array. Tom.
Tom Talpey <tom@talpey.com> writes: > If you want to reduce a bit of code change, you could let the > compiler generate the loads and stores via memcpy, with (perhaps) > a struct { u8[8] } instead of the bare array. Thanks for the suggestions. It turned out to be more changes than I expected. In this case, I'm gonna fix some remaining sparse warnings from my last patch and fix the commit message as you suggested. Of course, we can refactor this out later and start with something like: struct smb2_fid { __u8 Persistent[8]; __u8 Volatile[8]; } __packed; and then replace u64 integers with the above.
On 3/21/2022 11:01 AM, Steve French wrote: > Wouldn't u64 for these with no conversion (the original code was > consistent before half of use of fields converted to le64) be faster, > easier? I guess that's what Paulo is going to do. It's certainly faster and easier, but I predict it won't close the door on future code issues. Someone is going to try to byteswap, or treat it as an integer somehow. I'm advocating for correctness. But fast and easy are your call. Tom. > On Mon, Mar 21, 2022, 07:55 Tom Talpey <tom@talpey.com > <mailto:tom@talpey.com>> wrote: > > On 3/21/2022 8:30 AM, Paulo Alcantara wrote: > > Tom Talpey <tom@talpey.com <mailto:tom@talpey.com>> writes: > > > >> On 3/20/2022 8:20 PM, Paulo Alcantara wrote: > >>> The client used to partially convert the fids to le64, while > storing > >>> or sending them by using host endianness. This broke the client on > >>> big-endian machines. Instead of converting them to le64, store > them > >>> verbatim and then avoid byteswapping when sending them over wire. > >>> > >>> Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz > <mailto:pc@cjr.nz>> > >>> --- > >>> fs/cifs/smb2misc.c | 4 ++-- > >>> fs/cifs/smb2ops.c | 8 +++---- > >>> fs/cifs/smb2pdu.c | 53 > ++++++++++++++++++++-------------------------- > >>> 3 files changed, 29 insertions(+), 36 deletions(-) > >>> > >>> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > >>> index b25623e3fe3d..3b7c636be377 100644 > >>> --- a/fs/cifs/smb2misc.c > >>> +++ b/fs/cifs/smb2misc.c > >>> @@ -832,8 +832,8 @@ smb2_handle_cancelled_mid(struct > mid_q_entry *mid, struct TCP_Server_Info *serve > >>> rc = __smb2_handle_cancelled_cmd(tcon, > >>> le16_to_cpu(hdr->Command), > >>> le64_to_cpu(hdr->MessageId), > >>> - > le64_to_cpu(rsp->PersistentFileId), > >>> - > le64_to_cpu(rsp->VolatileFileId)); > >>> + rsp->PersistentFileId, > >>> + rsp->VolatileFileId); > >> > >> This conflicts with the statement "store them verbatim". Because the > >> rsp->{Persistent,Volatile}FileId fields are u64 (integer) types, > >> they are not being stored verbatim, they are being manipulated > >> by the CPU load/store instructions. Storing them into a u8[8] > >> array is more to the point. > > > > Yes, makes sense. > > > >> If course, if the rsp structure is purely private to the code, then > >> the structure element type is similarly private. But a debugger, or > >> a future structure reference, may once again get it wrong > >> > >> Are you rejecting the idea of using a byte array? > > > > No. That would work, too. I was just trying to avoid changing a > lot of > > places and eventually making it harder to backport. > > > > I'll go with the byte array then. > > If you want to reduce a bit of code change, you could let the > compiler generate the loads and stores via memcpy, with (perhaps) > a struct { u8[8] } instead of the bare array. > > Tom. >
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index b25623e3fe3d..3b7c636be377 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -832,8 +832,8 @@ smb2_handle_cancelled_mid(struct mid_q_entry *mid, struct TCP_Server_Info *serve rc = __smb2_handle_cancelled_cmd(tcon, le16_to_cpu(hdr->Command), le64_to_cpu(hdr->MessageId), - le64_to_cpu(rsp->PersistentFileId), - le64_to_cpu(rsp->VolatileFileId)); + rsp->PersistentFileId, + rsp->VolatileFileId); if (rc) cifs_put_tcon(tcon); diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 0ecd6e1832a1..c122530e5043 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -900,8 +900,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, atomic_inc(&tcon->num_remote_opens); o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; - oparms.fid->persistent_fid = le64_to_cpu(o_rsp->PersistentFileId); - oparms.fid->volatile_fid = le64_to_cpu(o_rsp->VolatileFileId); + oparms.fid->persistent_fid = o_rsp->PersistentFileId; + oparms.fid->volatile_fid = o_rsp->VolatileFileId; #ifdef CONFIG_CIFS_DEBUG2 oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId); #endif /* CIFS_DEBUG2 */ @@ -2410,8 +2410,8 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon, cifs_dbg(FYI, "query_dir_first: open failed rc=%d\n", rc); goto qdf_free; } - fid->persistent_fid = le64_to_cpu(op_rsp->PersistentFileId); - fid->volatile_fid = le64_to_cpu(op_rsp->VolatileFileId); + fid->persistent_fid = op_rsp->PersistentFileId; + fid->volatile_fid = op_rsp->VolatileFileId; /* Anything else than ENODATA means a genuine error */ if (rc && rc != -ENODATA) { diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 7e7909b1ae11..178af70331f8 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2734,13 +2734,10 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode, goto err_free_req; } - trace_smb3_posix_mkdir_done(xid, le64_to_cpu(rsp->PersistentFileId), - tcon->tid, - ses->Suid, CREATE_NOT_FILE, - FILE_WRITE_ATTRIBUTES); + trace_smb3_posix_mkdir_done(xid, rsp->PersistentFileId, tcon->tid, ses->Suid, + CREATE_NOT_FILE, FILE_WRITE_ATTRIBUTES); - SMB2_close(xid, tcon, le64_to_cpu(rsp->PersistentFileId), - le64_to_cpu(rsp->VolatileFileId)); + SMB2_close(xid, tcon, rsp->PersistentFileId, rsp->VolatileFileId); /* Eventually save off posix specific response info and timestaps */ @@ -3009,14 +3006,12 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path, } else if (rsp == NULL) /* unlikely to happen, but safer to check */ goto creat_exit; else - trace_smb3_open_done(xid, le64_to_cpu(rsp->PersistentFileId), - tcon->tid, - ses->Suid, oparms->create_options, - oparms->desired_access); + trace_smb3_open_done(xid, rsp->PersistentFileId, tcon->tid, ses->Suid, + oparms->create_options, oparms->desired_access); atomic_inc(&tcon->num_remote_opens); - oparms->fid->persistent_fid = le64_to_cpu(rsp->PersistentFileId); - oparms->fid->volatile_fid = le64_to_cpu(rsp->VolatileFileId); + oparms->fid->persistent_fid = rsp->PersistentFileId; + oparms->fid->volatile_fid = rsp->VolatileFileId; oparms->fid->access = oparms->desired_access; #ifdef CONFIG_CIFS_DEBUG2 oparms->fid->mid = le64_to_cpu(rsp->hdr.MessageId); @@ -3313,8 +3308,8 @@ SMB2_close_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server, if (rc) return rc; - req->PersistentFileId = cpu_to_le64(persistent_fid); - req->VolatileFileId = cpu_to_le64(volatile_fid); + req->PersistentFileId = persistent_fid; + req->VolatileFileId = volatile_fid; if (query_attrs) req->Flags = SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB; else @@ -3677,8 +3672,8 @@ SMB2_notify_init(const unsigned int xid, struct smb_rqst *rqst, if (rc) return rc; - req->PersistentFileId = cpu_to_le64(persistent_fid); - req->VolatileFileId = cpu_to_le64(volatile_fid); + req->PersistentFileId = persistent_fid; + req->VolatileFileId = volatile_fid; /* See note 354 of MS-SMB2, 64K max */ req->OutputBufferLength = cpu_to_le32(SMB2_MAX_BUFFER_SIZE - MAX_SMB2_HDR_SIZE); @@ -3951,8 +3946,8 @@ SMB2_flush_init(const unsigned int xid, struct smb_rqst *rqst, if (rc) return rc; - req->PersistentFileId = cpu_to_le64(persistent_fid); - req->VolatileFileId = cpu_to_le64(volatile_fid); + req->PersistentFileId = persistent_fid; + req->VolatileFileId = volatile_fid; iov[0].iov_base = (char *)req; iov[0].iov_len = total_len; @@ -4033,8 +4028,8 @@ smb2_new_read_req(void **buf, unsigned int *total_len, shdr = &req->hdr; shdr->Id.SyncId.ProcessId = cpu_to_le32(io_parms->pid); - req->PersistentFileId = cpu_to_le64(io_parms->persistent_fid); - req->VolatileFileId = cpu_to_le64(io_parms->volatile_fid); + req->PersistentFileId = io_parms->persistent_fid; + req->VolatileFileId = io_parms->volatile_fid; req->ReadChannelInfoOffset = 0; /* reserved */ req->ReadChannelInfoLength = 0; /* reserved */ req->Channel = 0; /* reserved */ @@ -4094,8 +4089,8 @@ smb2_new_read_req(void **buf, unsigned int *total_len, */ shdr->SessionId = cpu_to_le64(0xFFFFFFFFFFFFFFFF); shdr->Id.SyncId.TreeId = cpu_to_le32(0xFFFFFFFF); - req->PersistentFileId = cpu_to_le64(0xFFFFFFFFFFFFFFFF); - req->VolatileFileId = cpu_to_le64(0xFFFFFFFFFFFFFFFF); + req->PersistentFileId = (u64)-1; + req->VolatileFileId = (u64)-1; } } if (remaining_bytes > io_parms->length) @@ -4312,10 +4307,8 @@ SMB2_read(const unsigned int xid, struct cifs_io_parms *io_parms, io_parms->offset, io_parms->length, rc); } else - trace_smb3_read_done(xid, - le64_to_cpu(req->PersistentFileId), - io_parms->tcon->tid, ses->Suid, - io_parms->offset, 0); + trace_smb3_read_done(xid, req->PersistentFileId, io_parms->tcon->tid, + ses->Suid, io_parms->offset, 0); free_rsp_buf(resp_buftype, rsp_iov.iov_base); cifs_small_buf_release(req); return rc == -ENODATA ? 0 : rc; @@ -4463,8 +4456,8 @@ smb2_async_writev(struct cifs_writedata *wdata, shdr = (struct smb2_hdr *)req; shdr->Id.SyncId.ProcessId = cpu_to_le32(wdata->cfile->pid); - req->PersistentFileId = cpu_to_le64(wdata->cfile->fid.persistent_fid); - req->VolatileFileId = cpu_to_le64(wdata->cfile->fid.volatile_fid); + req->PersistentFileId = wdata->cfile->fid.persistent_fid; + req->VolatileFileId = wdata->cfile->fid.volatile_fid; req->WriteChannelInfoOffset = 0; req->WriteChannelInfoLength = 0; req->Channel = 0; @@ -4615,8 +4608,8 @@ SMB2_write(const unsigned int xid, struct cifs_io_parms *io_parms, req->hdr.Id.SyncId.ProcessId = cpu_to_le32(io_parms->pid); - req->PersistentFileId = cpu_to_le64(io_parms->persistent_fid); - req->VolatileFileId = cpu_to_le64(io_parms->volatile_fid); + req->PersistentFileId = io_parms->persistent_fid; + req->VolatileFileId = io_parms->volatile_fid; req->WriteChannelInfoOffset = 0; req->WriteChannelInfoLength = 0; req->Channel = 0;
The client used to partially convert the fids to le64, while storing or sending them by using host endianness. This broke the client on big-endian machines. Instead of converting them to le64, store them verbatim and then avoid byteswapping when sending them over wire. Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> --- fs/cifs/smb2misc.c | 4 ++-- fs/cifs/smb2ops.c | 8 +++---- fs/cifs/smb2pdu.c | 53 ++++++++++++++++++++-------------------------- 3 files changed, 29 insertions(+), 36 deletions(-)