socket splicing sleeps in task thread

2016-12-20 Thread Alexander Bluhm
Hi,

As NET_LOCK() is a read/write lock, it can sleep in sotask().  So
the TASKQ_CANTSLEEP flag is no longer valid for the splicing thread.

ok?

bluhm

Index: kern/uipc_socket.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.170
diff -u -p -r1.170 uipc_socket.c
--- kern/uipc_socket.c  20 Dec 2016 21:15:36 -  1.170
+++ kern/uipc_socket.c  20 Dec 2016 23:29:00 -
@@ -1070,8 +1070,7 @@ sosplice(struct socket *so, int fd, off_
int  s, error = 0;
 
if (sosplice_taskq == NULL)
-   sosplice_taskq = taskq_create("sosplice", 1, IPL_SOFTNET,
-   TASKQ_CANTSLEEP);
+   sosplice_taskq = taskq_create("sosplice", 1, IPL_SOFTNET, 0);
if (sosplice_taskq == NULL)
return (ENOMEM);
 



Re: if attach/detach netlocks

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 06:47:31PM +0100, Mike Belopuhov wrote:
> @@ -1109,11 +1115,16 @@ if_clone_destroy(const char *name)
>   s = splnet();
>   if_down(ifp);
>   splx(s);
>   }
>  
> - return ((*ifc->ifc_destroy)(ifp));
> + /* XXXSMP breaks atomicity */
> + rw_exit_write();
> + ret = (*ifc->ifc_destroy)(ifp);
> + rw_enter_write();
> +
> + return (ret);
>  }

This broke pflow again.

panic: netlock: lock not held
Stopped at  Debugger+0x7:   leave
   TIDPIDUID PRFLAGS PFLAGS  CPU  COMMAND
*440943   4323  0 0x2  01  ifconfig
Debugger(d09facfd,f57a2ca8,d09d23c6,f57a2ca8,d76fd000) at Debugger+0x7
panic(d09d23c6,d09dc32f,f57a2cec,d76fd000,0) at panic+0x71
rw_assert_wrlock(d0b56f38,40,f57a2cec,d03e506b,d8e1eb00) at rw_assert_wrlock+0x
3d
rw_exit_write(d0b56f38,0,7a2d4c,d046b949,d76fd000) at rw_exit_write+0x19
pflow_clone_destroy(d76fd000,0,c0186943,d055d8ed,d96ad7a0) at pflow_clone_destr
oy+0x71
if_clone_destroy(f57a2e74,0,d76fd000,1,d76fd000) at if_clone_destroy+0x7b
ifioctl(d9587648,80206979,f57a2e74,d97e22d8,d956d26c) at ifioctl+0x1ed
soo_ioctl(d96405a8,80206979,f57a2e74,d97e22d8,d97b1f64) at soo_ioctl+0x21c
sys_ioctl(d97e22d8,f57a2f5c,f57a2f7c,0,f57a2fa8) at sys_ioctl+0x19f
syscall() at syscall+0x250
--- syscall (number 9) ---
0x14:
https://www.openbsd.org/ddb.html describes the minimum info required in bug
reports.  Insufficient info makes it difficult to find and fix bugs.
ddb{1}> 

Now we have two conflicting XXXSMP workarounds in if_clone_destroy()
and pflow_clone_destroy().

bluhm



Re: sleep.1: "while true;" -> "while :;"

2016-12-20 Thread sven falempin
On Tue, Dec 20, 2016 at 5:49 PM, Jason McIntyre  wrote:

> On Tue, Dec 20, 2016 at 10:58:40PM +0100, Michal Mazurek wrote:
> > While there is nothing wrong with "while true", "while :" is better
> > and used a lot more often in the source tree.
> >
> > OK?
> >
>
> i'm not sure why "while :" is better in this example, but "while true"
> is clearer, i think. allowing for differences in preference, is their a
> good reason to change what's there?
>
> jmc
>
> > Index: bin/sleep/sleep.1
> > ===
> > RCS file: /cvs/src/bin/sleep/sleep.1,v
> > retrieving revision 1.22
> > diff -u -p -r1.22 sleep.1
> > --- bin/sleep/sleep.1 16 Aug 2016 18:51:25 -  1.22
> > +++ bin/sleep/sleep.1 20 Dec 2016 21:46:07 -
> > @@ -94,7 +94,7 @@ job.
> >  .Pp
> >  To monitor the growth of a file without consuming too many resources:
> >  .Bd -literal -offset indent
> > -while true; do
> > +while :; do
> >   ls -l file
> >   sleep 5
> >  done
> >
> > --
> > Michal Mazurek
> >
>
>
Thanks,

i did not know a ksh nop was a thing.

I tend to agree with jmc,

void* p = NULL;
if (p) is neat but not clear like , if ( p != NULL )

as jw013 said:

I agree. true for a condition, : for a NOP. – jw013 *

*
http://unix.stackexchange.com/questions/37473/what-is-the-utility-of-the-command-in-shell-scripting-given-that-it-explicitl


-- 
-
() ascii ribbon campaign - against html e-mail
/\


Re: sleep.1: "while true;" -> "while :;"

2016-12-20 Thread Jason McIntyre
On Tue, Dec 20, 2016 at 10:58:40PM +0100, Michal Mazurek wrote:
> While there is nothing wrong with "while true", "while :" is better
> and used a lot more often in the source tree.
> 
> OK?
> 

i'm not sure why "while :" is better in this example, but "while true"
is clearer, i think. allowing for differences in preference, is their a
good reason to change what's there?

jmc

> Index: bin/sleep/sleep.1
> ===
> RCS file: /cvs/src/bin/sleep/sleep.1,v
> retrieving revision 1.22
> diff -u -p -r1.22 sleep.1
> --- bin/sleep/sleep.1 16 Aug 2016 18:51:25 -  1.22
> +++ bin/sleep/sleep.1 20 Dec 2016 21:46:07 -
> @@ -94,7 +94,7 @@ job.
>  .Pp
>  To monitor the growth of a file without consuming too many resources:
>  .Bd -literal -offset indent
> -while true; do
> +while :; do
>   ls -l file
>   sleep 5
>  done
> 
> -- 
> Michal Mazurek
> 



Re: mld6 & splsoftnet()

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 09:42:00PM +0100, Martin Pieuchot wrote:
> On 20/12/16(Tue) 20:01, Alexander Bluhm wrote:
> > On Tue, Dec 20, 2016 at 12:14:22PM +0100, Martin Pieuchot wrote:
> > > mld6_{start,stop}_listeting() are called in ioctl(2) context, thus at
> > > IPL_SOFTNET.
> > 
> > These are called from in6_addmulti() and in6_delmulti().  They still
> > have a splsoftnet() in them.  Please remove this first and put an
> > assert there.
> 
> That's mostly what my mail called 'inet6 ioctl(2) and splsoftnet()' do
> :)

Perhaps I should not work through my inbox backwards.

OK bluhm@



Re: inet6 ioctl(2) and splsoftnet()

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 11:50:40AM +0100, Martin Pieuchot wrote:
> Interface ioctl(2)s are now always executed at IPL_SOFTNET, so remove
> recursive splsoftnet() calls.

Could you put an splsoftassert into in6_addmulti() and in6_delmulti()?

> ok?

anyway OK bluhm@

