On Fri, Jul 31, 2020 at 09:36:32AM +0900, YASUOKA Masahiko wrote: > On Thu, 30 Jul 2020 22:43:10 +0300 > Vitaliy Makkoveev <[email protected]> wrote: > > On Thu, Jul 30, 2020 at 10:05:13PM +0900, YASUOKA Masahiko wrote: > >> On Thu, 30 Jul 2020 15:34:09 +0300 > >> Vitaliy Makkoveev <[email protected]> wrote: > >> > On Thu, Jul 30, 2020 at 09:13:46PM +0900, YASUOKA Masahiko wrote: > >> >> Hi, > >> >> > >> >> sys/net/if_ethersubr.c: > >> >> 372 void > >> >> 373 ether_input(struct ifnet *ifp, struct mbuf *m) > >> >> (snip) > >> >> 519 #if NPPPOE > 0 || defined(PIPEX) > >> >> 520 case ETHERTYPE_PPPOEDISC: > >> >> 521 case ETHERTYPE_PPPOE: > >> >> 522 if (m->m_flags & (M_MCAST | M_BCAST)) > >> >> 523 goto dropanyway; > >> >> 524 #ifdef PIPEX > >> >> 525 if (pipex_enable) { > >> >> 526 struct pipex_session *session; > >> >> 527 > >> >> 528 if ((session = > >> >> pipex_pppoe_lookup_session(m)) != NULL) { > >> >> 529 pipex_pppoe_input(m, session); > >> >> 530 return; > >> >> 531 } > >> >> 532 } > >> >> 533 #endif > >> >> > >> >> previously a packet which branchces to #529 is enqueued. > >> >> > >> >> If the diff removes the queue, then the pipex input routine is > >> >> executed by the NIC's interrupt handler. > >> >> > >> >> The queues had been made to avoid that kind of situations. > >> > > >> > It's not enqueued in pppoe case. According pipex_pppoe_input() code we > >> > call pipex_common_input() with `useq' argument set to '0', so we don't > >> > enqueue mbuf(9) but pass it to pipex_ppp_input() which will pass it to > >> > ipv{4,6}_input(). > >> > >> You are right. Sorry, I forgot about this which I did that by myself. > >> > > > > I'm interesting the reason why you did that. > > > >> >> Also I don't see a relation of the use-after-free problem and killing > >> >> queues. Can't we fix the problem unless we kill the queues? > >> > > >> > Yes we can. Reference counters allow us to keep orphan sessions in these > >> > queues without use after free issue. > >> > > >> > I will wait your commentaries current enqueuing before to do something. > >> > >> I have another concern. > >> > >> You might know, when L2TP/IPsec is used heavily, the crypto thread > >> uses 100% of 1 CPU core. In that case, that thread becomes like > >> below: > >> > >> crypto thread -> udp_userreq -> pipex_l2tp_input > >> > >> some clients are using MPPE(RC4 encryption) on CCP. It's not so > >> light. > >> > >> How do we offload this for CPUs? I am thinking that "pipex" can have > >> a dedicated thread. Do we have another scenario? > >> > > > > I suppose you mean udp_input(). What is you call "crypto thread"? I did > > a little backtrace but I didn't find this thread. > > > > ether_resolve > > if_input_local > > ipv4_input > > ip_input_if > > ip_ours > > ip_deliver > > udp_input (through pr_input) > > pipex_l2tp_input > > > > ipi{,6}_mloopback > > if_input_local > > ipv4_input > > ... > > udp_input (through pr_input) > > pipex_l2tp_input > > > > loinput > > if_input_local > > ipv4_input > > ... > > udp_input (through pr_input) > > pipex_l2tp_input > > > > Also various pseudo drivers call ipv{4,6}_input() and underlay > > udp_unput() too. > > > > Except nfs, we call udp_usrreq() through socket layer only. Do you mean > > userland as "crypto thread"? > > Sorry, udp_usrreq() should be usr_input() and crypto thread meant a > kthread for crypto_taskq_mp_safe, whose name is "crynlk" (see > crypto_init()). > > A packet of L2TP/IPsec (encapsulated IP/PPP/L2TP/UDP/ESP/UDP/IP) is > processed like: > > ipv4_input > ... > udp_input > ipsec_common_input > esp_input > crypto_dispatch > => crypto_taskq_mp_safe > > kthread "crynlk" > crypto_invoke > ... (*1) > crypto_done > esp_input_cb > ipsec_common_input_cb > ip_deliver > udp_input > pipex_l2tp_input > pipex_common_input > (*2) > pipex_ppp_input > pipex_mppe_input (*3) > pipex_ppp_input > pipex_ip_input > ipv4_input > ... > > At *2 there was a queue. "crynlk" is a busy thread, since it is doing > decryption at *1. I think it's better pipex input is be done by > another thread than crypto since it also has decryption at *3.
Thanks for explanation. esp_input_cb() and underlay routines called under NET_LOCK() so we have no simultaneous execution in this path too. With current locking scheme pipexinrt() only introduces delay caused by required context switch. And there is no offload to other CPU. To reach offload we should introduce new rwlock `pipexlock' and move pipex(4) under. I have ho objections against but since we have not yet triggered use after free issue caused by unsafe session destruction, I wish to fix it first. Also we can do fine grained locks for pipex(4) instead of introducing `pipexlock'. Are you agreed with this direction? Martin, do you have objections?
