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

[Jan Kiszka]

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.

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