net: Fix double-free snoop bug
[akaros.git] / kern / src / net / ipifc.c
index a6d7d12..2ab62be 100644 (file)
@@ -1,4 +1,31 @@
-// INFERNO
+/* Copyright © 1994-1999 Lucent Technologies Inc.  All rights reserved.
+ * Portions Copyright © 1997-1999 Vita Nuova Limited
+ * Portions Copyright © 2000-2007 Vita Nuova Holdings Limited
+ *                                (www.vitanuova.com)
+ * Revisions Copyright © 2000-2007 Lucent Technologies Inc. and others
+ *
+ * Modified for the Akaros operating system:
+ * Copyright (c) 2013-2014 The Regents of the University of California
+ * Copyright (c) 2013-2015 Google Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE. */
+
 #include <vfs.h>
 #include <kfs.h>
 #include <slab.h>
@@ -67,11 +94,12 @@ static void addselfcache(struct Fs *f, struct Ipifc *ifc, struct Iplifc *lifc,
                                                 uint8_t * a, int type);
 static void remselfcache(struct Fs *f,
                                                 struct Ipifc *ifc, struct Iplifc *lifc, uint8_t * a);
-static char *ipifcjoinmulti(struct Ipifc *ifc, char **argv, int argc);
-static char *ipifcleavemulti(struct Ipifc *ifc, char **argv, int argc);
+static void ipifcjoinmulti(struct Ipifc *ifc, char **argv, int argc);
+static void ipifcleavemulti(struct Ipifc *ifc, char **argv, int argc);
 static void ipifcregisterproxy(struct Fs *, struct Ipifc *,
                                                           uint8_t * unused_uint8_p_t);
-static char *ipifcremlifc(struct Ipifc *, struct Iplifc *);
+static void ipifcremlifc(struct Ipifc *, struct Iplifc *);
+static void ipifcaddpref6(struct Ipifc *ifc, char **argv, int argc);
 
 /*
  *  link in a new medium
@@ -104,26 +132,26 @@ struct medium *ipfindmedium(char *name)
  *  attach a device (or pkt driver) to the interface.
  *  called with c locked
  */
-static char *ipifcbind(struct conv *c, char **argv, int argc)
+static void ipifcbind(struct conv *c, char **argv, int argc)
 {
        ERRSTACK(1);
        struct Ipifc *ifc;
        struct medium *m;
 
        if (argc < 2)
-               return Ebadarg;
+               error(EINVAL, "Too few args (%d) to %s", argc, __func__);
 
        ifc = (struct Ipifc *)c->ptcl;
 
        /* bind the device to the interface */
        m = ipfindmedium(argv[1]);
        if (m == NULL)
-               return "unknown interface type";
+               error(EFAIL, "unknown interface type");
 
        wlock(&ifc->rwlock);
        if (ifc->m != NULL) {
                wunlock(&ifc->rwlock);
-               return "interface already bound";
+               error(EFAIL, "interfacr already bound");
        }
        if (waserror()) {
                wunlock(&ifc->rwlock);
@@ -135,10 +163,9 @@ static char *ipifcbind(struct conv *c, char **argv, int argc)
 
        /* set the bound device name */
        if (argc > 2)
-               strncpy(ifc->dev, argv[2], sizeof(ifc->dev));
+               strlcpy(ifc->dev, argv[2], sizeof(ifc->dev));
        else
-               snprintf(ifc->dev, sizeof ifc->dev, "%s%d", m->name, c->x);
-       ifc->dev[sizeof(ifc->dev) - 1] = 0;
+               snprintf(ifc->dev, sizeof(ifc->dev), "%s%d", m->name, c->x);
 
        /* set up parameters */
        ifc->m = m;
@@ -166,24 +193,22 @@ static char *ipifcbind(struct conv *c, char **argv, int argc)
 
        wunlock(&ifc->rwlock);
        poperror();
-
-       return NULL;
 }
 
 /*
  *  detach a device from an interface, close the interface
  *  called with ifc->conv closed
  */
-static char *ipifcunbind(struct Ipifc *ifc)
+static void ipifcunbind(struct Ipifc *ifc)
 {
        ERRSTACK(1);
        char *err;
 
+       wlock(&ifc->rwlock);
        if (waserror()) {
                wunlock(&ifc->rwlock);
                nexterror();
        }
-       wlock(&ifc->rwlock);
 
        /* dissociate routes */
        if (ifc->m != NULL && ifc->m->unbindonclose == 0)
@@ -203,20 +228,16 @@ static char *ipifcunbind(struct Ipifc *ifc)
        qclose(ifc->conv->sq);
 
        /* disassociate logical interfaces */
-       while (ifc->lifc) {
-               err = ipifcremlifc(ifc, ifc->lifc);
-               if (err)
-                       error(EFAIL, err);
-       }
+       while (ifc->lifc)
+               ipifcremlifc(ifc, ifc->lifc);
 
        ifc->m = NULL;
        wunlock(&ifc->rwlock);
        poperror();
-       return NULL;
 }
 
 char sfixedformat[] =
-       "device %s maxtu %d sendra %d recvra %d mflag %d oflag %d maxraint %d minraint %d linkmtu %d reachtime %d rxmitra %d ttl %d routerlt %d pktin %lu pktout %lu errin %lu errout %lu\n";
+       "device %s maxtu %d sendra %d recvra %d mflag %d oflag %d maxraint %d minraint %d linkmtu %d reachtime %d rxmitra %d ttl %d routerlt %d pktin %lu pktout %lu errin %lu errout %lu tracedrop %lu\n";
 
 char slineformat[] = " %-40I %-10M %-40I %-12lu %-12lu\n";
 
@@ -233,7 +254,7 @@ static int ipifcstate(struct conv *c, char *state, int n)
                                 ifc->rp.mflag, ifc->rp.oflag, ifc->rp.maxraint,
                                 ifc->rp.minraint, ifc->rp.linkmtu, ifc->rp.reachtime,
                                 ifc->rp.rxmitra, ifc->rp.ttl, ifc->rp.routerlt,
-                                ifc->in, ifc->out, ifc->inerr, ifc->outerr);
+                                ifc->in, ifc->out, ifc->inerr, ifc->outerr, ifc->tracedrop);
 
        rlock(&ifc->rwlock);
        for (lifc = ifc->lifc; lifc && n > m; lifc = lifc->next)
