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

Mathis Marion posted 4 patches 2 years, 11 months ago
Maintainers: Laurent Vivier <laurent@vivier.eu>
There is a newer version of this series
[PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness
Posted by Mathis Marion 2 years, 11 months ago
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

Re: [PATCH v2 2/4] linux-user: fix sockaddr_in6 endianness
Posted by Laurent Vivier 2 years, 11 months ago
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 2/4] linux-user: fix sockaddr_in6 endianness
Posted by Mathis MARION 2 years, 11 months ago

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
Posted by Laurent Vivier 2 years, 11 months ago
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 2/4] linux-user: fix sockaddr_in6 endianness
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago
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
Posted by Mathis MARION 2 years, 11 months ago

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 2/4] linux-user: fix sockaddr_in6 endianness
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago
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.