awk fpecatch() update

[Michael W. Bombardieri]

Hello,

Updated version of awk (Dec 20, 2012) [1] has a simplification
in fpecatch() function. I tried to test this to compare the
error output. 

Old version:
$ /usr/bin/awk '{}'
*** receive SIGFPE via kill(1) ***
floating point exception
 source line number 1

New version:
$ ./awk '{}'
*** receive SIGFPE via kill(1) ***
awk: floating point exception 8
 source line number 1

And with debugging flag set, the core dump still happens as expected...

Old version:
$ awk -d9 '{}'
awk version 20110810
program = |{}|
setsymtab set 0x7ac4f180: n=0 s="0" f=0 t=17
*** SNIP ***
lex token 123
lex token 59
lex token 125
errorflag=0
RS=<
>, FS=< >, ARGC=1, FILENAME=
argno=1, file=||
*** receive SIGFPE via kill(1) ***
floating point exception
 source line number 1
Abort trap (core dumped)

New version:
$ ./awk -d9 '{}'
awk version 20110810
program = |{}|
setsymtab set 0x80e58840: n=0 s="0" f=0 t=17
*** SNIP ***
lex token 123
lex token 59
lex token 125
errorflag=0
RS=<
>, FS=< >, ARGC=1, FILENAME=
argno=1, file=||
*** receive SIGFPE via kill(1) ***
awk: floating point exception 8
 source line number 1
Abort trap (core dumped)

No significant difference for this trivial test.
All I notice is the error now starts with command name.

- Michael


[1] http://www.cs.princeton.edu/~bwk/btl.mirror/awk.tar.gz

Index: lib.c
===
RCS file: /cvs/src/usr.bin/awk/lib.c,v
retrieving revision 1.22
diff -u -p -u -r1.22 lib.c
--- lib.c   12 Apr 2016 19:43:38 -  1.22
+++ lib.c   5 Sep 2017 04:59:58 -
@@ -529,39 +529,9 @@ void SYNTAX(const char *fmt, ...)
eprint();
 }
 
-void fpecatch(int sig)
+void fpecatch(int n)
 {
-   extern Node *curnode;
-   char buf[1024];
-
-   snprintf(buf, sizeof buf, "floating point exception\n");
-   write(STDERR_FILENO, buf, strlen(buf));
-
-   if (compile_time != 2 && NR && *NR > 0) {
-   snprintf(buf, sizeof buf, " input record number %d", (int) 
(*FNR));
-   write(STDERR_FILENO, buf, strlen(buf));
-
-   if (strcmp(*FILENAME, "-") != 0) {
-   snprintf(buf, sizeof buf, ", file %s", *FILENAME);
-   write(STDERR_FILENO, buf, strlen(buf));
-   }
-   write(STDERR_FILENO, "\n", 1);
-   }
-   if (compile_time != 2 && curnode) {
-   snprintf(buf, sizeof buf, " source line number %d", 
curnode->lineno);
-   write(STDERR_FILENO, buf, strlen(buf));
-   } else if (compile_time != 2 && lineno) {
-   snprintf(buf, sizeof buf, " source line number %d", lineno);
-   write(STDERR_FILENO, buf, strlen(buf));
-   }
-   if (compile_time == 1 && cursource() != NULL) {
-   snprintf(buf, sizeof buf, " source file %s", cursource());
-   write(STDERR_FILENO, buf, strlen(buf));
-   }
-   write(STDERR_FILENO, "\n", 1);
-   if (dbg > 1)/* core dump if serious debugging on */
-   abort();
-   _exit(1);
+   FATAL("floating point exception %d", n);
 }
 
 extern int bracecnt, brackcnt, parencnt;

Re: iwn/iwm/wpi: scanning for 2 vs 5 GHz APs

[Kevin Lo]

On Sun, Sep 03, 2017 at 09:37:02PM +0200, Stefan Sperling wrote:
> 
> Starting out in associated (not down!) state:
>  ifconfig iwn0 down; ifconfig iwn0 scan
> shows 2 GHz and 5 GHz APs, whereas just
>   ifconfig iwn0 scan
> does not show all APs. The diff below makes both cases show all APs.
> 
> ok?

I tested with iwn(4) on the sony vaio laptop.

$ dmesg | grep iwn0
iwn0 at pci2 dev 0 function 0 "Intel Wireless WiFi Link 4965" rev 0x61: msi, 
MIMO 2T3R, MoW1, address 00:1d:e0:b3:e6:c1

Without this diff, 'ifconfig iwn0 scan' sometimes can scan the available
2ghz and 5ghz APs and sometimes not.  With this diff, iwn(4) can scan
the available 2ghz and 5ghz APs all the time.

ok kevlo@

