On Sun, Aug 09, 2020 at 06:20:13PM +0300, Vitaliy Makkoveev wrote:
> Hello Yasuoka.
>
> You propose to unlink pppx(4) related session which reached timeout. I'm
> ok with this direction. But I see no reason to rework _get_closed()
> routines.
>
> in pppac(4) case it's assumed what if session is not yet destroyed by
> garbage collector, it will be destroyed while we performing PIPEXGCLOSED
> command. We can make pppx(4) behavior the same and I propose to
> pppx_get_closed() be like below.
>
> Also, nothing requires to modify pipex_get_closed().
>
> ---- cut begin ----
>
> pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> {
> struct pppx_if *pxi;
>
> pxi = pppx_if_find(pxd, req->pcr_session_id, req->pcr_protocol);
> if (pxi == NULL)
> return (EINVAL);
>
> memset(req, 0, sizeof(*req));
> req->plr_ppp_id[req->plr_ppp_id_count++] = pxi->pxi_session->ppp_id;
> pppx_if_destroy(pxi);
>
> return 0;
> }
>
> ---- cut end ----
>
Sorry, I mean
pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
{
struct pppx_if *pxi;
memset(req, 0, sizeof(*req));
while ((pxi = LIST_FIRST(&pxd->pxd_pxis))) {
if (pxi->pxi_session->state == session->state =
PIPEX_STATE_CLOSED) {
req->plr_ppp_id[req->plr_ppp_id_count++] =
pxi->pxi_session->ppp_id;
pppx_if_destroy(pxi);
}
}
return 0;
}
> Also I have one inlined comment within your diff.
>
> On Sun, Aug 09, 2020 at 05:14:13PM +0900, YASUOKA Masahiko wrote:
> > This diff makes pipex "idle-timeout" work with pppx(4).
> >
> > ok?
> >
> > Index: sys/net/if_pppx.c
> > ===================================================================
> > RCS file: /disk/cvs/openbsd/src/sys/net/if_pppx.c,v
> > retrieving revision 1.98
> > diff -u -p -r1.98 if_pppx.c
> > --- sys/net/if_pppx.c 28 Jul 2020 09:53:36 -0000 1.98
> > +++ sys/net/if_pppx.c 9 Aug 2020 08:05:16 -0000
> > @@ -185,6 +185,7 @@ int pppx_config_session(struct pppx_dev
> > struct pipex_session_config_req *);
> > int pppx_get_stat(struct pppx_dev *,
> > struct pipex_session_stat_req *);
> > +int pppx_is_owner(void *, struct pipex_session *);
> > int pppx_get_closed(struct pppx_dev *,
> > struct pipex_session_list_req *);
> > int pppx_set_session_descr(struct pppx_dev *,
> > @@ -645,14 +646,6 @@ pppx_add_session(struct pppx_dev *pxd, s
> > struct in_ifaddr *ia;
> > struct sockaddr_in ifaddr;
> >
> > - /*
> > - * XXX: As long as `session' is allocated as part of a `pxi'
> > - * it isn't possible to free it separately. So disallow
> > - * the timeout feature until this is fixed.
> > - */
> > - if (req->pr_timeout_sec != 0)
> > - return (EINVAL);
> > -
> > error = pipex_init_session(&session, req);
> > if (error)
> > return (error);
> > @@ -812,12 +805,22 @@ pppx_get_stat(struct pppx_dev *pxd, stru
> > }
> >
> > int
> > -pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> > +pppx_is_owner(void *ctx, struct pipex_session *session)
> > {
> > - /* XXX: Only opened sessions exist for pppx(4) */
> > - memset(req, 0, sizeof(*req));
> > + struct pppx_dev *pxd = ctx;
> > + struct pppx_if *pxi;
> >
> > - return 0;
> > + pxi = pppx_if_find(pxd, session->session_id, session->protocol);
> > + if (pxi != NULL)
> > + return (1);
> > +
> > + return (0);
> > +}
> > +
> > +int
> > +pppx_get_closed(struct pppx_dev *pxd, struct pipex_session_list_req *req)
> > +{
> > + return (pipex_get_closed(req, pppx_is_owner, pxd));
> > }
> >
> > int
> > @@ -1059,6 +1062,7 @@ static int pppac_ioctl(struct ifnet *, u
> > static int pppac_output(struct ifnet *, struct mbuf *, struct sockaddr *,
> > struct rtentry *);
> > static void pppac_start(struct ifnet *);
> > +static int pppac_is_owner(void *, struct pipex_session *);
> >
> > static inline struct pppac_softc *
> > pppac_lookup(dev_t dev)
> > @@ -1251,6 +1255,16 @@ pppacwrite(dev_t dev, struct uio *uio, i
> > }
> >
> > int
> > +pppac_is_owner(void *ctx, struct pipex_session *session)
> > +{
> > + struct pppac_softc *sc = ctx;
> > +
> > + if (session->ifindex == sc->sc_if.if_index)
> > + return (1);
> > + return (0);
> > +}
> > +
> > +int
> > pppacioctl(dev_t dev, u_long cmd, caddr_t data, int flags, struct proc *p)
> > {
> > struct pppac_softc *sc = pppac_lookup(dev);
> > @@ -1264,6 +1278,13 @@ pppacioctl(dev_t dev, u_long cmd, caddr_
> > break;
> > case FIONREAD:
> > *(int *)data = mq_hdatalen(&sc->sc_mq);
> > + break;
> > +
> > + case PIPEXGCLOSED:
> > + NET_LOCK();
> > + error = pipex_get_closed((struct pipex_session_list_req *)data,
> > + pppac_is_owner, sc);
> > + NET_UNLOCK();
> > break;
> >
> > default:
> > Index: sys/net/pipex.c
> > ===================================================================
> > RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
> > retrieving revision 1.123
> > diff -u -p -r1.123 pipex.c
> > --- sys/net/pipex.c 4 Aug 2020 09:32:05 -0000 1.123
> > +++ sys/net/pipex.c 9 Aug 2020 08:05:16 -0000
> > @@ -240,11 +240,6 @@ pipex_ioctl(struct pipex_iface_context *
> > pipex_iface);
> > break;
> >
> > - case PIPEXGCLOSED:
> > - ret = pipex_get_closed((struct pipex_session_list_req *)data,
> > - pipex_iface);
> > - break;
> > -
> > default:
> > ret = ENOTTY;
> > break;
> > @@ -430,6 +425,7 @@ pipex_link_session(struct pipex_session
> > struct pipex_iface_context *iface)
> > {
> > struct pipex_hash_head *chain;
> > + struct ifnet *ifp;
> >
> > NET_ASSERT_LOCKED();
> >
> > @@ -442,6 +438,11 @@ pipex_link_session(struct pipex_session
> > session->pipex_iface = iface;
> > session->ifindex = iface->ifindex;
> >
> > + ifp = if_get(iface->ifindex);
> > + if (ifp != NULL && ifp->if_flags & IFF_POINTOPOINT)
> > + session->is_p2p = 1;
> > + if_put(ifp);
> > +
>
> I guess NULL `ifp' here exposes us a bug. I like to have assertion here.
>
> > LIST_INSERT_HEAD(&pipex_session_list, session, session_list);
> > chain = PIPEX_ID_HASHTABLE(session->session_id);
> > LIST_INSERT_HEAD(chain, session, id_chain);
> > @@ -469,6 +470,8 @@ pipex_unlink_session(struct pipex_sessio
> > session->ifindex = 0;
> >
> > NET_ASSERT_LOCKED();
> > + if (session->state == PIPEX_STATE_CLOSED)
> > + return;
> > LIST_REMOVE(session, id_chain);
> > #if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> > switch (session->protocol) {
> > @@ -622,7 +625,7 @@ pipex_get_stat(struct pipex_session_stat
> >
> > Static int
> > pipex_get_closed(struct pipex_session_list_req *req,
> > - struct pipex_iface_context *iface)
> > + int (*isowner)(void *, struct pipex_session *), void *ctx)
> > {
> > struct pipex_session *session, *session_tmp;
> >
> > @@ -630,7 +633,7 @@ pipex_get_closed(struct pipex_session_li
> > bzero(req, sizeof(*req));
> > LIST_FOREACH_SAFE(session, &pipex_close_wait_list, state_list,
> > session_tmp) {
> > - if (session->pipex_iface != iface)
> > + if (!isowner(ctx, session))
> > continue;
> > req->plr_ppp_id[req->plr_ppp_id_count++] = session->ppp_id;
> > LIST_REMOVE(session, state_list);
> > @@ -766,13 +769,13 @@ pipex_timer(void *ignored_arg)
> > if (session->stat.idle_time < PIPEX_CLOSE_TIMEOUT)
> > continue;
> >
> > - if (session->state == PIPEX_STATE_CLOSE_WAIT)
> > - LIST_REMOVE(session, state_list);
> > - session->state = PIPEX_STATE_CLOSED;
> > /* FALLTHROUGH */
> >
> > case PIPEX_STATE_CLOSED:
> > - pipex_destroy_session(session);
> > + if (session->is_p2p)
> > + pipex_unlink_session(session);
> > + else
> > + pipex_destroy_session(session);
> > break;
> >
> > default:
> > Index: sys/net/pipex_local.h
> > ===================================================================
> > RCS file: /disk/cvs/openbsd/src/sys/net/pipex_local.h,v
> > retrieving revision 1.39
> > diff -u -p -r1.39 pipex_local.h
> > --- sys/net/pipex_local.h 4 Aug 2020 09:32:05 -0000 1.39
> > +++ sys/net/pipex_local.h 9 Aug 2020 08:05:16 -0000
> > @@ -169,7 +169,8 @@ struct pipex_session {
> > uint16_t ip_forward:1, /* [N] {en|dis}ableIP forwarding */
> > ip6_forward:1, /* [I] {en|dis}able IPv6 forwarding */
> > is_multicast:1, /* [I] virtual entry for multicast */
> > - reserved:13;
> > + is_p2p:1, /* [I] interface is point2point(pppx) */
> > + reserved:12;
> > uint16_t protocol; /* [I] tunnel protocol (PK) */
> > uint16_t session_id; /* [I] session-id (PK) */
> > uint16_t peer_session_id; /* [I] peer's session-id */
> > @@ -391,7 +392,7 @@ int pipex_config_sessi
> > int pipex_get_stat (struct pipex_session_stat_req *,
> > struct pipex_iface_context *);
> > int pipex_get_closed (struct pipex_session_list_req *,
> > - struct pipex_iface_context *);
> > + int (*)(void *, struct pipex_session *), void *);
> > int pipex_destroy_session (struct pipex_session *);
> > struct pipex_session *pipex_lookup_by_ip_address (struct in_addr);
> > struct pipex_session *pipex_lookup_by_session_id (int, int);
>
