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.
