start splitting kern.9 up

2018-04-18 Thread David Gwynne
like a small handful of man pages, kern.9 puts prototypes in places
other than the SYNOPSIS section. of these, it is probably the most
straightforward to fix. kern.9 documents a bunch of different apis
provided by "libkern", but they can be split out into manpages for
each type of api. this diff takes the kernel assert family out into
KASSERT.9

ok?

Index: KASSERT.9
===
RCS file: KASSERT.9
diff -N KASSERT.9
--- /dev/null   1 Jan 1970 00:00:00 -
+++ KASSERT.9   19 Apr 2018 00:14:50 -
@@ -0,0 +1,105 @@
+.\"$OpenBSD: kern.9,v 1.23 2015/12/20 08:10:36 sf Exp $
+.\"
+.\" Copyright (c) 2002, 2003 CubeSoft Communications, Inc.
+.\" 
+.\"
+.\" Redistribution and use in source and binary forms, with or without
+.\" modification, are permitted provided that the following conditions
+.\" are met:
+.\" 1. Redistributions of source code must retain the above copyright
+.\"notice, this list of conditions and the following disclaimer.
+.\" 2. Redistributions in binary form must reproduce the above copyright
+.\"notice, this list of conditions and the following disclaimer in the
+.\"documentation and/or other materials provided with the distribution.
+.\"
+.\" THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
+.\" IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+.\" WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+.\" ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT,
+.\" INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+.\" (INCLUDING BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+.\" SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+.\" HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+.\" STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
+.\" IN ANY WAY OUT OF THE USE OF THIS SOFTWARE EVEN IF ADVISED OF THE
+.\" POSSIBILITY OF SUCH DAMAGE.
+.\"
+.Dd $Mdocdate: December 20 2015 $
+.Dt KASSERT 9
+.Os
+.Sh NAME
+.Nm assert ,
+.Nm KASSERT ,
+.Nm KDASSERT ,
+.Nm KASSERTMSG ,
+.Nm KDASSERTMSG ,
+.Nm CTASSERT
+.Nd kernel assert library routines
+.Sh SYNOPSIS
+.In lib/libkern/libkern.h
+.Ft "void"
+.Fn assert "CONDITION"
+.Ft "void"
+.Fn KASSERT "CONDITION"
+.Ft "void"
+.Fn KDASSERT "CONDITION"
+.Ft "void"
+.Fn KASSERTMSG "CONDITION" "fmt" "..."
+.Ft "void"
+.Fn KDASSERTMSG "CONDITION" "fmt" "..."
+.Ft "void"
+.Fn CTASSERT "CONDITION"
+.Sh DESCRIPTION
+The
+kernel
+library implements a set of useful functions and macros implementing
+expression verification.
+.Pp
+These macros cause kernel
+.Xr panic 9
+if the given condition evaluates to false.
+.Fn assert
+tests are always compiled in.
+.Fn KASSERT
+and
+.Fn KASSERTMSG
+tests are only included if the kernel has
+.Dv DIAGNOSTIC
+enabled.
+.Fn KDASSERT
+and
+.Fn KDASSERTMSG
+tests are only included if the kernel has
+.Dv DEBUG
+enabled.
+The
+.Fn KASSERTMSG
+and
+.Fn KDASSERTMSG
+macros append
+to the
+.Xr panic 9
+format string the message specified by
+.Fa format
+and its subsequent arguments, similar to
+.Xr printf 9
+functions.
+.Pp
+.Fn CTASSERT
+causes a compile time error if the given condition evaluates to
+false.
+Its main purpose is to verify assertions about type and struct sizes that
+would otherwise make the code fail at run time.
+.Fn CTASSERT
+can be used in global scope or at the start of blocks, where variable
+declarations are allowed.
+.Sh SEE ALSO
+.Xr assert 3 ,
+.Xr panic 9
+.Sh HISTORY
+The
+.Fn KASSERTMSG
+and
+.Fn KDASSERTMSG
+macros are taken from
+.Nx .
Index: Makefile
===
RCS file: /cvs/src/share/man/man9/Makefile,v
retrieving revision 1.288
diff -u -p -r1.288 Makefile
--- Makefile14 Dec 2017 00:41:58 -  1.288
+++ Makefile19 Apr 2018 00:14:50 -
@@ -20,7 +20,7 @@ MAN=  aml_evalnode.9 atomic_add_int.9 ato
ieee80211_node.9 ieee80211_output.9 ieee80211_proto.9 \
ieee80211_radiotap.9 if_get.9 if_rxr_init.9 ifq_enqueue.9 \
ifq_deq_begin.9 iic.9 intro.9 inittodr.9 intr_barrier.9 \
-   kern.9 km_alloc.9 knote.9 kthread.9 ktrace.9 \
+   KASSERT.9 kern.9 km_alloc.9 knote.9 kthread.9 ktrace.9 \
loadfirmware.9 log.9 \
malloc.9 membar_sync.9 mbuf.9 mbuf_tags.9 md5.9 mi_switch.9 \
microtime.9 ml_init.9 mq_init.9 mutex.9 \
Index: kern.9
===
RCS file: /cvs/src/share/man/man9/kern.9,v
retrieving revision 1.23
diff -u -p -r1.23 kern.9
--- kern.9  20 Dec 2015 08:10:36 -  1.23
+++ kern.9  19 Apr 2018 00:14:50 -
@@ -25,7 +25,7 @@
 .\" POSSIBILITY OF SUCH DAMAGE.
 .\"
 .Dd $Mdocdate: December 20 2015 $
