2 Hi Vitaliy,
On Tue, May 19, 2020 at 12:11 PM Vitaliy Makkoveev
<henscheltig...@yahoo.com> wrote:
> 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 :)
When I tried to capture packets on a ppp (4) interface (with pipex
activated), I noticed that all the PPP CCP frames were ok, but all the
ingress PPP IP frames were mangled, and they did not contain the PPP
header at all.
Then I started digging the code and came across the fact that pipex
has two packet capture points. One place is in pipex_ip_input(), where
pure IP packets (without a PPP header) are passed to bpf, and the
other is in pipex_ppp_input(), where only CCP frames are passed to bpf
and only if the associated network interface is of PPP type.
My first thought was to add another one hook to the pipex_ppp_input()
to intercept IP frames before the PPP header stripping. But then I
spotted that pipex_ppp_input() have a common pattern in the frames
processing, but it was poorly expressed since before the proposed
change the code is looks like this (in pseudo code):
if COMP
COMP sanity checks
ppp header stripping <- duplicate #1
call MPPE handler
exit
if CCP
CCP sanity checks
bpf call
ppp header stripping <- duplicate #2
call MPPE handler
exit
ppp header stripping <- duplicate #3
packet data alignment
if IP
IP sanity checks
call IP handler
exit
if IPv6
IPv6 sanity checks
call IPv6 handler
exit
if unknown PPP frame type
produce a big fat error message
As you can see for each frame type the code performs a specific sanity
check, strips the PPP header and then call a frame specific handler.
Also CCP frames on a ppp(4) interface are passed to bpf.
So I rearrange the code to make I a bit more clear:
* group sanity checks in the beginning of the function;
* place bpf call for ppp(4) interfaces after sanity checks, but
_before_ PPP header stripping;
* place the header stripping common for all types with optional data alignment;
* group frame specific handler calls in the end of the function;
* use switch/case to help a compiler and human readers :)
Now the function code is looks like this:
if COMP
COMP sanity checks
if CCP
CCP sanity checks
if IP
IP sanity checks and prepare for alignment
if IPv6
IPv6 sanity checks and prepare for alignment
if unknown PPP frame type
produce a big fat error message
call bpf for ppp(4) interfaces while frame still has the PPP header
strip the ppp header and perform optional alignment
if COMP
COMP sanity checks
if CCP
CCP sanity checks
if IP
IP sanity checks and prepare for alignment
if IPv6
IPv6 sanity checks and prepare for alignment
Diff is just unable to express such change in a human friendly way :(
It is better to look at the code before diff applying and after to see
the change.
The final hank just blocks stripped IP packets passing to bpf on
ppp(4) network interfaces to avoid duplicate (and mangled) packets in
a dump.
So, after the change we will get: function behaves as early, tcpdump
perfectly sniffs on a ppp interface with activated pipex, checks are
performed before expensive packet modifications, and function code is
readable and now it is clear what exactly function do for each type of
frames.
Oh, also pipex.c become shorter by the 13 lines.
> 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.
The bpf call modifications just prevent frame duplication in the dump.
So I think Its better to keep them here.
Buf if someone is against such wide modifications, then I am Ok to
send two patches: one for mangled packets dumping prevention and one
for PPP input rework.
> 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)?
We definitely need to align IP packets after the PPP header stripping
to facilitate work of other networking code. But aligning of the PPP
frame could cause a double alignment: before and after the PPP header
stripping, since a PPP header is not always have a proper length. So I
think that aligning before the PPP header stripping could be too
expensive and we do not required to do it.
> > +
> > 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;
[skipped]
> > - 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".
This code was moved to the beginning of the function. So it is still
will us. Or did you mean that we should handle unknown frame types in
the second switch too?
--
Sergey
