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

[Helge Deller]

On 1/20/25 19:17, Laurent Vivier wrote:
> Le 19/01/2025 à 05:41, deller@kernel.org a écrit :
> > From: Helge Deller <deller@gmx.de>
> >
> > Add IP_MULTICAST_IF and share the code with IP_ADD_MEMBERSHIP / 
> > IP_DROP_MEMBERSHIP.
> > Sharing the code makes sense, because the manpage of ip(7)  says:
> >
> > 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.
> >
> > Signed-off-by: Helge Deller <deller@gmx.de>
> >
> > v2: (based on feedback by Laurent Vivier)
> > - refined commit message and restructure the copying of ip_mreqn fields
> >
> > 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:
> >           {
> >               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;
> > +            }
> >               if (optlen == sizeof(struct target_ip_mreqn)) {
> >                   ip_mreq.imr_ifindex = tswapal(target_smreqn->imr_ifindex);
> >                   optlen = sizeof(struct ip_mreqn);
>
> This part above seems to be in the commit message... and is the version 2, is 
> this what you want?

Ah... right.
I'll drop that patch version info, so it will be ok.


> > ---
> >   linux-user/syscall.c | 20 ++++++++++++++------
> >   1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index df8609b4d8..6ee02383da 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:
> >           {
> >               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,13 +2156,14 @@ 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_mreqn)) {
> > -                ip_mreq.imr_ifindex = tswapal(target_smreqn->imr_ifindex);
> > -                optlen = sizeof(struct ip_mreqn);
> > +            if (optlen >= sizeof(struct target_ip_mreq)) {
> > +                ip_mreq.imr_address.s_addr = target_smreqn->imr_address.s_addr;
> > +                if (optlen >= sizeof(struct target_ip_mreqn)) {
> > +                    __put_user(target_smreqn->imr_ifindex, 
> > &ip_mreq.imr_ifindex);
> > +                    optlen = sizeof(struct ip_mreqn);
> > +                }
> >               }
> >               unlock_user(target_smreqn, optval_addr, 0);
> > -
> >               ret = get_errno(setsockopt(sockfd, level, optname, &ip_mreq, 
> > optlen));
> >               break;
> >           }
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Thanks!

Helge