* [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
* 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
* 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.