From ea58a8103ceb70d20b88d37bfdbbe8ce8e9c6e71 Mon Sep 17 00:00:00 2001 From: Sylvain Rochet Date: Sun, 8 Mar 2015 02:48:52 +0100 Subject: [PATCH] PPP, PPPoS, fixed thread safety of pppos_input() PPPoS was actually not thread safe, pppos_input() can be called from lwIP user port at any time, whatever the PPP state is. It might even be called during pppos_connect() and pppos_listen(), this is quite unlikely the port do that but nothing prevent the user to since we document pppos_input() as being thread safe. Added a mutex if PPP_INPROC_MULTITHREADED is set and ensure pppos_input() is safe in regard to other pppos_* functions. --- src/include/netif/ppp/pppos.h | 3 ++ src/netif/ppp/pppos.c | 79 ++++++++++++++++++++++++++--------- 2 files changed, 63 insertions(+), 19 deletions(-) diff --git a/src/include/netif/ppp/pppos.h b/src/include/netif/ppp/pppos.h index da884503..01418806 100644 --- a/src/include/netif/ppp/pppos.h +++ b/src/include/netif/ppp/pppos.h @@ -67,6 +67,9 @@ typedef struct pppos_pcb_s pppos_pcb; struct pppos_pcb_s { /* -- below are data that will NOT be cleared between two sessions */ ppp_pcb *ppp; /* PPP PCB */ +#if PPP_INPROC_MULTITHREADED + sys_mutex_t mutex; /* Mutex for pppos_input() */ +#endif /* PPP_INPROC_MULTITHREADED */ sio_fd_t fd; /* File device ID of port. */ /* -- below are data that will be cleared between two sessions diff --git a/src/netif/ppp/pppos.c b/src/netif/ppp/pppos.c index 179b52da..9c4f5579 100644 --- a/src/netif/ppp/pppos.c +++ b/src/netif/ppp/pppos.c @@ -180,19 +180,33 @@ ppp_pcb *pppos_create(struct netif *pppif, sio_fd_t fd, ppp = ppp_new(pppif, link_status_cb, ctx_cb); if (ppp == NULL) { - return NULL; + goto ppp_new_fail; } pppos = (pppos_pcb *)memp_malloc(MEMP_PPPOS_PCB); if (pppos == NULL) { - ppp_free(ppp); - return NULL; + goto pppos_malloc_fail; } +#if PPP_INPROC_MULTITHREADED + if (sys_mutex_new(&pppos->mutex) != ERR_OK) { + goto mutex_new_fail; + } +#endif /* PPP_INPROC_MULTITHREADED */ + pppos->ppp = ppp; pppos->fd = fd; ppp_link_set_callbacks(ppp, &pppos_callbacks, pppos); return ppp; + +#if PPP_INPROC_MULTITHREADED +mutex_new_fail: + memp_free(MEMP_PPPOS_PCB, pppos); +#endif /* PPP_INPROC_MULTITHREADED */ +pppos_malloc_fail: + ppp_free(ppp); +ppp_new_fail: + return NULL; } /* Called by PPP core */ @@ -387,12 +401,18 @@ pppos_connect(ppp_pcb *ppp, void *ctx) ipcp_options *ipcp_ao; #endif /* !VJ_SUPPORT && PPP_IPV4_SUPPORT */ - /* input pbuf left over from last session? */ +#if PPP_INPROC_MULTITHREADED + sys_mutex_lock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ + /* input pbuf left ? */ pppos_free_current_input_packet(pppos); - - ppp_clear(ppp); /* reset PPPoS control block to its initial state */ memset(&pppos->out_accm, 0, sizeof(pppos_pcb) - ( (char*)&((pppos_pcb*)0)->out_accm - (char*)0 ) ); +#if PPP_INPROC_MULTITHREADED + sys_mutex_unlock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ + + ppp_clear(ppp); #if PPP_IPV4_SUPPORT #if VJ_SUPPORT @@ -437,12 +457,18 @@ pppos_listen(ppp_pcb *ppp, void *ctx, struct ppp_addrs *addrs) #endif /* PPP_IPV4_SUPPORT */ lcp_options *lcp_wo; - /* input pbuf left over from last session? */ +#if PPP_INPROC_MULTITHREADED + sys_mutex_lock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ + /* input pbuf left ? */ pppos_free_current_input_packet(pppos); - - ppp_clear(ppp); /* reset PPPoS control block to its initial state */ memset(&pppos->out_accm, 0, sizeof(pppos_pcb) - ( (char*)&((pppos_pcb*)0)->out_accm - (char*)0 ) ); +#if PPP_INPROC_MULTITHREADED + sys_mutex_unlock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ + + ppp_clear(ppp); /* Wait passively */ lcp_wo = &ppp->lcp_wantoptions; @@ -499,11 +525,17 @@ pppos_listen(ppp_pcb *ppp, void *ctx, struct ppp_addrs *addrs) static void pppos_disconnect(ppp_pcb *ppp, void *ctx) { - LWIP_UNUSED_ARG(ctx); + pppos_pcb *pppos = (pppos_pcb *)ctx; + + /* input pbuf left ? */ +#if PPP_INPROC_MULTITHREADED + sys_mutex_lock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ + pppos_free_current_input_packet(pppos); +#if PPP_INPROC_MULTITHREADED + sys_mutex_unlock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ - /* We cannot call pppos_free_current_input_packet() here because - * rx thread might still call pppos_input() - */ ppp_link_end(ppp); /* notify upper layers */ } @@ -516,6 +548,9 @@ pppos_destroy(ppp_pcb *ppp, void *ctx) /* input pbuf left ? */ pppos_free_current_input_packet(pppos); +#if PPP_INPROC_MULTITHREADED + sys_mutex_free(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ memp_free(MEMP_PPPOS_PCB, pppos); return ERR_OK; } @@ -550,15 +585,15 @@ pppos_input(ppp_pcb *ppp, u_char *s, int l) struct pbuf *next_pbuf; u_char cur_char; u_char escaped; - SYS_ARCH_DECL_PROTECT(lev); PPPDEBUG(LOG_DEBUG, ("pppos_input[%d]: got %d bytes\n", ppp->netif->num, l)); +#if PPP_INPROC_MULTITHREADED + sys_mutex_lock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ while (l-- > 0) { cur_char = *s++; - SYS_ARCH_PROTECT(lev); escaped = ESCAPE_P(pppos->in_accm, cur_char); - SYS_ARCH_UNPROTECT(lev); /* Handle special characters. */ if (escaped) { /* Check for escape sequences. */ @@ -762,6 +797,9 @@ pppos_input(ppp_pcb *ppp, u_char *s, int l) pppos->in_fcs = PPP_FCS(pppos->in_fcs, cur_char); } } /* while (l-- > 0), all bytes processed */ +#if PPP_INPROC_MULTITHREADED + sys_mutex_unlock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ magic_randomize(); } @@ -812,15 +850,18 @@ pppos_recv_config(ppp_pcb *ppp, void *ctx, u32_t accm) { int i; pppos_pcb *pppos = (pppos_pcb *)ctx; - SYS_ARCH_DECL_PROTECT(lev); LWIP_UNUSED_ARG(ppp); /* Load the ACCM bits for the 32 control codes. */ - SYS_ARCH_PROTECT(lev); +#if PPP_INPROC_MULTITHREADED + sys_mutex_lock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ for (i = 0; i < 32 / 8; i++) { pppos->in_accm[i] = (u_char)(accm >> (i * 8)); } - SYS_ARCH_UNPROTECT(lev); +#if PPP_INPROC_MULTITHREADED + sys_mutex_unlock(&pppos->mutex); +#endif /* PPP_INPROC_MULTITHREADED */ PPPDEBUG(LOG_INFO, ("pppos_recv_config[%d]: in_accm=%X %X %X %X\n", pppos->ppp->netif->num,