RE : [lwip-users] "the ARP layer is not protected against concurrentaccess"

[Frédéric BERNON]

Hi Simon,

It would be great if you can submit a patch to understand which function call 
which one. I think that perhaps a define (like TCPIP_PACKET_INPUT in this 
sample) would be another idea (for compatibility):

In opt.h

#ifndef TCPIP_PACKET_INPUT
#define TCPIP_PACKET_INPUT 1
#endif

In ethernetif.c :

static void
ethernetif_input(struct netif *netif)
{
  struct ethernetif *ethernetif;
  struct eth_hdr *ethhdr;
  struct pbuf *p;

  ethernetif = netif->state;
  
  /* move received packet into a new pbuf */
  p = low_level_input(netif);
  /* no packet could be read, silently ignore this */
  if (p == NULL) return;
 
#if TCPIP_PACKET_INPUT
  /* full packet send to tcpip_thread */
  netif->input(p, netif);

#else

  /* points to packet payload, which starts with an Ethernet header */
  ethhdr = p->payload;

#if LINK_STATS
  lwip_stats.link.recv++;
#endif /* LINK_STATS */
   
  switch (htons(ethhdr->type)) {
    /* IP packet? */
    case ETHTYPE_IP:
      #if ETHARP_TRUST_IP_MAC
      /* update ARP table */
      etharp_ip_input( netif, p);
      #endif
      /* skip Ethernet header */
      pbuf_header(p, -sizeof(struct eth_hdr));
      /* pass to network layer */
      netif->input(p, netif);
      break;
      
    case ETHTYPE_ARP:
      /* pass p to ARP module  */
      etharp_arp_input(netif, ethernetif->ethaddr, p);
      break;

    default:
      pbuf_free(p);
      p = NULL;
      break;
  }

#endif
}

And in tcpip.c:

#if TCPIP_PACKET_INPUT
err_t packet_input(struct pbuf *p, struct netif *inp)
{
  struct ethernetif *ethernetif;
  struct eth_hdr *ethhdr;
  struct pbuf *p;

  ethernetif = netif->state;
 
  /* points to packet payload, which starts with an Ethernet header */
  ethhdr = p->payload;

#if LINK_STATS
  lwip_stats.link.recv++;
#endif /* LINK_STATS */
    
  switch (htons(ethhdr->type)) {
    /* IP packet? */
    case ETHTYPE_IP:
      #if ETHARP_TRUST_IP_MAC
      /* update ARP table */
      etharp_ip_input( netif, p);
      #endif
      /* skip Ethernet header */
      pbuf_header(p, -sizeof(struct eth_hdr));
      /* pass to IP layer */
      ip_input(p, netif);
      break;
      
    case ETHTYPE_ARP:
      /* pass p to ARP module  */
      etharp_arp_input(netif, ethernetif->ethaddr, p);
      break;

    default:
      pbuf_free(p);
      p = NULL;
      break;
  }
}
#endif /* TCPIP_PACKET_INPUT */

static void
tcpip_thread(void *arg)
{
  struct tcpip_msg *msg;
  
#if IP_REASSEMBLY
  sys_timeout( 1000, ip_timer, NULL);
#endif
  if (tcpip_init_done != NULL) {
    tcpip_init_done(tcpip_init_done_arg);
  }
  
  while (1) {                          /* MAIN Loop */    
    sys_mbox_fetch(mbox, (void *)&msg, 0);
    switch (msg->type) {
    case TCPIP_MSG_API:
      LWIP_DEBUGF(TCPIP_DEBUG, ("tcpip_thread: API message %p\n", (void *)msg));
      api_msg_input(msg->msg.apimsg);
      break;
    case TCPIP_MSG_INPUT:
      LWIP_DEBUGF(TCPIP_DEBUG, ("tcpip_thread: IP packet %p\n", (void *)msg));
      #if TCPIP_PACKET_INPUT
      packet_input(msg->msg.inp.p, msg->msg.inp.netif);
      #else
      ip_input(msg->msg.inp.p, msg->msg.inp.netif);
      #endif
      break;
    case TCPIP_MSG_CALLBACK:
      LWIP_DEBUGF(TCPIP_DEBUG, ("tcpip_thread: CALLBACK %p\n", (void *)msg));
      msg->msg.cb.f(msg->msg.cb.ctx);
      break;
    default:
      break;
    }
    memp_free(MEMP_TCPIP_MSG, msg);
  }
}

====================================
Frédéric BERNON 
HYMATOM SA 
Chef de projet informatique 
Microsoft Certified Professional 
Tél. : +33 (0)4-67-87-61-10 
Fax. : +33 (0)4-67-70-85-44 
Email : address@hidden 
Web Site : http://www.hymatom.fr 
====================================
P Avant d'imprimer, penser à l'environnement

-----Message d'origine-----
De : address@hidden [mailto:address@hidden De la part de Goldschmidt Simon
Envoyé : vendredi 2 mars 2007 22:57
À : address@hidden
Objet : [lwip-users] "the ARP layer is not protected against concurrentaccess"


Hi list,

CHANGELOG says:
FUTURE
...
  * TODO: the ARP layer is not protected against concurrent access. If
    you run from a multitasking OS, serialize access to ARP (called from
    your network device driver and from a timeout thread.)

My idea was to do all ARP processing in context of the tcpip_thread (like 
Frédéric suggested). In fact, I have already done so in my local sources (and 
of course, it works for a while now :)

I've done this by adding 2 new functions (and some new messages) to tcpip.h/.c:

err_t tcpip_input_w_arp(struct pbuf *p, struct netif *inp);
  for incoming IP packets, ETH header not removed to update ARP entry
err_t tcpip_arp_input(struct pbuf *p, struct netif *inp);
  for incoming ARP packets

(tcpip_input() would not be needed any more, but you have the choice when 
writing the ethernetif.c)

All access to etharp.c is thus done in the context of the tcpip_thread, which 
saves us the concurrent access protection. Downside is that ARP requests take a 
little longer to process (context switch) but I think that's a minor one.

AS far as I know (but I'm not using it by now), that wouldn't affect raw API 
programs running without an OS, since they don't use messages, anyway. This 
would simply be a matter of assigning another input function for netifs (and 
maybe another input function, e.g. netif->arp_input()).

Please send your comments, or I will check in my changes and you'll have to 
live with it ;-)

Simon

Frédéric BERNON.vcf
Description: Frédéric BERNON.vcf