diff mbox series

cifs: fix pcchunk length type in smb2_copychunk_range

Message ID 4b402539-1b98-bfe6-fa60-d73d13794077@gmail.com
State New
Headers show
Series cifs: fix pcchunk length type in smb2_copychunk_range | expand

Commit Message

Pawel Witek May 5, 2023, 3:14 p.m. UTC 
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(-)

Comments

Steve French May 5, 2023, 4:47 p.m. UTC | #1 
---------- 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
>
>
Pawel Witek May 5, 2023, 7:31 p.m. UTC | #2 
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
Steve French May 6, 2023, 4:49 a.m. UTC | #3 
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
Steve French May 6, 2023, 4:51 a.m. UTC | #4 
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
Pawel Witek May 6, 2023, 8:06 a.m. UTC | #5 
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
> 
> 
>
diff mbox series

Patch

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);