From 33085cb309e902178a400405cf8f490d33b9b6ff Mon Sep 17 00:00:00 2001 From: Gregory Nutt Date: Sun, 31 May 2015 08:34:03 -0600 Subject: [PATCH] Networking: The network device list was protected by a re-entrant semaphore. With the recent change to support network device callback, the network stack needs to access the network device list too. Some drivers, however, run the network stack from the interrupt level -- this is bad but a fact in the current state. Of course,those drivers are unable to take the semaphore and will assert. The solution here is to eliminate the device devices semaphore altogether. This eliminates netdev_semtake() and netdev_semgive() and replaces them with net_lock() and net_unlock() which have larger scope as needed for this purpose. --- include/nuttx/net/net.h | 4 + net/net_initialize.c | 6 -- net/netdev/Make.defs | 4 +- net/netdev/netdev.h | 44 +------- net/netdev/netdev_count.c | 8 +- net/netdev/netdev_default.c | 11 +- net/netdev/netdev_findbyaddr.c | 17 ++-- net/netdev/netdev_findbyname.c | 11 +- net/netdev/netdev_foreach.c | 13 ++- net/netdev/netdev_register.c | 8 +- net/netdev/netdev_sem.c | 180 --------------------------------- net/netdev/netdev_unregister.c | 6 +- net/netdev/netdev_verify.c | 6 +- net/socket/getsockname.c | 14 +-- net/tcp/tcp.h | 6 +- net/utils/utils.h | 1 + 16 files changed, 74 insertions(+), 265 deletions(-) delete mode 100644 net/netdev/netdev_sem.c diff --git a/include/nuttx/net/net.h b/include/nuttx/net/net.h index 51ef97a460..77c7fd807a 100644 --- a/include/nuttx/net/net.h +++ b/include/nuttx/net/net.h @@ -49,6 +49,10 @@ #include #include +#ifndef CONFIG_NET_NOINTS +# include +#endif + /**************************************************************************** * Pre-processor Definitions ****************************************************************************/ diff --git a/net/net_initialize.c b/net/net_initialize.c index af0e8651f7..99090c650a 100644 --- a/net/net_initialize.c +++ b/net/net_initialize.c @@ -176,12 +176,6 @@ void net_setup(void) net_initroute(); #endif - -#if CONFIG_NSOCKET_DESCRIPTORS > 0 - /* Initialize the socket layer */ - - netdev_seminit(); -#endif } /**************************************************************************** diff --git a/net/netdev/Make.defs b/net/netdev/Make.defs index 40c08f9642..955745945c 100644 --- a/net/netdev/Make.defs +++ b/net/netdev/Make.defs @@ -37,8 +37,8 @@ NETDEV_CSRCS += netdev_register.c netdev_ioctl.c netdev_txnotify.c NETDEV_CSRCS += netdev_findbyname.c netdev_findbyaddr.c netdev_count.c -NETDEV_CSRCS += netdev_foreach.c netdev_unregister.c netdev_sem.c -NETDEV_CSRCS += netdev_carrier.c netdev_default.c netdev_verify.c +NETDEV_CSRCS += netdev_foreach.c netdev_unregister.c netdev_carrier.c +NETDEV_CSRCS += netdev_default.c netdev_verify.c ifeq ($(CONFIG_NET_RXAVAIL),y) NETDEV_CSRCS += netdev_rxnotify.c diff --git a/net/netdev/netdev.h b/net/netdev/netdev.h index 9b5ca75e6f..9e055effd0 100644 --- a/net/netdev/netdev.h +++ b/net/netdev/netdev.h @@ -1,7 +1,7 @@ /**************************************************************************** * net/netdev/netdev.h * - * Copyright (C) 2014 Gregory Nutt. All rights reserved. + * Copyright (C) 2014-2015 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -69,8 +69,10 @@ extern "C" #endif #if CONFIG_NSOCKET_DESCRIPTORS > 0 -/* List of registered Ethernet device drivers. This duplicates a declaration - * in net/netdev/netdev.h +/* List of registered Ethernet device drivers. You must have the network + * locked in order to access this list. + * + * NOTE that this duplicates a declaration in net/tcp/tcp.h */ EXTERN struct net_driver_s *g_netdevices; @@ -80,42 +82,6 @@ EXTERN struct net_driver_s *g_netdevices; * Public Function Prototypes ****************************************************************************/ -/**************************************************************************** - * Function: netdev_seminit - * - * Description: - * Initialize the network device semaphore. - * - ****************************************************************************/ - -#if CONFIG_NSOCKET_DESCRIPTORS > 0 -void netdev_seminit(void); -#endif - -/**************************************************************************** - * Function: netdev_semtake - * - * Description: - * Get exclusive access to the network device list. - * - ****************************************************************************/ - -#if CONFIG_NSOCKET_DESCRIPTORS > 0 -void netdev_semtake(void); -#endif - -/**************************************************************************** - * Function: netdev_semgive - * - * Description: - * Release exclusive access to the network device list - * - ****************************************************************************/ - -#if CONFIG_NSOCKET_DESCRIPTORS > 0 -void netdev_semgive(void); -#endif - /**************************************************************************** * Name: netdev_ifup / netdev_ifdown * diff --git a/net/netdev/netdev_count.c b/net/netdev/netdev_count.c index 2b3b987ac3..d036339c54 100644 --- a/net/netdev/netdev_count.c +++ b/net/netdev/netdev_count.c @@ -1,7 +1,7 @@ /**************************************************************************** * net/netdev/netdev_count.c * - * Copyright (C) 2007-2009 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2015 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -45,6 +45,7 @@ #include +#include "utils/utils.h" #include "netdev/netdev.h" /**************************************************************************** @@ -91,11 +92,12 @@ int netdev_count(void) { struct net_driver_s *dev; + net_lock_t save; int ndev; - netdev_semtake(); + save = net_lock(); for (dev = g_netdevices, ndev = 0; dev; dev = dev->flink, ndev++); - netdev_semgive(); + net_unlock(save); return ndev; } diff --git a/net/netdev/netdev_default.c b/net/netdev/netdev_default.c index b72d1083f6..addb67dcc7 100644 --- a/net/netdev/netdev_default.c +++ b/net/netdev/netdev_default.c @@ -1,7 +1,7 @@ /**************************************************************************** * net/netdev/netdev_default.c * - * Copyright (C) 2014 Gregory Nutt. All rights reserved. + * Copyright (C) 2014-2015 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -41,6 +41,8 @@ #if defined(CONFIG_NET) && CONFIG_NSOCKET_DESCRIPTORS > 0 #include + +#include "utils/utils.h" #include "netdev/netdev.h" /**************************************************************************** @@ -98,10 +100,11 @@ FAR struct net_driver_s *netdev_default(void) { FAR struct net_driver_s *dev; + net_lock_t save; /* Examine each registered network device */ - netdev_semtake(); + save = net_lock(); for (dev = g_netdevices; dev; dev = dev->flink) { /* Is the interface in the "up" state? */ @@ -112,12 +115,12 @@ FAR struct net_driver_s *netdev_default(void) * state. */ - netdev_semgive(); + net_unlock(save); return dev; } } - netdev_semgive(); + net_unlock(save); return NULL; } diff --git a/net/netdev/netdev_findbyaddr.c b/net/netdev/netdev_findbyaddr.c index 56fc0368b8..041189842a 100644 --- a/net/netdev/netdev_findbyaddr.c +++ b/net/netdev/netdev_findbyaddr.c @@ -50,9 +50,10 @@ #include #include -#include "netdev/netdev.h" +#include "utils/utils.h" #include "devif/devif.h" #include "route/route.h" +#include "netdev/netdev.h" /**************************************************************************** * Pre-processor Definitions @@ -97,10 +98,11 @@ static FAR struct net_driver_s *netdev_finddevice_ipv4addr(in_addr_t ripaddr) { FAR struct net_driver_s *dev; + net_lock_t save; /* Examine each registered network device */ - netdev_semtake(); + save = net_lock(); for (dev = g_netdevices; dev; dev = dev->flink) { /* Is the interface in the "up" state? */ @@ -114,7 +116,7 @@ static FAR struct net_driver_s *netdev_finddevice_ipv4addr(in_addr_t ripaddr) { /* Its a match */ - netdev_semgive(); + net_unlock(save); return dev; } } @@ -122,7 +124,7 @@ static FAR struct net_driver_s *netdev_finddevice_ipv4addr(in_addr_t ripaddr) /* No device with the matching address found */ - netdev_semgive(); + net_unlock(save); return NULL; } #endif /* CONFIG_NET_IPv4 */ @@ -151,10 +153,11 @@ static FAR struct net_driver_s * netdev_finddevice_ipv6addr(const net_ipv6addr_t ripaddr) { FAR struct net_driver_s *dev; + net_lock_t save; /* Examine each registered network device */ - netdev_semtake(); + save = net_lock(); for (dev = g_netdevices; dev; dev = dev->flink) { /* Is the interface in the "up" state? */ @@ -168,7 +171,7 @@ netdev_finddevice_ipv6addr(const net_ipv6addr_t ripaddr) { /* Its a match */ - netdev_semgive(); + net_unlock(save); return dev; } } @@ -176,7 +179,7 @@ netdev_finddevice_ipv6addr(const net_ipv6addr_t ripaddr) /* No device with the matching address found */ - netdev_semgive(); + net_unlock(save); return NULL; } #endif /* CONFIG_NET_IPv6 */ diff --git a/net/netdev/netdev_findbyname.c b/net/netdev/netdev_findbyname.c index d9d44a9701..93f90a9aab 100644 --- a/net/netdev/netdev_findbyname.c +++ b/net/netdev/netdev_findbyname.c @@ -1,7 +1,7 @@ /**************************************************************************** * net/netdev/netdev_findbyname.c * - * Copyright (C) 2007, 2008 Gregory Nutt. All rights reserved. + * Copyright (C) 2007, 2008, 2015 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -45,6 +45,7 @@ #include +#include "utils/utils.h" #include "netdev/netdev.h" /**************************************************************************** @@ -92,19 +93,21 @@ FAR struct net_driver_s *netdev_findbyname(FAR const char *ifname) { FAR struct net_driver_s *dev; + net_lock_t save; + if (ifname) { - netdev_semtake(); + save = net_lock(); for (dev = g_netdevices; dev; dev = dev->flink) { if (strcmp(ifname, dev->d_ifname) == 0) { - netdev_semgive(); + net_unlock(save); return dev; } } - netdev_semgive(); + net_unlock(save); } return NULL; diff --git a/net/netdev/netdev_foreach.c b/net/netdev/netdev_foreach.c index 2585f34827..8098dfd12e 100644 --- a/net/netdev/netdev_foreach.c +++ b/net/netdev/netdev_foreach.c @@ -1,7 +1,7 @@ /**************************************************************************** * net/netdev/netdev_foreach.c * - * Copyright (C) 2007-2009, 2012 Gregory Nutt. All rights reserved. + * Copyright (C) 2007-2009, 2012, 2015 Gregory Nutt. All rights reserved. * Author: Gregory Nutt * * Redistribution and use in source and binary forms, with or without @@ -90,14 +90,15 @@ * ****************************************************************************/ -int netdev_foreach(netdev_callback_t callback, void *arg) +int netdev_foreach(netdev_callback_t callback, FAR void *arg) { - struct net_driver_s *dev; + FAR struct net_driver_s *dev; + net_lock_t save; int ret = 0; if (callback) { - netdev_semtake(); + save = net_lock(); for (dev = g_netdevices; dev; dev = dev->flink) { if (callback(dev, arg) != 0) @@ -106,8 +107,10 @@ int netdev_foreach(netdev_callback_t callback, void *arg) break; } } - netdev_semgive(); + + net_unlock(save); } + return ret; } diff --git a/net/netdev/netdev_register.c b/net/netdev/netdev_register.c index e4b5361a76..524f6cb2d4 100644 --- a/net/netdev/netdev_register.c +++ b/net/netdev/netdev_register.c @@ -53,8 +53,9 @@ #include #include -#include "netdev/netdev.h" +#include "utils/utils.h" #include "igmp/igmp.h" +#include "netdev/netdev.h" /**************************************************************************** * Pre-processor Definitions @@ -175,6 +176,7 @@ int netdev_register(FAR struct net_driver_s *dev, enum net_lltype_e lltype) #ifdef CONFIG_NET_USER_DEVFMT FAR const char devfmt_str[IFNAMSIZ]; #endif + net_lock_t save; int devnum; if (dev) @@ -256,7 +258,7 @@ int netdev_register(FAR struct net_driver_s *dev, enum net_lltype_e lltype) * the interface */ - netdev_semtake(); + save = net_lock(); #ifdef CONFIG_NET_MULTILINK devnum = find_devnum(devfmt); #else @@ -282,7 +284,7 @@ int netdev_register(FAR struct net_driver_s *dev, enum net_lltype_e lltype) #ifdef CONFIG_NET_IGMP igmp_devinit(dev); #endif - netdev_semgive(); + net_unlock(save); #ifdef CONFIG_NET_ETHERNET nlldbg("Registered MAC: %02x:%02x:%02x:%02x:%02x:%02x as dev: %s\n", diff --git a/net/netdev/netdev_sem.c b/net/netdev/netdev_sem.c deleted file mode 100644 index 06c36b03c3..0000000000 --- a/net/netdev/netdev_sem.c +++ /dev/null @@ -1,180 +0,0 @@ -/**************************************************************************** - * net/netdev/netdev_sem.c - * - * Copyright (C) 2012 Gregory Nutt. All rights reserved. - * Author: Gregory Nutt - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions - * are met: - * - * 1. Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * 2. Redistributions in binary form must reproduce the above copyright - * notice, this list of conditions and the following disclaimer in - * the documentation and/or other materials provided with the - * distribution. - * 3. Neither the name NuttX nor the names of its contributors may be - * used to endorse or promote products derived from this software - * without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS - * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE - * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, - * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, - * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS - * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED - * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT - * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN - * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE - * POSSIBILITY OF SUCH DAMAGE. - * - ****************************************************************************/ - -/**************************************************************************** - * Included Files - ****************************************************************************/ - -#include - -#if defined(CONFIG_NET) && CONFIG_NSOCKET_DESCRIPTORS > 0 - -#include - -#include -#include -#include -#include - -#include - -#include "netdev/netdev.h" - -/**************************************************************************** - * Pre-processor Definitions - ****************************************************************************/ - -#define NO_HOLDER (pid_t)-1 - -/**************************************************************************** - * Priviate Types - ****************************************************************************/ - -/* There is at least on context in which recursive semaphores are required: - * When netdev_foreach is used with a telnet client, we will deadlock if we - * do not provide this capability. - */ - -struct netdev_sem_s -{ - sem_t sem; - pid_t holder; - unsigned int count; -}; - -/**************************************************************************** - * Private Data - ****************************************************************************/ - -/**************************************************************************** - * Public Data - ****************************************************************************/ - -static struct netdev_sem_s g_devlock; - -/**************************************************************************** - * Private Functions - ****************************************************************************/ - -/**************************************************************************** - * Public Functions - ****************************************************************************/ - -/**************************************************************************** - * Function: netdev_seminit - * - * Description: - * Initialize the network device semaphore. - * - ****************************************************************************/ - -void netdev_seminit(void) -{ - sem_init(&g_devlock.sem, 0, 1); - g_devlock.holder = NO_HOLDER; - g_devlock.count = 0; -} - -/**************************************************************************** - * Function: netdev_semtake - * - * Description: - * Get exclusive access to the network device list. - * - ****************************************************************************/ - -void netdev_semtake(void) -{ - pid_t me = getpid(); - - /* Does this thread already hold the semaphore? */ - - if (g_devlock.holder == me) - { - /* Yes.. just increment the reference count */ - - g_devlock.count++; - } - else - { - /* No.. take the semaphore (perhaps waiting) */ - - while (net_lockedwait(&g_devlock.sem) != 0) - { - /* The only case that an error should occur here is if - * the wait was awakened by a signal. - */ - - ASSERT(errno == EINTR); - } - - /* Now this thread holds the semaphore */ - - g_devlock.holder = me; - g_devlock.count = 1; - } -} - -/**************************************************************************** - * Function: netdev_semtake - * - * Description: - * Release exclusive access to the network device list - * - ****************************************************************************/ - -void netdev_semgive(void) -{ - DEBUGASSERT(g_devlock.holder == getpid() && g_devlock.count > 0); - - /* If the count would go to zero, then release the semaphore */ - - if (g_devlock.count == 1) - { - /* We no longer hold the semaphore */ - - g_devlock.holder = NO_HOLDER; - g_devlock.count = 0; - sem_post(&g_devlock.sem); - } - else - { - /* We still hold the semaphore. Just decrement the count */ - - g_devlock.count--; - } -} - -#endif /* CONFIG_NET && CONFIG_NSOCKET_DESCRIPTORS */ diff --git a/net/netdev/netdev_unregister.c b/net/netdev/netdev_unregister.c index 104bb3022d..9dad59c222 100644 --- a/net/netdev/netdev_unregister.c +++ b/net/netdev/netdev_unregister.c @@ -52,6 +52,7 @@ #include #include +#include "utils/utils.h" #include "netdev/netdev.h" /**************************************************************************** @@ -106,10 +107,11 @@ int netdev_unregister(FAR struct net_driver_s *dev) { struct net_driver_s *prev; struct net_driver_s *curr; + net_lock_t save; if (dev) { - netdev_semtake(); + save = net_lock(); /* Find the device in the list of known network devices */ @@ -139,7 +141,7 @@ int netdev_unregister(FAR struct net_driver_s *dev) curr->flink = NULL; } - netdev_semgive(); + net_unlock(save); #ifdef CONFIG_NET_ETHERNET nlldbg("Unregistered MAC: %02x:%02x:%02x:%02x:%02x:%02x as dev: %s\n", diff --git a/net/netdev/netdev_verify.c b/net/netdev/netdev_verify.c index 6f2540f688..8fdd42b53a 100644 --- a/net/netdev/netdev_verify.c +++ b/net/netdev/netdev_verify.c @@ -43,6 +43,7 @@ #include +#include "utils/utils.h" #include "netdev/netdev.h" /**************************************************************************** @@ -63,11 +64,12 @@ bool netdev_verify(FAR struct net_driver_s *dev) { FAR struct net_driver_s *chkdev; + net_lock_t save; bool valid = false; /* Search the list of registered devices */ - netdev_semtake(); + save = net_lock(); for (chkdev = g_netdevices; chkdev != NULL; chkdev = chkdev->flink) { /* Is the the network device that we are looking for? */ @@ -81,6 +83,6 @@ bool netdev_verify(FAR struct net_driver_s *dev) } } - netdev_semgive(); + net_unlock(save); return valid; } diff --git a/net/socket/getsockname.c b/net/socket/getsockname.c index 9cadc9f9ed..4c02fc6352 100644 --- a/net/socket/getsockname.c +++ b/net/socket/getsockname.c @@ -49,6 +49,7 @@ #include #include +#include "utils/utils.h" #include "netdev/netdev.h" #include "tcp/tcp.h" #include "udp/udp.h" @@ -98,6 +99,7 @@ int ipv4_getsockname(FAR struct socket *psock, FAR struct sockaddr *addr, in_addr_t lipaddr; in_addr_t ripaddr; #endif + net_lock_t save; /* Check if enough space has been provided for the full address */ @@ -150,7 +152,7 @@ int ipv4_getsockname(FAR struct socket *psock, FAR struct sockaddr *addr, * a single network device and only the network device knows the IP address. */ - netdev_semtake(); + save = net_lock(); #ifdef CONFIG_NETDEV_MULTINIC /* Find the device matching the IPv4 address in the connection structure */ @@ -164,7 +166,7 @@ int ipv4_getsockname(FAR struct socket *psock, FAR struct sockaddr *addr, if (!dev) { - netdev_semgive(); + net_unlock(save); return -EINVAL; } @@ -175,7 +177,7 @@ int ipv4_getsockname(FAR struct socket *psock, FAR struct sockaddr *addr, outaddr->sin_addr.s_addr = dev->d_ipaddr; *addrlen = sizeof(struct sockaddr_in); #endif - netdev_semgive(); + net_unlock(save); /* Return success */ @@ -273,7 +275,7 @@ int ipv6_getsockname(FAR struct socket *psock, FAR struct sockaddr *addr, * a single network device and only the network device knows the IP address. */ - netdev_semtake(); + save = net_lock(); #ifdef CONFIG_NETDEV_MULTINIC /* Find the device matching the IPv6 address in the connection structure */ @@ -287,7 +289,7 @@ int ipv6_getsockname(FAR struct socket *psock, FAR struct sockaddr *addr, if (!dev) { - netdev_semgive(); + net_unlock(save); return -EINVAL; } @@ -298,7 +300,7 @@ int ipv6_getsockname(FAR struct socket *psock, FAR struct sockaddr *addr, memcpy(outaddr->sin6_addr.in6_u.u6_addr8, dev->d_ipv6addr, 16); *addrlen = sizeof(struct sockaddr_in6); #endif - netdev_semgive(); + net_unlock(save); /* Return success */ diff --git a/net/tcp/tcp.h b/net/tcp/tcp.h index 85f56dd785..3957e9468c 100644 --- a/net/tcp/tcp.h +++ b/net/tcp/tcp.h @@ -332,8 +332,10 @@ extern "C" #endif #if CONFIG_NSOCKET_DESCRIPTORS > 0 -/* List of registered Ethernet device drivers. This duplicates a declaration - * in net/tcp/tcp.h +/* List of registered Ethernet device drivers. You must have the network + * locked in order to access this list. + * + * NOTE that this duplicates a declaration in net/netdev/netdev.h */ EXTERN struct net_driver_s *g_netdevices; diff --git a/net/utils/utils.h b/net/utils/utils.h index 951caf8d59..d3644d446a 100644 --- a/net/utils/utils.h +++ b/net/utils/utils.h @@ -41,6 +41,7 @@ ****************************************************************************/ #include +#include #include /****************************************************************************