@@ -315,7 +336,7 @@ static void ipifccreate(struct conv *c)
        struct Ipifc *ifc;
 
        c->rq = qopen(QMAX, 0, 0, 0);
-       c->sq = qopen(2 * QMAX, 0, 0, 0);
+       c->sq = qopen(2 * QMAX, Qmsg | Qcoalesce, 0, 0);
        c->wq = qopen(QMAX, Qkick, ipifckick, c);
        ifc = (struct Ipifc *)c->ptcl;
        ifc->conv = c;
@@ -342,26 +363,26 @@ static void ipifcclose(struct conv *c)
 /*
  *  change an interface's mtu
  */
-char *ipifcsetmtu(struct Ipifc *ifc, char **argv, int argc)
+static void ipifcsetmtu(struct Ipifc *ifc, char **argv, int argc)
 {
        int mtu;
 
        if (argc < 2)
-               return Ebadarg;
+               error(EINVAL, "Too few args (%d) to %s", argc, __func__);
        if (ifc->m == NULL)
-               return Ebadarg;
+               error(EFAIL, "No medium on IFC");
        mtu = strtoul(argv[1], 0, 0);
        if (mtu < ifc->m->mintu || mtu > ifc->m->maxtu)
-               return Ebadarg;
+               error(EFAIL, "Bad MTU size %d (%d, %d)", mtu, ifc->m->mintu,
+                     ifc->m->maxtu);
        ifc->maxtu = mtu;
-       return NULL;
 }
 
 /*
  *  add an address to an interface.
  */