-.Dt KASSERT 9
+.Dt imax 9
 .Os
 .Sh NAME
 .Nm imax ,
@@ -37,12 +37,6 @@
 .Nm ulmax ,
 .Nm ulmin ,
 .Nm abs ,
-.Nm assert ,
-.Nm KASSERT ,
-.Nm KDASSERT ,
-.Nm KASSERTMSG ,
-.Nm 

Re: remove unused rtentry parameter from rtm_addr()

2018-04-18 Thread Alexander Bluhm
On Wed, Apr 18, 2018 at 05:03:04PM +0200, Florian Obser wrote:
> @@ -1158,9 +1158,9 @@ rt_ifa_del(struct ifaddr *ifa, int flags, struct 
> sockaddr *dst)
>   error = rtrequest_delete(, prio, ifp, , rtableid);
>   if (error == 0) {
>   rtm_send(rt, RTM_DELETE, 0, rtableid);
> - if (flags & RTF_LOCAL)
> - rtm_addr(rt, RTM_DELADDR, ifa);
>   rtfree(rt);
> + if (flags & RTF_LOCAL)
> + rtm_addr(RTM_DELADDR, ifa);
>   }
>   m_free(m);
>  

Why do you change the order of rtfree() and rtm_addr()?

Have you checked that the rt->rt_ifa is not holding the last reference
to ifa?  Otherwise the ifafree() in rtfree() could free it.

bluhm



Re: ifconfig,route,netstat: s/tableid/rtable/ for consistency

