On Fri, Jul 31, 2020 at 08:26:22PM +0200, Martin Pieuchot wrote: > On 31/07/20(Fri) 12:15, Vitaliy Makkoveev wrote: > > 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? > > This isn't a realistic direction. The pipex part isn't the difficult > task for this. What needs to be addressed first is to be able to run > the IPsec/crypto stack in parallel of the rest of the network stack. >
What denies us to move pipex(4) under it's own lock?
