Re: [PATCH] cifs: fix bad fids sent over wire

From: Tom Talpey <tom@talpey.com>
To: Namjae Jeon <linkinjeon@kernel.org>, Steve French <smfrench@gmail.com>
Cc: Paulo Alcantara <pc@cjr.nz>, CIFS <linux-cifs@vger.kernel.org>
Subject: Re: [PATCH] cifs: fix bad fids sent over wire
Date: Sat, 19 Mar 2022 21:45:23 -0400	[thread overview]
Message-ID: <1ab1e73c-566c-7f1f-6cbf-5502b99611e8@talpey.com> (raw)
In-Reply-To: <CAKYAXd_dUnBLmAutFVWpOczRH-3U21vR51nHsNTjAVfUk1KEig@mail.gmail.com>

On 3/19/2022 9:22 PM, Namjae Jeon wrote:
> 2022-03-20 3:34 GMT+09:00, Steve French <smfrench@gmail.com>:
>> They probably should be always 'u64' everywhere (not le64) and change
>> the code back in fs/smbfs_common/smb2pdu.h (was this due to ksmbd
>> using the file and converting these fields in fs/smbfs_common) rather
>> than the ones you changed
> I don't understand why only FileId fields should be declared as u64 not le64.

Because they're opaque to the client.

> It means that FileID doesn't need byteswap in client?

Correct. They are tested only for equality, or are placed in a packet
verbatim and sent back to the server.

> samba seems to
> stores them in little-endian byte order.

Again, unnecessary and dangerous, IMO.

Tom.

> 
> #define SBVAL(p, ofs, v) PUSH_LE_U64(p, ofs, v)
> 
>          SBVAL(outbody.data, 0x40,
>                out_file_id_persistent);          /* file id (persistent) */
>          SBVAL(outbody.data, 0x48,
>                out_file_id_volatile);            /* file id (volatile) */
> 
> Am I missing something ?
>>
>>
>> On Sat, Mar 19, 2022 at 11:01 AM Paulo Alcantara <pc@cjr.nz> wrote:
>>>
>>> Yes, I agreed. Why not simply store them as le64 and avoid the
>>> byteswapping altogether?
>>>
>>> On 19 March 2022 09:06:55 GMT-03:00, Tom Talpey <tom@talpey.com> wrote:
>>>>
>>>>
>>>> On 3/19/2022 12:23 AM, Steve French wrote:
>>>>>
>>>>> Any comments on Paulo's patch? It fixes an endian conversion problem
>>>>> that can affect file ids used on big endian archs.  I will add cc:
>>>>> stable
>>>>>
>>>>> https://git.cjr.nz/linux.git/commit/?h=cifs-bad-fid-fixes&id=a857bb6b15646a7946fafb281878ddf498107dc3
>>>>
>>>>
>>>> It seems a bad idea to be storing opaque fields in le64 integers, then
>>>> byteswapping them when they go back on the wire. Wouldn't it be better
>>>> to make them u8[8] arrays and just use memcpy/memcmp?
>>>>
>>>> Tom.
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>>
> 