Re: [Qemu-devel] [PATCH V2 1/2] [SLIRP] Simple ARP table

[Jan Kiszka]

On 2011-07-30 11:09, Jan Kiszka wrote:
> On 2011-07-29 19:34, Jan Kiszka wrote:
>> On 2011-07-29 18:25, Fabien Chouteau wrote:
>>> This patch adds a simple ARP table in Slirp and also adds handling of
>>> gratuitous ARP requests.
>>>
>>> Signed-off-by: Fabien Chouteau <address@hidden>
>>> ---
>>>  Makefile.objs     |    2 +-
>>>  slirp/arp_table.c |   50 ++++++++++++++++++++++++++++++++++++++++++
>>>  slirp/bootp.c     |   23 ++++++++++++------
>>>  slirp/slirp.c     |   63 
>>> +++++++++++++---------------------------------------
>>>  slirp/slirp.h     |   50 +++++++++++++++++++++++++++++++++++++++--
>>>  5 files changed, 129 insertions(+), 59 deletions(-)
>>>  create mode 100644 slirp/arp_table.c
>>>
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index 6991a9f..0c10557 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -151,7 +151,7 @@ common-obj-y += qemu-timer.o qemu-timer-common.o
>>>
>>>  slirp-obj-y = cksum.o if.o ip_icmp.o ip_input.o ip_output.o
>>>  slirp-obj-y += slirp.o mbuf.o misc.o sbuf.o socket.o tcp_input.o 
>>> tcp_output.o
>>> -slirp-obj-y += tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o
>>> +slirp-obj-y += tcp_subr.o tcp_timer.o udp.o bootp.o tftp.o arp_table.o
>>>  common-obj-$(CONFIG_SLIRP) += $(addprefix slirp/, $(slirp-obj-y))
>>>
>>>  # xen backend driver support
>>> diff --git a/slirp/arp_table.c b/slirp/arp_table.c
>>> new file mode 100644
>>> index 0000000..5d7404b
>>> --- /dev/null
>>> +++ b/slirp/arp_table.c
>>> @@ -0,0 +1,50 @@
>>> +#include "slirp.h"
>>> +
>>> +void arp_table_add(ArpTable *arptbl,
>>> +                   int       ip_addr,
>>> +                   uint8_t   ethaddr[ETH_ALEN])
>>
>> I still prefer the condensed formatting, but that's a minor nit.
>>
>>> +{
>>> +    int i;
>>> +
>>> +    DEBUG_CALL("arp_table_add");
>>> +    DEBUG_ARG("ip = 0x%x", ip_addr);
>>> +    DEBUG_ARGS((dfd, " hw addr = %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> +                ethaddr[0], ethaddr[1], ethaddr[2],
>>> +                ethaddr[3], ethaddr[4], ethaddr[5]));
>>> +
>>> +    /* Search for an entry */
>>> +    for (i = 0; i < ARP_TABLE_SIZE && arptbl->table[i].ar_sip != 0; i++) {
>>
>> Missed that on round #1: Why treating 0.0.0.0 special? If ip_addr can be
>> zero, the current logic will append every "update" of that address as a
>> new entry.
>>
>>> +        if (arptbl->table[i].ar_sip == ip_addr) {
>>> +            /* Update the entry */
>>> +            memcpy(arptbl->table[i].ar_sha, ethaddr, ETH_ALEN);
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    /* No entry found, create a new one */
>>> +    arptbl->table[arptbl->next_victim].ar_sip = ip_addr;
>>> +    memcpy(arptbl->table[arptbl->next_victim].ar_sha,  ethaddr, ETH_ALEN);
>>> +    arptbl->next_victim = (arptbl->next_victim + 1) % ARP_TABLE_SIZE;
>>> +}
>>> +
>>> +bool arp_table_search(ArpTable *arptbl,
>>> +                      int       in_ip_addr,
>>> +                      uint8_t   out_ethaddr[ETH_ALEN])
>>> +{
>>> +    int i;
>>> +
>>> +    DEBUG_CALL("arp_table_search");
>>> +    DEBUG_ARG("ip = 0x%x", in_ip_addr);
>>> +
>>> +    for (i = 0; i < ARP_TABLE_SIZE; i++) {
>>> +        if (arptbl->table[i].ar_sip == in_ip_addr) {
>>> +            memcpy(out_ethaddr, arptbl->table[i].ar_sha,  ETH_ALEN);
>>> +            DEBUG_ARGS((dfd, " found hw addr = 
>>> %02x:%02x:%02x:%02x:%02x:%02x\n",
>>> +                        out_ethaddr[0], out_ethaddr[1], out_ethaddr[2],
>>> +                        out_ethaddr[3], out_ethaddr[4], out_ethaddr[5]));
>>> +            return 1;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> diff --git a/slirp/bootp.c b/slirp/bootp.c
>>> index 1eb2ed1..07cbb80 100644
>>> --- a/slirp/bootp.c
>>> +++ b/slirp/bootp.c
>>> @@ -149,6 +149,7 @@ static void bootp_reply(Slirp *slirp, const struct 
>>> bootp_t *bp)
>>>      struct in_addr preq_addr;
>>>      int dhcp_msg_type, val;
>>>      uint8_t *q;
>>> +    uint8_t client_ethaddr[ETH_ALEN];
>>>
>>>      /* extract exact DHCP msg type */
>>>      dhcp_decode(bp, &dhcp_msg_type, &preq_addr);
>>> @@ -164,8 +165,9 @@ static void bootp_reply(Slirp *slirp, const struct 
>>> bootp_t *bp)
>>>      if (dhcp_msg_type != DHCPDISCOVER &&
>>>          dhcp_msg_type != DHCPREQUEST)
>>>          return;
>>> -    /* XXX: this is a hack to get the client mac address */
>>> -    memcpy(slirp->client_ethaddr, bp->bp_hwaddr, 6);
>>> +
>>> +    /* Get client's hardware address from bootp request */
>>> +    memcpy(client_ethaddr, bp->bp_hwaddr, ETH_ALEN);
>>>
>>>      m = m_get(slirp);
>>>      if (!m) {
>>> @@ -178,25 +180,25 @@ static void bootp_reply(Slirp *slirp, const struct 
>>> bootp_t *bp)
>>>
>>>      if (dhcp_msg_type == DHCPDISCOVER) {
>>>          if (preq_addr.s_addr != htonl(0L)) {
>>> -            bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
>>> +            bc = request_addr(slirp, &preq_addr, client_ethaddr);
>>>              if (bc) {
>>>                  daddr.sin_addr = preq_addr;
>>>              }
>>>          }
>>>          if (!bc) {
>>>           new_addr:
>>> -            bc = get_new_addr(slirp, &daddr.sin_addr, 
>>> slirp->client_ethaddr);
>>> +            bc = get_new_addr(slirp, &daddr.sin_addr, client_ethaddr);
>>>              if (!bc) {
>>>                  DPRINTF("no address left\n");
>>>                  return;
>>>              }
>>>          }
>>> -        memcpy(bc->macaddr, slirp->client_ethaddr, 6);
>>> +        memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>>>      } else if (preq_addr.s_addr != htonl(0L)) {
>>> -        bc = request_addr(slirp, &preq_addr, slirp->client_ethaddr);
>>> +        bc = request_addr(slirp, &preq_addr, client_ethaddr);
>>>          if (bc) {
>>>              daddr.sin_addr = preq_addr;
>>> -            memcpy(bc->macaddr, slirp->client_ethaddr, 6);
>>> +            memcpy(bc->macaddr, client_ethaddr, ETH_ALEN);
>>>          } else {
>>>              daddr.sin_addr.s_addr = 0;
>>>          }
>>> @@ -209,6 +211,11 @@ static void bootp_reply(Slirp *slirp, const struct 
>>> bootp_t *bp)
>>>          }
>>>      }
>>>
>>> +    if (daddr.sin_addr.s_addr != 0) {
>>> +        /* Update ARP table for this IP address */
>>> +        arp_table_add(&slirp->arp_table, daddr.sin_addr.s_addr, 
>>> client_ethaddr);
>>
>> Here you prevent that arp_table_add is called with a zero IP, but not in
>> arp_input below. Likely harmless, but at least inconsistent.
>>
> 
> Unfortunately, this patch also has a more sever issues: it breaks DHCP e.g.
> 
> Slirp's bootp sends out all replies, also acks and offers, as
> broadcasts. Normal servers already use the clients IP address as
> destination here.

Obviously, using a broadcast address on return is valid as well. So this
is no slirp bug, it's purely an ARP table lookup issue introduced by
this patch.

> 
> Even if bootp is fixed, you still lack logic to deal with special
> addresses in your arp table lookup. At least broadcasts need to be
> handled, I think other multicasts aren't supported by slirp anyway.
> 

Jan

signature.asc
Description: OpenPGP digital signature