On Wed, Feb 22, 2017 at 05:06:34PM +0100, Martin Pieuchot wrote:
> I'm renaming the current ip6_ours() into ip6_local(). This function
> still need to be executed with the KERNEL_LOCK() held and most of the
> time it will be from the netisr loop.
Then the KERNEL_ASSERT_LOCKED should be in ip6_local(), not in
ip6_ours().
> Comments, oks?
Getting rid of the hbhcheck goto is good.
> --- netinet6/ip6_forward.c 5 Feb 2017 16:04:14 -0000 1.94
> +++ netinet6/ip6_forward.c 22 Feb 2017 15:54:26 -0000
> @@ -89,10 +89,21 @@ ip6_forward(struct mbuf *m, struct rtent
> struct ifnet *ifp = NULL;
> int error = 0, type = 0, code = 0;
> struct mbuf *mcopy = NULL;
> + int off, nxt, ours = 0;
> #ifdef IPSEC
> struct tdb *tdb = NULL;
> #endif /* IPSEC */
> char src6[INET6_ADDRSTRLEN], dst6[INET6_ADDRSTRLEN];
> +
> + if (ip6_hbhchcheck(m, &off, &nxt, &ours))
> + return;
> +
> + if (ours) {
> + KERNEL_LOCK();
> + ip6_local(m, off, nxt);
> + KERNEL_UNLOCK();
> + return;
> + }
You leak the route in the return case, it should be goto out. Apart
from that, I don't like to jump back to local delivery after we
have called ip6_forward(). The router alert detection should be
before that. Could you move the code back to ip6_input()?
> @@ -370,6 +371,8 @@ ip6_input(struct mbuf *m)
>
> #ifdef MROUTING
> if (ip6_mforwarding && ip6_mrouter) {
> + int rv;
> +
> if (ip6_hbhchcheck(m, &off, &nxt, &ours))
> goto out;
>
> @@ -383,14 +386,17 @@ ip6_input(struct mbuf *m)
> * ip6_mforward() returns a non-zero value, the packet
> * must be discarded, else it may be accepted below.
> */
> - if (ip6_mforward(ip6, ifp, m)) {
> + KERNEL_LOCK();
> + rv = ip6_mforward(ip6, ifp, m);
> + if (rv != 0) {
The rv variable is not used below. I think you should not
introduce it and keep the "if (ip6_mforward(ip6, ifp, m))".
> @@ -459,19 +465,6 @@ ip6_input(struct mbuf *m)
> goto bad;
> }
>
> - hbhcheck:
> -
> - if (ip6_hbhchcheck(m, &off, &nxt, &ours))
> - goto out;
> -
> - if (ours) {
> - ip6_ours(m, off, nxt);
> - goto out;
> - }
> -
> - /*
> - * Forward if desirable.
> - */
> ip6_forward(m, rt, srcrt);
> if_put(ifp);
> return;
Keep the ip6_hbhchcheck() and ip6_local() here.
> @@ -483,7 +476,20 @@ ip6_input(struct mbuf *m)
> }
>
> void
> -ip6_ours(struct mbuf *m, int off, int nxt)
> +ip6_ours(struct mbuf *m)
> +{
> + int off, nxt;
> +
> + KERNEL_ASSERT_LOCKED();
> +
> + if (ip6_hbhchcheck(m, &off, &nxt, NULL))
> + return;
> +
> + ip6_local(m, off, nxt);
> +}
When you unlock the forwarding path, there must be a KERNEL_LOCK()
around ip6_local(). Or do you plan to rewrite this function anyway?
> +
> +void
> +ip6_local(struct mbuf *m, int off, int nxt)
> {
Move the KERNEL_ASSERT_LOCKED() to here.
bluhm
