On Fri, Jun 26, 2020 at 10:23:42AM +0200, Martin Pieuchot wrote: > On 25/06/20(Thu) 19:56, Vitaliy Makkoveev wrote: > > Updated diff. > > > > OpenBSD uses 16 bit counter for allocate interface indexes. So we can't > > store index in session and be sure if_get(9) returned `ifnet' is our > > original `ifnet'. > > Why not? The point of if_get(9) is to be sure. If that doesn't work > for whatever reason then the if_get(9) interface has to be fixed. Which > case doesn't work for you? Do you have a reproducer? > > How does sessions stay around if their corresponding interface is > destroyed?
We have `pipexinq' and `pipexoutq' which can store pointers to session. pipexintr() process these queues. pipexintr() and pipex_destroy_session() are *always* different context. This mean we *can't* free pipex(4) session without be sure there is no reference to this session in `pipexinq' or `pipexoutq'. Elsewhere this will cause use afret free issue. Look please at net/pipex.c:846. The way pppx(4) destroy sessions is wrong. While pppac(4) destroys sessions by pipex_iface_fini() it's also wrong. Because we don't check `pipexinq' and `pipexoutq' state. I'am said it again and again. Look at net/pipex.c:777 Static int pipex_ppp_enqueue(struct mbuf *m0, struct pipex_session *session, struct mbuf_queue *mq) { m0->m_pkthdr.ph_cookie = session; /* [1] */ /* XXX need to support other protocols */ m0->m_pkthdr.ph_ppp_proto = PPP_IP; if (mq_enqueue(mq, m0) != 0) /* [2] */ return (1); schednetisr(NETISR_PIPEX); /* [3] */ return (0); } and net/pipex.c:643 Static int pipex_destroy_session(struct pipex_session *session) { struct radix_node *rn; /* remove from radix tree and hash chain */ NET_ASSERT_LOCKED(); if (!in_nullhost(session->ip_address.sin_addr)) { rn = rn_delete(&session->ip_address, &session->ip_netmask, pipex_rd_head4, (struct radix_node *)session); KASSERT(rn != NULL); } pipex_unlink_session(session); pool_put(&pipex_session_pool, session); /* [4] */ return (0); } and net/pipex.c:720 void pipexintr(void) { struct pipex_session *pkt_session; u_int16_t proto; struct mbuf *m; struct mbuf_list ml; /* ppp output */ mq_delist(&pipexoutq, &ml); while ((m = ml_dequeue(&ml)) != NULL) { pkt_session = m->m_pkthdr.ph_cookie; /* [5] */ /* ... */ pipex_ppp_output(m, pkt_session, proto); /* [7] */ } /* ... */ mq_delist(&pipexinq, &ml); while ((m = ml_dequeue(&ml)) != NULL) { pkt_session = m->m_pkthdr.ph_cookie; /* [6] */ /* ... */ pipex_ppp_input(m, pkt_session, 0); /* [8] */ } } and net/pipex.c:808 Static void pipex_timer(void *ignored_arg) { struct pipex_session *session, *session_tmp; timeout_add_sec(&pipex_timer_ch, pipex_prune); NET_LOCK(); /* walk through */ LIST_FOREACH_SAFE(session, &pipex_session_list, session_list, session_tmp) { switch (session->state) { /* ... */ case PIPEX_STATE_CLOSED: /* * mbuf queued in pipexinq or pipexoutq may have a * refererce to this session. */ /* [9] */ if (!mq_empty(&pipexinq) || !mq_empty(&pipexoutq)) continue; pipex_destroy_session(session); break; /* ... */ } 1. You pass pointer to mbuf 2. You enqueue this mbuf to `pipexinq' or `pipexoutq' 3. You call task_add(); Is it * always * garanteed that netisr() will be called here? What will be happened if netisr will be called after [4]? 4. You destroy session. 5. You use memory pointed by `pkt_session' It's netisr context. Are you shure `ph_cookie' is still valid? 6. You use memory pointed by `pkt_session' It's netisr context. Are you shure `ph_cookie' is still valid? Also we are going to move pipexintr() out of KERNEL_LOCK() and NET_LOCK(). This means pipex_destroy_session() and pipexintr() will be executed simultaneously. How to protect `ph_cookie'? I guess to use reference counters for pipex(4) sessions. We will grab extra reference before [1] and release this reference at [7] or [8]. I see absolutele *no* reason to sleep at [9]. So refcnt_finalize() is not applicable here. I want to use reference counters in the way of file(9). This means session can live after userland destroy it because it has reference in `pipexinq' or `pipexoutq'. But userland will not only release it's reference to session. It will destroy `ifnet' too. And you will have session referenced by `pipexinq' or `pipexoutq' but it's `ifnet' is already destoyed. The diff I posted should prevent this. you can run something like the command below: while true ; do ifconfig tun0 create ; ifconfig tun0 destroy ; done This command will overflow interface index counters and it will be start from null. It's ok. It will not overwrite indexes for existing `ifnet's. But if you are store session index as you suggested to store it's absolutely *NOT* true that if_get(9) will return NULL if the session you want to get is done. The case is: 1. The session is in pipexinq' or `pipexoutq'. 2. pppx(4) or pppac(4) already release its reference and corresponding `ifnet'. 3. pipexintr() want to access to this session erferenced by index: ifp = if_get(session->if_index); `ifp' can be NULL here, you are lucky. or NOT NULL here, that means you grab `ifnet' owned by someone else. The way I suggest to go albsolutely exclude this case.