XPost: linux.debian.devel

Hi Dima,

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.

Best regards
Manuel

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

XPost: linux.debian.devel

On Thu, Mar 07, 2024 at 03:25:01PM +0100, Manuel Traut wrote:

Hi Dima,

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.

Is this the expected behavior indeed?

G'luck,
Peter

--
Peter Pentchev roam@ringlet.net roam@debian.org pp@storpool.com
PGP key: http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint 2EE7 A7A5 17FC 124C F115 C354 651E EFB0 2527 DF13

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEELuenpRf8EkzxFcNUZR7vsCUn3xMFAmXp1v8ACgkQZR7vsCUn 3xNLAQ//VZA5lktc7gwkQCUanxKYqITzxCrSuTfOqWFuRpw+JLu0OxfL3Kd0krjI mdiKY7KYqKNbLl5AkQiNYdVE4fn0wvaDLKSre6lyMSbDdxQiLtGzVPNGNnZUYdTk mDPCVcbLcVKTdXkKSZ/TX707bBm29EjKwqnMZPujkOYi8Fk6YhhKdG9lUq3J+zf0 tt+ApN5v4kGrKxQn8419lNNkzx6gfyJpasY40dEsjjDIEPg3A3XgDRMUhonzoW4S /vNYRmhrjr0rXxfiH/nkYNGgjs0JjP1tZBj9e9iybIbDn3YKqQB2gGh5xn4njRor K7vRrgc/elFzUz2Dbmfs0rcC0aSuG64oHtEkbAkagPPgpQNAmPEZo/B5V0kcpY0v bPk44aFA+4OUMMwMXVO6EuXY7NvkytXopKr6kqMgFrZxvAFjryjHqKOXWTmBqyVH VvhpR452XrvfpxG6V3jAubBgcVtsBWn3c4UsZTZQmgH+k1dAE3Wj9x4tgscJN8xg jC7SG4DYuQ/JNEhA/+So/OWbisCzmVAVTHCuWeXSl+1yKtnkFJb9VXiwsHka5vbO oSoYuY05bakj1or5VvBKZSSfkTwygSUx2FW0LrtJuJllfmskAnM/WuSQDKcq68n6 sk/mNZ248eZa1ca9ScSwPsRkgAtiEoXdWNfiExRJ4AnZjglrVUw=
=dPiO
-

XPost: linux.debian.devel

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:

Hi Dima,

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?

--
Peter Pentchev roam@ringlet.net roam@debian.org pp@storpool.com
PGP key: http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint 2EE7 A7A5 17FC 124C F115 C354 651E EFB0 2527 DF13

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEELuenpRf8EkzxFcNUZR7vsCUn3xMFAmXp2IgACgkQZR7vsCUn 3xPKDBAAiYCWbk69o9knc5vE18KeDlRAwtjjj55Xw8tPFWWbVcj+NlkwSIZwFYUo WvEdML1Yo6MA25tLwI+jn2K+Mry6dDoS1vbeWpTtZl0Yy0l7/P1bRLjOxs7YviXc pEiDymIuVnNYH6Eb2UPHXGC4UK4Q3VM6dg0BRjEtSxZ1HXxIA4jR2xN5GJKx0KYR T7ic4Jg1qys71srRrVqevoALWMXhZuX1Kdr+mgrVeAmAS/xiynjGmtd/y3jqWskd HKTQG8csDRXcPVbI+sDWbsbaOiLzLvLYEqrX2KBnQcoMguJapxhp9NhpSYN0SqYc 4ypzJlKAAt+W17Wg9uDGab5hHxJRQ2/9OLLL1YWPDrhibHq971mvcd+tiSi6VxcR qg+sbcc101zkW5lVhQDqD8IeYR6Mv1LjMkU4QfTrmuWet/5/IzoBjAy5s3hCmfOk po27TOKUevN01A0OinFq6Er76S42VDaCwX5ga2QEqWUVDPJmQRasvlp/JwYhQDDR MRyWUPwsqn6PsmnocpqS7Ug7JIgBTw1X+MB8yJFNrfl1XPlX56EBdDyXrZDK1PgK GgoqR0g7t91vzygbsHuWiDzGnQ6BBLoqwdOhWVg3t33m6A43MaBS2Qd+LayojxK4 4gnmOnBj+l+8gvuw6LE4Mv2J3nqUrqX1Gop9BunL0KEd9Pfbkok=
=buMb
-

XPost: linux.debian.devel

On Wed, Mar 13, 2024 at 10:40:51AM +0000, Traut Manuel LCPF-CH wrote:

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

Yeah, after I sent the e-mails I realized that I forgot to mention that
using uint32_t would be one of the best ways to fix that.

Thanks for the fast reaction!

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.

...and this part I have no opinion about. I don't have any realistic
idea about how people actually use libuio (or how they will use it),
and I do not think that I will use it soon, so this should be left for
others to decide.

Thanks again for paying attention to the comments!

G'luck,
Peter

--
Peter Pentchev roam@ringlet.net roam@debian.org pp@storpool.com
PGP key: http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint 2EE7 A7A5 17FC 124C F115 C354 651E EFB0 2527 DF13

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEELuenpRf8EkzxFcNUZR7vsCUn3xMFAmXxqnsACgkQZR7vsCUn 3xMvxA/+LBIqZohLN4gu6V39aTgacnOhEzUF5E9F7kLklmK7GYuhlDsjnlIxwDM7 WeWxrxmseWt3VWIcbZ4qf325Dh9Vhkcq7uvOkq+2lF1lv7ZU2kJI3KcJlYmuvsrB BR/Oz36w5MXVC+WqI71/MXth6L+NGvgqsyIGPCdChH0aJW3fXLlQPDk6Dbwswnu0 4eXunCR4xnd2af6zBfUSI7DcwuCY2HD6oBf0IHh09m8o0/IdupvM5wUzYRvlaSDM vnjFvJeIJytOyrduQrlfpdjr09CD51KH9gXqNceQzTu2rUVgMjXtRxrSacZPfTb0 WfBURFCDmoueQnrDsNks/prJCT2Lh5o5Wu6cEyYxo7k7DXKl+iawi8SQ7nBEYgOE oSXoqXTNbovskEsyCwb4VffojIL4rckISGqpn892ps2jhqroarmBTJ709P5j1Ayb +SL29aK1e3OlypoANgChbNE/xXUHENhPhsF6dftqtrmWu0RGModVq/rUWA5uksk/ Povjn1rCDgTc2l4CqxbzMeoSy/kGTHdBePh8asrxhkZ4Wy1kSvgHTzgjZgfgHaop 6w+2pvCoGVKfYGW+wiM7ylYAB/gq1YEdmubICKD/8NbCnr02T52/iw55Vwrnhd7d EFJLGb3ZLcyDI1hIlOwUb0qYQG2uiHFv6s/ba7583ww5WohAqfw=
=9Fz5
-

Hello


Here [1] is a first attempt to package libuio

[1]: https://salsa.debian.org/burakg/libuio


Regards

Burak

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)

Hi Bastian

Thank you very much! I'll get in contact with Manuel and apply your
suggestions

BR
Burak

--- SoupGate-Win32 v1.05
* Origin: fsxNet Usenet Gateway (21:1/5)