Sorry about delay. > On 20 May 2020, at 10:54, Martin Pieuchot <m...@openbsd.org> wrote: > > On 14/05/20(Thu) 15:53, Vitaliy Makkoveev wrote: >> Each `struct pppx_if' holds it's own `pipex_session' and this session is >> used directly within ifnet's related handlers pppx_if_start() and >> pppx_if_output(). > > I don't see a problem with keeping a reference on a pipex_session inside > the softc. Is there one? If so we might want to get rid of the descriptor > completely and always use a lookup function.
This diff was for my private question about if_detach(). I was wrong about if_detach(), so we can drop this diff :) Since we are going to move pipex(4) internals back, I tried to go in pppac(4) way. Pseudo-device logic, you pointed, is very similar. Also as pipex(4) was designed, session should be destroyed by pipex_ioctl() command PIPEXDSESSION. Corresponding function pipex_close_session() doesn’t kill session, but places it to queue, and this session will be deleted later if `pipexinq’ and `pipexoutq’ are empty. This was done to be sure there is no mbufs in queues with reference to this session. Real pipex(4) destruction is done by pipex_destroy_session(). pppac(4) hasn’t references to it’s sessions, so since I tried pppac(4) way did a step to remove reference in pppx(4) too. But in fact, pipex_iface_stop() uses direct pipex_destroy_session(). Also pppx_if_destroy() uses code copypasted from this function. It’s not clean for me why this is safe. Also we have disabled in-kernel timer for pppx(4) which wants to destroy session within pipex(4). > > Always doing lookups might adds an overhead, so it should be understood. pppac_start() does one search per mbuf within pipex_output(). In my diff ppppx_if_start() does one search per queue processing. pipex_output() uses radix tree, pipex_lookup_by_session_id() uses hash. > >> pppx_if_destroy() at first destroys `pipex_session' and calls >> if_deatch() which can cause context switch. Hypothetically, >> pppx_if_start() or pppx_if_output() can receive cpu at this moment and >> start to operate with already destroyed session. > > What do you mean with 'destroyed session'? If if_detach() sleeps the > session still exists, it is just no longer linked. I don't see what bug > exist in the scenario you're describing. In fact this code doesn't destroy session, but since it was copypasted from pipex_destroy_session() (without pool_put()) the meaning of this code is session destruction. May be the first step to deduplicate should be to move this code to new function pipex_session_unlink() and call it within pipex_destroy_session() and pppx_if_destroy(). Also we can introduce pipex_session_link() for pipex session creation. > >> I guess the order of `pppx_if' destruction in pppx_if_destroy() is >> right. If we call if_detach() before `pipex_session' destruction, after >> context switch caused by this if_detach() this session can be accessed >> from network stack, but it's own `ifnet' is already destoyed by >> if_detach(). > > if_detach() doesn't destroy anything. `ifp' is still valid, just no > longer referenced by the global data structures of the network stack. > > Concerning the order of operations I'd suggest being coherent with > other pseudo-devices. So look at vlan_clone_destroy(), > gre_clone_destroy(), etc for example. > >> Also pppx_add_session() has the same problem: newly created >> `pipex_session' can start to operate with half initilaized ifnet, so >> ifnet should be initialized before session creation. > > Currently the KERNEL_LOCK() is protecting all of that, but yes getting > the order of operations right helps. > >> Ifnet should be already exist outside corresponding `pipex_session' >> life time. And within ifnet's handlers we should be sure that session is >> valid. > > If we follow other pseudo-device logic we have indeed first a 'clone' > operation that creates and `ifp' then an ioctl like SIOCSVNETID which > links the descriptor to a global list. > > Your diff goes in this direction but misses some of the bits related to > the creation of interface like if_addgroup(), bpfattach(), etc. I'd > suggest you look at net/if_vlan.c :) > >> Diff below drops direct access of this `pipex_session' in ifnet's >> handlers. > > I don't see the point of this :/ > >> In pppx_if_start() we obtain corresponding session by >> pipex_lookup_by_session_id(). If pppx_if_start() was called in half of >> pppx_if_destroy(), pipex_lookup_by_session_id() will return NULL. >> Context switch can't be occured in pppx_if_start(). > > The IFF_RUNNING flag should be used to prevent this scenario. I'd > suggest imitating the logic in vlan_clone_destroy() in this case and > bring the interface down as a first step of the destroy process. pppx_if_start() already checks this flag, but pppx_if_destroy() doesn’t clear it. I will make diff for it. > > >> In pppx_if_output() the only ppp_id is requred. >> >> In pppx_add_session() if_attach() moved before `pipex_session' can be >> accessed by pipex(4) internals. > > As said above, please keep all the `ifp' initialization together. > >> Also this will be useful for future. We are going to move out pipex(4) >> related code from pppx(4) and after this diff it will be possible just >> remove all duplicating code from pppx_if_destroy() and >> pppx_add_session(). > > Please yes, remove the duplication before trying to address concurrency > issues. It will make review easier :) We stopped at `pppx_ifs_lk’ removal, so let’s continue but after [1] finished. 1. https://marc.info/?t=158998611500002&r=1&w=2