-char *ipifcadd(struct Ipifc *ifc, char **argv, int argc, int tentative,
-                          struct Iplifc *lifcp)
+static void ipifcadd(struct Ipifc *ifc, char **argv, int argc, int tentative,
+                     struct Iplifc *lifcp)
 {
        uint8_t ip[IPaddrlen], mask[IPaddrlen], rem[IPaddrlen];
        uint8_t bcast[IPaddrlen], net[IPaddrlen];
@@ -371,7 +392,7 @@ char *ipifcadd(struct Ipifc *ifc, char **argv, int argc, int tentative,
        int sendnbrdisc = 0;
 
        if (ifc->m == NULL)
-               return "ipifc not yet bound to device";
+               error(EFAIL, "ipifc not yet bound to device");
 
        f = ifc->conv->p->f;
 
@@ -408,8 +429,7 @@ char *ipifcadd(struct Ipifc *ifc, char **argv, int argc, int tentative,
                        maskip(rem, mask, net);
                        break;
                default:
-                       return Ebadarg;
-                       break;
+                       error(EINVAL, "Bad arg num to %s", __func__);
        }
        if (isv4(ip))
                tentative = 0;
@@ -529,14 +549,13 @@ out:
        wunlock(&ifc->rwlock);
        if (tentative && sendnbrdisc)
                icmpns(f, 0, SRC_UNSPEC, ip, TARG_MULTI, ifc->mac);
-       return NULL;
 }
 
 /*
  *  remove a logical interface from an ifc
  *  always called with ifc wlock'd
  */
-static char *ipifcremlifc(struct Ipifc *ifc, struct Iplifc *lifc)
+static void ipifcremlifc(struct Ipifc *ifc, struct Iplifc *lifc)
 {
        struct Iplifc **l;
        struct Fs *f;
@@ -550,7 +569,7 @@ static char *ipifcremlifc(struct Ipifc *ifc, struct Iplifc *lifc)
         */
        for (l = &ifc->lifc; *l != NULL && *l != lifc; l = &(*l)->next) ;
        if (*l == NULL)
-               return "address not on this interface";
+               error(EFAIL, "address not on this interface");
        *l = lifc->next;
 
        /* disassociate any addresses */
@@ -571,24 +590,22 @@ static char *ipifcremlifc(struct Ipifc *ifc, struct Iplifc *lifc)
        }
 
        kfree(lifc);
-       return NULL;
-
 }
 
 /*
  *  remove an address from an interface.
  *  called with c locked
  */
