* [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range
@ 2023-05-05 15:14 Pawel Witek
[not found] ` <CAH2r5mszw6sayQfJqiYwTjPCqKDB7Dk-Hmtr0-Z3fXf=e3t0aQ@mail.gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Pawel Witek @ 2023-05-05 15:14 UTC (permalink / raw)
To: linux-cifs
Change type of pcchunk->Length from u32 to u64 to match
smb2_copychunk_range arguments type. Fixes the problem where performing
server-side copy with CIFS_IOC_COPYCHUNK_FILE ioctl resulted in incomplete
copy of large files while returning -EINVAL.
Signed-off-by: Pawel Witek <pawel.ireneusz.witek@gmail.com>
---
fs/cifs/smb2ops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index a81758225fcd..35c7c35882c9 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1682,7 +1682,7 @@ smb2_copychunk_range(const unsigned int xid,
pcchunk->SourceOffset = cpu_to_le64(src_off);
pcchunk->TargetOffset = cpu_to_le64(dest_off);
pcchunk->Length =
- cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk));
+ cpu_to_le64(min_t(u64, len, tcon->max_bytes_chunk));
/* Request server copy to target from src identified by key */
kfree(retbuf);
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread[parent not found: <CAH2r5mszw6sayQfJqiYwTjPCqKDB7Dk-Hmtr0-Z3fXf=e3t0aQ@mail.gmail.com>]
* Fwd: [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range [not found] ` <CAH2r5mszw6sayQfJqiYwTjPCqKDB7Dk-Hmtr0-Z3fXf=e3t0aQ@mail.gmail.com> @ 2023-05-05 16:47 ` Steve French 2023-05-05 19:31 ` Pawel Witek 1 sibling, 0 replies; 6+ messages in thread From: Steve French @ 2023-05-05 16:47 UTC (permalink / raw) To: CIFS ---------- Forwarded message --------- From: Steve French <smfrench@gmail.com> Date: Fri, May 5, 2023 at 11:47 AM Subject: Re: [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range To: Pawel Witek <pawel.ireneusz.witek@gmail.com> Cc: <linux-cifs@vger.kernel.org> since pcchunk->Length is a 32 bit field doing cpu_to_le64 seems wrong. Perhaps one option is to split this into two lines do the minimum(u64, len, tcon->max_bytes_chunk) on one line and the cpu_to_le32 of the result on the next What is "len" in the example you see failing? On Fri, May 5, 2023 at 10:15 AM Pawel Witek <pawel.ireneusz.witek@gmail.com> wrote: > > Change type of pcchunk->Length from u32 to u64 to match > smb2_copychunk_range arguments type. Fixes the problem where performing > server-side copy with CIFS_IOC_COPYCHUNK_FILE ioctl resulted in incomplete > copy of large files while returning -EINVAL. > > Signed-off-by: Pawel Witek <pawel.ireneusz.witek@gmail.com> > --- > fs/cifs/smb2ops.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index a81758225fcd..35c7c35882c9 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -1682,7 +1682,7 @@ smb2_copychunk_range(const unsigned int xid, > pcchunk->SourceOffset = cpu_to_le64(src_off); > pcchunk->TargetOffset = cpu_to_le64(dest_off); > pcchunk->Length = > - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); > + cpu_to_le64(min_t(u64, len, tcon->max_bytes_chunk)); > > /* Request server copy to target from src identified by key */ > kfree(retbuf); > -- > 2.40.1 > > -- Thanks, Steve -- Thanks, Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range [not found] ` <CAH2r5mszw6sayQfJqiYwTjPCqKDB7Dk-Hmtr0-Z3fXf=e3t0aQ@mail.gmail.com> 2023-05-05 16:47 ` Fwd: " Steve French @ 2023-05-05 19:31 ` Pawel Witek [not found] ` <CAH2r5mvCCY=VT9ZUkjz4c+QGZ0MjR4ggdG56QUa-apZ7eoeo0A@mail.gmail.com> 1 sibling, 1 reply; 6+ messages in thread From: Pawel Witek @ 2023-05-05 19:31 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs I've tried to copy a 5 GB file which resulted in -EINVAL (same for larger files). After wiresharking the protocol I've observed that one packet requests ioctl with 'Transfer Length: 0', to which the server responded with an error. Some investigation showed, that this happens when len=4294967296. On 5/5/23 18:47, Steve French wrote: > since pcchunk->Length is a 32 bit field doing cpu_to_le64 seems wrong. > > Perhaps one option is to split this into two lines do the minimum(u64, len, tcon->max_bytes_chunk) on one line and the cpu_to_le32 of the result on the next > > What is "len" in the example you see failing? > > On Fri, May 5, 2023 at 10:15 AM Pawel Witek <pawel.ireneusz.witek@gmail.com <mailto:pawel.ireneusz.witek@gmail.com>> wrote: > > Change type of pcchunk->Length from u32 to u64 to match > smb2_copychunk_range arguments type. Fixes the problem where performing > server-side copy with CIFS_IOC_COPYCHUNK_FILE ioctl resulted in incomplete > copy of large files while returning -EINVAL. > > Signed-off-by: Pawel Witek <pawel.ireneusz.witek@gmail.com <mailto:pawel.ireneusz.witek@gmail.com>> > --- > fs/cifs/smb2ops.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index a81758225fcd..35c7c35882c9 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -1682,7 +1682,7 @@ smb2_copychunk_range(const unsigned int xid, > pcchunk->SourceOffset = cpu_to_le64(src_off); > pcchunk->TargetOffset = cpu_to_le64(dest_off); > pcchunk->Length = > - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); > + cpu_to_le64(min_t(u64, len, tcon->max_bytes_chunk)); > > /* Request server copy to target from src identified by key */ > kfree(retbuf); > -- > 2.40.1 > > > > > -- > Thanks, > > Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CAH2r5mvCCY=VT9ZUkjz4c+QGZ0MjR4ggdG56QUa-apZ7eoeo0A@mail.gmail.com>]
* Re: [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range [not found] ` <CAH2r5mvCCY=VT9ZUkjz4c+QGZ0MjR4ggdG56QUa-apZ7eoeo0A@mail.gmail.com> @ 2023-05-06 4:49 ` Steve French 2023-05-06 4:51 ` Steve French 0 siblings, 1 reply; 6+ messages in thread From: Steve French @ 2023-05-06 4:49 UTC (permalink / raw) To: Pawel Witek; +Cc: CIFS [-- Attachment #1: Type: text/plain, Size: 2741 bytes --] Wouldn't it be safer (since pcchunk->Length is a u32 to do the following minor change to your patch pcchunk->Length = - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); + cpu_to_le32(min_t(u64, len, tcon->max_bytes_chunk)); Also added Cc: stable and Fixes: tags. See attached. On Fri, May 5, 2023 at 2:48 PM Steve French <smfrench@gmail.com> wrote: > > Good catch > > On Fri, May 5, 2023, 14:31 Pawel Witek <pawel.ireneusz.witek@gmail.com> wrote: >> >> I've tried to copy a 5 GB file which resulted in -EINVAL (same for larger >> files). After wiresharking the protocol I've observed that one packet >> requests ioctl with 'Transfer Length: 0', to which the server responded >> with an error. Some investigation showed, that this happens when >> len=4294967296. >> >> On 5/5/23 18:47, Steve French wrote: >> > since pcchunk->Length is a 32 bit field doing cpu_to_le64 seems wrong. >> > >> > Perhaps one option is to split this into two lines do the minimum(u64, len, tcon->max_bytes_chunk) on one line and the cpu_to_le32 of the result on the next >> > >> > What is "len" in the example you see failing? >> > >> > On Fri, May 5, 2023 at 10:15 AM Pawel Witek <pawel.ireneusz.witek@gmail.com <mailto:pawel.ireneusz.witek@gmail.com>> wrote: >> > >> > Change type of pcchunk->Length from u32 to u64 to match >> > smb2_copychunk_range arguments type. Fixes the problem where performing >> > server-side copy with CIFS_IOC_COPYCHUNK_FILE ioctl resulted in incomplete >> > copy of large files while returning -EINVAL. >> > >> > Signed-off-by: Pawel Witek <pawel.ireneusz.witek@gmail.com <mailto:pawel.ireneusz.witek@gmail.com>> >> > --- >> > fs/cifs/smb2ops.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c >> > index a81758225fcd..35c7c35882c9 100644 >> > --- a/fs/cifs/smb2ops.c >> > +++ b/fs/cifs/smb2ops.c >> > @@ -1682,7 +1682,7 @@ smb2_copychunk_range(const unsigned int xid, >> > pcchunk->SourceOffset = cpu_to_le64(src_off); >> > pcchunk->TargetOffset = cpu_to_le64(dest_off); >> > pcchunk->Length = >> > - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); >> > + cpu_to_le64(min_t(u64, len, tcon->max_bytes_chunk)); >> > >> > /* Request server copy to target from src identified by key */ >> > kfree(retbuf); >> > -- >> > 2.40.1 >> > >> > >> > >> > >> > -- >> > Thanks, >> > >> > Steve -- Thanks, Steve [-- Attachment #2: 0001-cifs-fix-pcchunk-length-type-in-smb2_copychunk_range.patch --] [-- Type: text/x-patch, Size: 1329 bytes --] From 431bbb480d05cdf5d7457c2ae417af72e5cebc82 Mon Sep 17 00:00:00 2001 From: Pawel Witek <pawel.ireneusz.witek@gmail.com> Date: Fri, 5 May 2023 17:14:59 +0200 Subject: [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range Change type of pcchunk->Length from u32 to u64 to match smb2_copychunk_range arguments type. Fixes the problem where performing server-side copy with CIFS_IOC_COPYCHUNK_FILE ioctl resulted in incomplete copy of large files while returning -EINVAL. Fixes: 9bf0c9cd4314 ("CIFS: Fix SMB2/SMB3 Copy offload support (refcopy) for large files") Cc: <stable@vger.kernel.org> Signed-off-by: Pawel Witek <pawel.ireneusz.witek@gmail.com> Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/smb2ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index a81758225fcd..a295e4c2d54e 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -1682,7 +1682,7 @@ smb2_copychunk_range(const unsigned int xid, pcchunk->SourceOffset = cpu_to_le64(src_off); pcchunk->TargetOffset = cpu_to_le64(dest_off); pcchunk->Length = - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); + cpu_to_le32(min_t(u64, len, tcon->max_bytes_chunk)); /* Request server copy to target from src identified by key */ kfree(retbuf); -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range 2023-05-06 4:49 ` Steve French @ 2023-05-06 4:51 ` Steve French 2023-05-06 8:06 ` Pawel Witek 0 siblings, 1 reply; 6+ messages in thread From: Steve French @ 2023-05-06 4:51 UTC (permalink / raw) To: Pawel Witek; +Cc: CIFS I was also able to reproduce it by using xfs_io to execute copy_range on a file of exactly 4GB and verified that the patches (both versions) fix it. On Fri, May 5, 2023 at 11:49 PM Steve French <smfrench@gmail.com> wrote: > > Wouldn't it be safer (since pcchunk->Length is a u32 to do the > following minor change to your patch > > pcchunk->Length = > - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); > + cpu_to_le32(min_t(u64, len, tcon->max_bytes_chunk)); > > Also added Cc: stable and Fixes: tags. See attached. > > On Fri, May 5, 2023 at 2:48 PM Steve French <smfrench@gmail.com> wrote: > > > > Good catch > > > > On Fri, May 5, 2023, 14:31 Pawel Witek <pawel.ireneusz.witek@gmail.com> wrote: > >> > >> I've tried to copy a 5 GB file which resulted in -EINVAL (same for larger > >> files). After wiresharking the protocol I've observed that one packet > >> requests ioctl with 'Transfer Length: 0', to which the server responded > >> with an error. Some investigation showed, that this happens when > >> len=4294967296. > >> > >> On 5/5/23 18:47, Steve French wrote: > >> > since pcchunk->Length is a 32 bit field doing cpu_to_le64 seems wrong. > >> > > >> > Perhaps one option is to split this into two lines do the minimum(u64, len, tcon->max_bytes_chunk) on one line and the cpu_to_le32 of the result on the next > >> > > >> > What is "len" in the example you see failing? > >> > > >> > On Fri, May 5, 2023 at 10:15 AM Pawel Witek <pawel.ireneusz.witek@gmail.com <mailto:pawel.ireneusz.witek@gmail.com>> wrote: > >> > > >> > Change type of pcchunk->Length from u32 to u64 to match > >> > smb2_copychunk_range arguments type. Fixes the problem where performing > >> > server-side copy with CIFS_IOC_COPYCHUNK_FILE ioctl resulted in incomplete > >> > copy of large files while returning -EINVAL. > >> > > >> > Signed-off-by: Pawel Witek <pawel.ireneusz.witek@gmail.com <mailto:pawel.ireneusz.witek@gmail.com>> > >> > --- > >> > fs/cifs/smb2ops.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > >> > index a81758225fcd..35c7c35882c9 100644 > >> > --- a/fs/cifs/smb2ops.c > >> > +++ b/fs/cifs/smb2ops.c > >> > @@ -1682,7 +1682,7 @@ smb2_copychunk_range(const unsigned int xid, > >> > pcchunk->SourceOffset = cpu_to_le64(src_off); > >> > pcchunk->TargetOffset = cpu_to_le64(dest_off); > >> > pcchunk->Length = > >> > - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); > >> > + cpu_to_le64(min_t(u64, len, tcon->max_bytes_chunk)); > >> > > >> > /* Request server copy to target from src identified by key */ > >> > kfree(retbuf); > >> > -- > >> > 2.40.1 > >> > > >> > > >> > > >> > > >> > -- > >> > Thanks, > >> > > >> > Steve > > > > -- > Thanks, > > Steve -- Thanks, Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range 2023-05-06 4:51 ` Steve French @ 2023-05-06 8:06 ` Pawel Witek 0 siblings, 0 replies; 6+ messages in thread From: Pawel Witek @ 2023-05-06 8:06 UTC (permalink / raw) To: Steve French; +Cc: CIFS You're right, that does look better and safer :) On 5/6/23 06:51, Steve French wrote: > I was also able to reproduce it by using xfs_io to execute copy_range > on a file of exactly 4GB and verified that the patches (both versions) > fix it. > > On Fri, May 5, 2023 at 11:49 PM Steve French <smfrench@gmail.com> wrote: >> >> Wouldn't it be safer (since pcchunk->Length is a u32 to do the >> following minor change to your patch >> >> pcchunk->Length = >> - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); >> + cpu_to_le32(min_t(u64, len, tcon->max_bytes_chunk)); >> >> Also added Cc: stable and Fixes: tags. See attached. >> >> On Fri, May 5, 2023 at 2:48 PM Steve French <smfrench@gmail.com> wrote: >>> >>> Good catch >>> >>> On Fri, May 5, 2023, 14:31 Pawel Witek <pawel.ireneusz.witek@gmail.com> wrote: >>>> >>>> I've tried to copy a 5 GB file which resulted in -EINVAL (same for larger >>>> files). After wiresharking the protocol I've observed that one packet >>>> requests ioctl with 'Transfer Length: 0', to which the server responded >>>> with an error. Some investigation showed, that this happens when >>>> len=4294967296. >>>> >>>> On 5/5/23 18:47, Steve French wrote: >>>>> since pcchunk->Length is a 32 bit field doing cpu_to_le64 seems wrong. >>>>> >>>>> Perhaps one option is to split this into two lines do the minimum(u64, len, tcon->max_bytes_chunk) on one line and the cpu_to_le32 of the result on the next >>>>> >>>>> What is "len" in the example you see failing? >>>>> >>>>> On Fri, May 5, 2023 at 10:15 AM Pawel Witek <pawel.ireneusz.witek@gmail.com <mailto:pawel.ireneusz.witek@gmail.com>> wrote: >>>>> >>>>> Change type of pcchunk->Length from u32 to u64 to match >>>>> smb2_copychunk_range arguments type. Fixes the problem where performing >>>>> server-side copy with CIFS_IOC_COPYCHUNK_FILE ioctl resulted in incomplete >>>>> copy of large files while returning -EINVAL. >>>>> >>>>> Signed-off-by: Pawel Witek <pawel.ireneusz.witek@gmail.com <mailto:pawel.ireneusz.witek@gmail.com>> >>>>> --- >>>>> fs/cifs/smb2ops.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c >>>>> index a81758225fcd..35c7c35882c9 100644 >>>>> --- a/fs/cifs/smb2ops.c >>>>> +++ b/fs/cifs/smb2ops.c >>>>> @@ -1682,7 +1682,7 @@ smb2_copychunk_range(const unsigned int xid, >>>>> pcchunk->SourceOffset = cpu_to_le64(src_off); >>>>> pcchunk->TargetOffset = cpu_to_le64(dest_off); >>>>> pcchunk->Length = >>>>> - cpu_to_le32(min_t(u32, len, tcon->max_bytes_chunk)); >>>>> + cpu_to_le64(min_t(u64, len, tcon->max_bytes_chunk)); >>>>> >>>>> /* Request server copy to target from src identified by key */ >>>>> kfree(retbuf); >>>>> -- >>>>> 2.40.1 >>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Thanks, >>>>> >>>>> Steve >> >> >> >> -- >> Thanks, >> >> Steve > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-05-06 8:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-05 15:14 [PATCH] cifs: fix pcchunk length type in smb2_copychunk_range Pawel Witek
[not found] ` <CAH2r5mszw6sayQfJqiYwTjPCqKDB7Dk-Hmtr0-Z3fXf=e3t0aQ@mail.gmail.com>
2023-05-05 16:47 ` Fwd: " Steve French
2023-05-05 19:31 ` Pawel Witek
[not found] ` <CAH2r5mvCCY=VT9ZUkjz4c+QGZ0MjR4ggdG56QUa-apZ7eoeo0A@mail.gmail.com>
2023-05-06 4:49 ` Steve French
2023-05-06 4:51 ` Steve French
2023-05-06 8:06 ` Pawel Witek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.