Re: [PATCH v2 4/6] linux-user: netlink: Add emulation of IP_MULTICAST_IF

[Laurent Vivier]

Le 27/12/2024 à 21:54, deller@kernel.org a écrit :
> From: Helge Deller <deller@gmx.de>
>
> Share code with IP_ADD_MEMBERSHIP/IP_DROP_MEMBERSHIP.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
>   linux-user/syscall.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index bbe2560927..4360543e20 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2130,16 +2130,23 @@ static abi_long do_setsockopt(int sockfd, int level, 
> int optname,
>               }
>               ret = get_errno(setsockopt(sockfd, level, optname, &val, 
> sizeof(val)));
>               break;
> +        case IP_MULTICAST_IF:
>           case IP_ADD_MEMBERSHIP:
>           case IP_DROP_MEMBERSHIP:

Could you put in the commit message the information from ip(7):

       IP_MULTICAST_IF (since Linux 1.2)
              Set the local device for a multicast socket.  The argument
              for setsockopt(2) is an ip_mreqn or (since Linux 3.5)
              ip_mreq structure similar to IP_ADD_MEMBERSHIP, or an
              in_addr structure.  (The kernel determines which structure
              is being passed based on the size passed in optlen.)  For
              getsockopt(2), the argument is an in_addr structure.

It would help to understand why we merge IP_MULTICAST_IF and IP_ADD_MEMBERSHIP 
code.

>           {
>               struct ip_mreqn ip_mreq;
>               struct target_ip_mreqn *target_smreqn;
> +            int min_size;
>   
>               QEMU_BUILD_BUG_ON(sizeof(struct ip_mreq) !=
>                                 sizeof(struct target_ip_mreq));
>   
> -            if (optlen < sizeof (struct target_ip_mreq) ||
> +            if (optname == IP_MULTICAST_IF) {
> +                min_size = sizeof(struct in_addr);
> +            } else {
> +                min_size = sizeof(struct target_ip_mreq);
> +            }
> +            if (optlen < min_size ||
>                   optlen > sizeof (struct target_ip_mreqn)) {
>                   return -TARGET_EINVAL;
>               }
> @@ -2149,7 +2156,9 @@ static abi_long do_setsockopt(int sockfd, int level, int 
> optname,
>                   return -TARGET_EFAULT;
>               }
>               ip_mreq.imr_multiaddr.s_addr = 
> target_smreqn->imr_multiaddr.s_addr;
> -            ip_mreq.imr_address.s_addr = target_smreqn->imr_address.s_addr;
> +            if (optlen >= sizeof(struct target_ip_mreq)) {
> +                ip_mreq.imr_address.s_addr = target_smreqn->imr_address.s_addr;
> +            }

I think you should have 3 parts here (like in the kernel):

if (optlen >= sizeof(struct target_ip_mreqn)) {
   ...
} else {
   if (optlen >= sizeof(struct target_ip_mreq)) {
      ...
   } else if (optlen >= sizeof(struct in_addr)) {
      ...
   }
}
>               if (optlen == sizeof(struct target_ip_mreqn)) {
>                   ip_mreq.imr_ifindex = tswapal(target_smreqn->imr_ifindex);
>                   optlen = sizeof(struct ip_mreqn);

Thanks.