Re: [Qemu-devel] [PATCH] linux-user: manage SOCK_PACKET socket type.

[Peter Maydell]

On 6 October 2015 at 18:11, Laurent Vivier <address@hidden> wrote:
> This is obsolete, but if we want to use dhcp with some distros (like debian
> ppc 8.2 jessie), we need it.
>
> At the bind level, we are not able to know the socket type so we try to
> guess it by analyzing the name. We manage only the case "ethX",
> "ethX" in spk_device is similar to set htons(0x6574) in sll_protocol in the
> normal case, and as this protocol does not exist, it's ok.
>
> SOCK_PACKET uses network endian to encode protocol in socket()
>
> in PACKET(7) :
>                                  protocol is the  IEEE  802.3  protocol
> number in network order.  See the <linux/if_ether.h> include file for a
> list of allowed protocols.  When protocol is  set  to  htons(ETH_P_ALL)
> then all protocols are received.  All incoming packets of that protocol
> type will be passed to the packet socket before they are passed to  the
> protocols implemented in the kernel.
>
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
> This patch is a remix of an old patch sent in 2012:
> https://patchwork.ozlabs.org/patch/208892/
>
>  linux-user/syscall.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 64be431..71cc1e2 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -111,6 +111,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
>  #include <linux/route.h>
>  #include <linux/filter.h>
>  #include <linux/blkpg.h>
> +#include <linux/if_packet.h>
>  #include "linux_loop.h"
>  #include "uname.h"
>
> @@ -1198,11 +1199,20 @@ static inline abi_long target_to_host_sockaddr(struct 
> sockaddr *addr,
>      memcpy(addr, target_saddr, len);
>      addr->sa_family = sa_family;
>      if (sa_family == AF_PACKET) {
> -       struct target_sockaddr_ll *lladdr;
> +        /* Manage an obsolete case :
> +         * if socket type is SOCK_PACKET, bind by name otherwise by index
> +         * but we are not able to know socket type, so check if the name
> +         * is usable...
> +         * see linux/net/packet/af_packet.c: packet_bind_spkt()
> +         */
> +        if (strncmp((char *)((struct sockaddr_pkt *)addr)->spkt_device,
> +                    "eth", 3) != 0) {
> +            struct target_sockaddr_ll *lladdr;

This confuses me. The packet(7) manpage suggests there are two flavours
of packet socket:
 (1) legacy AF_INET + SOCK_PACKET
 (2) new style AF_PACKET + SOCK_RAW / SOCK_DGRAM

but this comment suggests it's trying to handle AF_PACKET + SOCK_PACKET ?

If AF_PACKET was introduced as the new way of doing things, it's not
clear why it would be the family type used in the legacy approach's
sockaddr_pkt (though there seems to be code around that does this).
I suspect that 2.0 kernels just didn't check af_family here at all.

The code in the kernel's packet_recvmsg fills in the spkt_family
field with the netdevice's type field, which is an ARPHRD_* constant
as far as I can tell (I could well be wrong here).

Odd to have code in target_to_host_sockaddr and not in
host_to_target_sockaddr.

> -       lladdr = (struct target_sockaddr_ll *)addr;
> -       lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
> -       lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
> +            lladdr = (struct target_sockaddr_ll *)addr;
> +            lladdr->sll_ifindex = tswap32(lladdr->sll_ifindex);
> +            lladdr->sll_hatype = tswap16(lladdr->sll_hatype);
> +        }
>      }
>      unlock_user(target_saddr, target_addr, 0);
>
> @@ -2509,7 +2519,12 @@ static abi_long do_socketcall(int num, abi_ulong vptr)
>      /* now when we have the args, actually handle the call */
>      switch (num) {
>      case SOCKOP_socket: /* domain, type, protocol */
> -        return do_socket(a[0], a[1], a[2]);
> +        if (a[0] == AF_PACKET ||
> +            a[1] == TARGET_SOCK_PACKET) {
> +            return do_socket(a[0], a[1], tswap16(a[2]));
> +        } else {
> +            return do_socket(a[0], a[1], a[2]);
> +        }
>      case SOCKOP_bind: /* sockfd, addr, addrlen */
>          return do_bind(a[0], a[1], a[2]);
>      case SOCKOP_connect: /* sockfd, addr, addrlen */
> @@ -7500,7 +7515,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>  #endif
>  #ifdef TARGET_NR_socket
>      case TARGET_NR_socket:
> -        ret = do_socket(arg1, arg2, arg3);
> +        if (arg1 == AF_PACKET ||
> +            arg2 == TARGET_SOCK_PACKET) {
> +            /* in this case, socket() needs a network endian short */
> +            ret = do_socket(arg1, arg2, tswap16(arg3));
> +        } else {
> +            ret = do_socket(arg1, arg2, arg3);
> +        }

This doesn't make sense to me. The argument to socket()
is passed in via a register; so if the guest code correctly
passes the protocol value as htons(whatever) then that will
be the value in arg3 and we do not need to swap anything.

I see we had this discussion about the previous version of the
patch too... and my argument that we don't need the tswap16
in the socketcall code path either still makes sense to me.

>          fd_trans_unregister(ret);
>          break;
>  #endif
> --
> 2.4.3

thanks
-- PMM