Re: IPv6 forwarding path without KERNEL_LOCK
On 2016/07/06 14:43, Alexander Bluhm wrote:
> On Tue, Jul 05, 2016 at 12:22:36PM +0200, Martin Pieuchot wrote:
> > Fine, new diff doing that.
>
> OK bluhm@
I've been running with this, diff looks sane and no problems noticed yet.
Reading the diff reminded me..if anyone is interested in digging there are
some problems with hbh on OpenBSD that Antonios Atlasis found, some are
written up at http://www.secfu.net/
> > Index: netinet6/ip6_input.c
> > ===
> > RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> > retrieving revision 1.161
> > diff -u -p -r1.161 ip6_input.c
> > --- netinet6/ip6_input.c5 Jul 2016 10:17:14 - 1.161
> > +++ netinet6/ip6_input.c5 Jul 2016 10:21:10 -
> > @@ -122,6 +122,7 @@ struct ip6stat ip6stat;
> > void ip6_init2(void *);
> > int ip6_check_rh0hdr(struct mbuf *, int *);
> >
> > +int ip6_hbhchcheck(struct mbuf *, int *, int *, int *);
> > int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
> > struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
> >
> > @@ -192,7 +193,6 @@ ip6_input(struct mbuf *m)
> > struct ip6_hdr *ip6;
> > int off, nest;
> > u_int16_t src_scope, dst_scope;
> > - u_int32_t plen, rtalert = ~0;
> > int nxt, ours = 0;
> > #if NPF > 0
> > struct in6_addr odst;
> > @@ -495,78 +495,15 @@ ip6_input(struct mbuf *m)
> > }
> >
> >hbhcheck:
> > - /*
> > -* Process Hop-by-Hop options header if it's contained.
> > -* m may be modified in ip6_hopopts_input().
> > -* If a JumboPayload option is included, plen will also be modified.
> > -*/
> > - plen = (u_int32_t)ntohs(ip6->ip6_plen);
> > - off = sizeof(struct ip6_hdr);
> > - if (ip6->ip6_nxt == IPPROTO_HOPOPTS) {
> > - struct ip6_hbh *hbh;
> > -
> > - if (ip6_hopopts_input(&plen, &rtalert, &m, &off)) {
> > - if_put(ifp);
> > - return; /* m have already been freed */
> > - }
> > -
> > - /* adjust pointer */
> > - ip6 = mtod(m, struct ip6_hdr *);
> > -
> > - /*
> > -* if the payload length field is 0 and the next header field
> > -* indicates Hop-by-Hop Options header, then a Jumbo Payload
> > -* option MUST be included.
> > -*/
> > - if (ip6->ip6_plen == 0 && plen == 0) {
> > - /*
> > -* Note that if a valid jumbo payload option is
> > -* contained, ip6_hopopts_input() must set a valid
> > -* (non-zero) payload length to the variable plen.
> > -*/
> > - ip6stat.ip6s_badoptions++;
> > - icmp6_error(m, ICMP6_PARAM_PROB,
> > - ICMP6_PARAMPROB_HEADER,
> > - (caddr_t)&ip6->ip6_plen - (caddr_t)ip6);
> > - if_put(ifp);
> > - return;
> > - }
> > - IP6_EXTHDR_GET(hbh, struct ip6_hbh *, m, sizeof(struct ip6_hdr),
> > - sizeof(struct ip6_hbh));
> > - if (hbh == NULL) {
> > - ip6stat.ip6s_tooshort++;
> > - if_put(ifp);
> > - return;
> > - }
> > - nxt = hbh->ip6h_nxt;
> >
> > - /*
> > -* accept the packet if a router alert option is included
> > -* and we act as an IPv6 router.
> > -*/
> > - if (rtalert != ~0 && ip6_forwarding)
> > - ours = 1;
> > - } else
> > - nxt = ip6->ip6_nxt;
> > -
> > - /*
> > -* Check that the amount of data in the buffers
> > -* is as at least much as the IPv6 header would have us expect.
> > -* Trim mbufs if longer than we expect.
> > -* Drop packet if shorter than we expect.
> > -*/
> > - if (m->m_pkthdr.len - sizeof(struct ip6_hdr) < plen) {
> > - ip6stat.ip6s_tooshort++;
> > - goto bad;
> > - }
> > - if (m->m_pkthdr.len > sizeof(struct ip6_hdr) + plen) {
> > - if (m->m_len == m->m_pkthdr.len) {
> > - m->m_len = sizeof(struct ip6_hdr) + plen;
> > - m->m_pkthdr.len = sizeof(struct ip6_hdr) + plen;
> > - } else
> > - m_adj(m, sizeof(struct ip6_hdr) + plen -
> > m->m_pkthdr.len);
> > + if (ip6_hbhchcheck(m, &off, &nxt, &ours)) {
> > + if_put(ifp);
> > + return; /* m have already been freed */
> > }
> >
> > + /* adjust pointer */
> > + ip6 = mtod(m, struct ip6_hdr *);
> > +
> > /*
> > * Forward if desirable.
> > */
> > @@ -638,6 +575,89 @@ ip6_input(struct mbuf *m)
> > bad:
> > if_put(ifp);
> > m_freem(m);
> > +}
> > +
> > +int
> > +ip6_hbhchcheck(struct mbuf *m, int *offp, int *nxtp, int *oursp)
> > +{
> > + struct ip6_hdr *ip6;
> > + u_int32_t plen, rtalert = ~0;
> > +
> > + ip6 =
Re: IPv6 forwarding path without KERNEL_LOCK
On Tue, Jul 05, 2016 at 12:22:36PM +0200, Martin Pieuchot wrote:
> Fine, new diff doing that.
OK bluhm@
> Index: netinet6/ip6_input.c
> ===
> RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.161
> diff -u -p -r1.161 ip6_input.c
> --- netinet6/ip6_input.c 5 Jul 2016 10:17:14 - 1.161
> +++ netinet6/ip6_input.c 5 Jul 2016 10:21:10 -
> @@ -122,6 +122,7 @@ struct ip6stat ip6stat;
> void ip6_init2(void *);
> int ip6_check_rh0hdr(struct mbuf *, int *);
>
> +int ip6_hbhchcheck(struct mbuf *, int *, int *, int *);
> int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
> struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
>
> @@ -192,7 +193,6 @@ ip6_input(struct mbuf *m)
> struct ip6_hdr *ip6;
> int off, nest;
> u_int16_t src_scope, dst_scope;
> - u_int32_t plen, rtalert = ~0;
> int nxt, ours = 0;
> #if NPF > 0
> struct in6_addr odst;
> @@ -495,78 +495,15 @@ ip6_input(struct mbuf *m)
> }
>
>hbhcheck:
> - /*
> - * Process Hop-by-Hop options header if it's contained.
> - * m may be modified in ip6_hopopts_input().
> - * If a JumboPayload option is included, plen will also be modified.
> - */
> - plen = (u_int32_t)ntohs(ip6->ip6_plen);
> - off = sizeof(struct ip6_hdr);
> - if (ip6->ip6_nxt == IPPROTO_HOPOPTS) {
> - struct ip6_hbh *hbh;
> -
> - if (ip6_hopopts_input(&plen, &rtalert, &m, &off)) {
> - if_put(ifp);
> - return; /* m have already been freed */
> - }
> -
> - /* adjust pointer */
> - ip6 = mtod(m, struct ip6_hdr *);
> -
> - /*
> - * if the payload length field is 0 and the next header field
> - * indicates Hop-by-Hop Options header, then a Jumbo Payload
> - * option MUST be included.
> - */
> - if (ip6->ip6_plen == 0 && plen == 0) {
> - /*
> - * Note that if a valid jumbo payload option is
> - * contained, ip6_hopopts_input() must set a valid
> - * (non-zero) payload length to the variable plen.
> - */
> - ip6stat.ip6s_badoptions++;
> - icmp6_error(m, ICMP6_PARAM_PROB,
> - ICMP6_PARAMPROB_HEADER,
> - (caddr_t)&ip6->ip6_plen - (caddr_t)ip6);
> - if_put(ifp);
> - return;
> - }
> - IP6_EXTHDR_GET(hbh, struct ip6_hbh *, m, sizeof(struct ip6_hdr),
> - sizeof(struct ip6_hbh));
> - if (hbh == NULL) {
> - ip6stat.ip6s_tooshort++;
> - if_put(ifp);
> - return;
> - }
> - nxt = hbh->ip6h_nxt;
>
> - /*
> - * accept the packet if a router alert option is included
> - * and we act as an IPv6 router.
> - */
> - if (rtalert != ~0 && ip6_forwarding)
> - ours = 1;
> - } else
> - nxt = ip6->ip6_nxt;
> -
> - /*
> - * Check that the amount of data in the buffers
> - * is as at least much as the IPv6 header would have us expect.
> - * Trim mbufs if longer than we expect.
> - * Drop packet if shorter than we expect.
> - */
> - if (m->m_pkthdr.len - sizeof(struct ip6_hdr) < plen) {
> - ip6stat.ip6s_tooshort++;
> - goto bad;
> - }
> - if (m->m_pkthdr.len > sizeof(struct ip6_hdr) + plen) {
> - if (m->m_len == m->m_pkthdr.len) {
> - m->m_len = sizeof(struct ip6_hdr) + plen;
> - m->m_pkthdr.len = sizeof(struct ip6_hdr) + plen;
> - } else
> - m_adj(m, sizeof(struct ip6_hdr) + plen -
> m->m_pkthdr.len);
> + if (ip6_hbhchcheck(m, &off, &nxt, &ours)) {
> + if_put(ifp);
> + return; /* m have already been freed */
> }
>
> + /* adjust pointer */
> + ip6 = mtod(m, struct ip6_hdr *);
> +
> /*
>* Forward if desirable.
>*/
> @@ -638,6 +575,89 @@ ip6_input(struct mbuf *m)
> bad:
> if_put(ifp);
> m_freem(m);
> +}
> +
> +int
> +ip6_hbhchcheck(struct mbuf *m, int *offp, int *nxtp, int *oursp)
> +{
> + struct ip6_hdr *ip6;
> + u_int32_t plen, rtalert = ~0;
> +
> + ip6 = mtod(m, struct ip6_hdr *);
> +
> + /*
> + * Process Hop-by-Hop options header if it's contained.
> + * m may be modified in ip6_hopopts_input().
> + * If a JumboPayload option is included, plen will also be modified.
> + */
> + plen = (u_int32_t)ntohs(ip6->ip6_plen);
> + *offp = sizeof(struct ip6_hdr);
> + if (ip6->ip6_nxt == IPPROTO_HOPOPTS)
Re: IPv6 forwarding path without KERNEL_LOCK
On 04/07/16(Mon) 15:52, Alexander Bluhm wrote:
> On Mon, Jul 04, 2016 at 01:03:22PM +0200, Martin Pieuchot wrote:
> > + if (ip6_hbhchcheck(m, &off, &nxt, &ours)) {
> > + if_put(ifp);
> > + return; /* m have already been freed */
> > }
>
> As ip6_hbhchcheck() does ip6 = mtod(m, struct ip6_hdr *) after
> ip6_hopopts_input() you have to add this here, too.
>
> /* adjust pointer */
> ip6 = mtod(m, struct ip6_hdr *);
Updated thanks!
> > +int
> > +ip6_hbhchcheck(struct mbuf *m, int *offp, int *nxtp, int *oursp)
> > +{
> > + struct ip6_hdr *ip6;
> > + u_int32_t plen, rtalert = ~0;
> > + int ours, off, nxt;
>
> ours may be used uninitialized.
>
> > + *offp = off;
> > + *nxtp = nxt;
> > + *oursp = ours;
>
> I would prefer to use the passed values as *off, *nxt, *ours directly
> than to use another set of local variables. This also fixes
> initialization problem.
Fine, new diff doing that.
Index: netinet6/ip6_input.c
===
RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.161
diff -u -p -r1.161 ip6_input.c
--- netinet6/ip6_input.c5 Jul 2016 10:17:14 - 1.161
+++ netinet6/ip6_input.c5 Jul 2016 10:21:10 -
@@ -122,6 +122,7 @@ struct ip6stat ip6stat;
void ip6_init2(void *);
int ip6_check_rh0hdr(struct mbuf *, int *);
+int ip6_hbhchcheck(struct mbuf *, int *, int *, int *);
int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
@@ -192,7 +193,6 @@ ip6_input(struct mbuf *m)
struct ip6_hdr *ip6;
int off, nest;
u_int16_t src_scope, dst_scope;
- u_int32_t plen, rtalert = ~0;
int nxt, ours = 0;
#if NPF > 0
struct in6_addr odst;
@@ -495,78 +495,15 @@ ip6_input(struct mbuf *m)
}
hbhcheck:
- /*
-* Process Hop-by-Hop options header if it's contained.
-* m may be modified in ip6_hopopts_input().
-* If a JumboPayload option is included, plen will also be modified.
-*/
- plen = (u_int32_t)ntohs(ip6->ip6_plen);
- off = sizeof(struct ip6_hdr);
- if (ip6->ip6_nxt == IPPROTO_HOPOPTS) {
- struct ip6_hbh *hbh;
-
- if (ip6_hopopts_input(&plen, &rtalert, &m, &off)) {
- if_put(ifp);
- return; /* m have already been freed */
- }
-
- /* adjust pointer */
- ip6 = mtod(m, struct ip6_hdr *);
-
- /*
-* if the payload length field is 0 and the next header field
-* indicates Hop-by-Hop Options header, then a Jumbo Payload
-* option MUST be included.
-*/
- if (ip6->ip6_plen == 0 && plen == 0) {
- /*
-* Note that if a valid jumbo payload option is
-* contained, ip6_hopopts_input() must set a valid
-* (non-zero) payload length to the variable plen.
-*/
- ip6stat.ip6s_badoptions++;
- icmp6_error(m, ICMP6_PARAM_PROB,
- ICMP6_PARAMPROB_HEADER,
- (caddr_t)&ip6->ip6_plen - (caddr_t)ip6);
- if_put(ifp);
- return;
- }
- IP6_EXTHDR_GET(hbh, struct ip6_hbh *, m, sizeof(struct ip6_hdr),
- sizeof(struct ip6_hbh));
- if (hbh == NULL) {
- ip6stat.ip6s_tooshort++;
- if_put(ifp);
- return;
- }
- nxt = hbh->ip6h_nxt;
- /*
-* accept the packet if a router alert option is included
-* and we act as an IPv6 router.
-*/
- if (rtalert != ~0 && ip6_forwarding)
- ours = 1;
- } else
- nxt = ip6->ip6_nxt;
-
- /*
-* Check that the amount of data in the buffers
-* is as at least much as the IPv6 header would have us expect.
-* Trim mbufs if longer than we expect.
-* Drop packet if shorter than we expect.
-*/
- if (m->m_pkthdr.len - sizeof(struct ip6_hdr) < plen) {
- ip6stat.ip6s_tooshort++;
- goto bad;
- }
- if (m->m_pkthdr.len > sizeof(struct ip6_hdr) + plen) {
- if (m->m_len == m->m_pkthdr.len) {
- m->m_len = sizeof(struct ip6_hdr) + plen;
- m->m_pkthdr.len = sizeof(struct ip6_hdr) + plen;
- } else
- m_adj(m, sizeof(struct ip6_hdr) + plen -
m->m_pkthdr.len);
+ if (ip6_hbhchcheck(m, &off, &nxt, &ours)) {
+ if_put(ifp);
+ return; /* m have already been freed *
Re: IPv6 forwarding path without KERNEL_LOCK
On Mon, Jul 04, 2016 at 01:03:22PM +0200, Martin Pieuchot wrote:
> + if (ip6_hbhchcheck(m, &off, &nxt, &ours)) {
> + if_put(ifp);
> + return; /* m have already been freed */
> }
As ip6_hbhchcheck() does ip6 = mtod(m, struct ip6_hdr *) after
ip6_hopopts_input() you have to add this here, too.
/* adjust pointer */
ip6 = mtod(m, struct ip6_hdr *);
> +int
> +ip6_hbhchcheck(struct mbuf *m, int *offp, int *nxtp, int *oursp)
> +{
> + struct ip6_hdr *ip6;
> + u_int32_t plen, rtalert = ~0;
> + int ours, off, nxt;
ours may be used uninitialized.
> + *offp = off;
> + *nxtp = nxt;
> + *oursp = ours;
I would prefer to use the passed values as *off, *nxt, *ours directly
than to use another set of local variables. This also fixes
initialization problem.
bluhm
IPv6 forwarding path without KERNEL_LOCK
One of my trees now contain all the necessary plumbing to run the IPv6
forwarding path (mostly) without holding the KERNEL_LOCK. In other
words we should be able to unlock IPv6 and legacy IP at the same time.
However it's a bit tricky to enqueue packets for local delivery in IPv6
because of the Hop-by-Hop options header processing. So the solution I
came up with is to delay the option parsing a bit and call it ether from
the forwarding path or from the local delivery path. In order to do
that without code duplication, I introduced the function below.
This should be straightforward, ok?
Index: netinet6/ip6_input.c
===
RCS file: /cvs/src/sys/netinet6/ip6_input.c,v
retrieving revision 1.160
diff -u -p -r1.160 ip6_input.c
--- netinet6/ip6_input.c19 May 2016 11:34:40 - 1.160
+++ netinet6/ip6_input.c4 Jul 2016 10:50:35 -
@@ -122,6 +122,7 @@ struct ip6stat ip6stat;
void ip6_init2(void *);
int ip6_check_rh0hdr(struct mbuf *, int *);
+int ip6_hbhchcheck(struct mbuf *, int *, int *, int *);
int ip6_hopopts_input(u_int32_t *, u_int32_t *, struct mbuf **, int *);
struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
@@ -192,7 +193,6 @@ ip6_input(struct mbuf *m)
struct ip6_hdr *ip6;
int off, nest;
u_int16_t src_scope, dst_scope;
- u_int32_t plen, rtalert = ~0;
int nxt, ours = 0;
#if NPF > 0
struct in6_addr odst;
@@ -495,76 +495,10 @@ ip6_input(struct mbuf *m)
}
hbhcheck:
- /*
-* Process Hop-by-Hop options header if it's contained.
-* m may be modified in ip6_hopopts_input().
-* If a JumboPayload option is included, plen will also be modified.
-*/
- plen = (u_int32_t)ntohs(ip6->ip6_plen);
- off = sizeof(struct ip6_hdr);
- if (ip6->ip6_nxt == IPPROTO_HOPOPTS) {
- struct ip6_hbh *hbh;
-
- if (ip6_hopopts_input(&plen, &rtalert, &m, &off)) {
- if_put(ifp);
- return; /* m have already been freed */
- }
- /* adjust pointer */
- ip6 = mtod(m, struct ip6_hdr *);
-
- /*
-* if the payload length field is 0 and the next header field
-* indicates Hop-by-Hop Options header, then a Jumbo Payload
-* option MUST be included.
-*/
- if (ip6->ip6_plen == 0 && plen == 0) {
- /*
-* Note that if a valid jumbo payload option is
-* contained, ip6_hopopts_input() must set a valid
-* (non-zero) payload length to the variable plen.
-*/
- ip6stat.ip6s_badoptions++;
- icmp6_error(m, ICMP6_PARAM_PROB,
- ICMP6_PARAMPROB_HEADER,
- (caddr_t)&ip6->ip6_plen - (caddr_t)ip6);
- if_put(ifp);
- return;
- }
- IP6_EXTHDR_GET(hbh, struct ip6_hbh *, m, sizeof(struct ip6_hdr),
- sizeof(struct ip6_hbh));
- if (hbh == NULL) {
- ip6stat.ip6s_tooshort++;
- if_put(ifp);
- return;
- }
- nxt = hbh->ip6h_nxt;
-
- /*
-* accept the packet if a router alert option is included
-* and we act as an IPv6 router.
-*/
- if (rtalert != ~0 && ip6_forwarding)
- ours = 1;
- } else
- nxt = ip6->ip6_nxt;
-
- /*
-* Check that the amount of data in the buffers
-* is as at least much as the IPv6 header would have us expect.
-* Trim mbufs if longer than we expect.
-* Drop packet if shorter than we expect.
-*/
- if (m->m_pkthdr.len - sizeof(struct ip6_hdr) < plen) {
- ip6stat.ip6s_tooshort++;
- goto bad;
- }
- if (m->m_pkthdr.len > sizeof(struct ip6_hdr) + plen) {
- if (m->m_len == m->m_pkthdr.len) {
- m->m_len = sizeof(struct ip6_hdr) + plen;
- m->m_pkthdr.len = sizeof(struct ip6_hdr) + plen;
- } else
- m_adj(m, sizeof(struct ip6_hdr) + plen -
m->m_pkthdr.len);
+ if (ip6_hbhchcheck(m, &off, &nxt, &ours)) {
+ if_put(ifp);
+ return; /* m have already been freed */
}
/*
@@ -640,6 +574,93 @@ ip6_input(struct mbuf *m)
m_freem(m);
}
+int
+ip6_hbhchcheck(struct mbuf *m, int *offp, int *nxtp, int *oursp)
+{
+ struct ip6_hdr *ip6;
+ u_int32_t plen, rtalert = ~0;
+ int ours, off, nxt;
+
+ ip6 = mtod(m, struct ip6_hdr *);
+
+ /*
+* Process Hop-by-Hop op