> 
> Index: netinet6/in6.c
> ===
> RCS file: /cvs/src/sys/netinet6/in6.c,v
> retrieving revision 1.195
> diff -u -p -r1.195 in6.c
> --- netinet6/in6.c28 Nov 2016 14:14:39 -  1.195
> +++ netinet6/in6.c20 Dec 2016 10:48:37 -
> @@ -190,7 +190,6 @@ in6_ioctl(u_long cmd, caddr_t data, stru
>   struct  in6_ifaddr *ia6 = NULL;
>   struct  in6_aliasreq *ifra = (struct in6_aliasreq *)data;
>   struct sockaddr_in6 *sa6;
> - int s;
>  
>   if (ifp == NULL)
>   return (EOPNOTSUPP);
> @@ -423,14 +422,10 @@ in6_ioctl(u_long cmd, caddr_t data, stru
>* and link it to the list. try to enable inet6 if there
>* is no link-local yet.
>*/
> - s = splsoftnet();
>   error = in6_ifattach(ifp);
> - if (error != 0) {
> - splx(s);
> + if (error != 0)
>   return (error);
> - }
>   error = in6_update_ifa(ifp, ifra, ia6);
> - splx(s);
>   if (error != 0)
>   return (error);
>  
> @@ -458,24 +453,19 @@ in6_ioctl(u_long cmd, caddr_t data, stru
>   break;  /* No need to install a connected route. */
>   }
>  
> - s = splsoftnet();
>   error = rt_ifa_add(>ia_ifa, RTF_CLONING | RTF_CONNECTED,
>   ia6->ia_ifa.ifa_addr);
>   if (error) {
>   in6_purgeaddr(>ia_ifa);
> - splx(s);
>   return (error);
>   }
>   dohooks(ifp->if_addrhooks, 0);
> - splx(s);
>   break;
>   }
>  
>   case SIOCDIFADDR_IN6:
> - s = splsoftnet();
>   in6_purgeaddr(>ia_ifa);
>   dohooks(ifp->if_addrhooks, 0);
> - splx(s);
>   break;
>  
>   default:
> @@ -1231,7 +1221,6 @@ in6_addmulti(struct in6_addr *maddr6, st
>  {
>   struct  in6_ifreq ifr;
>   struct  in6_multi *in6m;
> - int s;
>  
>   *errorp = 0;
>   /*
> @@ -1277,10 +1266,8 @@ in6_addmulti(struct in6_addr *maddr6, st
>   return (NULL);
>   }
>  
> - s = splsoftnet();
>   TAILQ_INSERT_HEAD(>if_maddrlist, >in6m_ifma,
>   ifma_list);
> - splx(s);
>  
>   /*
>* Let MLD6 know that we have joined a new IP6 multicast
> @@ -1300,7 +1287,6 @@ in6_delmulti(struct in6_multi *in6m)
>  {
>   struct  in6_ifreq ifr;
>   struct  ifnet *ifp;
> - int s;
>  
>   if (--in6m->in6m_refcnt == 0) {
>   /*
> @@ -1321,10 +1307,8 @@ in6_delmulti(struct in6_multi *in6m)
>   ifr.ifr_addr.sin6_addr = in6m->in6m_addr;
>   (*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t));
>  
> - s = splsoftnet();
>   TAILQ_REMOVE(>if_maddrlist, >in6m_ifma,
>   ifma_list);
> - splx(s);
>   }
>   if_put(ifp);
>  



sleep.1: "while true;" -> "while :;"

2016-12-20 Thread Michal Mazurek
While there is nothing wrong with "while true", "while :" is better
and used a lot more often in the source tree.

OK?

Index: bin/sleep/sleep.1
===
RCS file: /cvs/src/bin/sleep/sleep.1,v
retrieving revision 1.22
diff -u -p -r1.22 sleep.1
--- bin/sleep/sleep.1   16 Aug 2016 18:51:25 -  1.22
+++ bin/sleep/sleep.1   20 Dec 2016 21:46:07 -
@@ -94,7 +94,7 @@ job.
 .Pp
 To monitor the growth of a file without consuming too many resources:
 .Bd -literal -offset indent
-while true; do
+while :; do
ls -l file
sleep 5
 done

-- 
Michal Mazurek



Re: loadfirmware.9: mention /mnt/etc/firmware

2016-12-20 Thread Theo de Raadt
I do not think this should be documented.

It is a trick.  Noone should rely upon it, because later on we might
use a different trick.  Then this documentation would be come false,
and it is another piece for us to worry about.

> /mnt/etc/firmware is only mentioned in plus44.html. Mention it in
> loadfirmware(9).
> 
> It was concluded that the FAQ is not a proper place to talk about
> this directory.
> 
> Index: ./share/man/man9/loadfirmware.9
> ===
> RCS file: /cvs/src/share/man/man9/loadfirmware.9,v
> retrieving revision 1.4
> diff -u -p -r1.4 loadfirmware.9
> --- ./share/man/man9/loadfirmware.9   4 Jun 2013 19:27:08 -   1.4
> +++ ./share/man/man9/loadfirmware.9   20 Dec 2016 21:37:42 -
> @@ -32,6 +32,10 @@ function loads a firmware from the file 
>  .Ar filename
>  in the directory
>  .Pa /etc/firmware .
> +In addition to that, if the kernel is a ramdisk kernel, the extra
> +directory
> +.Pa /mnt/etc/firmware
> +will also be used for the lookup.
>  Memory for the firmware is allocated using
>  .Xr malloc 9
>  with type
> 
> -- 
> Michal Mazurek
> 



loadfirmware.9: mention /mnt/etc/firmware

2016-12-20 Thread Michal Mazurek
/mnt/etc/firmware is only mentioned in plus44.html. Mention it in
loadfirmware(9).

It was concluded that the FAQ is not a proper place to talk about
this directory.

Index: ./share/man/man9/loadfirmware.9
===
RCS file: /cvs/src/share/man/man9/loadfirmware.9,v
retrieving revision 1.4
diff -u -p -r1.4 loadfirmware.9
--- ./share/man/man9/loadfirmware.9 4 Jun 2013 19:27:08 -   1.4
+++ ./share/man/man9/loadfirmware.9 20 Dec 2016 21:37:42 -
@@ -32,6 +32,10 @@ function loads a firmware from the file 
 .Ar filename
 in the directory
 .Pa /etc/firmware .
+In addition to that, if the kernel is a ramdisk kernel, the extra
+directory
+.Pa /mnt/etc/firmware
+will also be used for the lookup.
 Memory for the firmware is allocated using
 .Xr malloc 9
 with type

-- 
Michal Mazurek



Re: PATCH: Tell what trailing asterisk in sa output means

2016-12-20 Thread Jason McIntyre
On Tue, Dec 20, 2016 at 09:27:14PM +0300, Vadim Zhukov wrote:
> 2016-12-20 20:30 GMT+03:00 Theo de Raadt :
> >> I had to dig into sa sources to get know what trailing '*' in
> >> command names mean. IMHO, that's not how manuals should work. :)
> >>
> >> Okay?
> >
> > That seems a little awkward mentioning fork directly.  How about
> >
> > Children which have not yet called
> > .Xr execve 2
> > will have
> > .Sq *
> > appended to their command names.
> 
> Seems fine for me as well.
> 

it reads fine to me too, so please go ahead and fix. the last two
paragraphs of sa(8) are a bit clumsy i think since they more or less
repeat what -m and -u do and then, outwith the descriptions of -m and
-u, say which options are affected by -m and -u. i think we could clean
that whole bit up, and move this new text up, but it could easily be done
separately.

jmc



Re: More NET_LOCK()

2016-12-20 Thread Martin Pieuchot
On 20/12/16(Tue) 19:51, Alexander Bluhm wrote:
> [...] 
> The sosplice thread is only used for TCP.  UDP runs in the softnet
> thread.  As we are reading 64 bit values, we need some sort of
> locking.  Now everything is in the kernel lock, but when we start
> working to unlock the socket layer, be need a flag that something
> has to be done here.  Can we keep the splsoftnet() for that?  Or
> replace it with a NET_LOCK()?

I think that we should keep the code as simple as possible until we
know for sure what is the next step.  In other words if the KERNEL_LOCK
is what's needed then just keep that.

I don't want to put a NET_LOCK() when it is not needed.  Because I know
that a future step will be to shrink/remove the NET_LOCK().  So I'd
rather not make this task more complicated than it is already.

When we start working to unlock the socket layer, and we should, the
person that will lead the effort will come with a solution.  All the
fields will have to be audited anyway, so I'm not too worried.

Anyway I left it for the moment.



Re: mld6 & splsoftnet()

2016-12-20 Thread Martin Pieuchot
On 20/12/16(Tue) 20:01, Alexander Bluhm wrote:
> On Tue, Dec 20, 2016 at 12:14:22PM +0100, Martin Pieuchot wrote:
> > mld6_{start,stop}_listeting() are called in ioctl(2) context, thus at
> > IPL_SOFTNET.
> 
> These are called from in6_addmulti() and in6_delmulti().  They still
> have a splsoftnet() in them.  Please remove this first and put an
> assert there.

That's mostly what my mail called 'inet6 ioctl(2) and splsoftnet()' do
:)



Re: mld6 & splsoftnet()

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 12:14:22PM +0100, Martin Pieuchot wrote:
> mld6_{start,stop}_listeting() are called in ioctl(2) context, thus at
> IPL_SOFTNET.

These are called from in6_addmulti() and in6_delmulti().  They still
have a splsoftnet() in them.  Please remove this first and put an
assert there.

> mld6_fasttimeo(), like all *fasttimeo() routines is now always called
> at IPL_SOFTNET.

OK bluhm@ for that.

> 
> So let's get rid of these recursive splsoftnet(), ok?
> 
> Index: netinet6/mld6.c
> ===
> RCS file: /cvs/src/sys/netinet6/mld6.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 mld6.c
> --- netinet6/mld6.c   5 Jul 2016 10:17:14 -   1.48
> +++ netinet6/mld6.c   20 Dec 2016 10:59:27 -
> @@ -117,8 +117,6 @@ mld6_init(void)
>  void
>  mld6_start_listening(struct in6_multi *in6m)
>  {
> - int s = splsoftnet();
> -
>   /*
>* RFC2710 page 10:
>* The node never sends a Report or Done for the link-scope all-nodes
> @@ -139,14 +137,11 @@ mld6_start_listening(struct in6_multi *i
>   in6m->in6m_state = MLD_IREPORTEDLAST;
>   mld_timers_are_running = 1;
>   }
> - splx(s);
>  }
>  
>  void
>  mld6_stop_listening(struct in6_multi *in6m)
>  {
> - int s = splsoftnet();
> -
>   mld_all_nodes_linklocal.s6_addr16[1] = htons(in6m->in6m_ifidx);/* XXX */
>   mld_all_routers_linklocal.s6_addr16[1] =
>   htons(in6m->in6m_ifidx); /* XXX: necessary when mrouting */
> @@ -156,7 +151,6 @@ mld6_stop_listening(struct in6_multi *in
>   __IPV6_ADDR_MC_SCOPE(>in6m_addr) > 
> __IPV6_ADDR_SCOPE_INTFACELOCAL)
>   mld6_sendpkt(in6m, MLD_LISTENER_DONE,
>   _all_routers_linklocal);
> - splx(s);
>  }
>  
>  void
> @@ -331,7 +325,6 @@ void
>  mld6_fasttimeo(void)
>  {
>   struct ifnet *ifp;
> - int s;
>  
>   /*
>* Quick check to see if any work needs to be done, in order
> @@ -340,11 +333,9 @@ mld6_fasttimeo(void)
>   if (!mld_timers_are_running)
>   return;
>  
> - s = splsoftnet();
>   mld_timers_are_running = 0;
>   TAILQ_FOREACH(ifp, , if_list)
>   mld6_checktimer(ifp);
> - splx(s);
>  }
>  
>  void



Re: More NET_LOCK()

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 01:55:03PM +0100, Martin Pieuchot wrote:
> rzalamena@ found that the IPv4 multicast code have two code paths ending
> in ip_output() that are not yet taking the NET_LOCK: sosetopt() and
> pffasttimo().

I see this also on my regress test machine.

splassert: ip_output: want 1 have 0
Starting stack trace...
ip_output(1,0,d09e32c1,d97702ec,0) at ip_output+0x6d
ip_output(d8edd000,d9804200,0,0,f57a2d30) at ip_output+0x6d
igmp_sendpkt(d384c030,d76fc1c0,16,0,0) at igmp_sendpkt+0x119
igmp_joingroup(d76fc1c0,80206931,f57a2dac,f57a2dc8,0) at igmp_joingroup+0x79
in_addmulti(d8edda14,d384c030,0,d03cc600,d8edda08) at in_addmulti+0x139
ip_setmoptions(c,d8dcebbc,d8edda00,0,d0b56f38) at ip_setmoptions+0x54d
ip_ctloutput(1,d8e4d6a0,0,c,f57a2eac) at ip_ctloutput+0x577
sosetopt(d8e4d6a0,0,c,d8edda00,d0bbe320) at sosetopt+0x7e
sys_setsockopt(d973fe28,f57a2f5c,f57a2f7c,0,286) at sys_setsockopt+0x14f
syscall() at syscall+0x250

splassert: ip_output: want 1 have 0
Starting stack trace...
ip_output(1,0,d09e32c1,d3831980,f52eed0c) at ip_output+0x6d
ip_output(d8eda300,d9804200,0,0,f52eed78) at ip_output+0x6d
igmp_sendpkt(d3701c00,d3d14240,16,0,da7a) at igmp_sendpkt+0x119
igmp_checktimer(d3701c00,d09e297d,d0952d9e,8541cb9c,80051622) at 
igmp_checktimer+0x7e
igmp_fasttimo(40,f52eee60,30,8541cb9c,80051622) at igmp_fasttimo+0x4a
pffasttimo(d0b7b624,f52eee60,f52eee40,d02031b9,200282) at pffasttimo+0x39
timeout_run(d0b7b624,f52eee90,d03a04e4,1d,f52eeecc) at timeout_run+0x40
softclock(0,f52eeeb0,d0866c11,d0c0256c,d0bbe320) at softclock+0x117
softintr_dispatch(0) at softintr_dispatch+0x5f
Xsoftclock() at Xsoftclock+0x12

> Diff below fixes that and include pfslowtimo() as well as sogetopt() for
> completeness.

OK bluhm@

> While here remove a superfluous splsoftnet() which doesn't protect
> anything in the SO_SPLICE case since we're running the input path
> in a thread. 

The sosplice thread is only used for TCP.  UDP runs in the softnet
thread.  As we are reading 64 bit values, we need some sort of
locking.  Now everything is in the kernel lock, but when we start
working to unlock the socket layer, be need a flag that something
has to be done here.  Can we keep the splsoftnet() for that?  Or
replace it with a NET_LOCK()?

bluhm

> Index: kern/uipc_domain.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_domain.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 uipc_domain.c
> --- kern/uipc_domain.c22 Nov 2016 10:32:31 -  1.46
> +++ kern/uipc_domain.c20 Dec 2016 10:18:50 -
> @@ -99,8 +99,8 @@ domaininit(void)
>   max_linkhdr = 64;
>  
>   max_hdr = max_linkhdr + max_protohdr;
> - timeout_set(_timeout, pffasttimo, _timeout);
> - timeout_set(_timeout, pfslowtimo, _timeout);
> + timeout_set_proc(_timeout, pffasttimo, _timeout);
> + timeout_set_proc(_timeout, pfslowtimo, _timeout);
>   timeout_add(_timeout, 1);
>   timeout_add(_timeout, 1);
>  }
> @@ -238,13 +238,13 @@ pfslowtimo(void *arg)
>   struct protosw *pr;
>   int i, s;
>  
> - s = splsoftnet();
> + NET_LOCK(s);
>   for (i = 0; (dp = domains[i]) != NULL; i++) {
>   for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++)
>   if (pr->pr_slowtimo)
>   (*pr->pr_slowtimo)();
>   }
> - splx(s);
> + NET_UNLOCK(s);
>   timeout_add_msec(to, 500);
>  }
>  
> @@ -256,12 +256,12 @@ pffasttimo(void *arg)
>   struct protosw *pr;
>   int i, s;
>  
> - s = splsoftnet();
> + NET_LOCK(s);
>   for (i = 0; (dp = domains[i]) != NULL; i++) {
>   for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++)
>   if (pr->pr_fasttimo)
>   (*pr->pr_fasttimo)();
>   }
> - splx(s);
> + NET_UNLOCK(s);
>   timeout_add_msec(to, 200);
>  }
> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.169
> diff -u -p -r1.169 uipc_socket.c
> --- kern/uipc_socket.c19 Dec 2016 08:36:49 -  1.169
> +++ kern/uipc_socket.c20 Dec 2016 10:22:18 -
> @@ -1555,10 +1555,10 @@ sosetopt(struct socket *so, int level, i
>  
>   if (level != SOL_SOCKET) {
>   if (so->so_proto && so->so_proto->pr_ctloutput) {
> - s = splsoftnet();
> + NET_LOCK(s);
>   error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
>   level, optname, );
> - splx(s);
> + NET_UNLOCK(s);
>   return (error);
>   }
>   error = ENOPROTOOPT;
> @@ -1702,10 +1702,10 @@ sosetopt(struct socket *so, int level, i
>   struct domain *dom = so->so_proto->pr_domain;
>  
>

Re: if attach/detach netlocks

2016-12-20 Thread Martin Pieuchot
On 20/12/16(Tue) 18:47, Mike Belopuhov wrote:
> On Tue, Dec 20, 2016 at 17:06 +0100, Martin Pieuchot wrote:
> > 
> > You'll need to release the lock before calling ifc->ifc_create in
> > if_clone_create() and do the same dance in if_clone_destroy().
> >
> 
> Indeed.
> 
> > But I think that's the way to go.  If you can create/destroy
> > pseudo-interface without panicing we're good.
> >
> 
> Seems to work here, but it feels wrong.

I don't think we can do much better, let's go with that we'll refine
later.

ok mpi@

> 
> > >  void
> > > @@ -941,6 +941,7 @@ if_detach(struct ifnet *ifp)
> > >  
> > >   ifq_clr_oactive(>if_snd);
> > >  
> > > + rw_enter_write();
> > >   s = splnet();
> > >   /* Other CPUs must not have a reference before we start destroying. */
> > >   if_idxmap_remove(ifp);
> > > @@ -1017,6 +1018,7 @@ if_detach(struct ifnet *ifp)
> > >   /* Announce that the interface is gone. */
> > >   rt_ifannouncemsg(ifp, IFAN_DEPARTURE);
> > >   splx(s);
> > > + rw_exit_write();
> > >  
> > >   ifq_destroy(>if_snd);
> > >  }
> > 
> > Please keep the NET_LOCK/UNLOCK() macro.  The places where we
> > grab/release the `netlock' directly are places that need attention.
> 
> Done.
> 
> diff --git sys/net/if.c sys/net/if.c
> index 1e103564710..bd8b9267d5f 100644
> --- sys/net/if.c
> +++ sys/net/if.c
> @@ -523,15 +523,15 @@ if_attachhead(struct ifnet *ifp)
>  void
>  if_attach(struct ifnet *ifp)
>  {
>   int s;
>  
> - s = splsoftnet();
> + NET_LOCK(s);
>   if_attach_common(ifp);
>   TAILQ_INSERT_TAIL(, ifp, if_list);
>   if_attachsetup(ifp);
> - splx(s);
> + NET_UNLOCK(s);
>  }
>  
>  void
>  if_attach_common(struct ifnet *ifp)
>  {
> @@ -939,10 +939,11 @@ if_detach(struct ifnet *ifp)
>   /* Undo pseudo-driver changes. */
>   if_deactivate(ifp);
>  
>   ifq_clr_oactive(>if_snd);
>  
> + NET_LOCK(s);
>   s = splnet();
>   /* Other CPUs must not have a reference before we start destroying. */
>   if_idxmap_remove(ifp);
>  
>   ifp->if_start = if_detached_start;
> @@ -1015,10 +1016,11 @@ if_detach(struct ifnet *ifp)
>   }
>  
>   /* Announce that the interface is gone. */
>   rt_ifannouncemsg(ifp, IFAN_DEPARTURE);
>   splx(s);
> + NET_UNLOCK(s);
>  
>   ifq_destroy(>if_snd);
>  }
>  
>  /*
> @@ -1068,11 +1070,14 @@ if_clone_create(const char *name, int rdomain)
>   return (EINVAL);
>  
>   if (ifunit(name) != NULL)
>   return (EEXIST);
>  
> + /* XXXSMP breaks atomicity */
> + rw_exit_write();
>   ret = (*ifc->ifc_create)(ifc, unit);
> + rw_enter_write();
>  
>   if (ret != 0 || (ifp = ifunit(name)) == NULL)
>   return (ret);
>  
>   if_addgroup(ifp, ifc->ifc_name);
> @@ -1088,10 +1093,11 @@ if_clone_create(const char *name, int rdomain)
>  int
>  if_clone_destroy(const char *name)
>  {
>   struct if_clone *ifc;
>   struct ifnet *ifp;
> + int ret;
>  
>   splsoftassert(IPL_SOFTNET);
>  
>   ifc = if_clone_lookup(name, NULL);
>   if (ifc == NULL)
> @@ -1109,11 +1115,16 @@ if_clone_destroy(const char *name)
>   s = splnet();
>   if_down(ifp);
>   splx(s);
>   }
>  
> - return ((*ifc->ifc_destroy)(ifp));
> + /* XXXSMP breaks atomicity */
> + rw_exit_write();
> + ret = (*ifc->ifc_destroy)(ifp);
> + rw_enter_write();
> +
> + return (ret);
>  }
>  
>  /*
>   * Look up a network interface cloner.
>   */



Re: PATCH: Tell what trailing asterisk in sa output means

2016-12-20 Thread Vadim Zhukov
2016-12-20 20:30 GMT+03:00 Theo de Raadt :
>> I had to dig into sa sources to get know what trailing '*' in
>> command names mean. IMHO, that's not how manuals should work. :)
>>
>> Okay?
>
> That seems a little awkward mentioning fork directly.  How about
>
> Children which have not yet called
> .Xr execve 2
> will have
> .Sq *
> appended to their command names.

Seems fine for me as well.

--
  WBR,
  Vadim Zhukov



Re: if attach/detach netlocks

2016-12-20 Thread Mike Belopuhov
On Tue, Dec 20, 2016 at 17:06 +0100, Martin Pieuchot wrote:
> 
> You'll need to release the lock before calling ifc->ifc_create in
> if_clone_create() and do the same dance in if_clone_destroy().
>

Indeed.

> But I think that's the way to go.  If you can create/destroy
> pseudo-interface without panicing we're good.
>

Seems to work here, but it feels wrong.

> >  void
> > @@ -941,6 +941,7 @@ if_detach(struct ifnet *ifp)
> >  
> > ifq_clr_oactive(>if_snd);
> >  
> > +   rw_enter_write();
> > s = splnet();
> > /* Other CPUs must not have a reference before we start destroying. */
> > if_idxmap_remove(ifp);
> > @@ -1017,6 +1018,7 @@ if_detach(struct ifnet *ifp)
> > /* Announce that the interface is gone. */
> > rt_ifannouncemsg(ifp, IFAN_DEPARTURE);
> > splx(s);
> > +   rw_exit_write();
> >  
> > ifq_destroy(>if_snd);
> >  }
> 
> Please keep the NET_LOCK/UNLOCK() macro.  The places where we
> grab/release the `netlock' directly are places that need attention.

Done.

diff --git sys/net/if.c sys/net/if.c
index 1e103564710..bd8b9267d5f 100644
--- sys/net/if.c
+++ sys/net/if.c
@@ -523,15 +523,15 @@ if_attachhead(struct ifnet *ifp)
 void
 if_attach(struct ifnet *ifp)
 {
int s;
 
-   s = splsoftnet();
+   NET_LOCK(s);
if_attach_common(ifp);
TAILQ_INSERT_TAIL(, ifp, if_list);
if_attachsetup(ifp);
-   splx(s);
+   NET_UNLOCK(s);
 }
 
 void
 if_attach_common(struct ifnet *ifp)
 {
@@ -939,10 +939,11 @@ if_detach(struct ifnet *ifp)
/* Undo pseudo-driver changes. */
if_deactivate(ifp);
 
ifq_clr_oactive(>if_snd);
 
+   NET_LOCK(s);
s = splnet();
/* Other CPUs must not have a reference before we start destroying. */
if_idxmap_remove(ifp);
 
ifp->if_start = if_detached_start;
@@ -1015,10 +1016,11 @@ if_detach(struct ifnet *ifp)
}
 
/* Announce that the interface is gone. */
rt_ifannouncemsg(ifp, IFAN_DEPARTURE);
splx(s);
+   NET_UNLOCK(s);
 
ifq_destroy(>if_snd);
 }
 
 /*
@@ -1068,11 +1070,14 @@ if_clone_create(const char *name, int rdomain)
return (EINVAL);
 
if (ifunit(name) != NULL)
return (EEXIST);
 
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
ret = (*ifc->ifc_create)(ifc, unit);
+   rw_enter_write();
 
if (ret != 0 || (ifp = ifunit(name)) == NULL)
return (ret);
 
if_addgroup(ifp, ifc->ifc_name);
@@ -1088,10 +1093,11 @@ if_clone_create(const char *name, int rdomain)
 int
 if_clone_destroy(const char *name)
 {
struct if_clone *ifc;
struct ifnet *ifp;
+   int ret;
 
splsoftassert(IPL_SOFTNET);
 
ifc = if_clone_lookup(name, NULL);
if (ifc == NULL)
@@ -1109,11 +1115,16 @@ if_clone_destroy(const char *name)
s = splnet();
if_down(ifp);
splx(s);
}
 
-   return ((*ifc->ifc_destroy)(ifp));
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
+   ret = (*ifc->ifc_destroy)(ifp);
+   rw_enter_write();
+
+   return (ret);
 }
 
 /*
  * Look up a network interface cloner.
  */



NET_LOCK() pr_sysctl

2016-12-20 Thread Alexander Bluhm
Hi,

I have seen spl softnet assert failures like this.

splassert: sowwakeup: want 1 have 0
Starting stack trace...
sowwakeup(1,0,d09d8bd7,d03ba9f9,40) at sowwakeup+0x3f
sowwakeup(db549818,d6dc850e,f54d9be8,0,dae674d4) at sowwakeup+0x3f
soisdisconnected(db549818,0,db549818,2,f54d9c00) at soisdisconnected+0x2c
tcp_close(dae674d4,35,ff94,bc51bc0a,a88c) at tcp_close+0x97
tcp_ident(0,f54d9ef8,cf7e2a50,20c,1) at tcp_ident+0x302
tcp_sysctl(f54d9ed4,1,0,f54d9ef8,cf7e2a50) at tcp_sysctl+0x28b
sys_sysctl(d9f0188c,f54d9f5c,f54d9f7c,0,f54d9fa8) at sys_sysctl+0x20e
syscall() at syscall+0x250

Obviosly a NET_LOCK() is missing in tcp_sysctl().

I think it is better to place the lock into net_sysctl() where all
the protocol sysctls are called via pr_sysctl.  Then we don't have
to decide each case individually.  As calling sysctl(2) is in the
slow path, doing fine grained locking has no benefit.  Many sysctl
cases copy out a struct.  Having a lock around that keeps the struct
consistent.

ok?

bluhm

Index: kern/uipc_domain.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_domain.c,v
retrieving revision 1.46
diff -u -p -r1.46 uipc_domain.c
--- kern/uipc_domain.c  22 Nov 2016 10:32:31 -  1.46
+++ kern/uipc_domain.c  20 Dec 2016 14:57:26 -
@@ -165,7 +165,7 @@ net_sysctl(int *name, u_int namelen, voi
 {
struct domain *dp;
struct protosw *pr;
-   int family, protocol;
+   int s, error, family, protocol;
 
/*
 * All sysctl names at this level are nonterminal.
@@ -199,18 +199,26 @@ net_sysctl(int *name, u_int namelen, voi
 #ifdef MPLS
/* XXX WARNING: big fat ugly hack */
/* stupid net.mpls is special as it does not have a protocol */
-   if (family == PF_MPLS)
-   return (dp->dom_protosw[0].pr_sysctl(name + 1, namelen - 1,
-   oldp, oldlenp, newp, newlen));
+   if (family == PF_MPLS) {
+   NET_LOCK(s);
+   error = (dp->dom_protosw[0].pr_sysctl)(name + 1, namelen - 1,
+   oldp, oldlenp, newp, newlen);
+   NET_UNLOCK(s);
+   return (error);
+   }
 #endif
 
if (namelen < 3)
return (EISDIR);/* overloaded */
protocol = name[1];
for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++)
-   if (pr->pr_protocol == protocol && pr->pr_sysctl)
-   return ((*pr->pr_sysctl)(name + 2, namelen - 2,
-   oldp, oldlenp, newp, newlen));
+   if (pr->pr_protocol == protocol && pr->pr_sysctl) {
+   NET_LOCK(s);
+   error = (*pr->pr_sysctl)(name + 2, namelen - 2,
+   oldp, oldlenp, newp, newlen);
+   NET_UNLOCK(s);
+   return (error);
+   }
return (ENOPROTOOPT);
 }
 
Index: net/rtsock.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/rtsock.c,v
retrieving revision 1.211
diff -u -p -r1.211 rtsock.c
--- net/rtsock.c19 Dec 2016 08:36:49 -  1.211
+++ net/rtsock.c20 Dec 2016 15:12:06 -
@@ -1563,12 +1563,14 @@ int
 sysctl_rtable(int *name, u_int namelen, void *where, size_t *given, void *new,
 size_t newlen)
 {
-   int  i, s, error = EINVAL;
+   int  i, error = EINVAL;
u_char   af;
struct walkarg   w;
struct rt_tableinfo  tableinfo;
u_inttableid = 0;
 
+   NET_ASSERT_LOCKED();
+
if (new)
return (EPERM);
if (namelen < 3 || namelen > 4)
@@ -1588,7 +1590,6 @@ sysctl_rtable(int *name, u_int namelen, 
} else
tableid = curproc->p_p->ps_rtableid;
 
-   s = splsoftnet();
switch (w.w_op) {
case NET_RT_DUMP:
case NET_RT_FLAGS:
@@ -1611,25 +1612,20 @@ sysctl_rtable(int *name, u_int namelen, 
case NET_RT_STATS:
error = sysctl_rdstruct(where, given, new,
, sizeof(rtstat));
-   splx(s);
return (error);
case NET_RT_TABLE:
tableid = w.w_arg;
-   if (!rtable_exists(tableid)) {
-   splx(s);
+   if (!rtable_exists(tableid))
return (ENOENT);
-   }
tableinfo.rti_tableid = tableid;
tableinfo.rti_domainid = rtable_l2(tableid);
error = sysctl_rdstruct(where, given, new,
, sizeof(tableinfo));
-   splx(s);
return (error);
case NET_RT_IFNAMES:
error = sysctl_ifnames();
break;
}
-   splx(s);
free(w.w_tmem, M_RTABLE, 0);
w.w_needed += w.w_given;
   

Re: PATCH: Tell what trailing asterisk in sa output means

2016-12-20 Thread Theo de Raadt
> I had to dig into sa sources to get know what trailing '*' in
> command names mean. IMHO, that's not how manuals should work. :)
> 
> Okay?

That seems a little awkward mentioning fork directly.  How about

Children which have not yet called
.Xr execve 2
will have
.Sq *
appended to their command names.


> Index: sa.8
> ===
> RCS file: /cvs/src/usr.sbin/sa/sa.8,v
> retrieving revision 1.19
> diff -u -p -r1.19 sa.8
> --- sa.8  16 Jul 2013 11:13:34 -  1.19
> +++ sa.8  20 Dec 2016 12:26:51 -
> @@ -205,6 +205,14 @@ flag is specified, only the
>  and
>  .Fl s
>  flags are honored.
> +.Pp
> +Processes created with
> +.Xr fork 2
> +without
> +.Xr exec 3
> +will have
> +.Sq *
> +appended to their command names.
>  .Sh FILES
>  .Bl -tag -width /var/account/usracct -compact
>  .It Pa /var/account/acct
> 



Re: [patch] Minor corrections to xenocara man pages.

2016-12-20 Thread Ingo Schwarze
Hi Salvador,

thanks for noticing defects and providing patches!

Salvador Sabaini wrote on Wed, Dec 14, 2016 at 12:55:08PM -0300:

> I've just found man references in the SEE ALSO section to xfs(1),
> the X font server which was unlinked from base in 5.7, in the
> following man pages:
> 
> fslsfonts(1)
> fstobdf(1)
> mkfontdir(1)
> mkfontscale(1)
> showfont(1)
> xfsinfo(1)
> Xserver(1)
> X(7)

We generally avoid changing X.org manuals except for unusually
severe issues, in order to keep the tree managable for the
Xenocara maintainers.  Dead links are clearly below the threshold
we should apply for local patching, in particular when these dead
links refer to software that may still exist on other operating
systems.

If xfs(1) still exists upstream, even though it was deleted from
OpenBSD, these links should probably just stay.

If xfs(1) no longer exists at all, not even upstream, please
report these required deletions upstream.

> Also, in X(7) there's a reference to fsinfo(1), which as per
> xfsinfo(1) HISTORY section, it was renamed to xfsinfo "to avoid
> a clash with the fsinfo utility from the Berkeley automounter amd".

Please check whether that was fixed upstream, and if not, please
report it there.

> I'm providing these trivial patches to address them, in case
> that helps.

In the base system, we appreciate such patches a lot.  In Xenocara,
i hope you don't mind working with upstream, then everybody gets
your improvements and we also get them automatically with the
next regular update of the components in question.

Thanks again,
  Ingo



Re: if attach/detach netlocks

2016-12-20 Thread Martin Pieuchot
On 20/12/16(Tue) 16:33, Mike Belopuhov wrote:
> Attach:
> 
>   splassert: sowakeup: want 1 have 0
>   Starting stack trace...
>   sowakeup() at sowakeup+0x41
>   sorwakeup() at sorwakeup+0xb4
>   route_input() at route_input+0x2d7
>   if_attach() at if_attach+0x58
>   xnf_attach() at xnf_attach+0x45f
>   config_attach() at config_attach+0x1bc
>   xen_attach_device() at xen_attach_device+0x146
>   xen_hotplug() at xen_hotplug+0x23f
>   taskq_thread() at taskq_thread+0x6c
>   end trace frame: 0x0, count: 248
>   End of stack trace.
> 
> Detach:
> 
>   splassert: sowakeup: want 1 have 0
>   Starting stack trace...
>   sowakeup() at sowakeup+0x41
>   sorwakeup() at sorwakeup+0xb4
>   route_input() at route_input+0x2d7
>   if_detach() at if_detach+0x27e
>   xnf_detach() at xnf_detach+0x4d
>   config_detach() at config_detach+0x13c
>   xen_hotplug() at xen_hotplug+0x2e5
>   taskq_thread() at taskq_thread+0x6c
>   end trace frame: 0x0, count: 249
>   End of stack trace.
>   xnf2 detached
> 
> The taskq running xen_hotplug is KERNEL_LOCK'ed.  The diff below solves
> the issue, but is it any good?
> 
> diff --git sys/net/if.c sys/net/if.c
> index 1e103564710..a5c25b89560 100644
> --- sys/net/if.c
> +++ sys/net/if.c
> @@ -525,11 +525,11 @@ if_attach(struct ifnet *ifp)
>  {
>   int s;
>  
> - s = splsoftnet();
> + NET_LOCK(s);
>   if_attach_common(ifp);
>   TAILQ_INSERT_TAIL(, ifp, if_list);
>   if_attachsetup(ifp);
> - splx(s);
> + NET_UNLOCK(s);

You'll need to release the lock before calling ifc->ifc_create in
if_clone_create() and do the same dance in if_clone_destroy().

But I think that's the way to go.  If you can create/destroy
pseudo-interface without panicing we're good.

>  void
> @@ -941,6 +941,7 @@ if_detach(struct ifnet *ifp)
>  
>   ifq_clr_oactive(>if_snd);
>  
> + rw_enter_write();
>   s = splnet();
>   /* Other CPUs must not have a reference before we start destroying. */
>   if_idxmap_remove(ifp);
> @@ -1017,6 +1018,7 @@ if_detach(struct ifnet *ifp)
>   /* Announce that the interface is gone. */
>   rt_ifannouncemsg(ifp, IFAN_DEPARTURE);
>   splx(s);
> + rw_exit_write();
>  
>   ifq_destroy(>if_snd);
>  }

Please keep the NET_LOCK/UNLOCK() macro.  The places where we
grab/release the `netlock' directly are places that need attention.



if attach/detach netlocks

2016-12-20 Thread Mike Belopuhov
Attach:

  splassert: sowakeup: want 1 have 0
  Starting stack trace...
  sowakeup() at sowakeup+0x41
  sorwakeup() at sorwakeup+0xb4
  route_input() at route_input+0x2d7
  if_attach() at if_attach+0x58
  xnf_attach() at xnf_attach+0x45f
  config_attach() at config_attach+0x1bc
  xen_attach_device() at xen_attach_device+0x146
  xen_hotplug() at xen_hotplug+0x23f
  taskq_thread() at taskq_thread+0x6c
  end trace frame: 0x0, count: 248
  End of stack trace.

Detach:

  splassert: sowakeup: want 1 have 0
  Starting stack trace...
  sowakeup() at sowakeup+0x41
  sorwakeup() at sorwakeup+0xb4
  route_input() at route_input+0x2d7
  if_detach() at if_detach+0x27e
  xnf_detach() at xnf_detach+0x4d
  config_detach() at config_detach+0x13c
  xen_hotplug() at xen_hotplug+0x2e5
  taskq_thread() at taskq_thread+0x6c
  end trace frame: 0x0, count: 249
  End of stack trace.
  xnf2 detached

The taskq running xen_hotplug is KERNEL_LOCK'ed.  The diff below solves
the issue, but is it any good?

diff --git sys/net/if.c sys/net/if.c
index 1e103564710..a5c25b89560 100644
--- sys/net/if.c
+++ sys/net/if.c
@@ -525,11 +525,11 @@ if_attach(struct ifnet *ifp)
 {
int s;
 
-   s = splsoftnet();
+   NET_LOCK(s);
if_attach_common(ifp);
TAILQ_INSERT_TAIL(, ifp, if_list);
if_attachsetup(ifp);
-   splx(s);
+   NET_UNLOCK(s);
 }
 
 void
@@ -941,6 +941,7 @@ if_detach(struct ifnet *ifp)
 
ifq_clr_oactive(>if_snd);
 
+   rw_enter_write();
s = splnet();
/* Other CPUs must not have a reference before we start destroying. */
if_idxmap_remove(ifp);
@@ -1017,6 +1018,7 @@ if_detach(struct ifnet *ifp)
/* Announce that the interface is gone. */
rt_ifannouncemsg(ifp, IFAN_DEPARTURE);
splx(s);
+   rw_exit_write();
 
ifq_destroy(>if_snd);
 }



Re: bioctl: don't print uninitialized memory

2016-12-20 Thread Mike Belopuhov
On 20 December 2016 at 16:03, Patrick Wildt  wrote:
> Hi,
>
> I just realized that bioctl can print its uninitialized memory.  The
> function bio_status() prints information from a struct basically after
> each ioctl().  The BIOCLOCATE ioctl() though never sets this in the
> struct.  Thus each BIOCLOCATE ioctl() keeps the struct in the state
> as it has been before.
>
> There are two cases where bioctl calls bio_status() after a BIOCLOCATE.
> In one case the struct is zeroed before, in the other it isn't.
>
> I propose removing the prints after the BIOLOCATEs to not make anyone
> believe there should actually be a status print.  Additionally I'd like
> to add a memset in the one case where there's none.
>
> ok?
>
> Patrick

Looks good to me.  OK mikeb



bioctl: don't print uninitialized memory

2016-12-20 Thread Patrick Wildt
Hi,

I just realized that bioctl can print its uninitialized memory.  The
function bio_status() prints information from a struct basically after
each ioctl().  The BIOCLOCATE ioctl() though never sets this in the
struct.  Thus each BIOCLOCATE ioctl() keeps the struct in the state
as it has been before.

There are two cases where bioctl calls bio_status() after a BIOCLOCATE.
In one case the struct is zeroed before, in the other it isn't.

I propose removing the prints after the BIOLOCATEs to not make anyone
believe there should actually be a status print.  Additionally I'd like
to add a memset in the one case where there's none.

ok?

Patrick

diff --git a/sbin/bioctl/bioctl.c b/sbin/bioctl/bioctl.c
index aad5b965d669..ace216fc55cb 100644
--- a/sbin/bioctl/bioctl.c
+++ b/sbin/bioctl/bioctl.c
@@ -231,13 +231,12 @@ main(int argc, char *argv[])
if (devh == -1)
err(1, "Can't open %s", "/dev/bio");
 
+   memset(, 0, sizeof(bl));
bl.bl_name = devicename;
if (ioctl(devh, BIOCLOCATE, ))
errx(1, "Can't locate %s device via %s",
bl.bl_name, "/dev/bio");
 
-   bio_status(_bio.bio_status);
-
bio_cookie = bl.bl_bio.bio_cookie;
biodev = 1;
devicename = NULL;
@@ -805,8 +804,6 @@ bio_blink(char *enclosure, int target, int blinktype)
bl.bl_name = enclosure;
if (ioctl(bioh, BIOCLOCATE, ))
errx(1, "Can't locate %s device via %s", enclosure, "/dev/bio");
- 
-   bio_status(_bio.bio_status);
 
memset(, 0, sizeof(blink));
blink.bb_bio.bio_cookie = bio_cookie;



Re: NET_LOCK() pflow panic

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 03:28:38PM +0100, Martin Pieuchot wrote:
> I don't have a solution for the moment and I want to be sure we know all

Me neither.  That's why I did not send a diff, but only the stack trace.

> recursions before trying to write a fix.  So here's a diff that mark the
> recursions with a XXXSMP like in the NFS case.

I think this approach is fine.

> ok?

You missed the returns in pflowioctl() between rw_exit_write() and
rw_enter_write().  I would suggest that you follow the splnet() and
put a rw_enter_write() after each splx().

bluhm

> 
> Index: net/if_pflow.c
> ===
> RCS file: /cvs/src/sys/net/if_pflow.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 if_pflow.c
> --- net/if_pflow.c4 Oct 2016 13:54:32 -   1.62
> +++ net/if_pflow.c20 Dec 2016 14:23:32 -
> @@ -267,7 +267,10 @@ pflow_clone_destroy(struct ifnet *ifp)
>   pflow_flush(sc);
>   m_freem(sc->send_nam);
>   if (sc->so != NULL) {
> + /* XXXSMP breaks atomicity */
> + rw_exit_write();
>   error = soclose(sc->so);
> + rw_enter_write();
>   sc->so = NULL;
>   }
>   if (sc->sc_flowdst != NULL)
> @@ -375,6 +378,8 @@ pflowioctl(struct ifnet *ifp, u_long cmd
>   }
>   }
>  
> + /* XXXSMP breaks atomicity */
> + rw_exit_write();
>   s = splnet();
>   pflow_flush(sc);
>  
> @@ -530,6 +535,7 @@ pflowioctl(struct ifnet *ifp, u_long cmd
>   } else
>   ifp->if_flags &= ~IFF_RUNNING;
>  
> + rw_enter_write();
>   break;
>  
>   default:



Re: NET_LOCK() pflow panic

2016-12-20 Thread Martin Pieuchot
On 20/12/16(Tue) 14:50, Alexander Bluhm wrote:
> Hi,
> 
> This crash happened during regress/sys/net/pflow on my regression test
> machine:
> 
> panic: rw_enter: netlock locking against myself
> Stopped at  Debugger+0x7:   leave
>TIDPIDUID PRFLAGS PFLAGS  CPU  COMMAND
> *291613  71462  0 0x2  01  ifconfig
>  96946  67690  0 0x14000  0x2000  zerothread
> Debugger(d09facbd,f57a49e8,d09d242c,f57a49e8,0) at Debugger+0x7
> panic(d09d242c,d09dc32f,f57a4a2c,d0504491,d0b7b1ec) at panic+0x71
> rw_enter(d0b56f38,1,f57a4a7c,d03be305,d0b55fe4) at rw_enter+0x1b4
> rw_enter_write(d0b56f38,2,2,10,0) at rw_enter_write+0x3c
> socreate(2,f57a4ce8,2,0,f57a4c04) at socreate+0x8f
> pflowioctl(d3e18800,802069fd,f57a4e74,0,d3e18800) at pflowioctl+0x5f3
> in_ioctl(802069fd,f57a4e74,d3e18800,1,d3e18800) at in_ioctl+0xf9
> ifioctl(d8bfce80,802069fd,f57a4e74,d8883b60,d87a9604) at ifioctl+0x168
> soo_ioctl(d884dbec,802069fd,f57a4e74,d8883b60,0) at soo_ioctl+0x21c
> sys_ioctl(d8883b60,f57a4f5c,f57a4f7c,0,f57a4fa8) at sys_ioctl+0x19f
> syscall() at syscall+0x250
> --- syscall (number -2110258080) ---
> 0x6:
> 
> NET_LOCK() is taken in soo_ioctl()
> NET_LOCK(s);
> error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL, 
> (struct mbuf *)cmd, (struct mbuf *)data, (struct mbuf *)NULL, p));
> NET_UNLOCK(s);
> and in socreate().

I don't have a solution for the moment and I want to be sure we know all
recursions before trying to write a fix.  So here's a diff that mark the
recursions with a XXXSMP like in the NFS case.

It is safe since we're still running everything under KERNEL_LOCK().

ok?

Index: net/if_pflow.c
===
RCS file: /cvs/src/sys/net/if_pflow.c,v
retrieving revision 1.62
diff -u -p -r1.62 if_pflow.c
--- net/if_pflow.c  4 Oct 2016 13:54:32 -   1.62
+++ net/if_pflow.c  20 Dec 2016 14:23:32 -
@@ -267,7 +267,10 @@ pflow_clone_destroy(struct ifnet *ifp)
pflow_flush(sc);
m_freem(sc->send_nam);
if (sc->so != NULL) {
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
error = soclose(sc->so);
+   rw_enter_write();
sc->so = NULL;
}
if (sc->sc_flowdst != NULL)
@@ -375,6 +378,8 @@ pflowioctl(struct ifnet *ifp, u_long cmd
}
}
 
+   /* XXXSMP breaks atomicity */
+   rw_exit_write();
s = splnet();
pflow_flush(sc);
 
@@ -530,6 +535,7 @@ pflowioctl(struct ifnet *ifp, u_long cmd
} else
ifp->if_flags &= ~IFF_RUNNING;
 
+   rw_enter_write();
break;
 
default:



Re: carp(4) fix vs NET_LOCK

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 03:09:11PM +0100, Martin Pieuchot wrote:
> No we can't because we're already holding it.  So carp_master_down()
> must be split in two, diff below does that and only grab the lock in
> the timer routine.

OK bluhm@

> 
> Index: netinet/ip_carp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.297
> diff -u -p -r1.297 ip_carp.c
> --- netinet/ip_carp.c 19 Dec 2016 08:36:49 -  1.297
> +++ netinet/ip_carp.c 20 Dec 2016 14:04:55 -
> @@ -221,9 +221,11 @@ int  carp_prepare_ad(struct mbuf *, struc
>   struct carp_header *);
>  void carp_send_ad_all(void);
>  void carp_vhe_send_ad_all(struct carp_softc *);
> -void carp_send_ad(void *);
> +void carp_timer_ad(void *);
> +void carp_send_ad(struct carp_vhost_entry *);
>  void carp_send_arp(struct carp_softc *);
> -void carp_master_down(void *);
> +void carp_timer_down(void *);
> +void carp_master_down(struct carp_vhost_entry *);
>  int  carp_ioctl(struct ifnet *, u_long, caddr_t);
>  int  carp_vhids_ioctl(struct carp_softc *, struct carpreq *);
>  int  carp_check_dup_vhids(struct carp_softc *, struct carp_if *,
> @@ -831,9 +833,9 @@ carp_new_vhost(struct carp_softc *sc, in
>   vhe->vhid = vhid;
>   vhe->advskew = advskew;
>   vhe->state = INIT;
> - timeout_set_proc(>ad_tmo, carp_send_ad, vhe);
> - timeout_set_proc(>md_tmo, carp_master_down, vhe);
> - timeout_set_proc(>md6_tmo, carp_master_down, vhe);
> + timeout_set_proc(>ad_tmo, carp_timer_ad, vhe);
> + timeout_set_proc(>md_tmo, carp_timer_down, vhe);
> + timeout_set_proc(>md6_tmo, carp_timer_down, vhe);
>  
>   KERNEL_ASSERT_LOCKED(); /* touching carp_vhosts */
>  
> @@ -1027,26 +1029,34 @@ carp_vhe_send_ad_all(struct carp_softc *
>  }
>  
>  void
> -carp_send_ad(void *v)
> +carp_timer_ad(void *v)
> +{
> + int s;
> +
> + NET_LOCK(s);
> + carp_send_ad(v);
> + NET_UNLOCK(s);
> +}
> +
> +void
> +carp_send_ad(struct carp_vhost_entry *vhe)
>  {
>   struct carp_header ch;
>   struct timeval tv;
> - struct carp_vhost_entry *vhe = v;
>   struct carp_softc *sc = vhe->parent_sc;
>   struct carp_header *ch_ptr;
> -
>   struct mbuf *m;
> - int error, len, advbase, advskew, s;
> + int error, len, advbase, advskew;
>   struct ifaddr *ifa;
>   struct sockaddr sa;
>  
> + NET_ASSERT_LOCKED();
> +
>   if (sc->sc_carpdev == NULL) {
>   sc->sc_if.if_oerrors++;
>   return;
>   }
>  
> - NET_LOCK(s);
> -
>   /* bow out if we've gone to backup (the carp interface is going down) */
>   if (sc->sc_bow_out) {
>   advbase = 255;
> @@ -1246,7 +1256,6 @@ carp_send_ad(void *v)
>  
>  retry_later:
>   sc->cur_vhe = NULL;
> - NET_UNLOCK(s);
>   if (advbase != 255 || advskew != 255)
>   timeout_add(>ad_tmo, tvtohz());
>  }
> @@ -1261,7 +1270,6 @@ carp_send_arp(struct carp_softc *sc)
>  {
>   struct ifaddr *ifa;
>   in_addr_t in;
> - int s = splsoftnet();
>  
>   TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
>  
> @@ -1272,7 +1280,6 @@ carp_send_arp(struct carp_softc *sc)
>   arprequest(>sc_if, , , sc->sc_ac.ac_enaddr);
>   DELAY(1000);/* XXX */
>   }
> - splx(s);
>  }
>  
>  #ifdef INET6
> @@ -1282,7 +1289,6 @@ carp_send_na(struct carp_softc *sc)
>   struct ifaddr *ifa;
>   struct in6_addr *in6;
>   static struct in6_addr mcast = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
> - int s = splsoftnet();
>  
>   TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
>  
> @@ -1295,7 +1301,6 @@ carp_send_na(struct carp_softc *sc)
>   (ip6_forwarding ? ND_NA_FLAG_ROUTER : 0), 1, NULL);
>   DELAY(1000);/* XXX */
>   }
> - splx(s);
>  }
>  #endif /* INET6 */
>  
> @@ -1504,10 +1509,21 @@ done:
>  }
>  
>  void
> -carp_master_down(void *v)
> +carp_timer_down(void *v)
> +{
> + int s;
> +
> + NET_LOCK(s);
> + carp_master_down(v);
> + NET_UNLOCK(s);
> +}
> +
> +void
> +carp_master_down(struct carp_vhost_entry *vhe)
>  {
> - struct carp_vhost_entry *vhe = v;
>   struct carp_softc *sc = vhe->parent_sc;
> +
> + NET_ASSERT_LOCKED();
>  
>   switch (vhe->state) {
>   case INIT:



Re: carp(4) fix vs NET_LOCK

2016-12-20 Thread Martin Pieuchot
On 20/12/16(Tue) 14:31, Alexander Bluhm wrote:
> On Tue, Dec 20, 2016 at 12:08:57PM +0100, Martin Pieuchot wrote:
> > I introduced a bug in carp(4), one code path tries to take the NET_LOCK()
> > twice:
> > 
> > panic() at panic+0xfe
> > rw_enter() at rw_enter+0x1f8
> > carp_send_ad() at carp_send_ad+0x4d
> > carp_vhe_send_ad_all() at carp_vhe_send_ad_all+0x4b
> > carp_ioctl() at carp_ioctl+0x491
> > ifioctl() at ifioctl+0x7be
> > soo_ioctl() at soo_ioctl+0x260
> > sys_ioctl() at sys_ioctl+0x196
> > syscall() at syscall+0x27b
> > 
> > Diff below fixes that by respecting the rule of always taking the
> > NET_LOCK() at the beginning of every timer. 
> > 
> > While here remove two recursive splsoftnet() dances.
> > 
> > ok?
> 
> What about
> 
> carp_proto_input
> carp_proto_input_if
> carp_proto_input_c
> carp_master_down
> NET_LOCK

Good catch.

> May we take the NET_LOCK in the ip input path?

No we can't because we're already holding it.  So carp_master_down()
must be split in two, diff below does that and only grab the lock in
the timer routine.

Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.297
diff -u -p -r1.297 ip_carp.c
--- netinet/ip_carp.c   19 Dec 2016 08:36:49 -  1.297
+++ netinet/ip_carp.c   20 Dec 2016 14:04:55 -
@@ -221,9 +221,11 @@ intcarp_prepare_ad(struct mbuf *, struc
struct carp_header *);
 void   carp_send_ad_all(void);
 void   carp_vhe_send_ad_all(struct carp_softc *);
-void   carp_send_ad(void *);
+void   carp_timer_ad(void *);
+void   carp_send_ad(struct carp_vhost_entry *);
 void   carp_send_arp(struct carp_softc *);
-void   carp_master_down(void *);
+void   carp_timer_down(void *);
+void   carp_master_down(struct carp_vhost_entry *);
 intcarp_ioctl(struct ifnet *, u_long, caddr_t);
 intcarp_vhids_ioctl(struct carp_softc *, struct carpreq *);
 intcarp_check_dup_vhids(struct carp_softc *, struct carp_if *,
@@ -831,9 +833,9 @@ carp_new_vhost(struct carp_softc *sc, in
vhe->vhid = vhid;
vhe->advskew = advskew;
vhe->state = INIT;
-   timeout_set_proc(>ad_tmo, carp_send_ad, vhe);
-   timeout_set_proc(>md_tmo, carp_master_down, vhe);
-   timeout_set_proc(>md6_tmo, carp_master_down, vhe);
+   timeout_set_proc(>ad_tmo, carp_timer_ad, vhe);
+   timeout_set_proc(>md_tmo, carp_timer_down, vhe);
+   timeout_set_proc(>md6_tmo, carp_timer_down, vhe);
 
KERNEL_ASSERT_LOCKED(); /* touching carp_vhosts */
 
@@ -1027,26 +1029,34 @@ carp_vhe_send_ad_all(struct carp_softc *
 }
 
 void
-carp_send_ad(void *v)
+carp_timer_ad(void *v)
+{
+   int s;
+
+   NET_LOCK(s);
+   carp_send_ad(v);
+   NET_UNLOCK(s);
+}
+
+void
+carp_send_ad(struct carp_vhost_entry *vhe)
 {
struct carp_header ch;
struct timeval tv;
-   struct carp_vhost_entry *vhe = v;
struct carp_softc *sc = vhe->parent_sc;
struct carp_header *ch_ptr;
-
struct mbuf *m;
-   int error, len, advbase, advskew, s;
+   int error, len, advbase, advskew;
struct ifaddr *ifa;
struct sockaddr sa;
 
+   NET_ASSERT_LOCKED();
+
if (sc->sc_carpdev == NULL) {
sc->sc_if.if_oerrors++;
return;
}
 
-   NET_LOCK(s);
-
/* bow out if we've gone to backup (the carp interface is going down) */
if (sc->sc_bow_out) {
advbase = 255;
@@ -1246,7 +1256,6 @@ carp_send_ad(void *v)
 
 retry_later:
sc->cur_vhe = NULL;
-   NET_UNLOCK(s);
if (advbase != 255 || advskew != 255)
timeout_add(>ad_tmo, tvtohz());
 }
@@ -1261,7 +1270,6 @@ carp_send_arp(struct carp_softc *sc)
 {
struct ifaddr *ifa;
in_addr_t in;
-   int s = splsoftnet();
 
TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
 
@@ -1272,7 +1280,6 @@ carp_send_arp(struct carp_softc *sc)
arprequest(>sc_if, , , sc->sc_ac.ac_enaddr);
DELAY(1000);/* XXX */
}
-   splx(s);
 }
 
 #ifdef INET6
@@ -1282,7 +1289,6 @@ carp_send_na(struct carp_softc *sc)
struct ifaddr *ifa;
struct in6_addr *in6;
static struct in6_addr mcast = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
-   int s = splsoftnet();
 
TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
 
@@ -1295,7 +1301,6 @@ carp_send_na(struct carp_softc *sc)
(ip6_forwarding ? ND_NA_FLAG_ROUTER : 0), 1, NULL);
DELAY(1000);/* XXX */
}
-   splx(s);
 }
 #endif /* INET6 */
 
@@ -1504,10 +1509,21 @@ done:
 }
 
 void
-carp_master_down(void *v)
+carp_timer_down(void *v)
+{
+   int s;
+
+   NET_LOCK(s);
+   carp_master_down(v);
+   NET_UNLOCK(s);
+}
+
+void
+carp_master_down(struct carp_vhost_entry *vhe)
 {
-   struct carp_vhost_entry *vhe = v;
struct carp_softc *sc = 

NET_LOCK() pflow panic

2016-12-20 Thread Alexander Bluhm
Hi,

This crash happened during regress/sys/net/pflow on my regression test
machine:

panic: rw_enter: netlock locking against myself
Stopped at  Debugger+0x7:   leave
   TIDPIDUID PRFLAGS PFLAGS  CPU  COMMAND
*291613  71462  0 0x2  01  ifconfig
 96946  67690  0 0x14000  0x2000  zerothread
Debugger(d09facbd,f57a49e8,d09d242c,f57a49e8,0) at Debugger+0x7
panic(d09d242c,d09dc32f,f57a4a2c,d0504491,d0b7b1ec) at panic+0x71
rw_enter(d0b56f38,1,f57a4a7c,d03be305,d0b55fe4) at rw_enter+0x1b4
rw_enter_write(d0b56f38,2,2,10,0) at rw_enter_write+0x3c
socreate(2,f57a4ce8,2,0,f57a4c04) at socreate+0x8f
pflowioctl(d3e18800,802069fd,f57a4e74,0,d3e18800) at pflowioctl+0x5f3
in_ioctl(802069fd,f57a4e74,d3e18800,1,d3e18800) at in_ioctl+0xf9
ifioctl(d8bfce80,802069fd,f57a4e74,d8883b60,d87a9604) at ifioctl+0x168
soo_ioctl(d884dbec,802069fd,f57a4e74,d8883b60,0) at soo_ioctl+0x21c
sys_ioctl(d8883b60,f57a4f5c,f57a4f7c,0,f57a4fa8) at sys_ioctl+0x19f
syscall() at syscall+0x250
--- syscall (number -2110258080) ---
0x6:

NET_LOCK() is taken in soo_ioctl()
NET_LOCK(s);
error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL, 
(struct mbuf *)cmd, (struct mbuf *)data, (struct mbuf *)NULL, p));
NET_UNLOCK(s);
and in socreate().

bluhm



Re: carp(4) fix vs NET_LOCK

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 12:08:57PM +0100, Martin Pieuchot wrote:
> I introduced a bug in carp(4), one code path tries to take the NET_LOCK()
> twice:
> 
>   panic() at panic+0xfe
>   rw_enter() at rw_enter+0x1f8
>   carp_send_ad() at carp_send_ad+0x4d
>   carp_vhe_send_ad_all() at carp_vhe_send_ad_all+0x4b
>   carp_ioctl() at carp_ioctl+0x491
>   ifioctl() at ifioctl+0x7be
>   soo_ioctl() at soo_ioctl+0x260
>   sys_ioctl() at sys_ioctl+0x196
>   syscall() at syscall+0x27b
> 
> Diff below fixes that by respecting the rule of always taking the
> NET_LOCK() at the beginning of every timer. 
> 
> While here remove two recursive splsoftnet() dances.
> 
> ok?

What about

carp_proto_input
carp_proto_input_if
carp_proto_input_c
carp_master_down
NET_LOCK

May we take the NET_LOCK in the ip input path?

bluhm

> 
> Index: netinet/ip_carp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.297
> diff -u -p -r1.297 ip_carp.c
> --- netinet/ip_carp.c 19 Dec 2016 08:36:49 -  1.297
> +++ netinet/ip_carp.c 20 Dec 2016 11:03:22 -
> @@ -221,7 +221,8 @@ int   carp_prepare_ad(struct mbuf *, struc
>   struct carp_header *);
>  void carp_send_ad_all(void);
>  void carp_vhe_send_ad_all(struct carp_softc *);
> -void carp_send_ad(void *);
> +void carp_timer_ad(void *);
> +void carp_send_ad(struct carp_vhost_entry *);
>  void carp_send_arp(struct carp_softc *);
>  void carp_master_down(void *);
>  int  carp_ioctl(struct ifnet *, u_long, caddr_t);
> @@ -831,7 +832,7 @@ carp_new_vhost(struct carp_softc *sc, in
>   vhe->vhid = vhid;
>   vhe->advskew = advskew;
>   vhe->state = INIT;
> - timeout_set_proc(>ad_tmo, carp_send_ad, vhe);
> + timeout_set_proc(>ad_tmo, carp_timer_ad, vhe);
>   timeout_set_proc(>md_tmo, carp_master_down, vhe);
>   timeout_set_proc(>md6_tmo, carp_master_down, vhe);
>  
> @@ -1027,26 +1028,34 @@ carp_vhe_send_ad_all(struct carp_softc *
>  }
>  
>  void
> -carp_send_ad(void *v)
> +carp_timer_ad(void *v)
> +{
> + int s;
> +
> + NET_LOCK(s);
> + carp_send_ad(v);
> + NET_UNLOCK(s);
> +}
> +
> +void
> +carp_send_ad(struct carp_vhost_entry *vhe)
>  {
>   struct carp_header ch;
>   struct timeval tv;
> - struct carp_vhost_entry *vhe = v;
>   struct carp_softc *sc = vhe->parent_sc;
>   struct carp_header *ch_ptr;
> -
>   struct mbuf *m;
> - int error, len, advbase, advskew, s;
> + int error, len, advbase, advskew;
>   struct ifaddr *ifa;
>   struct sockaddr sa;
>  
> + NET_ASSERT_LOCKED();
> +
>   if (sc->sc_carpdev == NULL) {
>   sc->sc_if.if_oerrors++;
>   return;
>   }
>  
> - NET_LOCK(s);
> -
>   /* bow out if we've gone to backup (the carp interface is going down) */
>   if (sc->sc_bow_out) {
>   advbase = 255;
> @@ -1246,7 +1255,6 @@ carp_send_ad(void *v)
>  
>  retry_later:
>   sc->cur_vhe = NULL;
> - NET_UNLOCK(s);
>   if (advbase != 255 || advskew != 255)
>   timeout_add(>ad_tmo, tvtohz());
>  }
> @@ -1261,7 +1269,6 @@ carp_send_arp(struct carp_softc *sc)
>  {
>   struct ifaddr *ifa;
>   in_addr_t in;
> - int s = splsoftnet();
>  
>   TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
>  
> @@ -1272,7 +1279,6 @@ carp_send_arp(struct carp_softc *sc)
>   arprequest(>sc_if, , , sc->sc_ac.ac_enaddr);
>   DELAY(1000);/* XXX */
>   }
> - splx(s);
>  }
>  
>  #ifdef INET6
> @@ -1282,7 +1288,6 @@ carp_send_na(struct carp_softc *sc)
>   struct ifaddr *ifa;
>   struct in6_addr *in6;
>   static struct in6_addr mcast = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
> - int s = splsoftnet();
>  
>   TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
>  
> @@ -1295,7 +1300,6 @@ carp_send_na(struct carp_softc *sc)
>   (ip6_forwarding ? ND_NA_FLAG_ROUTER : 0), 1, NULL);
>   DELAY(1000);/* XXX */
>   }
> - splx(s);
>  }
>  #endif /* INET6 */
>  
> @@ -1508,7 +1512,9 @@ carp_master_down(void *v)
>  {
>   struct carp_vhost_entry *vhe = v;
>   struct carp_softc *sc = vhe->parent_sc;
> + int s;
>  
> + NET_LOCK(s);
>   switch (vhe->state) {
>   case INIT:
>   printf("%s: master_down event in INIT state\n",
> @@ -1531,6 +1537,7 @@ carp_master_down(void *v)
>   carpstats.carps_preempt++;
>   break;
>   }
> + NET_UNLOCK(s);
>  }
>  
>  void



Re: tcp_ctloutput() and splsoftnet()

2016-12-20 Thread Alexander Bluhm
On Tue, Dec 20, 2016 at 12:11:40PM +0100, Martin Pieuchot wrote:
> Kill a recursive splsoftnet()/splx() dance, ``pr_ctloutput'' routines
> are now always called at IPL_SOFTNET.
> 
> ok?

OK bluhm@

> 
> Index: netinet/tcp_usrreq.c
> ===
> RCS file: /cvs/src/sys/netinet/tcp_usrreq.c,v
> retrieving revision 1.137
> diff -u -p -r1.137 tcp_usrreq.c
> --- netinet/tcp_usrreq.c  19 Dec 2016 08:36:49 -  1.137
> +++ netinet/tcp_usrreq.c  20 Dec 2016 10:52:05 -
> @@ -451,16 +451,14 @@ int
>  tcp_ctloutput(int op, struct socket *so, int level, int optname,
>  struct mbuf **mp)
>  {
> - int error = 0, s;
> + int error = 0;
>   struct inpcb *inp;
>   struct tcpcb *tp;
>   struct mbuf *m;
>   int i;
>  
> - s = splsoftnet();
>   inp = sotoinpcb(so);
>   if (inp == NULL) {
> - splx(s);
>   if (op == PRCO_SETOPT)
>   (void) m_free(*mp);
>   return (ECONNRESET);
> @@ -479,7 +477,6 @@ tcp_ctloutput(int op, struct socket *so,
>   error = EAFNOSUPPORT;   /*?*/
>   break;
>   }
> - splx(s);
>   return (error);
>   }
>   tp = intotcpcb(inp);
> @@ -606,7 +603,6 @@ tcp_ctloutput(int op, struct socket *so,
>   }
>   break;
>   }
> - splx(s);
>   return (error);
>  }
>  



More NET_LOCK()

2016-12-20 Thread Martin Pieuchot
rzalamena@ found that the IPv4 multicast code have two code paths ending
in ip_output() that are not yet taking the NET_LOCK: sosetopt() and
pffasttimo().

Diff below fixes that and include pfslowtimo() as well as sogetopt() for
completeness.

While here remove a superfluous splsoftnet() which doesn't protect
anything in the SO_SPLICE case since we're running the input path
in a thread. 

I just finished baking a muild on NFS with that, ok?

Index: kern/uipc_domain.c
===
RCS file: /cvs/src/sys/kern/uipc_domain.c,v
retrieving revision 1.46
diff -u -p -r1.46 uipc_domain.c
--- kern/uipc_domain.c  22 Nov 2016 10:32:31 -  1.46
+++ kern/uipc_domain.c  20 Dec 2016 10:18:50 -
@@ -99,8 +99,8 @@ domaininit(void)
max_linkhdr = 64;
 
max_hdr = max_linkhdr + max_protohdr;
-   timeout_set(_timeout, pffasttimo, _timeout);
-   timeout_set(_timeout, pfslowtimo, _timeout);
+   timeout_set_proc(_timeout, pffasttimo, _timeout);
+   timeout_set_proc(_timeout, pfslowtimo, _timeout);
timeout_add(_timeout, 1);
timeout_add(_timeout, 1);
 }
@@ -238,13 +238,13 @@ pfslowtimo(void *arg)
struct protosw *pr;
int i, s;
 
-   s = splsoftnet();
+   NET_LOCK(s);
for (i = 0; (dp = domains[i]) != NULL; i++) {
for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++)
if (pr->pr_slowtimo)
(*pr->pr_slowtimo)();
}
-   splx(s);
+   NET_UNLOCK(s);
timeout_add_msec(to, 500);
 }
 
@@ -256,12 +256,12 @@ pffasttimo(void *arg)
struct protosw *pr;
int i, s;
 
-   s = splsoftnet();
+   NET_LOCK(s);
for (i = 0; (dp = domains[i]) != NULL; i++) {
for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++)
if (pr->pr_fasttimo)
(*pr->pr_fasttimo)();
}
-   splx(s);
+   NET_UNLOCK(s);
timeout_add_msec(to, 200);
 }
Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.169
diff -u -p -r1.169 uipc_socket.c
--- kern/uipc_socket.c  19 Dec 2016 08:36:49 -  1.169
+++ kern/uipc_socket.c  20 Dec 2016 10:22:18 -
@@ -1555,10 +1555,10 @@ sosetopt(struct socket *so, int level, i
 
if (level != SOL_SOCKET) {
if (so->so_proto && so->so_proto->pr_ctloutput) {
-   s = splsoftnet();
+   NET_LOCK(s);
error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
level, optname, );
-   splx(s);
+   NET_UNLOCK(s);
return (error);
}
error = ENOPROTOOPT;
@@ -1702,10 +1702,10 @@ sosetopt(struct socket *so, int level, i
struct domain *dom = so->so_proto->pr_domain;
 
level = dom->dom_protosw->pr_protocol;
-   s = splsoftnet();
+   NET_LOCK(s);
error = (*so->so_proto->pr_ctloutput)
(PRCO_SETOPT, so, level, optname, );
-   splx(s);
+   NET_UNLOCK(s);
return (error);
}
error = ENOPROTOOPT;
@@ -1734,10 +1734,10 @@ sosetopt(struct socket *so, int level, i
break;
}
if (error == 0 && so->so_proto && so->so_proto->pr_ctloutput) {
-   s = splsoftnet();
+   NET_LOCK(s);
(*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so,
level, optname, );
-   splx(s);
+   NET_UNLOCK(s);
m = NULL;   /* freed by protocol */
}
}
@@ -1755,10 +1755,10 @@ sogetopt(struct socket *so, int level, i
 
if (level != SOL_SOCKET) {
if (so->so_proto && so->so_proto->pr_ctloutput) {
-   s = splsoftnet();
+   NET_LOCK(s);
error = (*so->so_proto->pr_ctloutput)(PRCO_GETOPT, so,
level, optname, mp);
-   splx(s);
+   NET_UNLOCK(s);
return (error);
} else
return (ENOPROTOOPT);
@@ -1839,10 +1839,10 @@ sogetopt(struct socket *so, int level, i
struct domain *dom = so->so_proto->pr_domain;
 
level = dom->dom_protosw->pr_protocol;
-   s = splsoftnet();
+   

PATCH: Tell what trailing asterisk in sa output means

2016-12-20 Thread Vadim Zhukov
Hi all.

I had to dig into sa sources to get know what trailing '*' in
command names mean. IMHO, that's not how manuals should work. :)

Okay?

--
WBR,
  Vadim Zhukov


Index: sa.8
===
RCS file: /cvs/src/usr.sbin/sa/sa.8,v
retrieving revision 1.19
diff -u -p -r1.19 sa.8
--- sa.816 Jul 2013 11:13:34 -  1.19
+++ sa.820 Dec 2016 12:26:51 -
@@ -205,6 +205,14 @@ flag is specified, only the
 and
 .Fl s
 flags are honored.
+.Pp
+Processes created with
+.Xr fork 2
+without
+.Xr exec 3
+will have
+.Sq *
+appended to their command names.
 .Sh FILES
 .Bl -tag -width /var/account/usracct -compact
 .It Pa /var/account/acct



Re: tcp_ctloutput() and splsoftnet()

2016-12-20 Thread Mike Belopuhov
On 20 December 2016 at 12:11, Martin Pieuchot  wrote:
> Kill a recursive splsoftnet()/splx() dance, ``pr_ctloutput'' routines
> are now always called at IPL_SOFTNET.
>
> ok?
>

Yes, please.



Re: mpe(4), mpw(4) and splsoftnet()

2016-12-20 Thread Rafael Zalamena
On Mon, Dec 19, 2016 at 11:48:31AM +0100, Martin Pieuchot wrote:
> Interface ioctl(2) are now always run at IPL_SOFTNET, so let's get rid
> of recursive splsoftnet()/splx() dances.
> 
> ok?

ok rzalamena@



mld6 & splsoftnet()

2016-12-20 Thread Martin Pieuchot
mld6_{start,stop}_listeting() are called in ioctl(2) context, thus at
IPL_SOFTNET.

mld6_fasttimeo(), like all *fasttimeo() routines is now always called
at IPL_SOFTNET.

So let's get rid of these recursive splsoftnet(), ok?

Index: netinet6/mld6.c
===
RCS file: /cvs/src/sys/netinet6/mld6.c,v
retrieving revision 1.48
diff -u -p -r1.48 mld6.c
--- netinet6/mld6.c 5 Jul 2016 10:17:14 -   1.48
+++ netinet6/mld6.c 20 Dec 2016 10:59:27 -
@@ -117,8 +117,6 @@ mld6_init(void)
 void
 mld6_start_listening(struct in6_multi *in6m)
 {
-   int s = splsoftnet();
-
/*
 * RFC2710 page 10:
 * The node never sends a Report or Done for the link-scope all-nodes
@@ -139,14 +137,11 @@ mld6_start_listening(struct in6_multi *i
in6m->in6m_state = MLD_IREPORTEDLAST;
mld_timers_are_running = 1;
}
-   splx(s);
 }
 
 void
 mld6_stop_listening(struct in6_multi *in6m)
 {
-   int s = splsoftnet();
-
mld_all_nodes_linklocal.s6_addr16[1] = htons(in6m->in6m_ifidx);/* XXX */
mld_all_routers_linklocal.s6_addr16[1] =
htons(in6m->in6m_ifidx); /* XXX: necessary when mrouting */
@@ -156,7 +151,6 @@ mld6_stop_listening(struct in6_multi *in
__IPV6_ADDR_MC_SCOPE(>in6m_addr) > 
__IPV6_ADDR_SCOPE_INTFACELOCAL)
mld6_sendpkt(in6m, MLD_LISTENER_DONE,
_all_routers_linklocal);
-   splx(s);
 }
 
 void
@@ -331,7 +325,6 @@ void
 mld6_fasttimeo(void)
 {
struct ifnet *ifp;
-   int s;
 
/*
 * Quick check to see if any work needs to be done, in order
@@ -340,11 +333,9 @@ mld6_fasttimeo(void)
if (!mld_timers_are_running)
return;
 
-   s = splsoftnet();
mld_timers_are_running = 0;
TAILQ_FOREACH(ifp, , if_list)
mld6_checktimer(ifp);
-   splx(s);
 }
 
 void



tcp_ctloutput() and splsoftnet()

2016-12-20 Thread Martin Pieuchot
Kill a recursive splsoftnet()/splx() dance, ``pr_ctloutput'' routines
are now always called at IPL_SOFTNET.

ok?

Index: netinet/tcp_usrreq.c
===
RCS file: /cvs/src/sys/netinet/tcp_usrreq.c,v
retrieving revision 1.137
diff -u -p -r1.137 tcp_usrreq.c
--- netinet/tcp_usrreq.c19 Dec 2016 08:36:49 -  1.137
+++ netinet/tcp_usrreq.c20 Dec 2016 10:52:05 -
@@ -451,16 +451,14 @@ int
 tcp_ctloutput(int op, struct socket *so, int level, int optname,
 struct mbuf **mp)
 {
-   int error = 0, s;
+   int error = 0;
struct inpcb *inp;
struct tcpcb *tp;
struct mbuf *m;
int i;
 
-   s = splsoftnet();
inp = sotoinpcb(so);
if (inp == NULL) {
-   splx(s);
if (op == PRCO_SETOPT)
(void) m_free(*mp);
return (ECONNRESET);
@@ -479,7 +477,6 @@ tcp_ctloutput(int op, struct socket *so,
error = EAFNOSUPPORT;   /*?*/
break;
}
-   splx(s);
return (error);
}
tp = intotcpcb(inp);
@@ -606,7 +603,6 @@ tcp_ctloutput(int op, struct socket *so,
}
break;
}
-   splx(s);
return (error);
 }
 



carp(4) fix vs NET_LOCK

2016-12-20 Thread Martin Pieuchot
I introduced a bug in carp(4), one code path tries to take the NET_LOCK()
twice:

panic() at panic+0xfe
rw_enter() at rw_enter+0x1f8
carp_send_ad() at carp_send_ad+0x4d
carp_vhe_send_ad_all() at carp_vhe_send_ad_all+0x4b
carp_ioctl() at carp_ioctl+0x491
ifioctl() at ifioctl+0x7be
soo_ioctl() at soo_ioctl+0x260
sys_ioctl() at sys_ioctl+0x196
syscall() at syscall+0x27b

Diff below fixes that by respecting the rule of always taking the
NET_LOCK() at the beginning of every timer. 

While here remove two recursive splsoftnet() dances.

ok?

Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.297
diff -u -p -r1.297 ip_carp.c
--- netinet/ip_carp.c   19 Dec 2016 08:36:49 -  1.297
+++ netinet/ip_carp.c   20 Dec 2016 11:03:22 -
@@ -221,7 +221,8 @@ int carp_prepare_ad(struct mbuf *, struc
struct carp_header *);
 void   carp_send_ad_all(void);
 void   carp_vhe_send_ad_all(struct carp_softc *);
-void   carp_send_ad(void *);
+void   carp_timer_ad(void *);
+void   carp_send_ad(struct carp_vhost_entry *);
 void   carp_send_arp(struct carp_softc *);
 void   carp_master_down(void *);
 intcarp_ioctl(struct ifnet *, u_long, caddr_t);
@@ -831,7 +832,7 @@ carp_new_vhost(struct carp_softc *sc, in
vhe->vhid = vhid;
vhe->advskew = advskew;
vhe->state = INIT;
-   timeout_set_proc(>ad_tmo, carp_send_ad, vhe);
+   timeout_set_proc(>ad_tmo, carp_timer_ad, vhe);
timeout_set_proc(>md_tmo, carp_master_down, vhe);
timeout_set_proc(>md6_tmo, carp_master_down, vhe);
 
@@ -1027,26 +1028,34 @@ carp_vhe_send_ad_all(struct carp_softc *
 }
 
 void
-carp_send_ad(void *v)
+carp_timer_ad(void *v)
+{
+   int s;
+
+   NET_LOCK(s);
+   carp_send_ad(v);
+   NET_UNLOCK(s);
+}
+
+void
+carp_send_ad(struct carp_vhost_entry *vhe)
 {
struct carp_header ch;
struct timeval tv;
-   struct carp_vhost_entry *vhe = v;
struct carp_softc *sc = vhe->parent_sc;
struct carp_header *ch_ptr;
-
struct mbuf *m;
-   int error, len, advbase, advskew, s;
+   int error, len, advbase, advskew;
struct ifaddr *ifa;
struct sockaddr sa;
 
+   NET_ASSERT_LOCKED();
+
if (sc->sc_carpdev == NULL) {
sc->sc_if.if_oerrors++;
return;
}
 
-   NET_LOCK(s);
-
/* bow out if we've gone to backup (the carp interface is going down) */
if (sc->sc_bow_out) {
advbase = 255;
@@ -1246,7 +1255,6 @@ carp_send_ad(void *v)
 
 retry_later:
sc->cur_vhe = NULL;
-   NET_UNLOCK(s);
if (advbase != 255 || advskew != 255)
timeout_add(>ad_tmo, tvtohz());
 }
@@ -1261,7 +1269,6 @@ carp_send_arp(struct carp_softc *sc)
 {
struct ifaddr *ifa;
in_addr_t in;
-   int s = splsoftnet();
 
TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
 
@@ -1272,7 +1279,6 @@ carp_send_arp(struct carp_softc *sc)
arprequest(>sc_if, , , sc->sc_ac.ac_enaddr);
DELAY(1000);/* XXX */
}
-   splx(s);
 }
 
 #ifdef INET6
@@ -1282,7 +1288,6 @@ carp_send_na(struct carp_softc *sc)
struct ifaddr *ifa;
struct in6_addr *in6;
static struct in6_addr mcast = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
-   int s = splsoftnet();
 
TAILQ_FOREACH(ifa, >sc_if.if_addrlist, ifa_list) {
 
@@ -1295,7 +1300,6 @@ carp_send_na(struct carp_softc *sc)
(ip6_forwarding ? ND_NA_FLAG_ROUTER : 0), 1, NULL);
DELAY(1000);/* XXX */
}
-   splx(s);
 }
 #endif /* INET6 */
 
@@ -1508,7 +1512,9 @@ carp_master_down(void *v)
 {
struct carp_vhost_entry *vhe = v;
struct carp_softc *sc = vhe->parent_sc;
+   int s;
 
+   NET_LOCK(s);
switch (vhe->state) {
case INIT:
printf("%s: master_down event in INIT state\n",
@@ -1531,6 +1537,7 @@ carp_master_down(void *v)
carpstats.carps_preempt++;
break;
}
+   NET_UNLOCK(s);
 }
 
 void