Hello Sergey. I am not the developer, but I works in pipex(4) layer too. Also mpi@ wants I did rewiev for your diffs.
On Mon, May 04, 2020 at 10:03:40PM +0300, Sergey Ryazanov wrote: > Split checks from frame accepting with header removing in the common > PPP input function. This should fix packet capture on a PPP interfaces. Can you describe the problem you fix? As mpi@ pointed to me, reviewers are stupid and have no telepathy skills :) Also your diff does two different things: (1) split frames checks and input, and (2) modify frames passing to bpf(4). I guess you should split your diff by two different. I did some inlined comments below. > > Also forbid IP/IPv6 frames (without PPP header) passing to BPF on > PPP interfaces to avoid mess. > > Initialy this change was made as a part of pipex(4) and ppp(4) > integration work. But, since this change make the core a bit more clear > I would like to publish it now. > > Ok? > > --- > sys/net/pipex.c | 95 ++++++++++++++++++++++++++++--------------------- > 1 file changed, 54 insertions(+), 41 deletions(-) > > diff --git sys/net/pipex.c sys/net/pipex.c > index c433e4beaa6..e0066a61598 100644 > --- sys/net/pipex.c > +++ sys/net/pipex.c > @@ -970,41 +970,68 @@ drop: > Static void > pipex_ppp_input(struct mbuf *m0, struct pipex_session *session, int > decrypted) > { > - int proto, hlen = 0; > + int proto, hlen = 0, align = 0; > struct mbuf *n; > > KASSERT(m0->m_pkthdr.len >= PIPEX_PPPMINLEN); > proto = pipex_ppp_proto(m0, session, 0, &hlen); > + switch (proto) { > + case PPP_IP: > + if (session->ip_forward == 0) > + goto drop; > + if (!decrypted && pipex_session_is_mppe_required(session)) > + /* > + * if ip packet received when mppe > + * is required, discard it. > + */ > + goto drop; > + align = 1; > + break; > +#ifdef INET6 > + case PPP_IPV6: > + if (session->ip6_forward == 0) > + goto drop; > + if (!decrypted && pipex_session_is_mppe_required(session)) > + /* > + * if ip packet received when mppe > + * is required, discard it. > + */ > + goto drop; > + align = 1; > + break; > +#endif > #ifdef PIPEX_MPPE > - if (proto == PPP_COMP) { > + case PPP_COMP: > if (decrypted) > goto drop; > > /* checked this on ppp_common_input() already. */ > KASSERT(pipex_session_is_mppe_accepted(session)); > - > - m_adj(m0, hlen); > - pipex_mppe_input(m0, session); > - return; > - } > - if (proto == PPP_CCP) { > + break; > + case PPP_CCP: > if (decrypted) > goto drop; > + break; > +#endif > + default: > + if (decrypted) > + goto drop; > + /* protocol must be checked on pipex_common_input() already */ > + KASSERT(0); > + goto drop; > + } > > #if NBPFILTER > 0 > - { > + { > struct ifnet *ifp = session->pipex_iface->ifnet_this; > + > if (ifp->if_bpf && ifp->if_type == IFT_PPP) > bpf_mtap(ifp->if_bpf, m0, BPF_DIRECTION_IN); > - } > -#endif > - m_adj(m0, hlen); > - pipex_ccp_input(m0, session); > - return; > } > #endif For INET[46] cases you can align frame after it's passed to bpf(4). Should this frame be aligned before passing to bpf(4)? > + > m_adj(m0, hlen); > - if (!ALIGNED_POINTER(mtod(m0, caddr_t), uint32_t)) { > + if (align && !ALIGNED_POINTER(mtod(m0, caddr_t), uint32_t)) { > n = m_dup_pkt(m0, 0, M_NOWAIT); > if (n == NULL) > goto drop; > @@ -1014,35 +1041,21 @@ pipex_ppp_input(struct mbuf *m0, struct pipex_session > *session, int decrypted) > > switch (proto) { > case PPP_IP: > - if (session->ip_forward == 0) > - goto drop; > - if (!decrypted && pipex_session_is_mppe_required(session)) > - /* > - * if ip packet received when mppe > - * is required, discard it. > - */ > - goto drop; > pipex_ip_input(m0, session); > - return; > + break; > #ifdef INET6 > case PPP_IPV6: > - if (session->ip6_forward == 0) > - goto drop; > - if (!decrypted && pipex_session_is_mppe_required(session)) > - /* > - * if ip packet received when mppe > - * is required, discard it. > - */ > - goto drop; > pipex_ip6_input(m0, session); > - return; > + break; > +#endif > +#ifdef PIPEX_MPPE > + case PPP_COMP: > + pipex_mppe_input(m0, session); > + break; > + case PPP_CCP: > + pipex_ccp_input(m0, session); > + break; > #endif > - default: > - if (decrypted) > - goto drop; > - /* protocol must be checked on pipex_common_input() already */ > - KASSERT(0); > - goto drop; Hipotethically, someone inncaurate can introduce memory leak here in future. I like to keep default case with assertion or with "goto drop". > } > > return; > @@ -1105,7 +1118,7 @@ pipex_ip_input(struct mbuf *m0, struct pipex_session > *session) > len = m0->m_pkthdr.len; > > #if NBPFILTER > 0 > - if (ifp->if_bpf) > + if (ifp->if_bpf && ifp->if_type != IFT_PPP) > bpf_mtap_af(ifp->if_bpf, AF_INET, m0, BPF_DIRECTION_IN); > #endif > > @@ -1151,7 +1164,7 @@ pipex_ip6_input(struct mbuf *m0, struct pipex_session > *session) > len = m0->m_pkthdr.len; > > #if NBPFILTER > 0 > - if (ifp->if_bpf) > + if (ifp->if_bpf && ifp->if_type != IFT_PPP) > bpf_mtap_af(ifp->if_bpf, AF_INET6, m0, BPF_DIRECTION_IN); > #endif > > -- > 2.26.0 >