On Thu, Apr 02, 2020 at 09:26:23AM +0200, Martin Pieuchot wrote:
> Hello Vitaliy,
>
> On 01/04/20(Wed) 12:59, Vitaliy Makkoveev wrote:
> > Updated diff. The idea is to use already existing pipex API for
> > pipex_session creation and destruction. pppx_if now holds a reference
> > to pipex_session.
>
> This is great!
>
> There are many things in this diff which makes it complicated to review,
> at least to me. Note that I'm not really familiar with this code.
>
> For example the panic() after the RBT_REMOVE() is now questionable since
> the lock is released then retaken. Which brings a question for later:
> what is the lock really protecting? That can be documented in the header
> like it is done for other data structures.
>
> So the changes includes:
>
> - pool_get() with PR_WAITOK should never fail. This could be the first
> step and also move the allocation at the beginning to make a pattern
> appear.
>
> - Allocation of `pxi_session' separately from `pxi', this creates quite
> some noise because of the line changing indirection.
>
> - Change in the error paths, which aren't correct and IMHO should be
> left out for the moment.
>
> - Use of pipex_add_session(), isn't that independent from the allocation
> of `session'?
>
> Could you help me review this by submitting smaller diffs? Thanks a
> lot!
Let's take a break, another pipex(4) issue found. I'll send diff for it
it today later with new thread.
>
> >
> > Index: sys/net/if_pppx.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/if_pppx.c,v
> > retrieving revision 1.78
> > diff -u -p -r1.78 if_pppx.c
> > --- sys/net/if_pppx.c 1 Apr 2020 07:15:59 -0000 1.78
> > +++ sys/net/if_pppx.c 1 Apr 2020 09:50:19 -0000
> > @@ -155,7 +155,7 @@ struct pppx_if {
> > int pxi_unit;
> > struct ifnet pxi_if;
> > struct pppx_dev *pxi_dev;
> > - struct pipex_session pxi_session;
> > + struct pipex_session *pxi_session;
> > struct pipex_iface_context pxi_ifcontext;
> > };
> >
> > @@ -655,15 +655,10 @@ int
> > pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
> > {
> > struct pppx_if *pxi;
> > - struct pipex_session *session;
> > - struct pipex_hash_head *chain;
> > struct ifnet *ifp;
> > int unit, error = 0;
> > struct in_ifaddr *ia;
> > struct sockaddr_in ifaddr;
> > -#ifdef PIPEX_PPPOE
> > - struct ifnet *over_ifp = NULL;
> > -#endif
> >
> > /*
> > * XXX: As long as `session' is allocated as part of a `pxi'
> > @@ -673,157 +668,16 @@ pppx_add_session(struct pppx_dev *pxd, s
> > if (req->pr_timeout_sec != 0)
> > return (EINVAL);
> >
> > - switch (req->pr_protocol) {
> > -#ifdef PIPEX_PPPOE
> > - case PIPEX_PROTO_PPPOE:
> > - over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
> > - if (over_ifp == NULL)
> > - return (EINVAL);
> > - if (req->pr_peer_address.ss_family != AF_UNSPEC)
> > - return (EINVAL);
> > - break;
> > -#endif
> > -#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> > - case PIPEX_PROTO_PPTP:
> > - case PIPEX_PROTO_L2TP:
> > - switch (req->pr_peer_address.ss_family) {
> > - case AF_INET:
> > - if (req->pr_peer_address.ss_len != sizeof(struct
> > sockaddr_in))
> > - return (EINVAL);
> > - break;
> > -#ifdef INET6
> > - case AF_INET6:
> > - if (req->pr_peer_address.ss_len != sizeof(struct
> > sockaddr_in6))
> > - return (EINVAL);
> > - break;
> > -#endif
> > - default:
> > - return (EPROTONOSUPPORT);
> > - }
> > - if (req->pr_peer_address.ss_family !=
> > - req->pr_local_address.ss_family ||
> > - req->pr_peer_address.ss_len !=
> > - req->pr_local_address.ss_len)
> > - return (EINVAL);
> > - break;
> > -#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
> > - default:
> > - return (EPROTONOSUPPORT);
> > - }
> > -
> > pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
> > - if (pxi == NULL)
> > - return (ENOMEM);
> > -
> > - session = &pxi->pxi_session;
> > ifp = &pxi->pxi_if;
> >
> > - /* fake a pipex interface context */
> > - session->pipex_iface = &pxi->pxi_ifcontext;
> > - session->pipex_iface->ifnet_this = ifp;
> > - session->pipex_iface->pipexmode = PIPEX_ENABLED;
> > -
> > - /* setup session */
> > - session->state = PIPEX_STATE_OPENED;
> > - session->protocol = req->pr_protocol;
> > - session->session_id = req->pr_session_id;
> > - session->peer_session_id = req->pr_peer_session_id;
> > - session->peer_mru = req->pr_peer_mru;
> > - session->timeout_sec = req->pr_timeout_sec;
> > - session->ppp_flags = req->pr_ppp_flags;
> > - session->ppp_id = req->pr_ppp_id;
> > -
> > - session->ip_forward = 1;
> > -
> > - session->ip_address.sin_family = AF_INET;
> > - session->ip_address.sin_len = sizeof(struct sockaddr_in);
> > - session->ip_address.sin_addr = req->pr_ip_address;
> > -
> > - session->ip_netmask.sin_family = AF_INET;
> > - session->ip_netmask.sin_len = sizeof(struct sockaddr_in);
> > - session->ip_netmask.sin_addr = req->pr_ip_netmask;
> > -
> > - if (session->ip_netmask.sin_addr.s_addr == 0L)
> > - session->ip_netmask.sin_addr.s_addr = 0xffffffffL;
> > - session->ip_address.sin_addr.s_addr &=
> > - session->ip_netmask.sin_addr.s_addr;
> > -
> > - if (req->pr_peer_address.ss_len > 0)
> > - memcpy(&session->peer, &req->pr_peer_address,
> > - MIN(req->pr_peer_address.ss_len, sizeof(session->peer)));
> > - if (req->pr_local_address.ss_len > 0)
> > - memcpy(&session->local, &req->pr_local_address,
> > - MIN(req->pr_local_address.ss_len, sizeof(session->local)));
> > -#ifdef PIPEX_PPPOE
> > - if (req->pr_protocol == PIPEX_PROTO_PPPOE)
> > - session->proto.pppoe.over_ifidx = over_ifp->if_index;
> > -#endif
> > -#ifdef PIPEX_PPTP
> > - if (req->pr_protocol == PIPEX_PROTO_PPTP) {
> > - struct pipex_pptp_session *sess_pptp = &session->proto.pptp;
> > -
> > - sess_pptp->snd_gap = 0;
> > - sess_pptp->rcv_gap = 0;
> > - sess_pptp->snd_una = req->pr_proto.pptp.snd_una;
> > - sess_pptp->snd_nxt = req->pr_proto.pptp.snd_nxt;
> > - sess_pptp->rcv_nxt = req->pr_proto.pptp.rcv_nxt;
> > - sess_pptp->rcv_acked = req->pr_proto.pptp.rcv_acked;
> > -
> > - sess_pptp->winsz = req->pr_proto.pptp.winsz;
> > - sess_pptp->maxwinsz = req->pr_proto.pptp.maxwinsz;
> > - sess_pptp->peer_maxwinsz = req->pr_proto.pptp.peer_maxwinsz;
> > - /* last ack number */
> > - sess_pptp->ul_snd_una = sess_pptp->snd_una - 1;
> > - }
> > -#endif
> > -#ifdef PIPEX_L2TP
> > - if (req->pr_protocol == PIPEX_PROTO_L2TP) {
> > - struct pipex_l2tp_session *sess_l2tp = &session->proto.l2tp;
> > -
> > - /* session keys */
> > - sess_l2tp->tunnel_id = req->pr_proto.l2tp.tunnel_id;
> > - sess_l2tp->peer_tunnel_id = req->pr_proto.l2tp.peer_tunnel_id;
> > -
> > - /* protocol options */
> > - sess_l2tp->option_flags = req->pr_proto.l2tp.option_flags;
> > -
> > - /* initial state of dynamic context */
> > - sess_l2tp->ns_gap = sess_l2tp->nr_gap = 0;
> > - sess_l2tp->ns_nxt = req->pr_proto.l2tp.ns_nxt;
> > - sess_l2tp->nr_nxt = req->pr_proto.l2tp.nr_nxt;
> > - sess_l2tp->ns_una = req->pr_proto.l2tp.ns_una;
> > - sess_l2tp->nr_acked = req->pr_proto.l2tp.nr_acked;
> > - /* last ack number */
> > - sess_l2tp->ul_ns_una = sess_l2tp->ns_una - 1;
> > - }
> > -#endif
> > -#ifdef PIPEX_MPPE
> > - if ((req->pr_ppp_flags & PIPEX_PPP_MPPE_ACCEPTED) != 0)
> > - pipex_session_init_mppe_recv(session,
> > - req->pr_mppe_recv.stateless, req->pr_mppe_recv.keylenbits,
> > - req->pr_mppe_recv.master_key);
> > - if ((req->pr_ppp_flags & PIPEX_PPP_MPPE_ENABLED) != 0)
> > - pipex_session_init_mppe_send(session,
> > - req->pr_mppe_send.stateless, req->pr_mppe_send.keylenbits,
> > - req->pr_mppe_send.master_key);
> > -
> > - if (pipex_session_is_mppe_required(session)) {
> > - if (!pipex_session_is_mppe_enabled(session) ||
> > - !pipex_session_is_mppe_accepted(session)) {
> > - pool_put(pppx_if_pl, pxi);
> > - return (EINVAL);
> > - }
> > - }
> > -#endif
> > -
> > /* try to set the interface up */
> > rw_enter_write(&pppx_ifs_lk);
> > unit = pppx_if_next_unit();
> > if (unit < 0) {
> > - pool_put(pppx_if_pl, pxi);
> > - error = ENOMEM;
> > rw_exit_write(&pppx_ifs_lk);
> > - goto out;
> > + error = ENOMEM;
> > + goto err_free_iface;
> > }
> >
> > pxi->pxi_unit = unit;
> > @@ -833,10 +687,9 @@ pppx_add_session(struct pppx_dev *pxd, s
> >
> > /* this is safe without splnet since we're not modifying it */
> > if (RBT_FIND(pppx_ifs, &pppx_ifs, pxi) != NULL) {
> > - pool_put(pppx_if_pl, pxi);
> > - error = EADDRINUSE;
> > rw_exit_write(&pppx_ifs_lk);
> > - goto out;
> > + error = EADDRINUSE;
> > + goto err_free_iface;
> > }
> >
> > if (RBT_INSERT(pppx_ifs, &pppx_ifs, pxi) != NULL)
> > @@ -856,24 +709,21 @@ pppx_add_session(struct pppx_dev *pxd, s
> > ifp->if_softc = pxi;
> > /* ifp->if_rdomain = req->pr_rdomain; */
> >
> > - /* hook up pipex context */
> > - chain = PIPEX_ID_HASHTABLE(session->session_id);
> > - LIST_INSERT_HEAD(chain, session, id_chain);
> > - LIST_INSERT_HEAD(&pipex_session_list, session, session_list);
> > -#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> > - switch (req->pr_protocol) {
> > - case PIPEX_PROTO_PPTP:
> > - case PIPEX_PROTO_L2TP:
> > - chain = PIPEX_PEER_ADDR_HASHTABLE(
> > - pipex_sockaddr_hash_key(&session->peer.sa));
> > - LIST_INSERT_HEAD(chain, session, peer_addr_chain);
> > - break;
> > + /* pipex interface context */
> > + pipex_iface_init(&pxi->pxi_ifcontext, ifp);
> > + pipex_iface_start(&pxi->pxi_ifcontext);
> > +
> > + error = pipex_add_session(req, &pxi->pxi_ifcontext);
> > + if (error) {
> > + goto err_free_pipex;
> > }
> > -#endif
> >
> > - /* if first session is added, start timer */
> > - if (LIST_NEXT(session, session_list) == NULL)
> > - pipex_timer_start();
> > + pxi->pxi_session = pipex_lookup_by_session_id(
> > + pxi->pxi_key.pxik_protocol, pxi->pxi_key.pxik_session_id);
> > + if (pxi->pxi_session == NULL) {
> > + error = EINVAL;
> > + goto err_free_pipex;
> > + }
> >
> > /* XXXSMP breaks atomicity */
> > NET_UNLOCK();
> > @@ -928,7 +778,20 @@ pppx_add_session(struct pppx_dev *pxd, s
> > pxi->pxi_ready = 1;
> > rw_exit_write(&pppx_ifs_lk);
> >
> > -out:
> > + return (error);
> > +
> > +err_free_pipex:
> > + NET_UNLOCK();
> > + pipex_iface_fini(&pxi->pxi_ifcontext);
> > + NET_LOCK();
> > +err_free_iface:
> > + rw_enter_write(&pppx_ifs_lk);
> > + if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
> > + panic("%s: pppx_ifs modified while lock was held", __func__);
> > + LIST_REMOVE(pxi, pxi_list);
> > + rw_exit_write(&pppx_ifs_lk);
> > + pool_put(pppx_if_pl, pxi);
> > +
> > return (error);
> > }
> >
> > @@ -941,7 +804,7 @@ pppx_del_session(struct pppx_dev *pxd, s
> > if (pxi == NULL)
> > return (EINVAL);
> >
> > - req->pcr_stat = pxi->pxi_session.stat;
> > + req->pcr_stat = pxi->pxi_session->stat;
> >
> > pppx_if_destroy(pxd, pxi);
> > return (0);
> > @@ -970,29 +833,19 @@ pppx_if_destroy(struct pppx_dev *pxd, st
> > struct pipex_session *session;
> >
> > NET_ASSERT_LOCKED();
> > - session = &pxi->pxi_session;
> > + session = pxi->pxi_session;
> > ifp = &pxi->pxi_if;
> >
> > - LIST_REMOVE(session, id_chain);
> > - LIST_REMOVE(session, session_list);
> > -#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
> > - switch (session->protocol) {
> > - case PIPEX_PROTO_PPTP:
> > - case PIPEX_PROTO_L2TP:
> > - LIST_REMOVE(session, peer_addr_chain);
> > - break;
> > - }
> > -#endif
> > -
> > - /* if final session is destroyed, stop timer */
> > - if (LIST_EMPTY(&pipex_session_list))
> > - pipex_timer_stop();
> > -
> > /* XXXSMP breaks atomicity */
> > NET_UNLOCK();
> > if_detach(ifp);
> > NET_LOCK();
> >
> > + pipex_destroy_session(session);
> > + NET_UNLOCK();
> > + pipex_iface_fini(&pxi->pxi_ifcontext);
> > + NET_LOCK();
> > +
> > rw_enter_write(&pppx_ifs_lk);
> > if (RBT_REMOVE(pppx_ifs, &pppx_ifs, pxi) == NULL)
> > panic("%s: pppx_ifs modified while lock was held", __func__);
> > @@ -1024,7 +877,7 @@ pppx_if_start(struct ifnet *ifp)
> > ifp->if_obytes += m->m_pkthdr.len;
> > ifp->if_opackets++;
> >
> > - pipex_ppp_output(m, &pxi->pxi_session, proto);
> > + pipex_ppp_output(m, pxi->pxi_session, proto);
> > }
> > }
> >
> > @@ -1084,7 +937,7 @@ pppx_if_output(struct ifnet *ifp, struct
> > }
> > th = mtod(m, struct pppx_hdr *);
> > th->pppx_proto = 0; /* not used */
> > - th->pppx_id = pxi->pxi_session.ppp_id;
> > + th->pppx_id = pxi->pxi_session->ppp_id;
> > rw_enter_read(&pppx_devs_lk);
> > error = mq_enqueue(&pxi->pxi_dev->pxd_svcq, m);
> > if (error == 0) {
> > @@ -1123,7 +976,7 @@ pppx_if_ioctl(struct ifnet *ifp, u_long
> >
> > case SIOCSIFMTU:
> > if (ifr->ifr_mtu < 512 ||
> > - ifr->ifr_mtu > pxi->pxi_session.peer_mru)
> > + ifr->ifr_mtu > pxi->pxi_session->peer_mru)
> > error = EINVAL;
> > else
> > ifp->if_mtu = ifr->ifr_mtu;
> >
>
