On Tue, Aug 04, 2020 at 01:26:14AM +0900, YASUOKA Masahiko wrote:
> On Sat, 1 Aug 2020 18:52:27 +0300
> Vitaliy Makkoveev <[email protected]> wrote:
> > On Sat, Aug 01, 2020 at 07:44:17PM +0900, YASUOKA Masahiko wrote:
> >> I'm not sure when it is broken, in old versions, it was assumed the
> >> pipex queues are empty when pipex_iface_stop() is called. The problem
> >> mvs@ found is the assumption is not true any more.
> >>
> >> pipex has a mechanism that delete a session when the queues are empty.
> >>
> >> 819 Static void
> >> 820 pipex_timer(void *ignored_arg)
> >> 821 {
> >> (snip)
> >> 854 case PIPEX_STATE_CLOSED:
> >> 855 /*
> >> 856 * mbuf queued in pipexinq or pipexoutq
> >> may have a
> >> 857 * refererce to this session.
> >> 858 */
> >> 859 if (!mq_empty(&pipexinq) ||
> >> !mq_empty(&pipexoutq))
> >> 860 continue;
> >> 861
> >> 862 pipex_destroy_session(session);
> >> 863 break;
> >>
> >> I think using this is better.
> >>
> >> How about this?
> >
> > Unfortunately your diff is incorrect. It introduces memory leaks and
> > breaks pppx(4). Also it is incomplete.
>
> Thank you for your feedbacks.
>
> > We have multiple ways to kill pipex(sessions):
> >
> > 1. pppx(4)
> >
> > We have `struct pppx_if' which has pointer to corresponding session and
> > this session is accessed directly within pppx(4) layer. Since we can't
> > destroy `ppp_if' in pipex(4) layer we can't destroy these sessions by
> > pipex_timer(). The only way to destroy them is pppx_if_destroy() which:
> >
> > 1. unlink session by pipex_unlink_session()
> > 2. detach corresponding `ifnet' by if_detach()
> > 3. release session by pipex_rele_session()
> >
> > It's unsafe because mbuf queues can have references to this session.
>
> Yes.
>
> > 2. pppac(4)
> >
> > We have no direct access to corresponding sessions within pppac(4)
> > layer. Also there are multiple ways to do this:
> >
> > 1. pipex_ioctl() with `PIPEXSMODE' command. Underlay pipex_iface_stop()
> > walks through `pipex_session_list' and destroy sessions by
> > pipex_destroy_session() call. It's unsafe because we don't check queues.
> >
> > 2. pipex_ioctl() with `PIPEXDSESSION'. pipex_close_session() will change
> > session's state and pipex_timer() will kill this sessions later. This
> > is the only safe way.
> >
> > 3. pipex_iface_fini(). The same as `PIPEXSMODE', pipex_iface_stop()
> > kills sessions, Which is also unsafe. Also we have another use after
> > free issue:
> >
> > ---- cut begin ----
> >
> > pipex_iface_fini(struct pipex_iface_context *pipex_iface)
> > {
> > pool_put(&pipex_session_pool, pipex_iface->multicast_session);
> > NET_LOCK();
> > pipex_iface_stop(pipex_iface);
> > NET_UNLOCK();
> > }
> >
> > ---- cut end ----
> >
> > `multicast_session' should be protected too. It also can be pushed to
> > `pipexoutq'.
>
> Yes, I missed this point.
>
> > Also since this time pipexintr() and pipex_iface_fini() are
> > both serialized by KERNEL_LOCK() too we can't destroy `multicast_session'
> > which is in use by pipexintr(). But when we will drop KERNEL_LOCK()
> > around pipexintr() we can catch use after free issue here. I already did
> > diff for move this pool_put() under NET_LOCK(), but it was rejectedi by
> > mpi@ because:
> >
> > ---- cut begin ----
> > pipex_iface_fini() should be called on the last reference of the
> >
> > descriptor. So this shouldn't be necessary. If there's an issue
> >
> > with the current order of the operations, we should certainly fix
> >
> > it differently.
> > ---- cut end ----
>
> Yes, I understand what mpi@ is saying. But this is a separate story.
>
> > So I repeat it again: npppd(8) can be killed in every moment by SIGKILL
> > or by SIGSEGV and pppacclose() will be called and it will call
> > pipex_iface_fini(). `multicast_session' can be used in this moment by
> > pipexintr().
> >
> > And no locks protect `multicast_session' itself.
> >
> > The two diffs I proposed in this thread solve problems caused by
> > pipexintr().
>
> There are a lot of ways to solve the problems.
>
> The diff I sent few days ago is to destruct the pipex sessions in the
> pipex timer. As you pointed out it has some problems. Those problems
> can be fixed, but I'd suggest another way. I attached at last.
>
> The problem exposed is "use-after-free". Since I think this is not a
> problem of parallel processing, having reference counter seems too
> much for me.
This is object life time problem and it isn't MP specific. Since we do
all operations under lock we can avoid atomic operations for counter
modifications. Also we will introduce these counters just before
introduce fine grained locks for pipex(4) :) For places like below,
because there is no reason to hold lock which protects global pipex
lists:
---- cut begin ----
if ((session = pipex_pppoe_lookup_session(m)) != NULL)
pipex_pppoe_input(m, session);
---- cut end ----
Also I like my first solution which removes pipexintr() because it
removes context switch in packet processing and makes code simpler.
I understood the problem with crypto thread you pointed to me. But now
we can't offload MPPE to another cpu and I can't predict when it will
be possible.
We have `nettqmp' designed to run multiple threads in parallel, but
according comment above net_tq(), ipsec denies to do this :) I guess
crypto_dispatch() should go the same way and allow multiple
`crypto_taskq_mpsafe' to run in parallel. It can allow us at least
reduce lock awaits in packet processing path.
>
>
> The diff is not to refer the session by a pointer, but by the id.
> The idea is come from IPsec tdb.
>
> Comments?
>
You introduce `cookie' as
cookie = session->protocol << 16 | session->session_id;
also multicast sessions initialized as
session->protocol = PIPEX_PROTO_NONE;
session->session_id = ifindex;
`protocol' and `session_id' come from userland, so I like to have checks
like below. It's allow us to avoid `cookie' be broken while
`pr_session_id' exceeds 16 bit integer. Also userland should not pass
PIPEX_PROTO_NONE as `pr_protocol' because we shouldn't have multicast
and not multicast sessions with the same `cookie'.
---- cut begin ----
pipex_init_session(struct pipex_session **rsession,
struct pipex_session_req *req)
{
if (req->pr_protocol == PIPEX_PROTO_NONE)
return (EINVAL);
if (req->pr_session_id > 0xffff)
return (EINVAL);
---- cut end ----
Also cookies introduce invalidation problem. Yes, it has low
probability, but we can have operation order like below:
1. enqueue session with `protocol' = 0xaa and `session_id' = 0xbb, and
`cookie' = 0xaabb
2. kill this session
3. create new session `protocol' = 0xaa and `session_id' = 0xbb
4. this newly created session will be used by pipexintr()
As I have seen while played with refcounters, session can be enqueued
more than 10 times...
Also It's not obvious that interface index will never exceed 16 bit
counter. It's unsigned int and may be underlay counter's resolution
will be expanded in future. So I like to have at least corresponding
assertion in pipex_iface_init().
So, may be my first solution is the best here. And, as mpi@ pointed,
ipsec(4) should be reworked to allow parallelism.
>
> diff --git a/sys/net/pipex.c b/sys/net/pipex.c
> index 2ad7757fee9..5f24381878c 100644
> --- a/sys/net/pipex.c
> +++ b/sys/net/pipex.c
> @@ -148,6 +148,7 @@ void
> pipex_iface_init(struct pipex_iface_context *pipex_iface, u_int ifindex)
> {
> struct pipex_session *session;
> + struct pipex_hash_head *chain;
>
> pipex_iface->pipexmode = 0;
> pipex_iface->ifindex = ifindex;
> @@ -168,7 +169,12 @@ pipex_iface_init(struct pipex_iface_context
> *pipex_iface, u_int ifindex)
> session->is_multicast = 1;
> session->pipex_iface = pipex_iface;
> session->ifindex = ifindex;
> + /* register it to the pipex id hash */
> + session->protocol = PIPEX_PROTO_NONE;
> + session->session_id = ifindex;
> pipex_iface->multicast_session = session;
> + chain = PIPEX_ID_HASHTABLE(pipex_iface->multicast_session->session_id);
> + LIST_INSERT_HEAD(chain, pipex_iface->multicast_session, id_chain);
> }
>
> Static void
> @@ -197,10 +203,11 @@ pipex_iface_stop(struct pipex_iface_context
> *pipex_iface)
> void
> pipex_iface_fini(struct pipex_iface_context *pipex_iface)
> {
> - pool_put(&pipex_session_pool, pipex_iface->multicast_session);
> NET_LOCK();
> + LIST_REMOVE(pipex_iface->multicast_session, id_chain);
> pipex_iface_stop(pipex_iface);
> NET_UNLOCK();
> + pool_put(&pipex_session_pool, pipex_iface->multicast_session);
> }
>
> int
> @@ -734,11 +741,14 @@ pipexintr(void)
> u_int16_t proto;
> struct mbuf *m;
> struct mbuf_list ml;
> + uintptr_t cookie;
>
> /* ppp output */
> mq_delist(&pipexoutq, &ml);
> while ((m = ml_dequeue(&ml)) != NULL) {
> - pkt_session = m->m_pkthdr.ph_cookie;
> + cookie = (uintptr_t)m->m_pkthdr.ph_cookie;
> + pkt_session = pipex_lookup_by_session_id(
> + cookie >> 16, cookie & 0xffff);
> if (pkt_session == NULL) {
> m_freem(m);
> continue;
> @@ -775,7 +785,9 @@ pipexintr(void)
> /* ppp input */
> mq_delist(&pipexinq, &ml);
> while ((m = ml_dequeue(&ml)) != NULL) {
> - pkt_session = m->m_pkthdr.ph_cookie;
> + cookie = (uintptr_t)m->m_pkthdr.ph_cookie;
> + pkt_session = pipex_lookup_by_session_id(
> + cookie >> 16, cookie & 0xffff);
> if (pkt_session == NULL) {
> m_freem(m);
> continue;
> @@ -788,7 +800,10 @@ Static int
> pipex_ppp_enqueue(struct mbuf *m0, struct pipex_session *session,
> struct mbuf_queue *mq)
> {
> - m0->m_pkthdr.ph_cookie = session;
> + uintptr_t cookie;
> +
> + cookie = session->protocol << 16 | session->session_id;
> + m0->m_pkthdr.ph_cookie = (caddr_t)cookie;
> /* XXX need to support other protocols */
> m0->m_pkthdr.ph_ppp_proto = PPP_IP;
>
> diff --git a/sys/net/pipex.h b/sys/net/pipex.h
> index bba010b46aa..3f493f33c9a 100644
> --- a/sys/net/pipex.h
> +++ b/sys/net/pipex.h
> @@ -44,6 +44,7 @@
> { "outq", CTLTYPE_NODE }, \
> }
>
> +#define PIPEX_PROTO_NONE 0
> #define PIPEX_PROTO_L2TP 1 /* protocol L2TP */
> #define PIPEX_PROTO_PPTP 2 /* protocol PPTP */
> #define PIPEX_PROTO_PPPOE 3 /* protocol PPPoE */
