On Wed, May 20, 2020 at 04:08:01AM +0300, Sergey Ryazanov wrote:
> 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.
This time only pppx(4) and pppac(4) have pipex(4) support. I don't see
packet capture problems on them. Can you catch and share how to
reproduce this problem with pppx(4) or pppac(4)?
Also did you test your diff with pppx(4) and pppac(4)? I got this
---- tcpdump output begin ----
[otto@obsd-test ~]$ doas tcpdump -n -i pppx0
doas (otto@obsd-test.local) password:
tcpdump: listening on pppx0, link-type LOOP
21:49:12.138973
21:49:12.138988 10.0.0.1 > 10.0.0.247: icmp: echo reply
21:49:13.144026
21:49:13.144044 10.0.0.1 > 10.0.0.247: icmp: echo reply
21:49:14.145388
21:49:14.145403 10.0.0.1 > 10.0.0.247: icmp: echo reply
21:49:15.148127
21:49:15.148144 10.0.0.1 > 10.0.0.247: icmp: echo reply
21:49:16.150694
21:49:16.150710 10.0.0.1 > 10.0.0.247: icmp: echo reply
21:49:17.153204
21:49:17.153222 10.0.0.1 > 10.0.0.247: icmp: echo reply
21:49:18.155404
21:49:18.155422 10.0.0.1 > 10.0.0.247: icmp: echo reply
21:53:53.609206
21:53:53.609555
21:53:53.615620 192.168.0.1.53 > 10.0.0.247.61412: 56381 2/0/0 A
173.194.222.109, A 173.194.222.108(63)
---- tcpdump output end ----
with your diff applied.
>
> [Skip]
>
> 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.
Of course I applied and run your diff before answer to you :)
>
> [Skip]
>
> > 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.
Yes. I guess "one change - one diff" is the best way.
>
> [Skip]
>
> > 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.
No. Look at net/pipex.c:1023-1041 with your diff applied:
---- cut begin ----
#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);
if (align && !ALIGNED_POINTER(mtod(m0, caddr_t), uint32_t)) {
n = m_dup_pkt(m0, 0, M_NOWAIT);
if (n == NULL)
goto drop;
m_freem(m0);
m0 = n;
}
---- cut end ----
You 1. pass frame to bpf_mtap() 2. align this frame if it is requred. Are
you shure this is the right order?
>
> [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?
I like to keep default case in second switch too.
