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.