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

[Laurent Vivier <laurent@vivier.eu>]

Le 27/10/2015 04:09, Laurent Vivier a écrit :
> 
> 
> Le 26/10/2015 15:40, Peter Maydell a écrit :
>> On 6 October 2015 at 18:11, Laurent Vivier <laurent@vivier.eu> 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 <laurent@vivier.eu>
>>> ---
>>> 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 ?
> 
> In fact, I've started not from the man page, but from a non working dhcp
> client, originally with a m68k target and etch-m68k distro, and I've met
> again this problem on a ppc target and jessie distro.
> 
>>
>> 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.
> 
> by digging into the code, I have found:
> 
> $ apt-get source isc-dhcp-client
> $ vi isc-dhcp-4.3.1/common/lpf.c
> 
> ...
>  72 int if_register_lpf (info)
>  73         struct interface_info *info;
>  74 {
> ...
>  79         if ((sock = socket(PF_PACKET, SOCK_PACKET,
>  80                            htons((short)ETH_P_ALL))) < 0) {
> ...
> 
> So we can see socket() is used with domain PF_PACKET and type SOCK_PACKET.
> 
> Next, the interface name is put into the sa_data of sockaddr, and bind()
> is used with AF_PACKET:
> 
>  94         /* Bind to the interface name */
>  95         memset (&sa, 0, sizeof sa);
>  96         sa.sa_family = AF_PACKET;
>  97         strncpy (sa.sa_data, (const char *)info -> ifp, sizeof
> sa.sa_data);
>  98         if (bind (sock, &sa, sizeof sa)) {
> 
> ifp is initialized from a list of all discovered interfaces in
> common/discover.c:
> ...
>  238 int
>  239 begin_iface_scan(struct iface_conf_list *ifaces) {
> ...
>  283         if (ioctl(ifaces->sock, SIOCGLIFCONF, &ifaces->conf) < 0) {
> ...
>  918 void
>  919 discover_interfaces(int state) {
> ...
>  924         struct interface_info *tmp;
> ...
>  939         if (!begin_iface_scan(&ifaces)) {
> ...
>  955         while (next_iface(&info, &err, &ifaces)) {
>  956
>  957                 /* See if we've seen an interface that matches this
> one. */
>  958                 for (tmp = interfaces; tmp; tmp = tmp->next) {
>  959                         if (!strcmp(tmp->name, info.name))
>  960                                 break;
>  961                 }
> 
> ...
>  987                         strcpy(tmp->name, info.name);
> ...
> 1050         }
> ...
> 1063         for (tmp = interfaces ; tmp != NULL ; tmp = tmp->next) {
> 1064                 if (tmp->ifp == NULL) {
> 1065                         struct ifreq *tif;
> 1066
> 1067                         tif = (struct ifreq *)dmalloc(sizeof(struct
> ifreq),
> 1068                                                       MDL);
> 1069                         if (tif == NULL)
> 1070                                 log_fatal("no space for ifp mockup.");
> 1071                         strcpy(tif->ifr_name, tmp->name);
> 1072                         tmp->ifp = tif;
> 1073                 }
> 1074         }
> 
>>
>> 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).
> 
> kernel 4.3.0-rc3, net/packet/af_packet.c:
> 
>    2961
>    2962 static int packet_bind_spkt(struct socket *sock, struct sockaddr
> *uaddr,
>    2963                             int addr_len)
>    2964 {
>    2965         struct sock *sk = sock->sk;
>    2966         char name[15];
>    2967         struct net_device *dev;
>    2968         int err = -ENODEV;
>    2969
>    2970         /*
>    2971          *      Check legality
>    2972          */
>    2973
>    2974         if (addr_len != sizeof(struct sockaddr))
>    2975                 return -EINVAL;
>    2976         strlcpy(name, uaddr->sa_data, sizeof(name));
>    2977
>    2978         dev = dev_get_by_name(sock_net(sk), name);
>    2979         if (dev)
>    2980                 err = packet_do_bind(sk, dev, pkt_sk(sk)->num);
>    2981         return err;
>    2982 }
> ...
>    4246 static const struct proto_ops packet_ops_spkt = {
>    4247         .family =       PF_PACKET,
> ...
>    4250         .bind =         packet_bind_spkt,
> ...
>    3022
>    3023 static int packet_create(struct net *net, struct socket *sock,
> int protocol,
>    3024                          int kern)
> ...
>    3045         if (sock->type == SOCK_PACKET)
>    3046                 sock->ops = &packet_ops_spkt;
> ...
> 
>> Odd to have code in target_to_host_sockaddr and not in
>> host_to_target_sockaddr.
> 
> In the case of host_to_target_sockaddr(), there is no "if (sa_family ==
> AF_PACKET) {" as it was in target_to_host_sockaddr(), it's why I didn't
> add it (and I don't like to add code I don't test).
> 
>>
>>> -       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.
> 
> I agree with you, I think I have confused socketcall() that passes
> parameters by memory, and socket() that passes parameters by registers.
> I will remove this part and resend a patch.

And for the socketcall part, we need the tswap16():

for instance,

    int a = htons(0x0003);

On a LE host:

    a = 0x00000300

On a BE host:

    a = 0x00000003

If the guest is BE, it will put in memory:

    0x00 0x00 0x00 0x03

Then a LE host, will read:

    int b = 0x03000000

but get_user_ual() in do_socketcall() will byte-swap it and put
0x00000003 in a[2].

so without the byte-swap, we call do_socket(..., 0x0003),
whereas the syscall is waiting for htons(0x0003) -> 0x0300 as we are on
LE host.

I'm sorry, I can't explain that better...

>>
>>>          fd_trans_unregister(ret);
>>>          break;
>>>  #endif
>>> --
>>> 2.4.3
>>
>> thanks
>> -- PMM
>>
> 
> Thank you for your comments,
> Laurent
>