Re: Bug#1063380: ITP: libuio -- Linux Kernel UserspaceIO helper library
> On Thu, Mar 07, 2024 at 05:02:29PM +0200, Peter Pentchev wrote:
>> On Thu, Mar 07, 2024 at 03:25:01PM +0100, Manuel Traut wrote:
>>> On 7 Feb 2024, at 18:27, Dima Kogan <dkogan@debian.org> wrote:
>>>
>>> Hi. Thanks for your contribution. I looked at the upstream code a tiny
>>> bit, and it looks like it might have portability bug, at least on
>>> big-endian architectures. For instance:
>>>
>>> https://github.com/missinglinkelectronics/libuio/blob/6ef3d8d096a641686bfdd112035aa04aa16fe81a/irq.c#L78
>>>
>>> This assumes that sizeof(long)==4. Maybe this is benign, but it would be
>>> nice to fix. Are you upstream or do you know upstream? Can yall fix
>>> these?
>>>
>> The kernel expects a 4 byte write here, since unsigned long is defined as at least 32 bit this shall work on all architectures.
>>
>> If your concern is about endianess this is not in the current scope of libuio and needs to be addressed by a higher layer.
>>
>> I am very familiar with library and in close contact with upstream.
>>
> In the case of uio_disable_irq() the bug is nicely hidden by the fact
> that tmp is guaranteed to be all-zeroes. However, consider the previous
> function in the file, uio_enable_irq(). If sizeof(unsigned long) == 8,
> then the "tmp" variable's value of 1 will be encoded in memory as
> 8 bytes containing the values 0, 0, 0, 0, 0, 0, 0, and 1 respectively.
> When uio_enable_irq() calls write(..., &tmp, 4), it will only send
> the first four bytes to the kernel - and they are 0, 0, 0, and... 0.
> So it turns out that uio_disable_irq() and uio_enable_irq() do
> exactly the same - send a 32-bit zero value to the kernel.
>
> ...of course, this will only happen on a big-endian system, as
> Dima Kogan was concerned about. On a little-endian system, this
> will work by chance.
>
> Is this the expected behavior indeed?
No it is not :) Thanks for the detailed explanation.
Looks this fix sane to you?
https://github.com/manut/libuio/commit/1edcf262fbfaf0b7f8519875ed5b357964dd1e55
I am not sure if users also expect an automatic conversion for the read/write functions:
https://github.com/manut/libuio/commit/d0319169359bffc73374f1011e942702e9bafb1e
It will be somehow inconsistent since a user could also access the device memory by pointer:
https://github.com/manut/libuio/blob/add-ci/mem.c#L93
and than needs to take care on the endianess by its own anyway.
Reply to: