Re: [PATCH] pipex(4): rework PPP input
Sorry for delayed reply. On Wed, 27 May 2020 01:29:36 +0300 Sergey Ryazanov wrote: > On Tue, May 26, 2020 at 12:07 PM Vitaliy Makkoveev > wrote: >>> On 25 May 2020, at 22:04, Sergey Ryazanov wrote: >>> On Sat, May 23, 2020 at 3:07 PM Vitaliy Makkoveev >>> wrote: For example, each pipex session should have unique pair of `protocol’ and `session_id’. These values are passed from userland. While the only instance of npppd(8) uses pipex(4) this is not the problem. But you introduce the case while pipex(4) will be used by multiple independent userland programs. At least, I have interest how you handle this. >>> >>> This should not be a problem here. npppd(8) support server mode only. >>> While my work is to implement acceleration for client side of L2TP >>> connection. >> >> I guess they can coexist. Also you can have multiple connections to >> ppp servers simultaneously. > > With 16 bits long session id field, according to birthday problem to > reach 0.9 collision probability I need 549 simultaneous sessions. > Should I still be worried or I have a time to complete integration > work and then update UDP filter for love of the game? usr.sbin/npppd/l2tp/l2tp_local.h 79 #define L2TP_SESSION_ID_MASK0x7fff npppd uses 0-32767
Re: [PATCH] pipex(4): rework PPP input
> On 27 May 2020, at 01:29, Sergey Ryazanov wrote: > > On Tue, May 26, 2020 at 12:07 PM Vitaliy Makkoveev > wrote: >>> On 25 May 2020, at 22:04, Sergey Ryazanov wrote: >>> On Sat, May 23, 2020 at 3:07 PM Vitaliy Makkoveev >>> wrote: For example, each pipex session should have unique pair of `protocol’ and `session_id’. These values are passed from userland. While the only instance of npppd(8) uses pipex(4) this is not the problem. But you introduce the case while pipex(4) will be used by multiple independent userland programs. At least, I have interest how you handle this. >>> >>> This should not be a problem here. npppd(8) support server mode only. >>> While my work is to implement acceleration for client side of L2TP >>> connection. >> >> I guess they can coexist. Also you can have multiple connections to >> ppp servers simultaneously. > > With 16 bits long session id field, according to birthday problem to > reach 0.9 collision probability I need 549 simultaneous sessions. > Should I still be worried or I have a time to complete integration > work and then update UDP filter for love of the game? Why do you ask me? Do what you want. > > -- > Sergey >
Re: [PATCH] pipex(4): rework PPP input
On Wed, May 27, 2020 at 2:12 AM Vitaliy Makkoveev wrote: > > On 27 May 2020, at 01:29, Sergey Ryazanov wrote: > > On Tue, May 26, 2020 at 12:07 PM Vitaliy Makkoveev > > wrote: > >>> On 25 May 2020, at 22:04, Sergey Ryazanov wrote: > >>> On Sat, May 23, 2020 at 3:07 PM Vitaliy Makkoveev > >>> wrote: > For example, each pipex session should have unique pair of `protocol’ and > `session_id’. These values are passed from userland. While the only > instance of npppd(8) uses pipex(4) this is not the problem. But you > introduce the case while pipex(4) will be used by multiple independent > userland programs. At least, I have interest how you handle this. > >>> > >>> This should not be a problem here. npppd(8) support server mode only. > >>> While my work is to implement acceleration for client side of L2TP > >>> connection. > >> > >> I guess they can coexist. Also you can have multiple connections to > >> ppp servers simultaneously. > > > > With 16 bits long session id field, according to birthday problem to > > reach 0.9 collision probability I need 549 simultaneous sessions. > > Should I still be worried or I have a time to complete integration > > work and then update UDP filter for love of the game? > > Why do you ask me? Do what you want. I want to prioritize my todo list, and I in need of advice, is possible collision is non-critical at the moment, or I missed something and should solve it first? -- Serge
Re: [PATCH] pipex(4): rework PPP input
On Tue, May 26, 2020 at 12:07 PM Vitaliy Makkoveev wrote: >> On 25 May 2020, at 22:04, Sergey Ryazanov wrote: >> On Sat, May 23, 2020 at 3:07 PM Vitaliy Makkoveev >> wrote: >>> For example, each pipex session should have unique pair of `protocol’ and >>> `session_id’. These values are passed from userland. While the only >>> instance of npppd(8) uses pipex(4) this is not the problem. But you >>> introduce the case while pipex(4) will be used by multiple independent >>> userland programs. At least, I have interest how you handle this. >> >> This should not be a problem here. npppd(8) support server mode only. >> While my work is to implement acceleration for client side of L2TP >> connection. > > I guess they can coexist. Also you can have multiple connections to > ppp servers simultaneously. With 16 bits long session id field, according to birthday problem to reach 0.9 collision probability I need 549 simultaneous sessions. Should I still be worried or I have a time to complete integration work and then update UDP filter for love of the game? -- Sergey
Re: [PATCH] pipex(4): rework PPP input
> On 25 May 2020, at 22:04, Sergey Ryazanov wrote: > > Hello Vitaliy, > > On Sat, May 23, 2020 at 3:07 PM Vitaliy Makkoveev > wrote: >>> On 23 May 2020, at 13:11, Sergey Ryazanov wrote: >>> On Wed, May 20, 2020 at 10:13 PM Vitaliy Makkoveev >>> wrote: On Wed, May 20, 2020 at 04:08:01AM +0300, Sergey Ryazanov wrote: > On Tue, May 19, 2020 at 12:11 PM Vitaliy Makkoveev > 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. >>> >>> Yes, and as I wrote in the first mail, now I am working on ppp(4) & >>> pipex(4) integration to speed up client side of L2TP. >> >> May be you can share you work? Not for commit, but for feedback. >> > > I send a couple of diffs in separate mails. First change is for ppp(4) > to support pipex(4) acceleration in DL path. Second diff adds a new > option for pppd to control pipex activation. > > We will need also to update xl2tpd package to teach it how to > configure pppd for pipex usage. This is quite simple change, but it > still require some cleanup so I do not publish it yet. > >> For example, each pipex session should have unique pair of `protocol’ and >> `session_id’. These values are passed from userland. While the only >> instance of npppd(8) uses pipex(4) this is not the problem. But you >> introduce the case while pipex(4) will be used by multiple independent >> userland programs. At least, I have interest how you handle this. > > This should not be a problem here. npppd(8) support server mode only. > While my work is to implement acceleration for client side of L2TP > connection. I guess they can coexist. Also you can have multiple connections to ppp servers simultaneously.
Re: [PATCH] pipex(4): rework PPP input
Hello Vitaliy, On Sat, May 23, 2020 at 3:07 PM Vitaliy Makkoveev wrote: > > On 23 May 2020, at 13:11, Sergey Ryazanov wrote: > > On Wed, May 20, 2020 at 10:13 PM Vitaliy Makkoveev > > wrote: > >> On Wed, May 20, 2020 at 04:08:01AM +0300, Sergey Ryazanov wrote: > >>> On Tue, May 19, 2020 at 12:11 PM Vitaliy Makkoveev > >>> 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. > > > > Yes, and as I wrote in the first mail, now I am working on ppp(4) & > > pipex(4) integration to speed up client side of L2TP. > > May be you can share you work? Not for commit, but for feedback. > I send a couple of diffs in separate mails. First change is for ppp(4) to support pipex(4) acceleration in DL path. Second diff adds a new option for pppd to control pipex activation. We will need also to update xl2tpd package to teach it how to configure pppd for pipex usage. This is quite simple change, but it still require some cleanup so I do not publish it yet. > For example, each pipex session should have unique pair of `protocol’ and > `session_id’. These values are passed from userland. While the only > instance of npppd(8) uses pipex(4) this is not the problem. But you > introduce the case while pipex(4) will be used by multiple independent > userland programs. At least, I have interest how you handle this. This should not be a problem here. npppd(8) support server mode only. While my work is to implement acceleration for client side of L2TP connection. Anyway if a session collision become a case, we could additionally demux ingress traffic by the destination UDP port. -- Sergey
Re: [PATCH] pipex(4): rework PPP input
> On 23 May 2020, at 13:11, Sergey Ryazanov wrote: > > Hello, > > On Wed, May 20, 2020 at 10:13 PM Vitaliy Makkoveev > wrote: >> On Wed, May 20, 2020 at 04:08:01AM +0300, Sergey Ryazanov wrote: >>> On Tue, May 19, 2020 at 12:11 PM Vitaliy Makkoveev >>> 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. > > Yes, and as I wrote in the first mail, now I am working on ppp(4) & > pipex(4) integration to speed up client side of L2TP. May be you can share you work? Not for commit, but for feedback. For example, each pipex session should have unique pair of `protocol’ and `session_id’. These values are passed from userland. While the only instance of npppd(8) uses pipex(4) this is not the problem. But you introduce the case while pipex(4) will be used by multiple independent userland programs. At least, I have interest how you handle this. > >> 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 am entirely missed the fact that pppx(4) also have IFT_PPP type. > Thank you for pointing me. > > I will recheck pppx(4) work and return as soon as I will have a better > solution. Not only. Since you modify pipex(4) you should check pppac(4) too.
Re: [PATCH] pipex(4): rework PPP input
Hello, On Wed, May 20, 2020 at 10:13 PM Vitaliy Makkoveev wrote: > On Wed, May 20, 2020 at 04:08:01AM +0300, Sergey Ryazanov wrote: > > On Tue, May 19, 2020 at 12:11 PM Vitaliy Makkoveev > > 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. Yes, and as I wrote in the first mail, now I am working on ppp(4) & pipex(4) integration to speed up client side of L2TP. > 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 am entirely missed the fact that pppx(4) also have IFT_PPP type. Thank you for pointing me. I will recheck pppx(4) work and return as soon as I will have a better solution. -- Sergey
Re: [PATCH] pipex(4): rework PPP input
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
> 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.
Re: [PATCH] pipex(4): rework PPP input
2 Hi Vitaliy,
On Tue, May 19, 2020 at 12:11 PM Vitaliy Makkoveev
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 =
Re: [PATCH] pipex(4): rework PPP input
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, );
> + 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.
>
[PATCH] pipex(4): rework PPP input
Split checks from frame accepting with header removing in the common
PPP input function. This should fix packet capture on a PPP interfaces.
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, );
+ 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
+
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;
}
return;
@@ -1105,7 +1118,7 @@ pipex_ip_input(struct mbuf *m0, struct pipex_session
*session)
len =
