[PATCH v2 0/4] Missing endianness conversions in user mode

[PATCH v2 0/4] Missing endianness conversions in user mode

[Mathis Marion]

From: Mathis Marion <mathis.marion@silabs.com>

For a bit of context, I was trying to test a network border router [1]
daemon using the MIPS architecture (see [2]). I didn't have access to
real MIPS hardware so I figured I would emulate it using QEMU user mode.
I ran into a couple of problems all related to endianness conversion for
syscalls between host and target as MIPS is big endian and my x86 host
is little.

[1]: https://github.com/SiliconLabs/wisun-br-linux
[2]: https://github.com/SiliconLabs/wisun-br-linux/issues/5

v2:
- remove context from target_to_host_for_each_nlattr()

Mathis Marion (4):
  linux-user: fix timerfd read endianness conversion
  linux-user: fix sockaddr_in6 endianness
  linux-user: add target to host netlink conversions
  linux-user: handle netlink flag NLA_F_NESTED

 linux-user/fd-trans.c | 74 ++++++++++++++++++++++++++++++++++++++++---
 linux-user/fd-trans.h |  1 +
 linux-user/syscall.c  | 14 ++++++--
 3 files changed, 83 insertions(+), 6 deletions(-)

-- 
2.39.1

[PATCH v2 1/4] linux-user: fix timerfd read endianness conversion

[Mathis Marion]

From: Mathis Marion <mathis.marion@silabs.com>

When reading the expiration count from a timerfd, the endianness of the
64bit value read is the one of the host, just as for eventfds.

Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
---
 linux-user/fd-trans.c | 10 +++++++---
 linux-user/fd-trans.h |  1 +
 linux-user/syscall.c  |  8 ++++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
index 7b25468d02..146aaaafaa 100644
--- a/linux-user/fd-trans.c
+++ b/linux-user/fd-trans.c
@@ -1622,7 +1622,7 @@ TargetFdTrans target_signalfd_trans = {
     .host_to_target_data = host_to_target_data_signalfd,
 };
 
-static abi_long swap_data_eventfd(void *buf, size_t len)
+static abi_long swap_data_u64(void *buf, size_t len)
 {
     uint64_t *counter = buf;
     int i;
@@ -1640,8 +1640,12 @@ static abi_long swap_data_eventfd(void *buf, size_t len)
 }
 
 TargetFdTrans target_eventfd_trans = {
-    .host_to_target_data = swap_data_eventfd,
-    .target_to_host_data = swap_data_eventfd,
+    .host_to_target_data = swap_data_u64,
+    .target_to_host_data = swap_data_u64,
+};
+
+TargetFdTrans target_timerfd_trans = {
+    .host_to_target_data = swap_data_u64,
 };
 
 #if defined(CONFIG_INOTIFY) && (defined(TARGET_NR_inotify_init) || \
diff --git a/linux-user/fd-trans.h b/linux-user/fd-trans.h
index 1b9fa2041c..910faaf237 100644
--- a/linux-user/fd-trans.h
+++ b/linux-user/fd-trans.h
@@ -130,6 +130,7 @@ extern TargetFdTrans target_netlink_route_trans;
 extern TargetFdTrans target_netlink_audit_trans;
 extern TargetFdTrans target_signalfd_trans;
 extern TargetFdTrans target_eventfd_trans;
+extern TargetFdTrans target_timerfd_trans;
 #if (defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)) || \
     (defined(CONFIG_INOTIFY1) && defined(TARGET_NR_inotify_init1) && \
      defined(__NR_inotify_init1))
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1e868e9b0e..58549de125 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -13117,8 +13117,12 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
 
 #if defined(TARGET_NR_timerfd_create) && defined(CONFIG_TIMERFD)
     case TARGET_NR_timerfd_create:
-        return get_errno(timerfd_create(arg1,
-                          target_to_host_bitmask(arg2, fcntl_flags_tbl)));
+        ret = get_errno(timerfd_create(arg1,
+                        target_to_host_bitmask(arg2, fcntl_flags_tbl)));
+        if (ret >= 0) {
+            fd_trans_register(ret, &target_timerfd_trans);
+        }
+        return ret;
 #endif
 
 #if defined(TARGET_NR_timerfd_gettime) && defined(CONFIG_TIMERFD)
-- 
2.39.1

[PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness

[Mathis Marion]

From: Mathis Marion <mathis.marion@silabs.com>

Fields sin6_flowinfo and sin6_scope_id use the host byte order, so there
is a conversion to be made when host and target endianness differ.

Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
---
 linux-user/syscall.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 58549de125..1a6856abec 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1713,6 +1713,12 @@ static inline abi_long target_to_host_sockaddr(int fd, struct sockaddr *addr,
 	lladdr = (struct target_sockaddr_ll *)addr;
 	lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
 	lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
+    } else if (sa_family == AF_INET6) {
+        struct sockaddr_in6 *in6addr;
+
+        in6addr = (struct sockaddr_in6 *)addr;
+        in6addr->sin6_flowinfo = tswap32(in6addr->sin6_flowinfo);
+        in6addr->sin6_scope_id = tswap32(in6addr->sin6_scope_id);
     }
     unlock_user(target_saddr, target_addr, 0);
 
-- 
2.39.1

[PATCH v2 3/4] linux-user: add target to host netlink conversions

[Mathis Marion]

From: Mathis Marion <mathis.marion@silabs.com>

Added conversions for:
- IFLA_MTU
- IFLA_TXQLEN
- IFLA_AF_SPEC AF_INET6 IFLA_INET6_ADDR_GEN_MODE
These relate to the libnl functions rtnl_link_set_mtu,
rtnl_link_set_txqlen, and rtnl_link_inet6_set_addr_gen_mode.

Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
---
 linux-user/fd-trans.c | 62 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
index 146aaaafaa..4852a75d9d 100644
--- a/linux-user/fd-trans.c
+++ b/linux-user/fd-trans.c
@@ -1284,6 +1284,49 @@ static inline abi_long host_to_target_nlmsg_route(struct nlmsghdr *nlh,
     return host_to_target_for_each_nlmsg(nlh, len, host_to_target_data_route);
 }
 
+static abi_long target_to_host_for_each_nlattr(struct nlattr *nlattr,
+                                               size_t len,
+                                               abi_long (*target_to_host_nlattr)
+                                                        (struct nlattr *))
+{
+    unsigned short aligned_nla_len;
+    abi_long ret;
+
+    while (len > sizeof(struct nlattr)) {
+        if (tswap16(nlattr->nla_len) < sizeof(struct rtattr) ||
+            tswap16(nlattr->nla_len) > len) {
+            break;
+        }
+        nlattr->nla_len = tswap16(nlattr->nla_len);
+        nlattr->nla_type = tswap16(nlattr->nla_type);
+        ret = target_to_host_nlattr(nlattr);
+        if (ret < 0) {
+            return ret;
+        }
+
+        aligned_nla_len = NLA_ALIGN(nlattr->nla_len);
+        if (aligned_nla_len >= len) {
+            break;
+        }
+        len -= aligned_nla_len;
+        nlattr = (struct nlattr *)(((char *)nlattr) + aligned_nla_len);
+    }
+    return 0;
+}
+
+static abi_long target_to_host_data_inet6_nlattr(struct nlattr *nlattr)
+{
+    switch (nlattr->nla_type) {
+    /* uint8_t */
+    case QEMU_IFLA_INET6_ADDR_GEN_MODE:
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "Unknown target AF_INET6 type: %d\n",
+                      nlattr->nla_type);
+    }
+    return 0;
+}
+
 static abi_long target_to_host_for_each_rtattr(struct rtattr *rtattr,
                                                size_t len,
                                                abi_long (*target_to_host_rtattr)
@@ -1314,16 +1357,35 @@ static abi_long target_to_host_for_each_rtattr(struct rtattr *rtattr,
     return 0;
 }
 
+static abi_long target_to_host_data_spec_nlattr(struct nlattr *nlattr)
+{
+    switch (nlattr->nla_type) {
+    case AF_INET6:
+        return target_to_host_for_each_nlattr(NLA_DATA(nlattr), nlattr->nla_len,
+                                              target_to_host_data_inet6_nlattr);
+    default:
+        qemu_log_mask(LOG_UNIMP, "Unknown target AF_SPEC type: %d\n",
+                      nlattr->nla_type);
+        break;
+    }
+    return 0;
+}
+
 static abi_long target_to_host_data_link_rtattr(struct rtattr *rtattr)
 {
     uint32_t *u32;
 
     switch (rtattr->rta_type) {
     /* uint32_t */
+    case QEMU_IFLA_MTU:
+    case QEMU_IFLA_TXQLEN:
     case QEMU_IFLA_EXT_MASK:
         u32 = RTA_DATA(rtattr);
         *u32 = tswap32(*u32);
         break;
+    case QEMU_IFLA_AF_SPEC:
+        return target_to_host_for_each_nlattr(RTA_DATA(rtattr), rtattr->rta_len,
+                                              target_to_host_data_spec_nlattr);
     default:
         qemu_log_mask(LOG_UNIMP, "Unknown target QEMU_IFLA type: %d\n",
                       rtattr->rta_type);
-- 
2.39.1

[PATCH v2 4/4] linux-user: handle netlink flag NLA_F_NESTED

[Mathis Marion]

From: Mathis Marion <mathis.marion@silabs.com>

Newer kernel versions require this flag to be present contrary to older
ones. Depending on the libnl version it is added or not.

Typically when using rtnl_link_inet6_set_addr_gen_mode, the netlink
packet generated may contain the following attribute:

with libnl 3.4

  {nla_len=16, nla_type=IFLA_AF_SPEC},
  [
    {nla_len=12, nla_type=AF_INET6},
    [{nla_len=5, nla_type=IFLA_INET6_ADDR_GEN_MODE}, IN6_ADDR_GEN_MODE_NONE]
  ]

with libnl 3.7

  {nla_len=16, nla_type=NLA_F_NESTED|IFLA_AF_SPEC},
  [
    {nla_len=12, nla_type=NLA_F_NESTED|AF_INET6},
    [{nla_len=5, nla_type=IFLA_INET6_ADDR_GEN_MODE}, IN6_ADDR_GEN_MODE_NONE]]
  ]

Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
---
 linux-user/fd-trans.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
index 4852a75d9d..1fb8ef9d4e 100644
--- a/linux-user/fd-trans.c
+++ b/linux-user/fd-trans.c
@@ -1359,7 +1359,7 @@ static abi_long target_to_host_for_each_rtattr(struct rtattr *rtattr,
 
 static abi_long target_to_host_data_spec_nlattr(struct nlattr *nlattr)
 {
-    switch (nlattr->nla_type) {
+    switch (nlattr->nla_type & ~NLA_F_NESTED) {
     case AF_INET6:
         return target_to_host_for_each_nlattr(NLA_DATA(nlattr), nlattr->nla_len,
                                               target_to_host_data_inet6_nlattr);
@@ -1375,7 +1375,7 @@ static abi_long target_to_host_data_link_rtattr(struct rtattr *rtattr)
 {
     uint32_t *u32;
 
-    switch (rtattr->rta_type) {
+    switch (rtattr->rta_type & ~NLA_F_NESTED) {
     /* uint32_t */
     case QEMU_IFLA_MTU:
     case QEMU_IFLA_TXQLEN:
-- 
2.39.1

Re: [PATCH v2 3/4] linux-user: add target to host netlink conversions

[Philippe Mathieu-Daudé]

On 20/2/23 09:58, Mathis Marion wrote:
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> Added conversions for:
> - IFLA_MTU
> - IFLA_TXQLEN
> - IFLA_AF_SPEC AF_INET6 IFLA_INET6_ADDR_GEN_MODE
> These relate to the libnl functions rtnl_link_set_mtu,
> rtnl_link_set_txqlen, and rtnl_link_inet6_set_addr_gen_mode.
> 
> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
> ---
>   linux-user/fd-trans.c | 62 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness

[Philippe Mathieu-Daudé]

On 20/2/23 09:58, Mathis Marion wrote:
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> Fields sin6_flowinfo and sin6_scope_id use the host byte order, so there
> is a conversion to be made when host and target endianness differ.
> 
> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
> ---
>   linux-user/syscall.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 58549de125..1a6856abec 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1713,6 +1713,12 @@ static inline abi_long target_to_host_sockaddr(int fd, struct sockaddr *addr,
>   	lladdr = (struct target_sockaddr_ll *)addr;
>   	lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
>   	lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
> +    } else if (sa_family == AF_INET6) {
> +        struct sockaddr_in6 *in6addr;
> +
> +        in6addr = (struct sockaddr_in6 *)addr;
> +        in6addr->sin6_flowinfo = tswap32(in6addr->sin6_flowinfo);
> +        in6addr->sin6_scope_id = tswap32(in6addr->sin6_scope_id);
>       }
>       unlock_user(target_saddr, target_addr, 0);
>   

Same content as v1, right?

If you don't change patch content, please include the reviewer tags
so we don't have to review your patches again.

So similarly to
https://lore.kernel.org/qemu-devel/6be6bf58-cf92-7068-008e-83f5543a1f01@linaro.org/
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness

[Mathis MARION]

On 20/02/2023 10:06, Philippe Mathieu-Daudé wrote:
> CAUTION: This email originated from outside of the organization. Do not 
> click links or open attachments unless you recognize the sender and know 
> the content is safe.
> 
> 
> On 20/2/23 09:58, Mathis Marion wrote:
>> From: Mathis Marion <mathis.marion@silabs.com>
>>
>> Fields sin6_flowinfo and sin6_scope_id use the host byte order, so there
>> is a conversion to be made when host and target endianness differ.
>>
>> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
>> ---
>>   linux-user/syscall.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 58549de125..1a6856abec 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -1713,6 +1713,12 @@ static inline abi_long 
>> target_to_host_sockaddr(int fd, struct sockaddr *addr,
>>       lladdr = (struct target_sockaddr_ll *)addr;
>>       lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
>>       lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
>> +    } else if (sa_family == AF_INET6) {
>> +        struct sockaddr_in6 *in6addr;
>> +
>> +        in6addr = (struct sockaddr_in6 *)addr;
>> +        in6addr->sin6_flowinfo = tswap32(in6addr->sin6_flowinfo);
>> +        in6addr->sin6_scope_id = tswap32(in6addr->sin6_scope_id);
>>       }
>>       unlock_user(target_saddr, target_addr, 0);
>>
> 
> Same content as v1, right?
> 
> If you don't change patch content, please include the reviewer tags
> so we don't have to review your patches again.
> 
> So similarly to
> https://urldefense.com/v3/__https://lore.kernel.org/qemu-devel/6be6bf58-cf92-7068-008e-83f5543a1f01@linaro.org/__;!!N30Cs7Jr!X8OE0Z6gfU2FYtWrk0_Dhk_gUPlhqRPtJ60B7HxeicEaFDDFCLRsmoqhnC3MXGOw7ZfEkgLQhDwsyQv76w$
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Yes, I am still new to this. Thank you for you consideration, I will
remember it for next time.

Re: [PATCH v2 4/4] linux-user: handle netlink flag NLA_F_NESTED

[Mathis MARION]

On 20/02/2023 09:58, Mathis Marion wrote:
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> Newer kernel versions require this flag to be present contrary to older
> ones. Depending on the libnl version it is added or not.
> 
> Typically when using rtnl_link_inet6_set_addr_gen_mode, the netlink
> packet generated may contain the following attribute:
> 
> with libnl 3.4
> 
>    {nla_len=16, nla_type=IFLA_AF_SPEC},
>    [
>      {nla_len=12, nla_type=AF_INET6},
>      [{nla_len=5, nla_type=IFLA_INET6_ADDR_GEN_MODE}, IN6_ADDR_GEN_MODE_NONE]
>    ]
> 
> with libnl 3.7
> 
>    {nla_len=16, nla_type=NLA_F_NESTED|IFLA_AF_SPEC},
>    [
>      {nla_len=12, nla_type=NLA_F_NESTED|AF_INET6},
>      [{nla_len=5, nla_type=IFLA_INET6_ADDR_GEN_MODE}, IN6_ADDR_GEN_MODE_NONE]]
>    ]
> > Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
> ---
>   linux-user/fd-trans.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
> index 4852a75d9d..1fb8ef9d4e 100644
> --- a/linux-user/fd-trans.c
> +++ b/linux-user/fd-trans.c
> @@ -1359,7 +1359,7 @@ static abi_long target_to_host_for_each_rtattr(struct rtattr *rtattr,
>   
>   static abi_long target_to_host_data_spec_nlattr(struct nlattr *nlattr)
>   {
> -    switch (nlattr->nla_type) {
> +    switch (nlattr->nla_type & ~NLA_F_NESTED) {

After looking a bit more into it, linux/netlink.h defines:

/*
  * nla_type (16 bits)
  * +---+---+-------------------------------+
  * | N | O | Attribute Type                |
  * +---+---+-------------------------------+
  * N := Carries nested attributes
  * O := Payload stored in network byte order
  *
  * Note: The N and O flag are mutually exclusive.
  */
#define NLA_F_NESTED		(1 << 15)
#define NLA_F_NET_BYTEORDER	(1 << 14)
#define NLA_TYPE_MASK		~(NLA_F_NESTED | NLA_F_NET_BYTEORDER)

So maybe I should use NLA_TYPE_MASK.

However since NLA_F_NET_BYTEORDER is not supported it may make more
sense to return an error.

>       case AF_INET6:
>           return target_to_host_for_each_nlattr(NLA_DATA(nlattr), nlattr->nla_len,
>                                                 target_to_host_data_inet6_nlattr);
> @@ -1375,7 +1375,7 @@ static abi_long target_to_host_data_link_rtattr(struct rtattr *rtattr)
>   {
>       uint32_t *u32;
>   
> -    switch (rtattr->rta_type) {
> +    switch (rtattr->rta_type & ~NLA_F_NESTED) {
>       /* uint32_t */
>       case QEMU_IFLA_MTU:
>       case QEMU_IFLA_TXQLEN:

Re: [PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness

[Philippe Mathieu-Daudé]

On 20/2/23 10:09, Mathis MARION wrote:
> On 20/02/2023 10:06, Philippe Mathieu-Daudé wrote: >> On 20/2/23 09:58, Mathis Marion wrote:
>>> From: Mathis Marion <mathis.marion@silabs.com>
>>>
>>> Fields sin6_flowinfo and sin6_scope_id use the host byte order, so there
>>> is a conversion to be made when host and target endianness differ.
>>>
>>> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
>>> ---
>>>   linux-user/syscall.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 58549de125..1a6856abec 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -1713,6 +1713,12 @@ static inline abi_long 
>>> target_to_host_sockaddr(int fd, struct sockaddr *addr,
>>>       lladdr = (struct target_sockaddr_ll *)addr;
>>>       lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
>>>       lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
>>> +    } else if (sa_family == AF_INET6) {
>>> +        struct sockaddr_in6 *in6addr;
>>> +
>>> +        in6addr = (struct sockaddr_in6 *)addr;
>>> +        in6addr->sin6_flowinfo = tswap32(in6addr->sin6_flowinfo);
>>> +        in6addr->sin6_scope_id = tswap32(in6addr->sin6_scope_id);
>>>       }
>>>       unlock_user(target_saddr, target_addr, 0);
>>>
>>
>> Same content as v1, right?
>>
>> If you don't change patch content, please include the reviewer tags
>> so we don't have to review your patches again.
>>
>> So similarly to
>> https://urldefense.com/v3/__https://lore.kernel.org/qemu-devel/6be6bf58-cf92-7068-008e-83f5543a1f01@linaro.org/__;!!N30Cs7Jr!X8OE0Z6gfU2FYtWrk0_Dhk_gUPlhqRPtJ60B7HxeicEaFDDFCLRsmoqhnC3MXGOw7ZfEkgLQhDwsyQv76w$
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
> 
> Yes, I am still new to this. Thank you for you consideration, I will
> remember it for next time.

Sorry I didn't notice you are new because your patch series already
have a very high quality :)

You can see guidelines here:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#proper-use-of-reviewed-by-tags-can-aid-review

In particular:

   When reviewing a large series, a reviewer can reply to some of the
   patches with a Reviewed-by tag, stating that they are happy with
   that patch in isolation [...]. You should then update those commit
   messages by hand to include the Reviewed-by tag, so that in the next
   revision, reviewers can spot which patches were already clean from
   the previous round.

Regards,

Phil.

Re: [PATCH v2 1/4] linux-user: fix timerfd read endianness conversion

[Laurent Vivier]

Le 20/02/2023 à 09:58, Mathis Marion a écrit :
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> When reading the expiration count from a timerfd, the endianness of the
> 64bit value read is the one of the host, just as for eventfds.
> 
> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
> ---
>   linux-user/fd-trans.c | 10 +++++++---
>   linux-user/fd-trans.h |  1 +
>   linux-user/syscall.c  |  8 ++++++--
>   3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
> index 7b25468d02..146aaaafaa 100644
> --- a/linux-user/fd-trans.c
> +++ b/linux-user/fd-trans.c
> @@ -1622,7 +1622,7 @@ TargetFdTrans target_signalfd_trans = {
>       .host_to_target_data = host_to_target_data_signalfd,
>   };
>   
> -static abi_long swap_data_eventfd(void *buf, size_t len)
> +static abi_long swap_data_u64(void *buf, size_t len)
>   {
>       uint64_t *counter = buf;
>       int i;
> @@ -1640,8 +1640,12 @@ static abi_long swap_data_eventfd(void *buf, size_t len)
>   }
>   
>   TargetFdTrans target_eventfd_trans = {
> -    .host_to_target_data = swap_data_eventfd,
> -    .target_to_host_data = swap_data_eventfd,
> +    .host_to_target_data = swap_data_u64,
> +    .target_to_host_data = swap_data_u64,
> +};
> +
> +TargetFdTrans target_timerfd_trans = {
> +    .host_to_target_data = swap_data_u64,
>   };
>   
>   #if defined(CONFIG_INOTIFY) && (defined(TARGET_NR_inotify_init) || \
> diff --git a/linux-user/fd-trans.h b/linux-user/fd-trans.h
> index 1b9fa2041c..910faaf237 100644
> --- a/linux-user/fd-trans.h
> +++ b/linux-user/fd-trans.h
> @@ -130,6 +130,7 @@ extern TargetFdTrans target_netlink_route_trans;
>   extern TargetFdTrans target_netlink_audit_trans;
>   extern TargetFdTrans target_signalfd_trans;
>   extern TargetFdTrans target_eventfd_trans;
> +extern TargetFdTrans target_timerfd_trans;
>   #if (defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)) || \
>       (defined(CONFIG_INOTIFY1) && defined(TARGET_NR_inotify_init1) && \
>        defined(__NR_inotify_init1))
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1e868e9b0e..58549de125 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -13117,8 +13117,12 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>   
>   #if defined(TARGET_NR_timerfd_create) && defined(CONFIG_TIMERFD)
>       case TARGET_NR_timerfd_create:
> -        return get_errno(timerfd_create(arg1,
> -                          target_to_host_bitmask(arg2, fcntl_flags_tbl)));
> +        ret = get_errno(timerfd_create(arg1,
> +                        target_to_host_bitmask(arg2, fcntl_flags_tbl)));
> +        if (ret >= 0) {
> +            fd_trans_register(ret, &target_timerfd_trans);
> +        }
> +        return ret;
>   #endif
>   
>   #if defined(TARGET_NR_timerfd_gettime) && defined(CONFIG_TIMERFD)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness

[Laurent Vivier]

Le 20/02/2023 à 09:58, Mathis Marion a écrit :
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> Fields sin6_flowinfo and sin6_scope_id use the host byte order, so there
> is a conversion to be made when host and target endianness differ.
> 
> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
> ---
>   linux-user/syscall.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 58549de125..1a6856abec 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1713,6 +1713,12 @@ static inline abi_long target_to_host_sockaddr(int fd, struct sockaddr *addr,
>   	lladdr = (struct target_sockaddr_ll *)addr;
>   	lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
>   	lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
> +    } else if (sa_family == AF_INET6) {
> +        struct sockaddr_in6 *in6addr;
> +
> +        in6addr = (struct sockaddr_in6 *)addr;
> +        in6addr->sin6_flowinfo = tswap32(in6addr->sin6_flowinfo);

In /usr/include/linux/in6.h, it's defined as a __be32, so I don't think we need to change its 
endianness.

> +        in6addr->sin6_scope_id = tswap32(in6addr->sin6_scope_id);
>       }
>       unlock_user(target_saddr, target_addr, 0);
>   

Re: [PATCH v2 3/4] linux-user: add target to host netlink conversions

[Laurent Vivier]

Le 20/02/2023 à 09:58, Mathis Marion a écrit :
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> Added conversions for:
> - IFLA_MTU
> - IFLA_TXQLEN
> - IFLA_AF_SPEC AF_INET6 IFLA_INET6_ADDR_GEN_MODE
> These relate to the libnl functions rtnl_link_set_mtu,
> rtnl_link_set_txqlen, and rtnl_link_inet6_set_addr_gen_mode.
> 
> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
> ---
>   linux-user/fd-trans.c | 62 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
> 
> diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
> index 146aaaafaa..4852a75d9d 100644
> --- a/linux-user/fd-trans.c
> +++ b/linux-user/fd-trans.c
> @@ -1284,6 +1284,49 @@ static inline abi_long host_to_target_nlmsg_route(struct nlmsghdr *nlh,
>       return host_to_target_for_each_nlmsg(nlh, len, host_to_target_data_route);
>   }
>   
> +static abi_long target_to_host_for_each_nlattr(struct nlattr *nlattr,
> +                                               size_t len,
> +                                               abi_long (*target_to_host_nlattr)
> +                                                        (struct nlattr *))
> +{
> +    unsigned short aligned_nla_len;
> +    abi_long ret;
> +
> +    while (len > sizeof(struct nlattr)) {
> +        if (tswap16(nlattr->nla_len) < sizeof(struct rtattr) ||
> +            tswap16(nlattr->nla_len) > len) {
> +            break;
> +        }
> +        nlattr->nla_len = tswap16(nlattr->nla_len);
> +        nlattr->nla_type = tswap16(nlattr->nla_type);
> +        ret = target_to_host_nlattr(nlattr);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        aligned_nla_len = NLA_ALIGN(nlattr->nla_len);
> +        if (aligned_nla_len >= len) {
> +            break;
> +        }
> +        len -= aligned_nla_len;
> +        nlattr = (struct nlattr *)(((char *)nlattr) + aligned_nla_len);
> +    }
> +    return 0;
> +}
> +
> +static abi_long target_to_host_data_inet6_nlattr(struct nlattr *nlattr)
> +{
> +    switch (nlattr->nla_type) {
> +    /* uint8_t */
> +    case QEMU_IFLA_INET6_ADDR_GEN_MODE:
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "Unknown target AF_INET6 type: %d\n",
> +                      nlattr->nla_type);
> +    }
> +    return 0;
> +}
> +
>   static abi_long target_to_host_for_each_rtattr(struct rtattr *rtattr,
>                                                  size_t len,
>                                                  abi_long (*target_to_host_rtattr)
> @@ -1314,16 +1357,35 @@ static abi_long target_to_host_for_each_rtattr(struct rtattr *rtattr,
>       return 0;
>   }
>   
> +static abi_long target_to_host_data_spec_nlattr(struct nlattr *nlattr)
> +{
> +    switch (nlattr->nla_type) {
> +    case AF_INET6:
> +        return target_to_host_for_each_nlattr(NLA_DATA(nlattr), nlattr->nla_len,
> +                                              target_to_host_data_inet6_nlattr);
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "Unknown target AF_SPEC type: %d\n",
> +                      nlattr->nla_type);
> +        break;
> +    }
> +    return 0;
> +}
> +
>   static abi_long target_to_host_data_link_rtattr(struct rtattr *rtattr)
>   {
>       uint32_t *u32;
>   
>       switch (rtattr->rta_type) {
>       /* uint32_t */
> +    case QEMU_IFLA_MTU:
> +    case QEMU_IFLA_TXQLEN:
>       case QEMU_IFLA_EXT_MASK:
>           u32 = RTA_DATA(rtattr);
>           *u32 = tswap32(*u32);
>           break;
> +    case QEMU_IFLA_AF_SPEC:
> +        return target_to_host_for_each_nlattr(RTA_DATA(rtattr), rtattr->rta_len,
> +                                              target_to_host_data_spec_nlattr);
>       default:
>           qemu_log_mask(LOG_UNIMP, "Unknown target QEMU_IFLA type: %d\n",
>                         rtattr->rta_type);

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 3/4] linux-user: add target to host netlink conversions

[Laurent Vivier]

Le 20/02/2023 à 09:58, Mathis Marion a écrit :
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> Added conversions for:
> - IFLA_MTU
> - IFLA_TXQLEN
> - IFLA_AF_SPEC AF_INET6 IFLA_INET6_ADDR_GEN_MODE
> These relate to the libnl functions rtnl_link_set_mtu,
> rtnl_link_set_txqlen, and rtnl_link_inet6_set_addr_gen_mode.
> 
> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
> ---
>   linux-user/fd-trans.c | 62 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
> 
> diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
> index 146aaaafaa..4852a75d9d 100644
> --- a/linux-user/fd-trans.c
> +++ b/linux-user/fd-trans.c
> @@ -1284,6 +1284,49 @@ static inline abi_long host_to_target_nlmsg_route(struct nlmsghdr *nlh,
>       return host_to_target_for_each_nlmsg(nlh, len, host_to_target_data_route);
>   }
>   
> +static abi_long target_to_host_for_each_nlattr(struct nlattr *nlattr,
> +                                               size_t len,
> +                                               abi_long (*target_to_host_nlattr)
> +                                                        (struct nlattr *))
> +{
> +    unsigned short aligned_nla_len;
> +    abi_long ret;
> +
> +    while (len > sizeof(struct nlattr)) {
> +        if (tswap16(nlattr->nla_len) < sizeof(struct rtattr) ||
> +            tswap16(nlattr->nla_len) > len) {
> +            break;
> +        }
> +        nlattr->nla_len = tswap16(nlattr->nla_len);
> +        nlattr->nla_type = tswap16(nlattr->nla_type);
> +        ret = target_to_host_nlattr(nlattr);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        aligned_nla_len = NLA_ALIGN(nlattr->nla_len);
> +        if (aligned_nla_len >= len) {
> +            break;
> +        }
> +        len -= aligned_nla_len;
> +        nlattr = (struct nlattr *)(((char *)nlattr) + aligned_nla_len);
> +    }
> +    return 0;
> +}
> +
> +static abi_long target_to_host_data_inet6_nlattr(struct nlattr *nlattr)
> +{
> +    switch (nlattr->nla_type) {
> +    /* uint8_t */
> +    case QEMU_IFLA_INET6_ADDR_GEN_MODE:
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "Unknown target AF_INET6 type: %d\n",
> +                      nlattr->nla_type);
> +    }
> +    return 0;
> +}
> +
>   static abi_long target_to_host_for_each_rtattr(struct rtattr *rtattr,
>                                                  size_t len,
>                                                  abi_long (*target_to_host_rtattr)
> @@ -1314,16 +1357,35 @@ static abi_long target_to_host_for_each_rtattr(struct rtattr *rtattr,
>       return 0;
>   }
>   
> +static abi_long target_to_host_data_spec_nlattr(struct nlattr *nlattr)
> +{
> +    switch (nlattr->nla_type) {
> +    case AF_INET6:
> +        return target_to_host_for_each_nlattr(NLA_DATA(nlattr), nlattr->nla_len,
> +                                              target_to_host_data_inet6_nlattr);
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "Unknown target AF_SPEC type: %d\n",
> +                      nlattr->nla_type);
> +        break;
> +    }
> +    return 0;
> +}
> +
>   static abi_long target_to_host_data_link_rtattr(struct rtattr *rtattr)
>   {
>       uint32_t *u32;
>   
>       switch (rtattr->rta_type) {
>       /* uint32_t */
> +    case QEMU_IFLA_MTU:
> +    case QEMU_IFLA_TXQLEN:
>       case QEMU_IFLA_EXT_MASK:
>           u32 = RTA_DATA(rtattr);
>           *u32 = tswap32(*u32);
>           break;
> +    case QEMU_IFLA_AF_SPEC:
> +        return target_to_host_for_each_nlattr(RTA_DATA(rtattr), rtattr->rta_len,
> +                                              target_to_host_data_spec_nlattr);
>       default:
>           qemu_log_mask(LOG_UNIMP, "Unknown target QEMU_IFLA type: %d\n",
>                         rtattr->rta_type);

Applied to my linux-user-for-8.0 branch.

Thanks,
Laurent

Re: [PATCH v2 1/4] linux-user: fix timerfd read endianness conversion

[Laurent Vivier]

Le 20/02/2023 à 09:58, Mathis Marion a écrit :
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> When reading the expiration count from a timerfd, the endianness of the
> 64bit value read is the one of the host, just as for eventfds.
> 
> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
> ---
>   linux-user/fd-trans.c | 10 +++++++---
>   linux-user/fd-trans.h |  1 +
>   linux-user/syscall.c  |  8 ++++++--
>   3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
> index 7b25468d02..146aaaafaa 100644
> --- a/linux-user/fd-trans.c
> +++ b/linux-user/fd-trans.c
> @@ -1622,7 +1622,7 @@ TargetFdTrans target_signalfd_trans = {
>       .host_to_target_data = host_to_target_data_signalfd,
>   };
>   
> -static abi_long swap_data_eventfd(void *buf, size_t len)
> +static abi_long swap_data_u64(void *buf, size_t len)
>   {
>       uint64_t *counter = buf;
>       int i;
> @@ -1640,8 +1640,12 @@ static abi_long swap_data_eventfd(void *buf, size_t len)
>   }
>   
>   TargetFdTrans target_eventfd_trans = {
> -    .host_to_target_data = swap_data_eventfd,
> -    .target_to_host_data = swap_data_eventfd,
> +    .host_to_target_data = swap_data_u64,
> +    .target_to_host_data = swap_data_u64,
> +};
> +
> +TargetFdTrans target_timerfd_trans = {
> +    .host_to_target_data = swap_data_u64,
>   };
>   
>   #if defined(CONFIG_INOTIFY) && (defined(TARGET_NR_inotify_init) || \
> diff --git a/linux-user/fd-trans.h b/linux-user/fd-trans.h
> index 1b9fa2041c..910faaf237 100644
> --- a/linux-user/fd-trans.h
> +++ b/linux-user/fd-trans.h
> @@ -130,6 +130,7 @@ extern TargetFdTrans target_netlink_route_trans;
>   extern TargetFdTrans target_netlink_audit_trans;
>   extern TargetFdTrans target_signalfd_trans;
>   extern TargetFdTrans target_eventfd_trans;
> +extern TargetFdTrans target_timerfd_trans;
>   #if (defined(TARGET_NR_inotify_init) && defined(__NR_inotify_init)) || \
>       (defined(CONFIG_INOTIFY1) && defined(TARGET_NR_inotify_init1) && \
>        defined(__NR_inotify_init1))
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1e868e9b0e..58549de125 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -13117,8 +13117,12 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>   
>   #if defined(TARGET_NR_timerfd_create) && defined(CONFIG_TIMERFD)
>       case TARGET_NR_timerfd_create:
> -        return get_errno(timerfd_create(arg1,
> -                          target_to_host_bitmask(arg2, fcntl_flags_tbl)));
> +        ret = get_errno(timerfd_create(arg1,
> +                        target_to_host_bitmask(arg2, fcntl_flags_tbl)));
> +        if (ret >= 0) {
> +            fd_trans_register(ret, &target_timerfd_trans);
> +        }
> +        return ret;
>   #endif
>   
>   #if defined(TARGET_NR_timerfd_gettime) && defined(CONFIG_TIMERFD)

Applied to my linux-user-for-8.0 branch.

Thanks,
Laurent

Re: [PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness

[Mathis MARION]

On 06/03/2023 22:52, Laurent Vivier wrote:
> CAUTION: This email originated from outside of the organization. Do not 
> click links or open attachments unless you recognize the sender and know 
> the content is safe.
> 
> 
> Le 20/02/2023 à 09:58, Mathis Marion a écrit :
>> From: Mathis Marion <mathis.marion@silabs.com>
>>
>> Fields sin6_flowinfo and sin6_scope_id use the host byte order, so there
>> is a conversion to be made when host and target endianness differ.
>>
>> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
>> ---
>>   linux-user/syscall.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 58549de125..1a6856abec 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -1713,6 +1713,12 @@ static inline abi_long 
>> target_to_host_sockaddr(int fd, struct sockaddr *addr,
>>       lladdr = (struct target_sockaddr_ll *)addr;
>>       lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
>>       lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
>> +    } else if (sa_family == AF_INET6) {
>> +        struct sockaddr_in6 *in6addr;
>> +
>> +        in6addr = (struct sockaddr_in6 *)addr;
>> +        in6addr->sin6_flowinfo = tswap32(in6addr->sin6_flowinfo);
> 
> In /usr/include/linux/in6.h, it's defined as a __be32, so I don't think 
> we need to change its
> endianness.
> 

Right.
Thank you for integrating the other patches! Before I send a v3, do you
have any comments on patch 4?

>> +        in6addr->sin6_scope_id = tswap32(in6addr->sin6_scope_id);
>>       }
>>       unlock_user(target_saddr, target_addr, 0);
>>
> 

Re: [PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness

[Laurent Vivier]

Le 07/03/2023 à 12:27, Mathis MARION a écrit :
> 
> 
> On 06/03/2023 22:52, Laurent Vivier wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open 
>> attachments unless you recognize the sender and know the content is safe.
>>
>>
>> Le 20/02/2023 à 09:58, Mathis Marion a écrit :
>>> From: Mathis Marion <mathis.marion@silabs.com>
>>>
>>> Fields sin6_flowinfo and sin6_scope_id use the host byte order, so there
>>> is a conversion to be made when host and target endianness differ.
>>>
>>> Signed-off-by: Mathis Marion <mathis.marion@silabs.com>
>>> ---
>>>   linux-user/syscall.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 58549de125..1a6856abec 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -1713,6 +1713,12 @@ static inline abi_long target_to_host_sockaddr(int fd, struct sockaddr *addr,
>>>       lladdr = (struct target_sockaddr_ll *)addr;
>>>       lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
>>>       lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
>>> +    } else if (sa_family == AF_INET6) {
>>> +        struct sockaddr_in6 *in6addr;
>>> +
>>> +        in6addr = (struct sockaddr_in6 *)addr;
>>> +        in6addr->sin6_flowinfo = tswap32(in6addr->sin6_flowinfo);
>>
>> In /usr/include/linux/in6.h, it's defined as a __be32, so I don't think we need to change its
>> endianness.
>>
> 
> Right.
> Thank you for integrating the other patches! Before I send a v3, do you
> have any comments on patch 4?
> 

Yes, use *_MASK rather than ~*_NESTED. It looks cleaner. I prefer to ignore flags rather than return 
an error, so on architectures with same endiannes/wordsize it generally works.

Thanks,
Laurent

Re: [PATCH v2 0/4] Missing endianness conversions in user mode

[Michael Tokarev]

20.02.2023 11:58, Mathis Marion пишет:
> From: Mathis Marion <mathis.marion@silabs.com>
> 
> For a bit of context, I was trying to test a network border router [1]
> daemon using the MIPS architecture (see [2]). I didn't have access to
> real MIPS hardware so I figured I would emulate it using QEMU user mode.
> I ran into a couple of problems all related to endianness conversion for
> syscalls between host and target as MIPS is big endian and my x86 host
> is little.
> 
> [1]: https://github.com/SiliconLabs/wisun-br-linux
> [2]: https://github.com/SiliconLabs/wisun-br-linux/issues/5
> 
> v2:
> - remove context from target_to_host_for_each_nlattr()
> 
> Mathis Marion (4):
>    linux-user: fix timerfd read endianness conversion
>    linux-user: fix sockaddr_in6 endianness
>    linux-user: add target to host netlink conversions
>    linux-user: handle netlink flag NLA_F_NESTED

It looks like timefd and netlink conversions are good candidates
for -stable too. I've seen reports of these not working under
qemu linux-user emulation..

Thanks,

/mjt