-char *ipifcrem(struct Ipifc *ifc, char **argv, int argc)
+static void ipifcrem(struct Ipifc *ifc, char **argv, int argc)
 {
+       ERRSTACK(1);
        uint8_t ip[IPaddrlen];
        uint8_t mask[IPaddrlen];
        uint8_t rem[IPaddrlen];
        struct Iplifc *lifc;
-       char *rv;
 
        if (argc < 3)
-               return Ebadarg;
+               error(EINVAL, "Too few args (%d) to %s", argc, __func__);
 
        parseip(ip, argv[1]);
        parseipmask(mask, argv[2]);
@@ -598,6 +615,10 @@ char *ipifcrem(struct Ipifc *ifc, char **argv, int argc)
                parseip(rem, argv[3]);
 
        wlock(&ifc->rwlock);
+       if (waserror()) {
+               wunlock(&ifc->rwlock);
+               nexterror();
+       }
 
        /*
         *  find address on this interface and remove from chain.
@@ -611,9 +632,9 @@ char *ipifcrem(struct Ipifc *ifc, char **argv, int argc)
                        break;
        }
 
-       rv = ipifcremlifc(ifc, lifc);
+       ipifcremlifc(ifc, lifc);
+       poperror();
        wunlock(&ifc->rwlock);
-       return rv;
 }
 
 /*
@@ -665,7 +686,7 @@ void ipifcremroute(struct Fs *f, int vers, uint8_t * addr, uint8_t * mask)
  *  addresses.  This is a macro that means, remove all the old interfaces
  *  and add a new one.
  */
-static char *ipifcconnect(struct conv *c, char **argv, int argc)
+static void ipifcconnect(struct conv *c, char **argv, int argc)
 {
        ERRSTACK(1);
        char *err;
@@ -674,31 +695,24 @@ static char *ipifcconnect(struct conv *c, char **argv, int argc)
        ifc = (struct Ipifc *)c->ptcl;
 
        if (ifc->m == NULL)
-               return "ipifc not yet bound to device";
+               error(EFAIL, "ipifc not yet bound to device");
 
+       wlock(&ifc->rwlock);
        if (waserror()) {
                wunlock(&ifc->rwlock);
                nexterror();
        }
-       wlock(&ifc->rwlock);
-       while (ifc->lifc) {
-               err = ipifcremlifc(ifc, ifc->lifc);
-               if (err)
-                       error(EFAIL, err);
-       }
+       while (ifc->lifc)
+               ipifcremlifc(ifc, ifc->lifc);
        wunlock(&ifc->rwlock);
        poperror();
 
-       err = ipifcadd(ifc, argv, argc, 0, NULL);
-       if (err)
-               return err;
+       ipifcadd(ifc, argv, argc, 0, NULL);
 
        Fsconnected(c, NULL);
-
-       return NULL;
 }
 
-char *ipifcsetpar6(struct Ipifc *ifc, char **argv, int argc)
+static void ipifcsetpar6(struct Ipifc *ifc, char **argv, int argc)
 {
        int i, argsleft, vmax = ifc->rp.maxraint, vmin = ifc->rp.minraint;
 
@@ -706,7 +720,7 @@ char *ipifcsetpar6(struct Ipifc *ifc, char **argv, int argc)
        i = 1;
 
        if (argsleft % 2 != 0)
-               return Ebadarg;
+               error(EINVAL, "Non-even number of args (%d) to %s", argc, __func__);
 
        while (argsleft > 1) {
                if (strcmp(argv[i], "recvra") == 0)
@@ -732,7 +746,7 @@ char *ipifcsetpar6(struct Ipifc *ifc, char **argv, int argc)
                else if (strcmp(argv[i], "routerlt") == 0)
                        ifc->rp.routerlt = atoi(argv[i + 1]);
                else
-                       return Ebadarg;
+                       error(EINVAL, "unknown command to %s", __func__);
 
                argsleft -= 2;
                i += 2;
@@ -742,13 +756,11 @@ char *ipifcsetpar6(struct Ipifc *ifc, char **argv, int argc)
        if (ifc->rp.maxraint < ifc->rp.minraint) {
                ifc->rp.maxraint = vmax;
                ifc->rp.minraint = vmin;
-               return Ebadarg;
+               error(EINVAL, "inconsistent ifc->rp 'raint'");
        }
-
-       return NULL;
 }
 
-char *ipifcsendra6(struct Ipifc *ifc, char **argv, int argc)
+static void ipifcsendra6(struct Ipifc *ifc, char **argv, int argc)
 {
        int i;
 
@@ -756,10 +768,9 @@ char *ipifcsendra6(struct Ipifc *ifc, char **argv, int argc)
        if (argc > 1)
                i = atoi(argv[1]);
        ifc->sendra6 = (i != 0);
-       return NULL;
 }
 
-char *ipifcrecvra6(struct Ipifc *ifc, char **argv, int argc)
+static void ipifcrecvra6(struct Ipifc *ifc, char **argv, int argc)
 {
        int i;
 
@@ -767,53 +778,55 @@ char *ipifcrecvra6(struct Ipifc *ifc, char **argv, int argc)
        if (argc > 1)
                i = atoi(argv[1]);
        ifc->recvra6 = (i != 0);
-       return NULL;
+}
+
+static void ipifc_iprouting(struct Fs *f, char **argv, int argc)
+{
+       int i = 1;
+
+       if (argc > 1)
+               i = atoi(argv[1]);
+       iprouting(f, i);
 }
 
 /*
  *  non-standard control messages.
  *  called with c locked.
  */
-static char *ipifcctl(struct conv *c, char **argv, int argc)
+static void ipifcctl(struct conv *c, char **argv, int argc)
 {
        struct Ipifc *ifc;
        int i;
 
        ifc = (struct Ipifc *)c->ptcl;
        if (strcmp(argv[0], "add") == 0)
-               return ipifcadd(ifc, argv, argc, 0, NULL);
-       else if (strcmp(argv[0], "bootp") == 0)
-               return bootp(ifc);
+               ipifcadd(ifc, argv, argc, 0, NULL);
        else if (strcmp(argv[0], "try") == 0)
-               return ipifcadd(ifc, argv, argc, 1, NULL);
+               ipifcadd(ifc, argv, argc, 1, NULL);
        else if (strcmp(argv[0], "remove") == 0)
-               return ipifcrem(ifc, argv, argc);
+               ipifcrem(ifc, argv, argc);
        else if (strcmp(argv[0], "unbind") == 0)
-               return ipifcunbind(ifc);
+               ipifcunbind(ifc);
        else if (strcmp(argv[0], "joinmulti") == 0)
-               return ipifcjoinmulti(ifc, argv, argc);
+               ipifcjoinmulti(ifc, argv, argc);
        else if (strcmp(argv[0], "leavemulti") == 0)
-               return ipifcleavemulti(ifc, argv, argc);
+               ipifcleavemulti(ifc, argv, argc);
        else if (strcmp(argv[0], "mtu") == 0)
-               return ipifcsetmtu(ifc, argv, argc);
-       else if (strcmp(argv[0], "reassemble") == 0) {
+               ipifcsetmtu(ifc, argv, argc);
+       else if (strcmp(argv[0], "reassemble") == 0)
                ifc->reassemble = 1;
-               return NULL;
-       } else if (strcmp(argv[0], "iprouting") == 0) {
-               i = 1;
-               if (argc > 1)
-                       i = atoi(argv[1]);
-               iprouting(c->p->f, i);
-               return NULL;
-       } else if (strcmp(argv[0], "addpref6") == 0)
-               return ipifcaddpref6(ifc, argv, argc);
+       else if (strcmp(argv[0], "iprouting") == 0)
+               ipifc_iprouting(c->p->f, argv, argc);
+       else if (strcmp(argv[0], "addpref6") == 0)
+               ipifcaddpref6(ifc, argv, argc);
        else if (strcmp(argv[0], "setpar6") == 0)
-               return ipifcsetpar6(ifc, argv, argc);
+               ipifcsetpar6(ifc, argv, argc);
        else if (strcmp(argv[0], "sendra6") == 0)
-               return ipifcsendra6(ifc, argv, argc);
+               ipifcsendra6(ifc, argv, argc);
        else if (strcmp(argv[0], "recvra6") == 0)
-               return ipifcrecvra6(ifc, argv, argc);
-       return "unsupported ctl";
+               ipifcrecvra6(ifc, argv, argc);
+       else
+               error(EINVAL, "unknown command to %s", __func__);
 }
 
 int ipifcstats(struct Proto *ipifc, char *buf, int len)
@@ -851,6 +864,17 @@ void ipifcinit(struct Fs *f)
 }
 
 /*
+ * TODO: Change this to a proper release.
+ * At the moment, this is difficult since link removal
+ * requires access to more than just the kref/struct Iplink.
+ * E.g., the self and interface pointers.
+ */
+static void link_release(struct kref *kref)
+{
+       (void)kref;
+}
+
+/*
  *  add to self routing cache
  *     called with c locked
  */
@@ -891,7 +915,7 @@ addselfcache(struct Fs *f, struct Ipifc *ifc,
        /* allocate a lifc-to-local link and link to both */
        if (lp == NULL) {
                lp = kzmalloc(sizeof(*lp), 0);
-               kref_init(&lp->ref, fake_release, 1);
+               kref_init(&lp->ref, link_release, 1);
                lp->lifc = lifc;
                lp->self = p;
                lp->selflink = p->link;
@@ -968,7 +992,7 @@ static void ipselffree(struct Ipself *p)
  *     called with c locked
  */
 static void
-remselfcache(struct Fs *f, struct Ipifc *ifc, struct Iplifc *lifc, uint8_t * a)
+remselfcache(struct Fs *f, struct Ipifc *ifc, struct Iplifc *lifc, uint8_t *a)
 {
        struct Ipself *p, **l;
        struct Iplink *link, **l_self, **l_lifc;
@@ -1014,7 +1038,25 @@ remselfcache(struct Fs *f, struct Ipifc *ifc, struct Iplifc *lifc, uint8_t * a)
        if (link == NULL)
                panic("remselfcache");
 
-       if (kref_refcnt(&link->ref) > 1)
+       /*
+        * TODO: The check for 'refcnt > 0' here is awkward.  It
+        * exists so that 'remselfcache' can be called concurrently.
+        * In the original plan 9 code, the 'goto out' branch was
+        * taken if the decremented reference count was exactly zero.
+        * In other threads this could become -1, which plan 9 didn't
+        * care about since the logic would be skipped over, and since
+        * 'iplinkfree' won't _actually_ free the link for five seconds
+        * (see comments in that function), and since all of the actual
+        * link manipulation happens in the thread where the reference
+        * is exactly equal to zero.  But on Akaros, a negative kref
+        * will panic; hence checking for a positive ref count before
+        * decrementing.  This entire mechanism is dubious.  But Since
+        * this function is protected by a lock this is probably OK for
+        * the time being.
+        *
+        * However, it is a terrible design and we should fix it.
+        */
+       if (kref_refcnt(&link->ref) > 0 && kref_put(&link->ref) != 0)
                goto out;
 
        if ((p->type & Rmulti) && ifc->m->remmulti != NULL)
@@ -1457,11 +1499,11 @@ void ipifcaddmulti(struct conv *c, uint8_t * ma, uint8_t * ia)
                if ((*p)->inuse == 0)
                        continue;
                ifc = (struct Ipifc *)(*p)->ptcl;
+               wlock(&ifc->rwlock);
                if (waserror()) {
                        wunlock(&ifc->rwlock);
                        nexterror();
                }
-               wlock(&ifc->rwlock);
                for (lifc = ifc->lifc; lifc; lifc = lifc->next)
                        if (ipcmp(ia, lifc->local) == 0)
                                addselfcache(f, ifc, lifc, ma, Rmulti);
@@ -1470,6 +1512,22 @@ void ipifcaddmulti(struct conv *c, uint8_t * ma, uint8_t * ia)
        }
 }
 
+/* Trace a block @b that crosses the interface @ifc. */
+void ipifc_trace_block(struct Ipifc *ifc, struct block *bp)
+{
+       struct block *newb;
+
+       if (!atomic_read(&ifc->conv->snoopers))
+               return;
+       newb = copyblock(bp, MEM_ATOMIC);
+       if (!newb) {
+               ifc->tracedrop++;
+               return;
+       }
+       if (qpass(ifc->conv->sq, newb) < 0)
+               ifc->tracedrop++;
+}
+
 /*
  *  remove a multicast address from an interface, called with c locked
  */
@@ -1500,11 +1558,11 @@ void ipifcremmulti(struct conv *c, uint8_t * ma, uint8_t * ia)
                        continue;
 
                ifc = (struct Ipifc *)(*p)->ptcl;
+               wlock(&ifc->rwlock);
                if (waserror()) {
                        wunlock(&ifc->rwlock);
                        nexterror();
                }
-               wlock(&ifc->rwlock);
                for (lifc = ifc->lifc; lifc; lifc = lifc->next)
                        if (ipcmp(ia, lifc->local) == 0)
                                remselfcache(f, ifc, lifc, ma);
@@ -1518,14 +1576,14 @@ void ipifcremmulti(struct conv *c, uint8_t * ma, uint8_t * ia)
 /*
  *  make lifc's join and leave multicast groups
  */
-static char *ipifcjoinmulti(struct Ipifc *ifc, char **argv, int argc)
+static void ipifcjoinmulti(struct Ipifc *ifc, char **argv, int argc)
 {
-       return NULL;
+       warn_once("Not implemented, should it be?");
 }
 
-static char *ipifcleavemulti(struct Ipifc *ifc, char **argv, int argc)
+static void ipifcleavemulti(struct Ipifc *ifc, char **argv, int argc)
 {
-       return NULL;
+       warn_once("Not implemented, should it be?");
 }
 
 static void ipifcregisterproxy(struct Fs *f, struct Ipifc *ifc, uint8_t * ip)
@@ -1611,7 +1669,7 @@ enum {
        Ngates = 3,
 };
 
-char *ipifcaddpref6(struct Ipifc *ifc, char **argv, int argc)
+static void ipifcaddpref6(struct Ipifc *ifc, char **argv, int argc)
 {
        uint8_t onlink = 1;
        uint8_t autoflag = 1;
@@ -1642,13 +1700,13 @@ char *ipifcaddpref6(struct Ipifc *ifc, char **argv, int argc)
                case 2:
                        break;
                default:
-                       return Ebadarg;
+                       error(EINVAL, "Bad arg num to %s", __func__);
        }
 
        if ((parseip(prefix, argv[1]) != 6) ||
                (validlt < preflt) || (plen < 0) || (plen > 64) || (islinklocal(prefix))
                )
-               return Ebadarg;
+               error(EFAIL, "IP parsing failed");
 
        lifc = kzmalloc(sizeof(struct Iplifc), 0);
        lifc->onlink = (onlink != 0);
@@ -1660,7 +1718,7 @@ char *ipifcaddpref6(struct Ipifc *ifc, char **argv, int argc)
        if (ifc->m->pref2addr != NULL)
                ifc->m->pref2addr(prefix, ifc->mac);
        else
-               return Ebadarg;
+               error(EFAIL, "Null IFC pref");
 
        snprintf(addr, sizeof(addr), "%I", prefix);
        snprintf(preflen, sizeof(preflen), "/%d", plen);
@@ -1668,5 +1726,5 @@ char *ipifcaddpref6(struct Ipifc *ifc, char **argv, int argc)
        params[1] = addr;
        params[2] = preflen;
 
-       return ipifcadd(ifc, params, 3, 0, lifc);
+       ipifcadd(ifc, params, 3, 0, lifc);
 }