2018-04-18 Thread Klemens Nanni
On Fri, Apr 13, 2018 at 02:38:07PM +0200, Klemens Nanni wrote:
> On Thu, Apr 12, 2018 at 10:07:58AM +0200, Peter Hessler wrote:
> > On 2018 Apr 11 (Wed) at 23:01:45 +0200 (+0200), Klemens Nanni wrote:
> > :On Wed, Apr 11, 2018 at 09:28:03AM +0200, Peter Hessler wrote:
> > :> No, all of these uses are correct as-is.
> > :`tableid' surely isn't wrong, but using the argument name across manuals
> > :seems nicer to me.
> > :
> > 
> > No, they are different things.  Different names help with the concept.
> > 
> > 
> > :Or is there any real difference between `tableid' and `rtable' I'm not
> > :aware of?
> > :
> > 
> > rtables are layer 3.
> > 
> > rdomains are layer 2 (aka, arp and ndp lookups).
> > 
> > You can have multiple rtables within an rdomain.  An interface can only
> > be a member of a single rdomain at a time.
> Maybe my first mail wasn't clear enough: I'm talking about routing
> *tables* only.
> 
> Specifically, how they are referred to as `rtable' and `tableid' across
> different manual pages.
> 
> I propose to use `rtable' exclusively to ease searching and improve
> consistency as that's the wording already used across the majority of
> manuals including rtabe(4) and pf.conf(5) for example.
Ping.

Further feedback? Objections? OK?



RTM_CHGADDRATTR: route(8)

2018-04-18 Thread Florian Obser
diff --git route.c route.c
index 9c85f70de78..a9ee414c562 100644
--- route.c
+++ route.c
@@ -1257,7 +1257,8 @@ char *msgtypes[] = {
"RTM_DESYNC: route socket overflow",
"RTM_INVALIDATE: invalidate cache of L2 route",
"RTM_BFD: bidirectional forwarding detection",
-   "RTM_PROPOSAL: config proposal"
+   "RTM_PROPOSAL: config proposal",
+   "RTM_CHGADDRATTR: address attributes being changed"
 };
 
 char metricnames[] =
@@ -1328,6 +1329,7 @@ print_rtmsg(struct rt_msghdr *rtm, int msglen)
break;
case RTM_NEWADDR:
case RTM_DELADDR:
+   case RTM_CHGADDRATTR:
ifam = (struct ifa_msghdr *)rtm;
printf(", metric %d, flags:", ifam->ifam_metric);
bprintf(stdout, ifam->ifam_flags, routeflags);

-- 
I'm not entirely sure you are real.



RTM_CHGADDRATTR

2018-04-18 Thread Florian Obser
On Wed, Apr 18, 2018 at 05:05:59PM +0200, Florian Obser wrote:
> This is to inform userland (i.e. slaacd(8)) when duplicate address
> detection finishes.
> 
> Not a big fan of the lock/unlock dance but I guess it can't be helped
> for now.
> 
> Comments, OKs?

Theo points out that I suck at naming things. I guess we already knew
that.

How about RTM_CHGADDRATTR, we are changing the attribute of an
address. Also fixes a tab vs. space in previous.

diff --git net/route.h net/route.h
index 3c89348cb43..5fa12578e45 100644
--- net/route.h
+++ net/route.h
@@ -241,6 +241,7 @@ struct rt_msghdr {
 #define RTM_INVALIDATE 0x11/* Invalidate cache of L2 route */
 #define RTM_BFD0x12/* bidirectional forwarding detection */
 #define RTM_PROPOSAL   0x13/* proposal for netconfigd */
+#define RTM_CHGADDRATTR0x14/* address attribute change */
 
 #define RTV_MTU0x1 /* init or lock _mtu */
 #define RTV_HOPCOUNT   0x2 /* init or lock _hopcount */
diff --git netinet6/nd6_nbr.c netinet6/nd6_nbr.c
index cb5c04c24ed..ef1644aa6f6 100644
--- netinet6/nd6_nbr.c
+++ netinet6/nd6_nbr.c
@@ -1102,6 +1102,11 @@ nd6_dad_start(struct ifaddr *ifa)
KASSERT(ia6->ia6_flags & IN6_IFF_TENTATIVE);
if ((ia6->ia6_flags & IN6_IFF_ANYCAST) || (!ip6_dad_count)) {
ia6->ia6_flags &= ~IN6_IFF_TENTATIVE;
+
+   KERNEL_LOCK();
+   rtm_addr(RTM_CHGADDRATTR, ifa);
+   KERNEL_UNLOCK();
+
return;
}
 
@@ -1250,6 +1255,10 @@ nd6_dad_timer(void *xifa)
 */
ia6->ia6_flags &= ~IN6_IFF_TENTATIVE;
 
+   KERNEL_LOCK();
+   rtm_addr(RTM_CHGADDRATTR, ifa);
+   KERNEL_UNLOCK();
+
nd6log((LOG_DEBUG,
"%s: DAD complete for %s - no duplicates found\n",
ifa->ifa_ifp->if_xname,
@@ -1293,6 +1302,11 @@ nd6_dad_duplicated(struct dadq *dp)
ia6->ia_ifp->if_xname);
 
TAILQ_REMOVE(, dp, dad_list);
+
+   KERNEL_LOCK();
+   rtm_addr(RTM_CHGADDRATTR, dp->dad_ifa);
+   KERNEL_UNLOCK();
+
ifafree(dp->dad_ifa);
free(dp, M_IP6NDP, sizeof(*dp));
ip6_dad_pending--;



-- 
I'm not entirely sure you are real.



Re: whitelist /etc/networks with pledge("dns")

2018-04-18 Thread Florian Obser
On Wed, Apr 18, 2018 at 07:13:49PM +0200, Florian Obser wrote:
> So I was puzzled why route(8) pledges rpath. As far as I can work out
> it's because asr tries to open /etc/networks in getnetnamadr_async.c
> (line 183).
> 
> If we whitelist that file like all the other things asr needs we can
> drop the rpath pledge in route(8).
> 
> Thoughts?

previous diff was missing route/show.c


diff --git sbin/route/route.c sbin/route/route.c
index 9c85f70de78..cb3078427ca 100644
--- sbin/route/route.c
+++ sbin/route/route.c
@@ -243,7 +243,7 @@ main(int argc, char **argv)
break;
}
 
-   if (pledge("stdio rpath dns", NULL) == -1)
+   if (pledge("stdio dns", NULL) == -1)
err(1, "pledge");
 
switch (kw) {
@@ -342,7 +342,7 @@ flushroutes(int argc, char **argv)
break;
}
 
-   if (pledge("stdio rpath dns", NULL) == -1)
+   if (pledge("stdio dns", NULL) == -1)
err(1, "pledge");
 
if (verbose) {
@@ -1108,7 +1108,7 @@ monitor(int argc, char *argv[])
char msg[2048];
time_t now;
 
-   if (pledge("stdio rpath dns", NULL) == -1)
+   if (pledge("stdio dns", NULL) == -1)
err(1, "pledge");
 
verbose = 1;
diff --git sbin/route/show.c sbin/route/show.c
index 913baf6cdb6..c4e3655b91b 100644
--- sbin/route/show.c
+++ sbin/route/show.c
@@ -147,7 +147,7 @@ p_rttables(int af, u_int tableid, int hastable, char prio)
break;
}
 
-   if (pledge("stdio rpath dns", NULL) == -1)
+   if (pledge("stdio dns", NULL) == -1)
err(1, "pledge");
 
printf("Routing tables\n");
diff --git sys/kern/kern_pledge.c sys/kern/kern_pledge.c
index d0886473414..7bdfcbc6612 100644
--- sys/kern/kern_pledge.c
+++ sys/kern/kern_pledge.c
@@ -634,6 +634,8 @@ pledge_namei(struct proc *p, struct nameidata *ni, char 
*origpath)
return (0);
if (strcmp(path, "/etc/services") == 0)
return (0);
+   if (strcmp(path, "/etc/networks") == 0)
+   return (0);
}
 
if ((ni->ni_pledge == PLEDGE_RPATH) &&


-- 
I'm not entirely sure you are real.



Re: whitelist /etc/networks with pledge("dns")

2018-04-18 Thread Theo de Raadt
Well the other approach would be to finally delete support
for /etc/networks


Florian Obser  wrote:
> So I was puzzled why route(8) pledges rpath. As far as I can work out
> it's because asr tries to open /etc/networks in getnetnamadr_async.c
> (line 183).
> 
> If we whitelist that file like all the other things asr needs we can
> drop the rpath pledge in route(8).
> 
> Thoughts?
> 
> diff --git sbin/route/route.c sbin/route/route.c
> index 9c85f70de78..cb3078427ca 100644
> --- sbin/route/route.c
> +++ sbin/route/route.c
> @@ -243,7 +243,7 @@ main(int argc, char **argv)
>   break;
>   }
>  
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio dns", NULL) == -1)
>   err(1, "pledge");
>  
>   switch (kw) {
> @@ -342,7 +342,7 @@ flushroutes(int argc, char **argv)
>   break;
>   }
>  
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio dns", NULL) == -1)
>   err(1, "pledge");
>  
>   if (verbose) {
> @@ -1108,7 +1108,7 @@ monitor(int argc, char *argv[])
>   char msg[2048];
>   time_t now;
>  
> - if (pledge("stdio rpath dns", NULL) == -1)
> + if (pledge("stdio dns", NULL) == -1)
>   err(1, "pledge");
>  
>   verbose = 1;
> diff --git sys/kern/kern_pledge.c sys/kern/kern_pledge.c
> index d0886473414..7bdfcbc6612 100644
> --- sys/kern/kern_pledge.c
> +++ sys/kern/kern_pledge.c
> @@ -634,6 +634,8 @@ pledge_namei(struct proc *p, struct nameidata *ni, char 
> *origpath)
>   return (0);
>   if (strcmp(path, "/etc/services") == 0)
>   return (0);
> + if (strcmp(path, "/etc/networks") == 0)
> + return (0);
>   }
>  
>   if ((ni->ni_pledge == PLEDGE_RPATH) &&
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



whitelist /etc/networks with pledge("dns")

2018-04-18 Thread Florian Obser
So I was puzzled why route(8) pledges rpath. As far as I can work out
it's because asr tries to open /etc/networks in getnetnamadr_async.c
(line 183).

If we whitelist that file like all the other things asr needs we can
drop the rpath pledge in route(8).

Thoughts?

diff --git sbin/route/route.c sbin/route/route.c
index 9c85f70de78..cb3078427ca 100644
--- sbin/route/route.c
+++ sbin/route/route.c
@@ -243,7 +243,7 @@ main(int argc, char **argv)
break;
}
 
-   if (pledge("stdio rpath dns", NULL) == -1)
+   if (pledge("stdio dns", NULL) == -1)
err(1, "pledge");
 
switch (kw) {
@@ -342,7 +342,7 @@ flushroutes(int argc, char **argv)
break;
}
 
-   if (pledge("stdio rpath dns", NULL) == -1)
+   if (pledge("stdio dns", NULL) == -1)
err(1, "pledge");
 
if (verbose) {
@@ -1108,7 +1108,7 @@ monitor(int argc, char *argv[])
char msg[2048];
time_t now;
 
-   if (pledge("stdio rpath dns", NULL) == -1)
+   if (pledge("stdio dns", NULL) == -1)
err(1, "pledge");
 
verbose = 1;
diff --git sys/kern/kern_pledge.c sys/kern/kern_pledge.c
index d0886473414..7bdfcbc6612 100644
--- sys/kern/kern_pledge.c
+++ sys/kern/kern_pledge.c
@@ -634,6 +634,8 @@ pledge_namei(struct proc *p, struct nameidata *ni, char 
*origpath)
return (0);
if (strcmp(path, "/etc/services") == 0)
return (0);
+   if (strcmp(path, "/etc/networks") == 0)
+   return (0);
}
 
if ((ni->ni_pledge == PLEDGE_RPATH) &&


-- 
I'm not entirely sure you are real.



Re: remove unused rtentry parameter from rtm_addr()

2018-04-18 Thread Klemens Nanni
On Wed, Apr 18, 2018 at 05:03:04PM +0200, Florian Obser wrote:
> some dude commited it like this in '95.
It used to be used until mpi@ started passing ifa:

"Pass the configured ``ifa'' to rt_sendaddrmsg() instead of
getting it via ``rt->rt_ifa'' later"

http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/rtsock.c.diff?r1=1.191=1.192

> OK?
OK kn



RTM_CHGADDR: route(8)

2018-04-18 Thread Florian Obser
teach route(8) about RTM_CHGADDR

OK?

diff --git route.c route.c
index 9c85f70de78..fee99dcd24b 100644
--- route.c
+++ route.c
@@ -1257,7 +1257,8 @@ char *msgtypes[] = {
"RTM_DESYNC: route socket overflow",
"RTM_INVALIDATE: invalidate cache of L2 route",
"RTM_BFD: bidirectional forwarding detection",
-   "RTM_PROPOSAL: config proposal"
+   "RTM_PROPOSAL: config proposal",
+   "RTM_CHGADDR: address flags being changed"
 };
 
 char metricnames[] =
@@ -1328,6 +1329,7 @@ print_rtmsg(struct rt_msghdr *rtm, int msglen)
break;
case RTM_NEWADDR:
case RTM_DELADDR:
+   case RTM_CHGADDR:
ifam = (struct ifa_msghdr *)rtm;
printf(", metric %d, flags:", ifam->ifam_metric);
bprintf(stdout, ifam->ifam_flags, routeflags);


-- 
I'm not entirely sure you are real.



RTM_CHGADDR

2018-04-18 Thread Florian Obser
This is to inform userland (i.e. slaacd(8)) when duplicate address
detection finishes.

Not a big fan of the lock/unlock dance but I guess it can't be helped
for now.

Comments, OKs?

diff --git net/route.h net/route.h
index 3c89348cb43..3d958719049 100644
--- net/route.h
+++ net/route.h
@@ -241,6 +241,7 @@ struct rt_msghdr {
 #define RTM_INVALIDATE 0x11/* Invalidate cache of L2 route */
 #define RTM_BFD0x12/* bidirectional forwarding detection */
 #define RTM_PROPOSAL   0x13/* proposal for netconfigd */
+#define RTM_CHGADDR0x14/* address flags change */
 
 #define RTV_MTU0x1 /* init or lock _mtu */
 #define RTV_HOPCOUNT   0x2 /* init or lock _hopcount */
diff --git netinet6/nd6_nbr.c netinet6/nd6_nbr.c
index cb5c04c24ed..1ae431954a2 100644
--- netinet6/nd6_nbr.c
+++ netinet6/nd6_nbr.c
@@ -1102,6 +1102,11 @@ nd6_dad_start(struct ifaddr *ifa)
KASSERT(ia6->ia6_flags & IN6_IFF_TENTATIVE);
if ((ia6->ia6_flags & IN6_IFF_ANYCAST) || (!ip6_dad_count)) {
ia6->ia6_flags &= ~IN6_IFF_TENTATIVE;
+
+   KERNEL_LOCK();
+   rtm_addr(RTM_CHGADDR, ifa);
+   KERNEL_UNLOCK();
+
return;
}
 
@@ -1250,6 +1255,10 @@ nd6_dad_timer(void *xifa)
 */
ia6->ia6_flags &= ~IN6_IFF_TENTATIVE;
 
+   KERNEL_LOCK();
+   rtm_addr(RTM_CHGADDR, ifa);
+   KERNEL_UNLOCK();
+
nd6log((LOG_DEBUG,
"%s: DAD complete for %s - no duplicates found\n",
ifa->ifa_ifp->if_xname,
@@ -1293,6 +1302,11 @@ nd6_dad_duplicated(struct dadq *dp)
ia6->ia_ifp->if_xname);
 
TAILQ_REMOVE(, dp, dad_list);
+
+   KERNEL_LOCK();
+rtm_addr(RTM_CHGADDR, dp->dad_ifa);
+   KERNEL_UNLOCK();
+
ifafree(dp->dad_ifa);
free(dp, M_IP6NDP, sizeof(*dp));
ip6_dad_pending--;

-- 
I'm not entirely sure you are real.



remove unused rtentry parameter from rtm_addr()

2018-04-18 Thread Florian Obser
some dude commited it like this in '95.

I came accress it because I want to use it in a place where I don't
have a rtentry allocated already.

OK?

diff --git net/route.c net/route.c
index 30c8def301d..a0c5738d831 100644
--- net/route.c
+++ net/route.c
@@ -1103,7 +1103,7 @@ rt_ifa_add(struct ifaddr *ifa, int flags, struct sockaddr 
*dst)
 * userland that a new address has been added.
 */
if (flags & RTF_LOCAL)
-   rtm_addr(rt, RTM_NEWADDR, ifa);
+   rtm_addr(RTM_NEWADDR, ifa);
rtm_send(rt, RTM_ADD, 0, rtableid);
rtfree(rt);
}
@@ -1158,9 +1158,9 @@ rt_ifa_del(struct ifaddr *ifa, int flags, struct sockaddr 
*dst)
error = rtrequest_delete(, prio, ifp, , rtableid);
if (error == 0) {
rtm_send(rt, RTM_DELETE, 0, rtableid);
-   if (flags & RTF_LOCAL)
-   rtm_addr(rt, RTM_DELADDR, ifa);
rtfree(rt);
+   if (flags & RTF_LOCAL)
+   rtm_addr(RTM_DELADDR, ifa);
}
m_free(m);
 
diff --git net/route.h net/route.h
index 9f5459a9a62..3c89348cb43 100644
--- net/route.h
+++ net/route.h
@@ -427,7 +427,7 @@ void rt_maskedcopy(struct sockaddr *,
struct sockaddr *, struct sockaddr *);
 struct sockaddr *rt_plen2mask(struct rtentry *, struct sockaddr_in6 *);
 voidrtm_send(struct rtentry *, int, int, unsigned int);
-voidrtm_addr(struct rtentry *, int, struct ifaddr *);
+voidrtm_addr(int, struct ifaddr *);
 voidrtm_miss(int, struct rt_addrinfo *, int, uint8_t, u_int, int, u_int);
 int rt_setgate(struct rtentry *, struct sockaddr *, u_int);
 struct rtentry *rt_getll(struct rtentry *);
diff --git net/rtsock.c net/rtsock.c
index eb570e25698..c5590378259 100644
--- net/rtsock.c
+++ net/rtsock.c
@@ -1509,7 +1509,7 @@ rtm_ifchg(struct ifnet *ifp)
  * copies of it.
  */
 void
-rtm_addr(struct rtentry *rt, int cmd, struct ifaddr *ifa)
+rtm_addr(int cmd, struct ifaddr *ifa)
 {
struct ifnet*ifp = ifa->ifa_ifp;
struct mbuf *m;

-- 
I'm not entirely sure you are real.



re-run DAD on address update

2018-04-18 Thread Florian Obser
Run duplicate address detection again if an existing address gets
updated from userland that was marked duplicated or tentative.

Otherwise we would just lose the duplicated / tentative state and assume
that the address is now unique and usable.

OK?

diff --git in6.c in6.c
index 1c5ec065aa5..b347ec74621 100644
--- in6.c
+++ in6.c
@@ -420,15 +420,15 @@ in6_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, 
int privileged)
break;
}
 
+   /* Perform DAD, if needed. */
+   if (ia6->ia6_flags & IN6_IFF_TENTATIVE)
+   nd6_dad_start(>ia_ifa);
+
if (!newifaddr) {
dohooks(ifp->if_addrhooks, 0);
break;
}
 
-   /* Perform DAD, if needed. */
-   if (ia6->ia6_flags & IN6_IFF_TENTATIVE)
-   nd6_dad_start(>ia_ifa);
-
plen = in6_mask2len(>ia_prefixmask.sin6_addr, NULL);
if ((ifp->if_flags & IFF_LOOPBACK) || plen == 128) {
dohooks(ifp->if_addrhooks, 0);
@@ -655,6 +655,9 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra,
if ((error = in6_ifinit(ifp, ia6, hostIsNew)) != 0)
goto unlink;
 
+   /* re-run DAD */
+   if (ia6->ia6_flags & (IN6_IFF_TENTATIVE|IN6_IFF_DUPLICATED))
+   ifra->ifra_flags |= IN6_IFF_TENTATIVE;
/*
 * configure address flags.
 */


-- 
I'm not entirely sure you are real.



Test wanted: NFS locking (aka beck@ must fix this)

2018-04-18 Thread Martin Pieuchot
Diff below gets rid of the (un)famous XXX in NFS by implementing proper
locking for NFS nodes, similar to the inode one.

It needs *a lot* of tests.  If you run into a problem, try to reproduce
it by defining VFSLCKDEBUG and WITNESS in your kernel config file.  If
that's not enough, you can toggle the "#if 0" in the diff below.

Having proper locking is a requirement to fix more NFS races and
presumably VFS issues as well.  So any help would benefit a lot future
improvements in this area.

Comments?

Index: nfs/nfs_node.c
===
RCS file: /cvs/src/sys/nfs/nfs_node.c,v
retrieving revision 1.67
diff -u -p -r1.67 nfs_node.c
--- nfs/nfs_node.c  9 Apr 2018 09:39:53 -   1.67
+++ nfs/nfs_node.c  17 Apr 2018 14:15:23 -
@@ -134,6 +134,10 @@ loop:
}
 
vp = nvp;
+#ifdef VFSLCKDEBUG
+   vp->v_flag |= VLOCKSWORK;
+#endif
+   rrw_init_flags(>n_lock, "nfsnode", RWL_DUPOK | RWL_IS_VNODE);
vp->v_data = np;
/* we now have an nfsnode on this vnode */
vp->v_flag &= ~VLARVAL;
@@ -142,6 +146,8 @@ loop:
np->n_fhp = >n_fh;
bcopy(fh, np->n_fhp, fhsize);
np->n_fhsize = fhsize;
+   /* lock the nfsnode, then put it on the rbtree */
+   rrw_enter(>n_lock, RW_WRITE);
np2 = RBT_INSERT(nfs_nodetree, >nm_ntree, np);
KASSERT(np2 == NULL);
np->n_accstamp = -1;
@@ -183,9 +189,10 @@ nfs_inactive(void *v)
 * Remove the silly file that was rename'd earlier
 */
nfs_vinvalbuf(ap->a_vp, 0, sp->s_cred, curproc);
+   vn_lock(sp->s_dvp, LK_EXCLUSIVE | LK_RETRY, curproc);
nfs_removeit(sp);
crfree(sp->s_cred);
-   vrele(sp->s_dvp);
+   vput(sp->s_dvp);
free(sp, M_NFSREQ, sizeof(*sp));
}
np->n_flag &= (NMODIFIED | NFLUSHINPROG | NFLUSHWANT);
Index: nfs/nfs_vnops.c
===
RCS file: /cvs/src/sys/nfs/nfs_vnops.c,v
retrieving revision 1.172
diff -u -p -r1.172 nfs_vnops.c
--- nfs/nfs_vnops.c 17 Apr 2018 07:45:24 -  1.172
+++ nfs/nfs_vnops.c 17 Apr 2018 14:15:23 -
@@ -88,7 +88,9 @@ int nfs_flush(struct vnode *, struct ucr
 int nfs_fsync(void *);
 int nfs_getattr(void *);
 int nfs_getreq(struct nfsrv_descript *, struct nfsd *, int);
+int nfs_islocked(void *);
 int nfs_link(void *);
+int nfs_lock(void *);
 int nfs_lookitup(struct vnode *, char *, int, struct ucred *, struct proc *,
struct nfsnode **);
 int nfs_lookup(void *);
@@ -120,6 +122,7 @@ int nfs_sillyrename(struct vnode *, stru
 struct componentname *);
 int nfs_strategy(void *);
 int nfs_symlink(void *);
+int nfs_unlock(void *);
 
 void nfs_cache_enter(struct vnode *, struct vnode *, struct componentname *);
 
@@ -161,12 +164,12 @@ struct vops nfs_vops = {
.vop_abortop= vop_generic_abortop,
.vop_inactive   = nfs_inactive,
.vop_reclaim= nfs_reclaim,
-   .vop_lock   = vop_generic_lock, /* XXX: beck@ must fix this. */
-   .vop_unlock = vop_generic_unlock,
+   .vop_lock   = nfs_lock,
+   .vop_unlock = nfs_unlock,
.vop_bmap   = nfs_bmap,
.vop_strategy   = nfs_strategy,
.vop_print  = nfs_print,
-   .vop_islocked   = vop_generic_islocked,
+   .vop_islocked   = nfs_islocked,
.vop_pathconf   = nfs_pathconf,
.vop_advlock= nfs_advlock,
.vop_bwrite = nfs_bwrite
@@ -183,10 +186,10 @@ struct vops nfs_specvops = {
.vop_fsync  = nfs_fsync,
.vop_inactive   = nfs_inactive,
.vop_reclaim= nfs_reclaim,
-   .vop_lock   = vop_generic_lock,
-   .vop_unlock = vop_generic_unlock,
+   .vop_lock   = nfs_lock,
+   .vop_unlock = nfs_unlock,
.vop_print  = nfs_print,
-   .vop_islocked   = vop_generic_islocked,
+   .vop_islocked   = nfs_islocked,
 
/* XXX: Keep in sync with spec_vops. */
.vop_lookup = vop_generic_lookup,
@@ -224,10 +227,10 @@ struct vops nfs_fifovops = {
.vop_fsync  = nfs_fsync,
.vop_inactive   = nfs_inactive,
.vop_reclaim= nfsfifo_reclaim,
-   .vop_lock   = vop_generic_lock,
-   .vop_unlock = vop_generic_unlock,
+   .vop_lock   = nfs_lock,
+   .vop_unlock = nfs_unlock,
.vop_print  = nfs_print,
-   .vop_islocked   = vop_generic_islocked,
+   .vop_islocked   = nfs_islocked,
.vop_bwrite = vop_generic_bwrite,
 
/* XXX: Keep in sync with fifo_vops. */
@@ -1033,6 +1036,54 @@ nfs_readlink(void *v)
return (nfs_bioread(vp, ap->a_uio, 0, ap->a_cred));
 }
 
+#ifdef DDB
+#include 
+#endif
+
+/*
+ * Lock an inode.
+ */
+int
+nfs_lock(void *v)
+{
+   struct vop_lock_args *ap = v;
+   struct vnode *vp = ap->a_vp;
+
+#if 0 //def DDB
+

Re: fdcopy() w/o memcpy()

2018-04-18 Thread Martin Pieuchot
On 16/04/18(Mon) 12:33, Martin Pieuchot wrote:
> Diff below is a rewrite/cleanup of fdcopy() to avoid using memcpy().
> 
> The problem with the memcpys is that they introduce a window where
> some 'struct file *' are duplicated without being refcounted.  We
> should instead use the following common idiom:
> 
>   FREF(fp);
>   fdp->fd_ofiles[i] = fp;
>   fdp->fd_ofileflags[i] = flags;
>   fd_used(fdp, i);
> 
> I'm also reusing fdinit() to simplify the function.

Updated diff that also copies fd_cmask and corrects an off-by-one in
a comparison, both errors pointed by visa@.

ok?

Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.151
diff -u -p -r1.151 kern_descrip.c
--- kern/kern_descrip.c 18 Apr 2018 09:59:09 -  1.151
+++ kern/kern_descrip.c 18 Apr 2018 10:04:13 -
@@ -994,18 +994,19 @@ struct filedesc *
 fdcopy(struct process *pr)
 {
struct filedesc *newfdp, *fdp = pr->ps_fd;
-   struct file **fpp;
int i;
 
+   newfdp = fdinit();
+
fdplock(fdp);
-   newfdp = pool_get(_pool, PR_WAITOK);
-   memcpy(newfdp, fdp, sizeof(struct filedesc));
-   if (newfdp->fd_cdir)
-   vref(newfdp->fd_cdir);
-   if (newfdp->fd_rdir)
-   vref(newfdp->fd_rdir);
-   newfdp->fd_refcnt = 1;
-   rw_init(>fd_lock, "fdlock");
+   if (fdp->fd_cdir) {
+   vref(fdp->fd_cdir);
+   newfdp->fd_cdir = fdp->fd_cdir;
+   }
+   if (fdp->fd_rdir) {
+   vref(fdp->fd_rdir);
+   newfdp->fd_rdir = fdp->fd_rdir;
+   }
 
/*
 * If the number of open files fits in the internal arrays
@@ -1013,45 +1014,34 @@ fdcopy(struct process *pr)
 * additional memory for the number of descriptors currently
 * in use.
 */
-   if (newfdp->fd_lastfile < NDFILE) {
-   newfdp->fd_ofiles = ((struct filedesc0 *) newfdp)->fd_dfiles;
-   newfdp->fd_ofileflags =
-   ((struct filedesc0 *) newfdp)->fd_dfileflags;
-   i = NDFILE;
-   } else {
+   if (fdp->fd_lastfile > NDFILE) {
/*
 * Compute the smallest multiple of NDEXTENT needed
 * for the file descriptors currently in use,
 * allowing the table to shrink.
 */
-   i = newfdp->fd_nfiles;
-   while (i >= 2 * NDEXTENT && i > newfdp->fd_lastfile * 2)
+   i = fdp->fd_nfiles;
+   while (i >= 2 * NDEXTENT && i > fdp->fd_lastfile * 2)
i /= 2;
-   newfdp->fd_ofiles = mallocarray(i, OFILESIZE, M_FILEDESC, 
M_WAITOK);
+   newfdp->fd_ofiles = mallocarray(i, OFILESIZE, M_FILEDESC,
+   M_WAITOK);
newfdp->fd_ofileflags = (char *) >fd_ofiles[i];
+   newfdp->fd_nfiles = i;
}
-   if (NDHISLOTS(i) <= NDHISLOTS(NDFILE)) {
-   newfdp->fd_himap =
-   ((struct filedesc0 *) newfdp)->fd_dhimap;
-   newfdp->fd_lomap =
-   ((struct filedesc0 *) newfdp)->fd_dlomap;
-   } else {
+   if (NDHISLOTS(newfdp->fd_nfiles) > NDHISLOTS(NDFILE)) {
newfdp->fd_himap = mallocarray(NDHISLOTS(i), sizeof(u_int),
M_FILEDESC, M_WAITOK);
newfdp->fd_lomap = mallocarray(NDLOSLOTS(i), sizeof(u_int),
M_FILEDESC, M_WAITOK);
}
-   newfdp->fd_nfiles = i;
-   memcpy(newfdp->fd_ofiles, fdp->fd_ofiles, i * sizeof(struct file *));
-   memcpy(newfdp->fd_ofileflags, fdp->fd_ofileflags, i * sizeof(char));
-   memcpy(newfdp->fd_himap, fdp->fd_himap, NDHISLOTS(i) * sizeof(u_int));
-   memcpy(newfdp->fd_lomap, fdp->fd_lomap, NDLOSLOTS(i) * sizeof(u_int));
-   fdpunlock(fdp);
+   newfdp->fd_freefile = fdp->fd_freefile;
+   newfdp->fd_flags = fdp->fd_flags;
+   newfdp->fd_cmask = fdp->fd_cmask;
+
+   for (i = 0; i <= fdp->fd_lastfile; i++) {
+   struct file *fp = fdp->fd_ofiles[i];
 
-   fdplock(newfdp);
-   fpp = newfdp->fd_ofiles;
-   for (i = 0; i <= newfdp->fd_lastfile; i++, fpp++)
-   if (*fpp != NULL) {
+   if (fp != NULL) {
/*
 * XXX Gruesome hack. If count gets too high, fail
 * to copy an fd, since fdcopy()'s callers do not
@@ -1060,22 +1050,18 @@ fdcopy(struct process *pr)
 * tied to the process that opened them to enforce
 * their internal consistency, so close them here.
 */
-   if ((*fpp)->f_count == LONG_MAX-2 ||
-   (*fpp)->f_type == DTYPE_KQUEUE)
-   fdremove(newfdp, i);
-   else
-  

Re: Should rm(1) -Pf change file permission?

2018-04-18 Thread Grégoire Jadi
Ingo Schwarze  writes:

Hello Ingo,

> Hi Gregoire,
>
> Gregoire Jadi wrote on Tue, Apr 17, 2018 at 11:44:41AM +0200:
>> Ingo Schwarze  writes:
>
>>> Feedback is welcome both on the general idea and on the specific
>>> implementation.
>
> The result of that feeback was "we don't want it, the whole
> idea of changing this is pointless and dangerous", so i deleted
> my patch quite some time ago.

Ok.

> The conversation resulted in a clarification in the manual page,
> though.

Thanks for the clarification. It's clear now it's a "best effort".

>> Thank you for working on this.  I have tested your patch
>> and both `rm -P` and `rm -Pf` work as expected.
>
> Sorry, it won't be committed, it was definitely rejected.
> Besides, it wasn't correct, there were race conditions.
> Working on improving it would be a waste of time.

Ok. So, this issue is closed. :)

Best,

> Yours,
>   Ingo