> 
> Index: ieee80211.c
> ===
> RCS file: /cvs/src/sys/net80211/ieee80211.c,v
> retrieving revision 1.62
> diff -u -p -r1.62 ieee80211.c
> --- ieee80211.c   20 Jun 2017 13:51:46 -  1.62
> +++ ieee80211.c   3 Sep 2017 18:44:31 -
> @@ -877,7 +877,9 @@ ieee80211_next_mode(struct ifnet *ifp)
>   if (ic->ic_curmode == IEEE80211_MODE_11N)
>   continue;
>  
> - if (ic->ic_curmode >= IEEE80211_MODE_MAX) {
> + /* Always scan in AUTO mode if the driver scans all bands. */
> + if (ic->ic_curmode >= IEEE80211_MODE_MAX ||
> + (ic->ic_caps & IEEE80211_C_SCANALLBAND)) {
>   ic->ic_curmode = IEEE80211_MODE_AUTO;
>   break;
>   }
> 
> Long explanation:
> 
> The iwn, iwm, and wpi drivers run a firmware-based scan which receives
> beacons from all channels the device supports, across 2 and 5 GHz bands.
> Such drivers set a capability flag called 'SCANALLBAND'.
> 
> The net80211 stack filters received beacons based on the current active
> channel set. The active channel set depends on the current mode (11a/b/g/n).
> 
> For example, set the interface to 11g mode and scan:
>   ifconfig iwn0 mode 11g; ifconfig iwn0 scan
> Only 2GHz APs will be shown because net80211 filters out 5GHz ones.
> Conversely, with:
>   ifconfig iwn0 mode 11a; ifconfig iwn0 scan
> only 5GHz APs will be shown.
> Reset the mode to default (autoselect): ifconfig iwn0 -mode
> 
> The autoselect mode puts all channels in the active channel set during
> its first scan iteration. However, the automatic mode then also tries
> all fixed modes in sequence and recalculates the active channel set
> during each iteration (see net80211/ieee80211.c:ieee80211_setmode()).
> 
> This behaviour makes sense for devices which can only scan distinct
> channel sets, e.g. 11b and 11a, like ath(4).
> But with firmware-based scans which return beacons from all supported
> channels every time, it gets in the way.
> 
> While iwn is associated to a 2GHz AP, 'ifconfig iwn0 scan' will start out
> in 11g mode. The active channel set is restricted accordingly and only
> 2GHz APs will be seen during the first scan iteration. Even if there
> is a better 5GHz AP around it will only be selected if association to
> the 2GHz AP fails for some reason and another mode is tried.
> 
> If the interface is reset (ifconfig iwn0 down) then a scan always starts
> out in the initial 'auto' iteration with all channels in the initial active
> channel set. So to switch from a 2GHz AP to a better 5GHz AP, the interface
> must be stopped and restarted. Just running a scan is not enough.
> 
> The diff fixes this inconsistency by always keeping iwn, iwm, and wpi in
> auto mode during scans. This allows reception of beacons on all channels,
> unless 'ifconfig mode' was used to force a particular mode (not visible in
> context of the diff: this function returns early if a specific mode has
> been configured).
> 
> 

efiboot: Restore GOP mode on SetMode() failure

[Klemens Nanni]

This diff fixes missing video output when booting in UEFI mode on the
ThinkPad X121e (and possibly other boards with crappy implemenatations)
as reported earlier:

https://marc.info/?l=openbsd-bugs=150407033628497=2

When looking for the best mode in terms of resolution, we might end up
with modes providing higher resolutions than what's possible on the
actual display. Some buggy UEFI implemenations may also fail for other
non-obvious reasons.

Either ways, current behaviour is to just proceed after a failed attempt
of setting some better mode. With this diff the currently working GOP
mode is saved and set again in case SetMode() was unsuccessful.

That way my X121e boots fine using mode 7 (640x480x32bpp) instead of
blanking out, efifb(4) comes up until radeon(4) takes over with native
resolution.

Also tested successfully/without any regressions on a Dell Latitude
E5550 as well as a Fujitsu Lifebook A Series boards both running
latest stock UEFI firmware.


Not sure yet whether this is a mistake or I'm not getting it, but
either status indicating an error or ending up with an incorrect mode
should each be enough to call it a failure.
In fact, EFI_ERROR(status) may be non-zero although gop->Mode->Mode was
indeed set and equal to bestmode as this was the case on my X121e.


Feedback? Comments?

I also have a working and tested diff for `machine fb [n]' analog to
`machine video [n]' that enables listing/and setting the GOP mode just
like FreeBSD does it with `gop list | get | set '. I'd also like to
get this in together with some other minor clean ups in case this is
the way to go.

Testers, especially with broken implemenations like the X121e are
greatly appreciated.

Index: efiboot.c
===
RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
retrieving revision 1.23
diff -u -p -r1.23 efiboot.c
--- efiboot.c   7 Aug 2017 19:34:53 -   1.23
+++ efiboot.c   5 Sep 2017 00:29:02 -
@@ -702,13 +702,12 @@ static EFI_GUID smbios_guid = SMBIOS_TAB
 void
 efi_makebootargs(void)
 {
-   int  i;
EFI_STATUS   status;
EFI_GRAPHICS_OUTPUT *gop;
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
*gopi;
bios_efiinfo_t   ei;
-   int  bestmode = -1;
+   int  i, curmode, bestmode = -1;
UINTNsz, gopsiz, bestsiz = 0;
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION
*info;
@@ -746,9 +745,12 @@ efi_makebootargs(void)
}
}
if (bestmode >= 0) {
+   curmode = gop->Mode->Mode;
status = EFI_CALL(gop->SetMode, gop, bestmode);
-   if (EFI_ERROR(status) && gop->Mode->Mode != bestmode)
+   if (EFI_ERROR(status) || gop->Mode->Mode != bestmode) {
printf("GOP setmode failed(%d)\n", status);
+   EFI_CALL(gop->SetMode, gop, curmode);
+   }
}
 
gopi = gop->Mode->Info;

[vmd] vmctl console hung

[Mikhael Lialin]

Hi i'm running 6.2-current on Lenovo TP x240,

Installed alpine linux 3.6.2-virt as vmm guest.

I Faced issue that sometimes vmctl console  connection hungs and 
reconnection just point me to the thread that output that i'm typing 
without interaction. However while this happened i could connect to this 
vmm guest through ssh and all works.




OpenBSD 6.2-beta (GENERIC.MP) #63: Wed Aug 30 18:23:19 MDT 2017
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 8206696448 (7826MB)
avail mem = 7950970880 (7582MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xdcd3d000 (60 entries)
bios0: vendor LENOVO version "GIET89WW (2.39 )" date 04/26/2017
bios0: LENOVO 20AL006
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP SLIC DBGP ECDT HPET APIC MCFG SSDT SSDT SSDT SSDT SSDT 
SSDT SSDT SSDT PCCT SSDT UEFI MSDM ASF! BATB FPDT UEFI DMAR
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) EXP2(S4) XHCI(S3) EHC1(S3)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0
acpihpet0 at acpi0: 14318179 Hz
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 2694.13 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: TSC frequency 2694128190 Hz
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
cpu0: apic clock running at 99MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE
cpu1 at mainbus0: apid 1 (application processor)
cpu1: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 2693.77 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 1, core 0, package 0
cpu2 at mainbus0: apid 2 (application processor)
cpu2: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 2693.77 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu2: 256KB 64b/line 8-way L2 cache
cpu2: smt 0, core 1, package 0
cpu3 at mainbus0: apid 3 (application processor)
cpu3: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 2693.77 MHz
cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu3: 256KB 64b/line 8-way L2 cache
cpu3: smt 1, core 1, package 0
ioapic0 at mainbus0: apid 2 pa 0xfec0, version 20, 40 pins
acpimcfg0 at acpi0 addr 0xf800, bus 0-63
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus -1 (PEG_)
acpiprt2 at acpi0: bus 2 (EXP1)
acpiprt3 at acpi0: bus 3 (EXP2)
acpiprt4 at acpi0: bus -1 (EXP3)
acpicpu0 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), 
C1(1000@1 mwait.1), PSS
acpicpu1 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), 
C1(1000@1 mwait.1), PSS
acpicpu2 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), 
C1(1000@1 mwait.1), PSS
acpicpu3 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), 
C1(1000@1 mwait.1), PSS
acpipwrres0 at acpi0: PUBS, resource for XHCI, EHC1
acpitz0 at acpi0: critical temperature is 200 degC
acpibtn0 at acpi0: LID_
acpibtn1 at acpi0: SLPB
"LEN0071" at acpi0 not configured
"LEN0035" at acpi0 not configured
acpibat0 at acpi0: BAT0 model "45N1773" serial 16525 type LION oem "SANYO"
acpibat1 at acpi0: BAT1 model "45N1777" serial 27584 type LION oem "SANYO"
acpiac0 at acpi0: AC unit online
acpithinkpad0 at acpi0
"PNP0C14" at acpi0 not configured
"PNP0C14" at acpi0 not configured
"PNP0C14" at acpi0 not configured
"INT340F" at acpi0 not configured
acpivideo0 at acpi0: VID_
acpivout at acpivideo0 not configured
acpivideo1 at acpi0: VID_
cpu0: Enhanced SpeedStep 2694 MHz: speeds: 2701, 2700, 2600, 2400, 2300, 2100, 
2000, 1800, 1700, 1600, 1400, 1300, 1100, 1000, 800, 

divert_output() without ifa_ifwithaddr()

[Alexander Bluhm]

Hi,

This replaces the call to ifa_ifwithaddr() in divert_output() with
a route lookup to make it MP safe.  Only set the mbuf header fields
that are needed.  Validate the name input.

ok?

bluhm

Index: netinet/ip_divert.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_divert.c,v
retrieving revision 1.49
diff -u -p -r1.49 ip_divert.c
--- netinet/ip_divert.c 27 Jul 2017 12:04:42 -  1.49
+++ netinet/ip_divert.c 4 Sep 2017 21:32:49 -
@@ -61,8 +61,6 @@ int *divertctl_vars[DIVERTCTL_MAXID] = D
 
 int divbhashsize = DIVERTHASHSIZE;
 
-static struct sockaddr_in ipaddr = { sizeof(ipaddr), AF_INET };
-
 intdivert_output(struct inpcb *, struct mbuf *, struct mbuf *,
struct mbuf *);
 void
@@ -78,18 +76,14 @@ divert_output(struct inpcb *inp, struct 
 {
struct sockaddr_in *sin;
struct socket *so;
-   struct ifaddr *ifa;
-   int error = 0, min_hdrlen = 0, dir;
+   int error, min_hdrlen = 0, dir;
struct ip *ip;
u_int16_t off;
 
-   m->m_pkthdr.ph_ifidx = 0;
-   m->m_nextpkt = NULL;
-   m->m_pkthdr.ph_rtableid = inp->inp_rtableid;
-
m_freem(control);
 
-   sin = mtod(nam, struct sockaddr_in *);
+   if ((error = in_nam2sin(nam, )))
+   goto fail;
so = inp->inp_socket;
 
/* Do basic sanity checks. */
@@ -133,14 +127,17 @@ divert_output(struct inpcb *inp, struct 
m->m_pkthdr.pf.flags |= PF_TAG_DIVERTED_PACKET;
 
if (dir == PF_IN) {
-   ipaddr.sin_addr = sin->sin_addr;
-   /* XXXSMP ifa_ifwithaddr() is not safe. */
-   ifa = ifa_ifwithaddr(sintosa(), m->m_pkthdr.ph_rtableid);
-   if (ifa == NULL) {
+   struct rtentry *rt;
+   struct ifnet *ifp;
+
+   rt = rtalloc(sintosa(sin), 0, inp->inp_rtableid);
+   if (!rtisvalid(rt) || !ISSET(rt->rt_flags, RTF_LOCAL)) {
+   rtfree(rt);
error = EADDRNOTAVAIL;
goto fail;
}
-   m->m_pkthdr.ph_ifidx = ifa->ifa_ifp->if_index;
+   m->m_pkthdr.ph_ifidx = rt->rt_ifidx;
+   rtfree(rt);
 
/*
 * Recalculate IP and protocol checksums for the inbound packet
@@ -151,9 +148,16 @@ divert_output(struct inpcb *inp, struct 
ip->ip_sum = in_cksum(m, off);
in_proto_cksum_out(m, NULL);
 
-   /* XXXSMP ``ifa'' is not reference counted. */
-   ipv4_input(ifa->ifa_ifp, m);
+   ifp = if_get(m->m_pkthdr.ph_ifidx);
+   if (ifp == NULL) {
+   error = ENETDOWN;
+   goto fail;
+   }
+   ipv4_input(ifp, m);
+   if_put(ifp);
} else {
+   m->m_pkthdr.ph_rtableid = inp->inp_rtableid;
+
error = ip_output(m, NULL, >inp_route,
IP_ALLOWBROADCAST | IP_RAWOUTPUT, NULL, NULL, 0);
if (error == EACCES)/* translate pf(4) error for userland */

Re: ospfd: add IMSG_IFADDRADD to deal with "sh /etc/netstart if"

[Florian Riehm]

On 08/23/17 00:22, Florian Riehm wrote:

On 08/21/17 18:57, Remi Locherer wrote:

On Mon, Jul 24, 2017 at 04:59:46PM +0200, Remi Locherer wrote:

On Fri, Jul 21, 2017 at 06:24:06PM +0200, Remi Locherer wrote:

On Fri, Jul 21, 2017 at 02:45:03PM +0200, Florian Riehm wrote:

On 06/25/17 23:47, Remi Locherer wrote:

Hi,

ospfd does not react nicely when running "sh /etc/netstart if".

This is because adding the same address again do an interface results
in RTM_DELADDR and RTM_NEWADDR. ospfd handles the former but the later.
If this happens ospfd says "interface vether0:192.168.250.1 gone".
Adjacencies on that interface are down and ospfd can not recover.

The below patch adds IMSG_IFADDRADD to deal with that. With it ospfd
logs the following after "ifconfig vether0 192.168.250.1/24" (same address
as active before):



Hi Remi,

thanks for your report and your patch.
I think it is the right approach, but unfortunately it doesn't work in my setup.
If I run 'sh /etc/netstart vio1' vio1 goes down and stays down.
Please have a look to my config / logs. What is the differece between your and
my test?


I tested with an interface connected to stub network. Your output shows an
interface connected to a transit network. In my test setup I did not get the
error message: "if_join_group: error IP_ADD_MEMBERSHIP"

I'll look into it and try to fix this.


I could reproduce the behaviour you see with my patch when testing with
vmm and vio interfaces. It looks like in the IFADDRDEL case the interface
can not leave the multicast group.

I do not see this when testing with vether, pair or vlan (over ix)
interfaces. Could this be a bug with multicast handling in vio?



Today I did a test with an unpatched ospfd and a vio interface in vmm.

I started ospfd and waited till adjacency was up. Then I did
"ifconfig vio0 192.168.250.101/24" (same ip as set before).

The debug output from ospfd:

[...]
if_leave_group: error IP_DROP_MEMBERSHIP, interface vio0 address 224.0.0.5: 
Can't assign requested address
if_leave_group: error IP_DROP_MEMBERSHIP, interface vio0 address 224.0.0.6: 
Can't assign requested address
orig_rtr_lsa: area 0.0.0.0
orig_rtr_lsa: stub net, interface vio0
orig_rtr_lsa: stub net, interface vether0
if_act_elect: interface vio0 old dr 192.168.250.1 new dr 192.168.250.101, old 
bdr 192.168.250.101 new bdr none
orig_rtr_lsa: area 0.0.0.0
[...]

Doing the same with a pair or vether interface does not produce the message
"if_leave_group: error IP_DROP_MEMBERSHIP ".

My conclusion of this is that the error is problem with the vio driver and
not with my patch for ospfd.

Could we proceed with the proposed ospfd patch and attack the vio problem
separately?



Actually I would be fine with that approach after I am sure it is a vio(4)
problem.

I think vether(4) and pair(4) are a bit too exotic to prove your patch works ;)
Tomorrow I will test your change with em(4).

My problem is not the multicast error message. I just can't 'see' your
fix, since my interfaces stay down after netstart.


Hi,

Now I got it. We are seeing a race!
Ospfd calls if_leave_group() after the kernel has deleted the interface address.
Then the syscall enters ip_setmoptions() in sys/netinet/ip_output.c.
Here ifa_ifwithaddr() (see case IP_DROP_MEMBERSHIP) fails, since the address is
gone at the moment.

Putting sleep(1) before setsockopt(iface->fd, IPPROTO_IP, IP_DROP_MEMBERSHIP...)
let the problem disappear. Of course, this is not the fix.

I am thinking about the right way to fix this.

Re: update mi_switch(9)

[Philip Guenther]

On Sat, Sep 2, 2017 at 8:39 PM, Jonathan Matthew  wrote:

> Having looked through various parts of the context switching code this
> week,
> I noticed that mi_switch() has a man page, and it's quite out of date.
>
> tsleep() isn't the only sleep function, but I don't think there's much
> point
> listing them all.  Additionally, yield() and preempt() also exist, but
> aren't
> documented.
>
> The 'renice after 10 minutes of cpu time' thing was removed in 2003
> (r1.47 of kern_synch.c).
>
> It's more useful to mention the scheduler lock rather than a specific
> (wrong?)
> spl here, since mi_switch() does a SCHED_ASSERT_LOCKED().
>
> ok?
>

ok guenther@



> Alternatively, I'm not sure what the audience for this page is.  It's
> mostly
> a list of reasons you don't need to call mi_switch() because either it
> happens to you automatically or it's used to implement part of some other
> function.  Maybe we should just remove it and document the more useful
> kernel
> functions like yield()/preempt() better?
>

I think we should keep it.  Adding a preempt(9) page, or perhaps a
sched_pause(9) page that covers both itself, preempt(), and yield(), would
be a fine addition, but they don't obviate the utility of a page covering
the function that underlies them, IMO.

Re: ftp: leak in progressmeter()

[Alexander Bluhm]

On Sun, Sep 03, 2017 at 03:19:15PM -0500, Scott Cheloha wrote:
> Feedback?

It looks like progressmeter() can be called with flag -1 consecutively
due to some error path.  So better free the title.  It cannot hurt.

OK bluhm@

> Index: usr.bin/ftp/util.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/util.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 util.c
> --- usr.bin/ftp/util.c21 Jan 2017 08:33:07 -  1.84
> +++ usr.bin/ftp/util.c3 Sep 2017 19:52:26 -
> @@ -784,8 +784,10 @@ progressmeter(int flag, const char *file
>   ratio = MINIMUM(ratio, 100);
>   if (!verbose && flag == -1) {
>   filename = basename(filename);
> - if (filename != NULL)
> + if (filename != NULL) {
> + free(title);
>   title = strdup(filename);
> + }
>   }
>  
>   buf[0] = 0;

Re: refactoring of pf_find_or_create_ruleset()

[Alexander Bluhm]

On Mon, Sep 04, 2017 at 10:29:01AM +0200, Alexandr Nedvedicky wrote:
> anyway below is the patch, which Hrvoje was testing and it worked for him.
> I'd like to get some OK to proceed to commit.

I think it is correct.  OK bluhm@

> + if (parent != NULL) {
> + /*
> +  * Make sure path for levels 2, 3, ... is terminated by '/':
> +  *  1/2/3/...
> +  */
> + strlcpy(anchor->path, parent->path, sizeof (anchor->path));

Do not put space between sizeof and (

Re: *usrreq() need a socket lock

[Alexander Bluhm]

On Mon, Sep 04, 2017 at 10:03:32AM +0200, Martin Pieuchot wrote:
> For some of them it happens to be the NET_LOCK(), but not all of them
> need it.  So use the correct macro.

For consistency rip6_usrreq() and divert6_usrreq() should get that, too.

> ok?

OK bluhm@

> Index: net/rtsock.c
> ===
> RCS file: /cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.249
> diff -u -p -r1.249 rtsock.c
> --- net/rtsock.c  1 Sep 2017 15:05:31 -   1.249
> +++ net/rtsock.c  4 Sep 2017 08:00:14 -
> @@ -177,6 +177,8 @@ route_usrreq(struct socket *so, int req,
>   int  af;
>   int  error = 0;
>  
> + soassertlocked(so);
> +
>   rop = sotoroutecb(so);
>   if (rop == NULL) {
>   m_freem(m);
> Index: netinet/ip_divert.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_divert.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 ip_divert.c
> --- netinet/ip_divert.c   27 Jul 2017 12:04:42 -  1.49
> +++ netinet/ip_divert.c   4 Sep 2017 08:00:14 -
> @@ -248,7 +248,7 @@ divert_usrreq(struct socket *so, int req
>   struct inpcb *inp = sotoinpcb(so);
>   int error = 0;
>  
> - NET_ASSERT_LOCKED();
> + soassertlocked(so);
>  
>   if (req == PRU_CONTROL) {
>   return (in_control(so, (u_long)m, (caddr_t)addr,
> Index: netinet/raw_ip.c
> ===
> RCS file: /cvs/src/sys/netinet/raw_ip.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 raw_ip.c
> --- netinet/raw_ip.c  1 Sep 2017 15:05:31 -   1.102
> +++ netinet/raw_ip.c  4 Sep 2017 08:00:14 -
> @@ -392,7 +392,7 @@ rip_usrreq(struct socket *so, int req, s
>   struct inpcb *inp = sotoinpcb(so);
>   int error = 0;
>  
> - NET_ASSERT_LOCKED();
> + soassertlocked(so);
>  
>   if (req == PRU_CONTROL)
>   return (in_control(so, (u_long)m, (caddr_t)nam,
> Index: netinet/tcp_usrreq.c
> ===
> RCS file: /cvs/src/sys/netinet/tcp_usrreq.c,v
> retrieving revision 1.154
> diff -u -p -r1.154 tcp_usrreq.c
> --- netinet/tcp_usrreq.c  1 Sep 2017 15:05:31 -   1.154
> +++ netinet/tcp_usrreq.c  4 Sep 2017 08:00:14 -
> @@ -131,7 +131,7 @@ tcp_usrreq(struct socket *so, int req, s
>   int error = 0;
>   short ostate;
>  
> - NET_ASSERT_LOCKED();
> + soassertlocked(so);
>  
>   if (req == PRU_CONTROL) {
>  #ifdef INET6
> Index: netinet/udp_usrreq.c
> ===
> RCS file: /cvs/src/sys/netinet/udp_usrreq.c,v
> retrieving revision 1.239
> diff -u -p -r1.239 udp_usrreq.c
> --- netinet/udp_usrreq.c  11 Aug 2017 19:53:02 -  1.239
> +++ netinet/udp_usrreq.c  4 Sep 2017 08:00:14 -
> @@ -1060,7 +1060,7 @@ udp_usrreq(struct socket *so, int req, s
>   struct inpcb *inp;
>   int error = 0;
>  
> - NET_ASSERT_LOCKED();
> + soassertlocked(so);
>  
>   if (req == PRU_CONTROL) {
>  #ifdef INET6

Re: solock() & nfs_connect

[Alexander Bluhm]

On Fri, Sep 01, 2017 at 06:29:33PM +0200, Martin Pieuchot wrote:
> Here's an alternative approach that pre-allocate mbufs.  This reduce
> the number of MGET() in the function an allow us to do a single
> solock()/sounlock() dance.

Yes, I like that.  You could rename m to nam to be consistent with
other functions and make clear what this mbuf is used for.

> ok?

OK bluhm@

> Index: nfs//nfs_socket.c
> ===
> RCS file: /cvs/src/sys/nfs/nfs_socket.c,v
> retrieving revision 1.126
> diff -u -p -r1.126 nfs_socket.c
> --- nfs//nfs_socket.c 1 Sep 2017 15:05:31 -   1.126
> +++ nfs//nfs_socket.c 1 Sep 2017 16:09:16 -
> @@ -238,20 +238,28 @@ nfs_connect(struct nfsmount *nmp, struct
>   int s, error, rcvreserve, sndreserve;
>   struct sockaddr *saddr;
>   struct sockaddr_in *sin;
> - struct mbuf *m;
> + struct mbuf *m = NULL, *mopt = NULL;
>  
> - if (!(nmp->nm_sotype == SOCK_DGRAM || nmp->nm_sotype == SOCK_STREAM)) {
> - error = EINVAL;
> - goto bad;
> - }
> + if (!(nmp->nm_sotype == SOCK_DGRAM || nmp->nm_sotype == SOCK_STREAM))
> + return (EINVAL);
>  
>   nmp->nm_so = NULL;
>   saddr = mtod(nmp->nm_nam, struct sockaddr *);
>   error = socreate(saddr->sa_family, >nm_so, nmp->nm_sotype, 
>   nmp->nm_soproto);
> - if (error)
> - goto bad;
> + if (error) {
> + nfs_disconnect(nmp);
> + return (error);
> + }
> +
> + /* Allocate mbufs possibly waiting before grabbing the socket lock. */
> + if (nmp->nm_sotype == SOCK_STREAM || saddr->sa_family == AF_INET)
> + MGET(mopt, M_WAIT, MT_SOOPTS);
> + if (saddr->sa_family == AF_INET)
> + MGET(m, M_WAIT, MT_SONAME);
> +
>   so = nmp->nm_so;
> + s = solock(so);
>   nmp->nm_soflags = so->so_proto->pr_flags;
>  
>   /*
> @@ -260,42 +268,29 @@ nfs_connect(struct nfsmount *nmp, struct
>* disclosure through UDP port capture.
>*/
>   if (saddr->sa_family == AF_INET) {
> - struct mbuf *mopt;
>   int *ip;
>  
> - MGET(mopt, M_WAIT, MT_SOOPTS);
>   mopt->m_len = sizeof(int);
>   ip = mtod(mopt, int *);
>   *ip = IP_PORTRANGE_LOW;
> - s = solock(so);
>   error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> - sounlock(s);
> - m_freem(mopt);
>   if (error)
>   goto bad;
>  
> - MGET(m, M_WAIT, MT_SONAME);
>   sin = mtod(m, struct sockaddr_in *);
>   memset(sin, 0, sizeof(*sin));
>   sin->sin_len = m->m_len = sizeof(struct sockaddr_in);
>   sin->sin_family = AF_INET;
>   sin->sin_addr.s_addr = INADDR_ANY;
>   sin->sin_port = htons(0);
> - s = solock(so);
>   error = sobind(so, m, );
> - sounlock(s);
> - m_freem(m);
>   if (error)
>   goto bad;
>  
> - MGET(mopt, M_WAIT, MT_SOOPTS);
>   mopt->m_len = sizeof(int);
>   ip = mtod(mopt, int *);
>   *ip = IP_PORTRANGE_DEFAULT;
> - s = solock(so);
>   error = sosetopt(so, IPPROTO_IP, IP_PORTRANGE, mopt);
> - sounlock(s);
> - m_freem(mopt);
>   if (error)
>   goto bad;
>   }
> @@ -310,12 +305,9 @@ nfs_connect(struct nfsmount *nmp, struct
>   goto bad;
>   }
>   } else {
> - s = solock(so);
>   error = soconnect(so, nmp->nm_nam);
> - if (error) {
> - sounlock(s);
> + if (error)
>   goto bad;
> - }
>  
>   /*
>* Wait for the connection to complete. Cribbed from the
> @@ -328,23 +320,19 @@ nfs_connect(struct nfsmount *nmp, struct
>   so->so_error == 0 && rep &&
>   (error = nfs_sigintr(nmp, rep, rep->r_procp)) != 0){
>   so->so_state &= ~SS_ISCONNECTING;
> - sounlock(s);
>   goto bad;
>   }
>   }
>   if (so->so_error) {
>   error = so->so_error;
>   so->so_error = 0;
> - sounlock(s);
>   goto bad;
>   }
> - sounlock(s);
>   }
>   /*
>* Always set receive timeout to detect server crash and reconnect.
>* Otherwise, we can get stuck in soreceive forever.
>*/
> - s = solock(so);
>   so->so_rcv.sb_timeo = (5 * hz);
>   if (nmp->nm_flag & (NFSMNT_SOFT | NFSMNT_INT))
>   so->so_snd.sb_timeo = (5 * hz);
> @@ -356,18 +344,14 @@ nfs_connect(struct 

sdio: assert write lock instead of taking lock

[Patrick Wildt]

Hi,

apparently one of the main concepts in the SDMMC I/O subsystem is that
the driver attached to an SDIO card always holds the lock and only
releases it if it detaches.  Now with that in mind, some time ago the
sdmmc_io_function_disable() and sdmmc_io_function_ready() was changed
to only assert the write lock and not take it.  The funny thing though
is that sdmmc_io_function_enable() wasn't changed and still enters and
exits the lock, and then calls _ready(), which asserts the write lock.
This does not seem to be right.  Similarly, the interrupt code does
the enter/exit dance, even though it should be called out of a context
where the lock is taken.  I propose to remove the dance and change
those to simple write lock assertions.  Apparently there is no SDIO
driver yet/anymore which would trigger those issues.

Opinions?

Patrick

diff --git a/sys/dev/sdmmc/sdmmc_io.c b/sys/dev/sdmmc/sdmmc_io.c
index 110e9f15e1d..6a25bfae33a 100644
--- a/sys/dev/sdmmc/sdmmc_io.c
+++ b/sys/dev/sdmmc/sdmmc_io.c
@@ -236,14 +236,14 @@ sdmmc_io_function_enable(struct sdmmc_function *sf)
u_int8_t rv;
int retry = 5;
 
+   rw_assert_wrlock(>sc_lock);
+
if (sf->number == 0)
return 0;   /* FN0 is always enabled */
 
-   rw_enter_write(>sc_lock);
rv = sdmmc_io_read_1(sf0, SD_IO_CCCR_FN_ENABLE);
rv |= (1sc_lock);
 
while (!sdmmc_io_function_ready(sf) && retry-- > 0)
tsleep(, PPAUSE, "pause", 0);
@@ -621,11 +621,11 @@ sdmmc_intr_enable(struct sdmmc_function *sf)
struct sdmmc_function *sf0 = sc->sc_fn0;
u_int8_t imask;
 
-   rw_enter_write(>sc_lock);
+   rw_assert_wrlock(>sc_lock);
+
imask = sdmmc_io_read_1(sf0, SD_IO_CCCR_INT_ENABLE);
imask |= 1 << sf->number;
sdmmc_io_write_1(sf0, SD_IO_CCCR_INT_ENABLE, imask);
-   rw_exit(>sc_lock);
 }
 
 void
@@ -635,11 +635,11 @@ sdmmc_intr_disable(struct sdmmc_function *sf)
struct sdmmc_function *sf0 = sc->sc_fn0;
u_int8_t imask;
 
-   rw_enter_write(>sc_lock);
+   rw_assert_wrlock(>sc_lock);
+
imask = sdmmc_io_read_1(sf0, SD_IO_CCCR_INT_ENABLE);
imask &= ~(1 << sf->number);
sdmmc_io_write_1(sf0, SD_IO_CCCR_INT_ENABLE, imask);
-   rw_exit(>sc_lock);
 }
 
 /*

Re: readlink: be quiet about overlong argument without '-f' option

[Theo de Raadt]

I feel the same way about this.  It can help in deep trees.

> On Mon, Sep 04, 2017 at 11:43:56AM -0500, Scott Cheloha wrote:
> > Thoughts?
> 
> The PATH_MAX check was introduced in rev 1.10 1997/09/23.  That was
> after the doumentation, so it might be a mistake that the man page
> was not updated.
> 
> The feature worked for 20 years, I see no reason to remove it.
> 
> Personally I prefer to see the error when something goes wrong.
> 
> bluhm
> 
> > Index: usr.bin/readlink/readlink.c
> > ===
> > RCS file: /cvs/src/usr.bin/readlink/readlink.c,v
> > retrieving revision 1.27
> > diff -u -p -r1.27 readlink.c
> > --- usr.bin/readlink/readlink.c 9 Oct 2015 01:37:08 -   1.27
> > +++ usr.bin/readlink/readlink.c 4 Sep 2017 15:57:47 -
> > @@ -64,14 +64,6 @@ main(int argc, char *argv[])
> > if (argc != 1)
> > usage();
> >  
> > -   n = strlen(argv[0]);
> > -   if (n > PATH_MAX - 1) {
> > -   fprintf(stderr,
> > -   "readlink: filename longer than PATH_MAX-1 (%d)\n",
> > -   PATH_MAX - 1);
> > -   exit(1);
> > -   }
> > -
> > if (fflag) {
> > if (realpath(argv[0], buf) == NULL)
> > err(1, "%s", argv[0]);
> 

Re: readlink: be quiet about overlong argument without '-f' option

[Alexander Bluhm]

On Mon, Sep 04, 2017 at 11:43:56AM -0500, Scott Cheloha wrote:
> Thoughts?

The PATH_MAX check was introduced in rev 1.10 1997/09/23.  That was
after the doumentation, so it might be a mistake that the man page
was not updated.

The feature worked for 20 years, I see no reason to remove it.

Personally I prefer to see the error when something goes wrong.

bluhm

> Index: usr.bin/readlink/readlink.c
> ===
> RCS file: /cvs/src/usr.bin/readlink/readlink.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 readlink.c
> --- usr.bin/readlink/readlink.c   9 Oct 2015 01:37:08 -   1.27
> +++ usr.bin/readlink/readlink.c   4 Sep 2017 15:57:47 -
> @@ -64,14 +64,6 @@ main(int argc, char *argv[])
>   if (argc != 1)
>   usage();
>  
> - n = strlen(argv[0]);
> - if (n > PATH_MAX - 1) {
> - fprintf(stderr,
> - "readlink: filename longer than PATH_MAX-1 (%d)\n",
> - PATH_MAX - 1);
> - exit(1);
> - }
> -
>   if (fflag) {
>   if (realpath(argv[0], buf) == NULL)
>   err(1, "%s", argv[0]);

readlink: be quiet about overlong argument without '-f' option

[Scott Cheloha]

Hi,

readlink(1) has a check before calling readlink(2) that prints a
hand-rolled error message if your argument exceeds (PATH_MAX - 1)
bytes.

$ readlink $(perl -e 'print "z"x1024')

This contradicts the documentation, which says that absent the '-f'
flag nothing will be printed if the argument is not a symbolic link.
Exceeding (PATH_MAX - 1) bytes definitely excludes your argument, so
in that case readlink(1) should just say nothing.

With the '-f' option, readlink(1) already complains about an overlong
path or path component, so in that case the check is redundant.

$ readlink -f $(perl -e 'print "z"x256') 

Without the '-f' flag this change would put us in sync with GNU
readlink's behavior.  With the '-f' flag, it puts us in sync with GNU
realpath's behavior.

CC'd nicm@ because he expressed interest in this a few months back
when I submitted it as part of a larger patch that didn't make sense
in aggregate.

Thoughts?

--
Scott Cheloha

Index: usr.bin/readlink/readlink.c
===
RCS file: /cvs/src/usr.bin/readlink/readlink.c,v
retrieving revision 1.27
diff -u -p -r1.27 readlink.c
--- usr.bin/readlink/readlink.c 9 Oct 2015 01:37:08 -   1.27
+++ usr.bin/readlink/readlink.c 4 Sep 2017 15:57:47 -
@@ -64,14 +64,6 @@ main(int argc, char *argv[])
if (argc != 1)
usage();
 
-   n = strlen(argv[0]);
-   if (n > PATH_MAX - 1) {
-   fprintf(stderr,
-   "readlink: filename longer than PATH_MAX-1 (%d)\n",
-   PATH_MAX - 1);
-   exit(1);
-   }
-
if (fflag) {
if (realpath(argv[0], buf) == NULL)
err(1, "%s", argv[0]);

Re: [patch] Initialize "cur" to avoid undefined behavior is dmesg.c

[Nan Xiao]

Hi Tom,

Updated, thanks!

Best Regards
Nan Xiao

Index: dmesg.c
===
RCS file: /cvs/src/sbin/dmesg/dmesg.c,v
retrieving revision 1.29
diff -u -p -r1.29 dmesg.c
--- dmesg.c 1 Sep 2017 07:31:45 -   1.29
+++ dmesg.c 4 Sep 2017 13:17:01 -
@@ -65,12 +65,12 @@ main(int argc, char *argv[])
int ch, newl, skip, i;
char *p;
struct msgbuf cur;
-   char *memf, *nlistf, *bufdata = NULL;
+   char *memf = NULL, *nlistf = NULL, *bufdata = NULL;
char *allocated = NULL;
int startupmsgs = 0;
char buf[5];

-   memf = nlistf = NULL;
+   memset(, 0, sizeof(cur));
while ((ch = getopt(argc, argv, "sM:N:")) != -1)
switch(ch) {
case 's':


On 9/4/2017 8:07 PM, Tom Cosgrove wrote:
>> -free(allocated);
>> +if (allocated)
>> +free(allocated);
> 
> This is unnecessary, since free(NULL) is clearly defined as a no-op.
> See the malloc(3) man page.
> 
> Tom
> 
 Nan Xiao 4-Sep-17 12:11 >>>
>>
>> Hi tech@,
>>
>> This patch fixes the extreme case in dmesg.c: if memf or nlistf is not
>> NULL, and "NOKVM" macro is defined.
>>
>> Current code in dmesg.c:
>>
>>  struct msgbuf cur;
>>  
>> Since "cur" is not initialized, so the following code has undefined
>> behavior:
>>
>>  if (cur.msg_bufx >= cur.msg_bufs)
>>  cur.msg_bufx = 0;
>>  /*
>>   * The message buffer is circular; start at the read pointer, and
>>   * go to the write pointer - 1.
>>   */
>>  for (newl = skip = i = 0, p = bufdata + cur.msg_bufx;
>>  i < cur.msg_bufs; i++, p++) {
>>  .
>>  }
>>
>> My patch can skip the whole loop, and the "dmesg" program just prints
>> a newline:
>>
>>  if (!newl)
>>  putchar('\n');
>>
>> Best Regards
>> Nan Xiao
>>
>> Index: dmesg.c
>> ===
>> RCS file: /cvs/src/sbin/dmesg/dmesg.c,v
>> retrieving revision 1.29
>> diff -u -p -r1.29 dmesg.c
>> --- dmesg.c  1 Sep 2017 07:31:45 -   1.29
>> +++ dmesg.c  4 Sep 2017 08:55:50 -
>> @@ -65,12 +65,12 @@ main(int argc, char *argv[])
>>  int ch, newl, skip, i;
>>  char *p;
>>  struct msgbuf cur;
>> -char *memf, *nlistf, *bufdata = NULL;
>> +char *memf = NULL, *nlistf = NULL, *bufdata = NULL;
>>  char *allocated = NULL;
>>  int startupmsgs = 0;
>>  char buf[5];
>>
>> -memf = nlistf = NULL;
>> +memset(, 0, sizeof(cur));
>>  while ((ch = getopt(argc, argv, "sM:N:")) != -1)
>>  switch(ch) {
>>  case 's':
>> @@ -184,7 +184,8 @@ main(int argc, char *argv[])
>>  }
>>  if (!newl)
>>  putchar('\n');
>> -free(allocated);
>> +if (allocated)
>> +free(allocated);
>>  return (0);
>>  }

Re: Routing table update on link state change

[Gerhard Roth]

Hi Martin,


On Mon, 4 Sep 2017 14:18:50 +0200 Martin Pieuchot  wrote:
> On 04/09/17(Mon) 13:10, Gerhard Roth wrote:
> > Hi,
> > 
> > I noticed a problem with the routing table that is easy to reproduce: put
> > multiple IPs on the same carp interface:
> 
> Great bug analysis, however returning EAGAIN for every route update is a
> no go if you have a big routing table like BGP full feeds.
> 
> We should return EAGAIN only if the multipath list contains multiple
> elements.
> 
> But since we're now returning EAGAIN much often we want to skip route
> entries that have already been borough UP/taken down.
> 
> Diff below does that, does it work for you?  Do you mind adding a
> regression test?

I can confirm that your version works for me. Thanks for the improvement.

Regarding the regression test: gimme some time ;)

Gerhard


> 
> Index: net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.366
> diff -u -p -r1.366 route.c
> --- net/route.c   11 Aug 2017 21:24:19 -  1.366
> +++ net/route.c   4 Sep 2017 12:08:16 -
> @@ -1550,6 +1550,7 @@ rt_if_linkstate_change(struct rtentry *r
>  {
>   struct ifnet *ifp = arg;
>   struct sockaddr_in6 sa_mask;
> + int error;
>  
>   if (rt->rt_ifidx != ifp->if_index)
>   return (0);
> @@ -1561,38 +1562,37 @@ rt_if_linkstate_change(struct rtentry *r
>   }
>  
>   if (LINK_STATE_IS_UP(ifp->if_link_state) && ifp->if_flags & IFF_UP) {
> - if (!(rt->rt_flags & RTF_UP)) {
> - /* bring route up */
> - rt->rt_flags |= RTF_UP;
> - rtable_mpath_reprio(id, rt_key(rt),
> - rt_plen2mask(rt, _mask),
> - rt->rt_priority & RTP_MASK, rt);
> - }
> + if (ISSET(rt->rt_flags, RTF_UP))
> + return (0);
> +
> + /* bring route up */
> + rt->rt_flags |= RTF_UP;
> + error = rtable_mpath_reprio(id, rt_key(rt),
> + rt_plen2mask(rt, _mask), rt->rt_priority & RTP_MASK, rt);
>   } else {
> - if (rt->rt_flags & RTF_UP) {
> - /*
> -  * Remove redirected and cloned routes (mainly ARP)
> -  * from down interfaces so we have a chance to get
> -  * new routes from a better source.
> -  */
> - if (ISSET(rt->rt_flags, RTF_CLONED|RTF_DYNAMIC) &&
> - !ISSET(rt->rt_flags, RTF_CACHED|RTF_BFD)) {
> - int error;
> -
> - if ((error = rtdeletemsg(rt, ifp, id)))
> - return (error);
> - return (EAGAIN);
> - }
> - /* take route down */
> - rt->rt_flags &= ~RTF_UP;
> - rtable_mpath_reprio(id, rt_key(rt),
> - rt_plen2mask(rt, _mask),
> - rt->rt_priority | RTP_DOWN, rt);
> + /*
> +  * Remove redirected and cloned routes (mainly ARP)
> +  * from down interfaces so we have a chance to get
> +  * new routes from a better source.
> +  */
> + if (ISSET(rt->rt_flags, RTF_CLONED|RTF_DYNAMIC) &&
> + !ISSET(rt->rt_flags, RTF_CACHED|RTF_BFD)) {
> + if ((error = rtdeletemsg(rt, ifp, id)))
> + return (error);
> + return (EAGAIN);
>   }
> +
> + if (!ISSET(rt->rt_flags, RTF_UP))
> + return (0);
> +
> + /* take route down */
> + rt->rt_flags &= ~RTF_UP;
> + error = rtable_mpath_reprio(id, rt_key(rt),
> + rt_plen2mask(rt, _mask), rt->rt_priority | RTP_DOWN, rt);
>   }
>   if_group_routechange(rt_key(rt), rt_plen2mask(rt, _mask));
>  
> - return (0);
> + return (error);
>  }
>  
>  struct sockaddr *
> Index: net/rtable.c
> ===
> RCS file: /cvs/src/sys/net/rtable.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 rtable.c
> --- net/rtable.c  30 Jul 2017 18:18:08 -  1.61
> +++ net/rtable.c  4 Sep 2017 12:02:00 -
> @@ -751,6 +751,7 @@ rtable_mpath_reprio(unsigned int rtablei
>   rt->rt_priority = prio;
>   rtable_mpath_insert(an, rt);
>   rtfree(rt);
> + error = EAGAIN;
>   }
>   rw_exit_write(>ar_lock);
>  

Re: Routing table update on link state change

[Martin Pieuchot]

On 04/09/17(Mon) 13:10, Gerhard Roth wrote:
> Hi,
> 
> I noticed a problem with the routing table that is easy to reproduce: put
> multiple IPs on the same carp interface:

Great bug analysis, however returning EAGAIN for every route update is a
no go if you have a big routing table like BGP full feeds.

We should return EAGAIN only if the multipath list contains multiple
elements.

But since we're now returning EAGAIN much often we want to skip route
entries that have already been borough UP/taken down.

Diff below does that, does it work for you?  Do you mind adding a
regression test?

Index: net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.366
diff -u -p -r1.366 route.c
--- net/route.c 11 Aug 2017 21:24:19 -  1.366
+++ net/route.c 4 Sep 2017 12:08:16 -
@@ -1550,6 +1550,7 @@ rt_if_linkstate_change(struct rtentry *r
 {
struct ifnet *ifp = arg;
struct sockaddr_in6 sa_mask;
+   int error;
 
if (rt->rt_ifidx != ifp->if_index)
return (0);
@@ -1561,38 +1562,37 @@ rt_if_linkstate_change(struct rtentry *r
}
 
if (LINK_STATE_IS_UP(ifp->if_link_state) && ifp->if_flags & IFF_UP) {
-   if (!(rt->rt_flags & RTF_UP)) {
-   /* bring route up */
-   rt->rt_flags |= RTF_UP;
-   rtable_mpath_reprio(id, rt_key(rt),
-   rt_plen2mask(rt, _mask),
-   rt->rt_priority & RTP_MASK, rt);
-   }
+   if (ISSET(rt->rt_flags, RTF_UP))
+   return (0);
+
+   /* bring route up */
+   rt->rt_flags |= RTF_UP;
+   error = rtable_mpath_reprio(id, rt_key(rt),
+   rt_plen2mask(rt, _mask), rt->rt_priority & RTP_MASK, rt);
} else {
-   if (rt->rt_flags & RTF_UP) {
-   /*
-* Remove redirected and cloned routes (mainly ARP)
-* from down interfaces so we have a chance to get
-* new routes from a better source.
-*/
-   if (ISSET(rt->rt_flags, RTF_CLONED|RTF_DYNAMIC) &&
-   !ISSET(rt->rt_flags, RTF_CACHED|RTF_BFD)) {
-   int error;
-
-   if ((error = rtdeletemsg(rt, ifp, id)))
-   return (error);
-   return (EAGAIN);
-   }
-   /* take route down */
-   rt->rt_flags &= ~RTF_UP;
-   rtable_mpath_reprio(id, rt_key(rt),
-   rt_plen2mask(rt, _mask),
-   rt->rt_priority | RTP_DOWN, rt);
+   /*
+* Remove redirected and cloned routes (mainly ARP)
+* from down interfaces so we have a chance to get
+* new routes from a better source.
+*/
+   if (ISSET(rt->rt_flags, RTF_CLONED|RTF_DYNAMIC) &&
+   !ISSET(rt->rt_flags, RTF_CACHED|RTF_BFD)) {
+   if ((error = rtdeletemsg(rt, ifp, id)))
+   return (error);
+   return (EAGAIN);
}
+
+   if (!ISSET(rt->rt_flags, RTF_UP))
+   return (0);
+
+   /* take route down */
+   rt->rt_flags &= ~RTF_UP;
+   error = rtable_mpath_reprio(id, rt_key(rt),
+   rt_plen2mask(rt, _mask), rt->rt_priority | RTP_DOWN, rt);
}
if_group_routechange(rt_key(rt), rt_plen2mask(rt, _mask));
 
-   return (0);
+   return (error);
 }
 
 struct sockaddr *
Index: net/rtable.c
===
RCS file: /cvs/src/sys/net/rtable.c,v
retrieving revision 1.61
diff -u -p -r1.61 rtable.c
--- net/rtable.c30 Jul 2017 18:18:08 -  1.61
+++ net/rtable.c4 Sep 2017 12:02:00 -
@@ -751,6 +751,7 @@ rtable_mpath_reprio(unsigned int rtablei
rt->rt_priority = prio;
rtable_mpath_insert(an, rt);
rtfree(rt);
+   error = EAGAIN;
}
rw_exit_write(>ar_lock);
 

Re: [patch] Initialize "cur" to avoid undefined behavior is dmesg.c

[Tom Cosgrove]

> - free(allocated);
> + if (allocated)
> + free(allocated);

This is unnecessary, since free(NULL) is clearly defined as a no-op.
See the malloc(3) man page.

Tom

>>> Nan Xiao 4-Sep-17 12:11 >>>
>
> Hi tech@,
>
> This patch fixes the extreme case in dmesg.c: if memf or nlistf is not
> NULL, and "NOKVM" macro is defined.
>
> Current code in dmesg.c:
>
>   struct msgbuf cur;
>   
> Since "cur" is not initialized, so the following code has undefined
> behavior:
>
>   if (cur.msg_bufx >= cur.msg_bufs)
>   cur.msg_bufx = 0;
>   /*
>* The message buffer is circular; start at the read pointer, and
>* go to the write pointer - 1.
>*/
>   for (newl = skip = i = 0, p = bufdata + cur.msg_bufx;
>   i < cur.msg_bufs; i++, p++) {
>   .
>   }
>
> My patch can skip the whole loop, and the "dmesg" program just prints
> a newline:
>
>   if (!newl)
>   putchar('\n');
>
> Best Regards
> Nan Xiao
>
> Index: dmesg.c
> ===
> RCS file: /cvs/src/sbin/dmesg/dmesg.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 dmesg.c
> --- dmesg.c   1 Sep 2017 07:31:45 -   1.29
> +++ dmesg.c   4 Sep 2017 08:55:50 -
> @@ -65,12 +65,12 @@ main(int argc, char *argv[])
>   int ch, newl, skip, i;
>   char *p;
>   struct msgbuf cur;
> - char *memf, *nlistf, *bufdata = NULL;
> + char *memf = NULL, *nlistf = NULL, *bufdata = NULL;
>   char *allocated = NULL;
>   int startupmsgs = 0;
>   char buf[5];
>
> - memf = nlistf = NULL;
> + memset(, 0, sizeof(cur));
>   while ((ch = getopt(argc, argv, "sM:N:")) != -1)
>   switch(ch) {
>   case 's':
> @@ -184,7 +184,8 @@ main(int argc, char *argv[])
>   }
>   if (!newl)
>   putchar('\n');
> - free(allocated);
> + if (allocated)
> + free(allocated);
>   return (0);
>  }

[patch] Initialize "cur" to avoid undefined behavior is dmesg.c

[Nan Xiao]

Hi tech@,

This patch fixes the extreme case in dmesg.c: if memf or nlistf is not
NULL, and "NOKVM" macro is defined.

Current code in dmesg.c:

struct msgbuf cur;

Since "cur" is not initialized, so the following code has undefined
behavior:

if (cur.msg_bufx >= cur.msg_bufs)
cur.msg_bufx = 0;
/*
 * The message buffer is circular; start at the read pointer, and
 * go to the write pointer - 1.
 */
for (newl = skip = i = 0, p = bufdata + cur.msg_bufx;
i < cur.msg_bufs; i++, p++) {
.
}

My patch can skip the whole loop, and the "dmesg" program just prints
a newline:

if (!newl)
putchar('\n');

Best Regards
Nan Xiao

Index: dmesg.c
===
RCS file: /cvs/src/sbin/dmesg/dmesg.c,v
retrieving revision 1.29
diff -u -p -r1.29 dmesg.c
--- dmesg.c 1 Sep 2017 07:31:45 -   1.29
+++ dmesg.c 4 Sep 2017 08:55:50 -
@@ -65,12 +65,12 @@ main(int argc, char *argv[])
int ch, newl, skip, i;
char *p;
struct msgbuf cur;
-   char *memf, *nlistf, *bufdata = NULL;
+   char *memf = NULL, *nlistf = NULL, *bufdata = NULL;
char *allocated = NULL;
int startupmsgs = 0;
char buf[5];

-   memf = nlistf = NULL;
+   memset(, 0, sizeof(cur));
while ((ch = getopt(argc, argv, "sM:N:")) != -1)
switch(ch) {
case 's':
@@ -184,7 +184,8 @@ main(int argc, char *argv[])
}
if (!newl)
putchar('\n');
-   free(allocated);
+   if (allocated)
+   free(allocated);
return (0);
 }

Routing table update on link state change

[Gerhard Roth]

Hi,

I noticed a problem with the routing table that is easy to reproduce: put
multiple IPs on the same carp interface:

# ifconfig em0
em0: flags=8843 mtu 1500
lladdr 00:0a:e4:31:9d:6e
index 1 priority 0 llprio 3
groups: egress
media: Ethernet autoselect (100baseTX full-duplex,rxpause,txpause)
status: active
inet 10.1.254.56 netmask 0xfffc broadcast 10.3.255.255
# ifconfig carp0 up carpdev em0 vhid 3
# ifconfig carp0 inet alias 10.1.255.1 netmask 0xfffc
# ifconfig carp0 inet alias 10.1.255.2 netmask 0xfffc
# ifconfig carp0 inet alias 10.1.255.3 netmask 0xfffc
# route -n show -inet
Routing tables

Internet:
DestinationGatewayFlags   Refs  Use   Mtu  Prio Iface
...
10.0/1410.1.254.56UCn143573 - 4 em0  
10.0/1410.1.255.1 UCn00 -19 carp0
10.0/1410.1.255.3 UCn00 -19 carp0
10.0/1410.1.255.2 UCn00 -19 carp0
...

Now pull the cable on em0 and see what happens:

# route -n show -inet
Routing tables

Internet:
DestinationGatewayFlags   Refs  Use   Mtu  Prio Iface
...
10.0/1410.1.255.3 UCn00 -19 carp0
10.0/1410.1.255.2 UCn00 -19 carp0
10.0/1410.1.254.56Cn 143763 - 4 em0  
10.0/1410.1.255.1 Cn 00 -19 carp0
...


Some of the routes to 10.0/14 on carp0 still have the up ('U') flag set,
but not all of them.

The reason is that rt_if_linkstate_change() calls rtable_mpath_reprio()
and that function reorders the routing table while we're iterating over
it in rt_if_track().

Returning EAGAIN in case rtable_mpath_reprio() was called fixes the
problem.


Gerhard


Index: sys/net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.366
diff -u -p -u -p -r1.366 route.c
--- sys/net/route.c 11 Aug 2017 21:24:19 -  1.366
+++ sys/net/route.c 4 Sep 2017 08:25:05 -
@@ -1550,14 +1550,15 @@ rt_if_linkstate_change(struct rtentry *r
 {
struct ifnet *ifp = arg;
struct sockaddr_in6 sa_mask;
+   int ret = 0;
 
if (rt->rt_ifidx != ifp->if_index)
-   return (0);
+   return (ret);
 
/* Local routes are always usable. */
if (rt->rt_flags & RTF_LOCAL) {
rt->rt_flags |= RTF_UP;
-   return (0);
+   return (ret);
}
 
if (LINK_STATE_IS_UP(ifp->if_link_state) && ifp->if_flags & IFF_UP) {
@@ -1567,6 +1568,7 @@ rt_if_linkstate_change(struct rtentry *r
rtable_mpath_reprio(id, rt_key(rt),
rt_plen2mask(rt, _mask),
rt->rt_priority & RTP_MASK, rt);
+   ret = EAGAIN;
}
} else {
if (rt->rt_flags & RTF_UP) {
@@ -1588,11 +1590,12 @@ rt_if_linkstate_change(struct rtentry *r
rtable_mpath_reprio(id, rt_key(rt),
rt_plen2mask(rt, _mask),
rt->rt_priority | RTP_DOWN, rt);
+   ret = EAGAIN;
}
}
if_group_routechange(rt_key(rt), rt_plen2mask(rt, _mask));
 
-   return (0);
+   return (ret);
 }
 
 struct sockaddr *




















Script started on Mon Sep  4 10:18:22 2017
bash: warning: setlocale: LC_TIME: cannot change locale (en_US.UTF-8)
bash: warning: setlocale: LC_NUMERIC: cannot change locale (en_US.UTF-8)
]0;gerhard@null: ~gerhard@null> ifconfig em0
em0: flags=8843 mtu 1500
lladdr 00:0a:e4:31:9d:6e
index 1 priority 0 llprio 3
groups: egress
media: Ethernet autoselect (100baseTX full-duplex,rxpause,txpause)
status: active
inet 10.1.254.56 netmask 0xfffc broadcast 10.3.255.255
]0;gerhard@null: ~gerhard@null> ifconfig carp0 up carpdev em0 vhid 3
ifconfig: SIOCGIFFLAGS: Device not configured
]0;gerhard@null: ~gerhard@null> ifconfig carp0 up carpdev em0 vhid 
3[1@s[1@u[1@d[1@^C
]0;gerhard@null: ~gerhard@null> sudo bash
bash-4.4# ifconfig carp0 up carpdev em0 vhid 3
bash-4.4# ifconfig carp0 inet alias 10.1.255.1 netmask 0xfffc
bash-4.4# ifconfig carp0 inet alias 10.1.255.[1@2
bash-4.4# ifconfig carp0 inet alias 10.1.255.2 netmask 
0xfffc[[1@3
bash-4.4# route sho[[[n [[-n show -inet
Routing tables

Internet:
MPATH.before lines 1-23/71 

Re: refactoring of pf_find_or_create_ruleset()

[Alexandr Nedvedicky]

Hello,

> with this patch i can't trigger panic with or without WITH_PF_LOCK if
> that's matter for some reason.

anyway below is the patch, which Hrvoje was testing and it worked for him.
I'd like to get some OK to proceed to commit.

> thank you sasha for great work on MP pf :)

I'm very last in the long queue of smart folks, who are making MP
dream come true...

thanks and
regards
sasha

8<---8<---8<--8<
diff -r 78ea302507cb src/sbin/pfctl/pfctl.c
--- src/sbin/pfctl/pfctl.c  Thu Aug 31 21:27:47 2017 +0200
+++ src/sbin/pfctl/pfctl.c  Fri Sep 01 22:52:00 2017 +0200
@@ -1572,7 +1572,6 @@ pfctl_rules(int dev, char *filename, int
RB_INIT(_anchors);
memset(_main_anchor, 0, sizeof(pf_main_anchor));
pf_init_ruleset(_main_anchor.ruleset);
-   pf_main_anchor.ruleset.anchor = _main_anchor;
if (trans == NULL) {
bzero(, sizeof(buf));
buf.pfrb_type = PFRB_TRANS;
diff -r 78ea302507cb src/sys/net/pf_ruleset.c
--- src/sys/net/pf_ruleset.cThu Aug 31 21:27:47 2017 +0200
+++ src/sys/net/pf_ruleset.cFri Sep 01 22:52:00 2017 +0200
@@ -133,95 +133,150 @@ pf_find_ruleset(const char *path)
 }
 
 struct pf_ruleset *
+pf_get_leaf_ruleset(char *path, char **path_remainder)
+{
+   struct pf_ruleset   *ruleset;
+   char*leaf, *p;
+   int  i = 0;
+
+   p = path;
+   while (*p == '/')
+   p++;
+
+   ruleset = pf_find_ruleset(p);
+   leaf = p;
+   while (ruleset == NULL) {
+   leaf = strrchr(p, '/');
+   if (leaf != NULL) {
+   *leaf = '\0';
+   i++;
+   ruleset = pf_find_ruleset(p);
+   } else {
+   leaf = path;
+   /*
+* if no path component exists, then main ruleset is
+* our parent.
+*/
+   ruleset = _main_ruleset;
+   }
+   }
+
+   if (path_remainder != NULL)
+   *path_remainder = leaf;
+
+   /* restore slashes in path.  */
+   while (i != 0) {
+   while (*leaf != '\0')
+   leaf++;
+   *leaf = '/';
+   i--;
+   }
+
+   return (ruleset);
+}
+
+struct pf_anchor *
+pf_create_anchor(struct pf_anchor *parent, const char *aname)
+{
+   struct pf_anchor*anchor, *dup;
+
+   if (!*aname || (strlen(aname) >= PF_ANCHOR_NAME_SIZE) ||
+   ((parent != NULL) && (strlen(parent->path) >= PF_ANCHOR_MAXPATH)))
+   return (NULL);
+
+   anchor = rs_malloc(sizeof(*anchor));
+   if (anchor == NULL)
+   return (NULL);
+
+   RB_INIT(>children);
+   strlcpy(anchor->name, aname, sizeof(anchor->name));
+   if (parent != NULL) {
+   /*
+* Make sure path for levels 2, 3, ... is terminated by '/':
+*  1/2/3/...
+*/
+   strlcpy(anchor->path, parent->path, sizeof (anchor->path));
+   strlcat(anchor->path, "/", sizeof(anchor->path));
+   }
+   strlcat(anchor->path, anchor->name, sizeof(anchor->path));
+
+   if ((dup = RB_INSERT(pf_anchor_global, _anchors, anchor)) != NULL) {
+   DPFPRINTF(LOG_NOTICE,
+   "%s: RB_INSERT to global '%s' '%s' collides with '%s' '%s'",
+   __func__, anchor->path, anchor->name, dup->path, dup->name);
+   rs_free(anchor, sizeof(*anchor));
+   return (NULL);
+   }
+
+   if (parent != NULL) {
+   anchor->parent = parent;
+   dup = RB_INSERT(pf_anchor_node, >children, anchor);
+   if (dup != NULL) {
+   DPFPRINTF(LOG_NOTICE,
+   "%s: RB_INSERT to parent '%s' '%s' collides with "
+   "'%s' '%s'", __func__, anchor->path, anchor->name,
+   dup->path, dup->name);
+   RB_REMOVE(pf_anchor_global, _anchors,
+   anchor);
+   rs_free(anchor, sizeof(*anchor));
+   return (NULL);
+   }
+   }
+
+   pf_init_ruleset(>ruleset);
+   anchor->ruleset.anchor = anchor;
+
+   return (anchor);
+}
+
+struct pf_ruleset *
 pf_find_or_create_ruleset(const char *path)
 {
-   char*p, *q, *r;
+   char*p, *aname, *r;
struct pf_ruleset   *ruleset;
-   struct pf_anchor*anchor, *dup, *parent = NULL;
+   struct pf_anchor*anchor;
 
if (path[0] == 0)
return (_main_ruleset);
+
while (*path == '/')
path++;
+
ruleset = pf_find_ruleset(path);
if (ruleset != NULL)
return 

align struct rcs_num between cvs(1) and rcs(1)

[Michael W. Bombardieri]

Hi,

The struct rcs_num in rcs(1) had a pointer for rn_id, but the version
in cvs(1) had an array of size RCSNUM_MAX. This patch tries to make
rcs(1) follow cvs(1) here, so rn_id doesn't need to be freed.
As a result the rcsnum_free() function is replaced by one free().

After applying this patch I tried "ci", "co", "ci" (2nd rev), then
"rlog" on a test file and it seemed to work fine. I didn't try
rcsdiff or rcsmerge though. Does this break anything for you?

- Michael


Index: ci.c
===
RCS file: /cvs/src/usr.bin/rcs/ci.c,v
retrieving revision 1.224
diff -u -p -u -r1.224 ci.c
--- ci.c4 Jul 2016 01:39:12 -   1.224
+++ ci.c4 Sep 2017 08:21:45 -
@@ -314,7 +314,7 @@ checkin_main(int argc, char **argv)
 
rcs_close(pb.file);
if (rev_str != NULL)
-   rcsnum_free(pb.newrev);
+   free(pb.newrev);
pb.newrev = NULL;
}
 
@@ -400,7 +400,7 @@ checkin_getlogmsg(RCSNUM *rev, RCSNUM *r
rcsnum_tostr(rcsnum_inc(tmprev), nrev, sizeof(nrev));
else
rcsnum_tostr(rev2, nrev, sizeof(nrev));
-   rcsnum_free(tmprev);
+   free(tmprev);
 
if (!(flags & QUIET))
(void)fprintf(stderr, "new revision: %s; "
@@ -635,7 +635,7 @@ skipdesc:
if (fetchlog == 1) {
pb->rcs_msg = checkin_getlogmsg(pb->frev, pb->newrev,
pb->flags);
-   rcsnum_free(pb->frev);
+   free(pb->frev);
}
 
/*
Index: co.c
===
RCS file: /cvs/src/usr.bin/rcs/co.c,v
retrieving revision 1.123
diff -u -p -u -r1.123 co.c
--- co.c29 Aug 2017 16:47:33 -  1.123
+++ co.c4 Sep 2017 08:21:45 -
@@ -199,7 +199,7 @@ checkout_main(int argc, char **argv)
if (checkout_rev(file, rev, argv[i], flags,
username, author, state, date) < 0) {
rcs_close(file);
-   rcsnum_free(rev);
+   free(rev);
ret = 1;
continue;
}
@@ -207,7 +207,7 @@ checkout_main(int argc, char **argv)
if (!(flags & QUIET))
(void)fprintf(stderr, "done\n");
 
-   rcsnum_free(rev);
+   free(rev);
 
rcs_write(file);
if (flags & PRESERVETIME)
Index: rcs.c
===
RCS file: /cvs/src/usr.bin/rcs/rcs.c,v
retrieving revision 1.85
diff -u -p -u -r1.85 rcs.c
--- rcs.c   9 May 2016 13:03:55 -   1.85
+++ rcs.c   4 Sep 2017 08:21:46 -
@@ -165,7 +165,7 @@ rcs_close(RCSFILE *rfp)
while (!TAILQ_EMPTY(&(rfp->rf_symbols))) {
rsp = TAILQ_FIRST(&(rfp->rf_symbols));
TAILQ_REMOVE(&(rfp->rf_symbols), rsp, rs_list);
-   rcsnum_free(rsp->rs_num);
+   free(rsp->rs_num);
free(rsp->rs_name);
free(rsp);
}
@@ -173,13 +173,13 @@ rcs_close(RCSFILE *rfp)
while (!TAILQ_EMPTY(&(rfp->rf_locks))) {
rlp = TAILQ_FIRST(&(rfp->rf_locks));
TAILQ_REMOVE(&(rfp->rf_locks), rlp, rl_list);
-   rcsnum_free(rlp->rl_num);
+   free(rlp->rl_num);
free(rlp->rl_name);
free(rlp);
}
 
-   rcsnum_free(rfp->rf_head);
-   rcsnum_free(rfp->rf_branch);
+   free(rfp->rf_head);
+   free(rfp->rf_branch);
 
if (rfp->rf_file != NULL)
fclose(rfp->rf_file);
@@ -579,7 +579,7 @@ rcs_sym_remove(RCSFILE *file, const char
 
TAILQ_REMOVE(&(file->rf_symbols), symp, rs_list);
free(symp->rs_name);
-   rcsnum_free(symp->rs_num);
+   free(symp->rs_num);
free(symp);
 
/* not synced anymore */
@@ -738,7 +738,7 @@ rcs_lock_remove(RCSFILE *file, const cha
}
 
TAILQ_REMOVE(&(file->rf_locks), lkp, rl_list);
-   rcsnum_free(lkp->rl_num);
+   free(lkp->rl_num);
free(lkp->rl_name);
free(lkp);
 
@@ -1228,10 +1228,10 @@ rcs_rev_remove(RCSFILE *rf, RCSNUM *rev)
if (rcs_head_set(rf, prevrdp->rd_num) < 0)
errx(1, "rcs_head_set failed");
} else if (nextrdp != NULL) {
-   rcsnum_free(nextrdp->rd_next);
+   free(nextrdp->rd_next);
nextrdp->rd_next = rcsnum_alloc();
} else {
-   rcsnum_free(rf->rf_head);
+   free(rf->rf_head);
rf->rf_head = NULL;
}
 
@@ -1292,7 +1292,7 @@ rcs_findrev(RCSFILE *rfp, RCSNUM *rev)
frev = rdp->rd_next;
}
 
-   rcsnum_free(brev);
+   free(brev);
return (rdp);
}
 
@@ -1405,8 +1405,8 @@ 

*usrreq() need a socket lock

[Martin Pieuchot]

For some of them it happens to be the NET_LOCK(), but not all of them
need it.  So use the correct macro.

ok?

Index: net/rtsock.c
===
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.249
diff -u -p -r1.249 rtsock.c
--- net/rtsock.c1 Sep 2017 15:05:31 -   1.249
+++ net/rtsock.c4 Sep 2017 08:00:14 -
@@ -177,6 +177,8 @@ route_usrreq(struct socket *so, int req,
int  af;
int  error = 0;
 
+   soassertlocked(so);
+
rop = sotoroutecb(so);
if (rop == NULL) {
m_freem(m);
Index: netinet/ip_divert.c
===
RCS file: /cvs/src/sys/netinet/ip_divert.c,v
retrieving revision 1.49
diff -u -p -r1.49 ip_divert.c
--- netinet/ip_divert.c 27 Jul 2017 12:04:42 -  1.49
+++ netinet/ip_divert.c 4 Sep 2017 08:00:14 -
@@ -248,7 +248,7 @@ divert_usrreq(struct socket *so, int req
struct inpcb *inp = sotoinpcb(so);
int error = 0;
 
-   NET_ASSERT_LOCKED();
+   soassertlocked(so);
 
if (req == PRU_CONTROL) {
return (in_control(so, (u_long)m, (caddr_t)addr,
Index: netinet/raw_ip.c
===
RCS file: /cvs/src/sys/netinet/raw_ip.c,v
retrieving revision 1.102
diff -u -p -r1.102 raw_ip.c
--- netinet/raw_ip.c1 Sep 2017 15:05:31 -   1.102
+++ netinet/raw_ip.c4 Sep 2017 08:00:14 -
@@ -392,7 +392,7 @@ rip_usrreq(struct socket *so, int req, s
struct inpcb *inp = sotoinpcb(so);
int error = 0;
 
-   NET_ASSERT_LOCKED();
+   soassertlocked(so);
 
if (req == PRU_CONTROL)
return (in_control(so, (u_long)m, (caddr_t)nam,
Index: netinet/tcp_usrreq.c
===
RCS file: /cvs/src/sys/netinet/tcp_usrreq.c,v
retrieving revision 1.154
diff -u -p -r1.154 tcp_usrreq.c
--- netinet/tcp_usrreq.c1 Sep 2017 15:05:31 -   1.154
+++ netinet/tcp_usrreq.c4 Sep 2017 08:00:14 -
@@ -131,7 +131,7 @@ tcp_usrreq(struct socket *so, int req, s
int error = 0;
short ostate;
 
-   NET_ASSERT_LOCKED();
+   soassertlocked(so);
 
if (req == PRU_CONTROL) {
 #ifdef INET6
Index: netinet/udp_usrreq.c
===
RCS file: /cvs/src/sys/netinet/udp_usrreq.c,v
retrieving revision 1.239
diff -u -p -r1.239 udp_usrreq.c
--- netinet/udp_usrreq.c11 Aug 2017 19:53:02 -  1.239
+++ netinet/udp_usrreq.c4 Sep 2017 08:00:14 -
@@ -1060,7 +1060,7 @@ udp_usrreq(struct socket *so, int req, s
struct inpcb *inp;
int error = 0;
 
-   NET_ASSERT_LOCKED();
+   soassertlocked(so);
 
if (req == PRU_CONTROL) {
 #ifdef INET6

[PATCH v2 0/2] VMD: Prevent from crashing

[Carlos Cardenas]

This patch series contains:

1) Place plenty of log_debug statements in key places to assist
   with troubleshooting and debugging
2) Add new error code when attempting to stop a non-running vm
   (used by vmctl)
   Prevent vmd from crashing with updated logic when stopping a vm

Difference between v1 and v2:
v2 leaves log.c alone since it's replicated/shared with many other daemons.
All debug statements are now log_debug.

Comments? Ok?

[PATCH v2 2/2] VMD: Prevent vmd crashing when stopping a stopped vm

[Carlos Cardenas]

* Fix logic handling stopping a VM.  Prevents VMD from crashing.
* Add additional error code to notify the user that a vm cannot be
  stopped when not running.
* Add additional log_debug statements.

diff --git usr.sbin/vmctl/vmctl.c usr.sbin/vmctl/vmctl.c
index 64d82ca847d..5fb7fbfd74c 100644
--- usr.sbin/vmctl/vmctl.c
+++ usr.sbin/vmctl/vmctl.c
@@ -439,12 +439,19 @@ terminate_vm_complete(struct imsg *imsg, int *ret)
vmr = (struct vmop_result *)imsg->data;
res = vmr->vmr_result;
if (res) {
-   errno = res;
-   if (res == ENOENT)
-   warnx("vm not found");
-   else
-   warn("terminate vm command failed");
-   *ret = EIO;
+   switch (res) {
+   case VMD_VM_STOP_INVALID:
+   warnx("cannot stop vm that is not running");
+   *ret = EINVAL;
+   break;
+   case ENOENT:
+   warnx("vm not found");
+   *ret = EIO;
+   break;
+   default:
+   warn("terminate vm command failed");
+   *ret = EIO;
+   }
} else {
warnx("sent request to terminate vm %d", vmr->vmr_id);
*ret = 0;
@@ -453,6 +460,7 @@ terminate_vm_complete(struct imsg *imsg, int *ret)
warnx("unexpected response received from vmd");
*ret = EINVAL;
}
+   errno = *ret;
 
return (1);
 }
diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
index 22da6d58a7b..1240339db52 100644
--- usr.sbin/vmd/vmd.h
+++ usr.sbin/vmd/vmd.h
@@ -54,6 +54,7 @@
 #define VMD_BIOS_MISSING   1001
 #define VMD_DISK_MISSING   1002
 #define VMD_DISK_INVALID   1003
+#define VMD_VM_STOP_INVALID1004
 
 /* 100.64.0.0/10 from rfc6598 (IPv4 Prefix for Shared Address Space) */
 #define VMD_DHCP_PREFIX"100.64.0.0/10"
diff --git usr.sbin/vmd/vmm.c usr.sbin/vmd/vmm.c
index d3233147cff..66083a5f959 100644
--- usr.sbin/vmd/vmm.c
+++ usr.sbin/vmd/vmm.c
@@ -169,10 +169,9 @@ vmm_dispatch_parent(int fd, struct privsep_proc *p, struct 
imsg *imsg)
else
res = 0;
} else {
-   /* Terminate VMs that are unknown or shutting down */
-   vtp.vtp_vm_id = vm_vmid2id(vm->vm_vmid, vm);
-   res = terminate_vm();
-   vm_remove(vm);
+log_debug("%s: cannot stop vm that is not running",
+__func__);
+res = VMD_VM_STOP_INVALID;
}
cmd = IMSG_VMDOP_TERMINATE_VM_RESPONSE;
break;
@@ -346,6 +345,8 @@ vmm_sighdlr(int sig, short event, void *arg)
 
vmid = vm->vm_params.vmc_params.vcp_id;
vtp.vtp_vm_id = vmid;
+log_debug("%s: attempting to terminate vm %d",
+__func__, vm->vm_vmid);
if (terminate_vm() == 0) {
memset(, 0, sizeof(vmr));
vmr.vmr_result = ret;
@@ -505,7 +506,7 @@ vmm_dispatch_vm(int fd, short event, void *arg)
  * supplied vm_terminate_params structure (vtp->vtp_vm_id)
  *
  * Parameters
- *  vtp: vm_create_params struct containing the ID of the VM to terminate
+ *  vtp: vm_terminate_params struct containing the ID of the VM to terminate
  *
  * Return values:
  *  0: success
@@ -515,6 +516,7 @@ vmm_dispatch_vm(int fd, short event, void *arg)
 int
 terminate_vm(struct vm_terminate_params *vtp)
 {
+log_debug("%s: terminating vmid %d", __func__, vtp->vtp_vm_id);
if (ioctl(env->vmd_fd, VMM_IOC_TERM, vtp) < 0)
return (errno);
 
-- 
2.14.1

[PATCH v2 1/2] VMD: Place log_debug statements in key places

[Carlos Cardenas]

Add log_debug statements in key places to assist with
troubleshooting.

diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
index 9ea87eb86e8..623a9b9d4f3 100644
--- usr.sbin/vmd/config.c
+++ usr.sbin/vmd/config.c
@@ -81,14 +81,18 @@ config_purge(struct vmd *env, unsigned int reset)
struct vmd_switch   *vsw;
unsigned int what;
 
+log_debug("%s: purging vms and switches from running config",
+__func__);
/* Reset global configuration (prefix was verified before) */
(void)host(VMD_DHCP_PREFIX, >vmd_cfg.cfg_localprefix);
 
/* Reset other configuration */
what = ps->ps_what[privsep_process] & reset;
if (what & CONFIG_VMS && env->vmd_vms != NULL) {
-   while ((vm = TAILQ_FIRST(env->vmd_vms)) != NULL)
+   while ((vm = TAILQ_FIRST(env->vmd_vms)) != NULL) {
+log_debug("%s: calling vm_remove", __func__);
vm_remove(vm);
+}
env->vmd_nvm = 0;
}
if (what & CONFIG_SWITCHES && env->vmd_switches != NULL) {
@@ -104,6 +108,7 @@ config_setconfig(struct vmd *env)
struct privsep  *ps = >vmd_ps;
unsigned int id;
 
+log_debug("%s: setting config", __func__);
for (id = 0; id < PROC_MAX; id++) {
if (id == privsep_process)
continue;
@@ -117,6 +122,7 @@ config_setconfig(struct vmd *env)
 int
 config_getconfig(struct vmd *env, struct imsg *imsg)
 {
+log_debug("%s: retrieving config", __func__);
IMSG_SIZE_CHECK(imsg, >vmd_cfg);
memcpy(>vmd_cfg, imsg->data, sizeof(env->vmd_cfg));
 
@@ -129,6 +135,7 @@ config_setreset(struct vmd *env, unsigned int reset)
struct privsep  *ps = >vmd_ps;
unsigned int id;
 
+log_debug("%s: resetting state", __func__);
for (id = 0; id < PROC_MAX; id++) {
if ((reset & ps->ps_what[id]) == 0 ||
id == privsep_process)
@@ -147,6 +154,7 @@ config_getreset(struct vmd *env, struct imsg *imsg)
IMSG_SIZE_CHECK(imsg, );
memcpy(, imsg->data, sizeof(mode));
 
+log_debug("%s: resetting state", __func__);
config_purge(env, mode);
 
return (0);
@@ -374,6 +382,7 @@ config_setvm(struct privsep *ps, struct vmd_vm *vm, 
uint32_t peerid, uid_t uid)
free(tapfds);
}
 
+log_debug("%s: calling vm_remove", __func__);
vm_remove(vm);
errno = saved_errno;
if (errno == 0)
@@ -406,6 +415,7 @@ config_getvm(struct privsep *ps, struct imsg *imsg)
imsg->fd = -1;
}
 
+log_debug("%s: calling vm_remove", __func__);
vm_remove(vm);
if (errno == 0)
errno = EINVAL;
diff --git usr.sbin/vmd/control.c usr.sbin/vmd/control.c
index 1af8a0d5e14..9263d0a3ae1 100644
--- usr.sbin/vmd/control.c
+++ usr.sbin/vmd/control.c
@@ -276,6 +276,7 @@ control_close(int fd, struct control_sock *cs)
 {
struct ctl_conn *c;
 
+log_debug("%s", __func__);
if ((c = control_connbyfd(fd)) == NULL) {
log_warn("%s: fd %d: not found", __func__, fd);
return;
@@ -401,9 +402,14 @@ control_dispatch_imsg(int fd, short event, void *arg)
goto fail;
memcpy(, imsg.data, sizeof(vid));
vid.vid_uid = c->peercred.uid;
+log_debug("%s vid: %d, name: %s, uid: %d",
+__func__, vid.vid_id, vid.vid_name,
+vid.vid_uid);
 
if (proc_compose_imsg(ps, PROC_PARENT, -1,
imsg.hdr.type, fd, -1, , sizeof(vid)) == -1) {
+log_debug("%s: proc_compose_imsg failed...",
+__func__);
control_close(fd, cs);
return;
}
diff --git usr.sbin/vmd/proc.c usr.sbin/vmd/proc.c
index 183db93a678..2bf0a98c8ee 100644
--- usr.sbin/vmd/proc.c
+++ usr.sbin/vmd/proc.c
@@ -756,6 +756,8 @@ proc_compose_imsg(struct privsep *ps, enum privsep_procid 
id, int n,
 
proc_range(ps, id, , );
for (; n < m; n++) {
+log_debug("%s: about to compose_event to PROC %d", __func__,
+id);
if (imsg_compose_event(>ps_ievs[id][n],
type, peerid, ps->ps_instance + 1, fd, data, datalen) == -1)
return (-1);
diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
index c7b9247d326..f34917e1a3b 100644
--- usr.sbin/vmd/vmd.c
+++ usr.sbin/vmd/vmd.c
@@ -372,6 +372,8 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struct 
imsg *imsg)
case IMSG_VMDOP_TERMINATE_VM_RESPONSE:
IMSG_SIZE_CHECK(imsg, );
memcpy(, imsg->data, sizeof(vmr));
+

sendbug categories

[Michael W. Bombardieri]

Hello,

Are sparc and vax still considered valid categories for
sendbug or should people now use only sparc64?

- Michael


Index: sendbug.c
===
RCS file: /cvs/src/usr.bin/sendbug/sendbug.c,v
retrieving revision 1.78
diff -u -p -u -r1.78 sendbug.c
--- sendbug.c   21 Aug 2017 21:41:13 -  1.78
+++ sendbug.c   4 Sep 2017 06:44:39 -
@@ -44,7 +44,7 @@ void  template(FILE *);
 void   usbdevs(FILE *);
 
 const char *categories = "system user library documentation kernel "
-"alpha amd64 arm hppa i386 m88k mips64 powerpc sh sparc sparc64 vax";
+"alpha amd64 arm hppa i386 m88k mips64 powerpc sh sparc64";
 const char *comment[] = {
"",
"",

Re: lex: action_m4_define

[Michael W. Bombardieri]

I forgot to mention that action_m4_define() was previously removed from
upstream flex here:
https://github.com/westes/flex/commit/925549475edb3c8921227fdd96aa0d90a2c0745a

On Sun, Aug 27, 2017 at 06:41:31PM +0800, Michael W. Bombardieri wrote:
> Hello,
> 
> In lex(1) the function action_m4_define() would raise a fatal
> error if called. I couldn't see anything calling it though,
> and the program builds without it.
> 
> - Michael
> 
> 
> Index: misc.c
> ===
> RCS file: /cvs/src/usr.bin/lex/misc.c,v
> retrieving revision 1.19
> diff -u -p -u -r1.19 misc.c
> --- misc.c19 Nov 2015 23:34:56 -  1.19
> +++ misc.c27 Aug 2017 10:27:34 -
> @@ -116,28 +116,6 @@ action_define(defname, value)
>   buf_append(_buf, , 1);
>  }
>  
> -
> -/** Append "m4_define([[defname]],[[value]])m4_dnl\n" to the running buffer.
> - *  @param defname The macro name.
> - *  @param value The macro value, can be NULL, which is the same as the 
> empty string.
> - */
> -void 
> -action_m4_define(const char *defname, const char *value)
> -{
> - char buf[MAXLINE];
> -
> - flexfatal("DO NOT USE THIS FUNCTION!");
> -
> - if ((int) strlen(defname) > MAXLINE / 2) {
> - format_pinpoint_message(_
> - ("name \"%s\" ridiculously long"),
> - defname);
> - return;
> - }
> - snprintf(buf, sizeof(buf), "m4_define([[%s]],[[%s]])m4_dnl\n", defname, 
> value ? value : "");
> - add_action(buf);
> -}
> -
>  /* Append "new_text" to the running buffer. */
>  void 
>  add_action(new_text)