piixpm(4) support for AMD FCH watchdog

[Nathanael Rensen]

The diff below adds support for the watchdog as found in the embedded
AMD FCH (fusion controller hub) as found on APU2.

Index: sys/dev/pci/piixpm.c
===
RCS file: /cvs/src/sys/dev/pci/piixpm.c,v
retrieving revision 1.42
diff -u -p -r1.42 piixpm.c
--- sys/dev/pci/piixpm.c21 Jan 2020 06:37:24 -  1.42
+++ sys/dev/pci/piixpm.c8 Feb 2020 04:44:42 -
@@ -54,8 +54,10 @@ struct piixpm_softc {
struct device   sc_dev;
 
bus_space_tag_t sc_iot;
+   bus_space_tag_t sc_memt;
bus_space_handle_t  sc_ioh;
bus_space_handle_t  sc_sb800_ioh;
+   bus_space_handle_t  sc_wdt_mh;
void *  sc_ih;
int sc_poll;
int sc_is_sb800;
@@ -83,6 +85,8 @@ int   piixpm_i2c_exec(void *, i2c_op_t, i2
 
 intpiixpm_intr(void *);
 
+intpiixpm_wdt_cb(void *arg, int period);
+
 struct cfattach piixpm_ca = {
sizeof(struct piixpm_softc),
piixpm_match,
@@ -127,7 +131,7 @@ piixpm_attach(struct device *parent, str
struct piixpm_softc *sc = (struct piixpm_softc *)self;
struct pci_attach_args *pa = aux;
bus_space_handle_t ioh;
-   u_int16_t val, smb0en;
+   u_int16_t val, smb0en, wdten = 0;
bus_addr_t base;
pcireg_t conf;
pci_intr_handle_t ih;
@@ -136,6 +140,7 @@ piixpm_attach(struct device *parent, str
int numbusses, i;
 
sc->sc_iot = pa->pa_iot;
+   sc->sc_memt = pa->pa_memt;
numbusses = 1;
 
if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
@@ -160,7 +165,7 @@ piixpm_attach(struct device *parent, str
 
/*
 * AMD Bolton matches PCI_PRODUCT_AMD_HUDSON2_SMB but
-* uses old register layout. Therefor check PCI_REVISION.
+* uses old register layout. Therefore check PCI_REVISION.
 */
if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD &&
((PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB &&
@@ -170,6 +175,7 @@ piixpm_attach(struct device *parent, str
AMDFCH41_PM_DECODE_EN);
val = bus_space_read_1(sc->sc_iot, ioh, 1);
smb0en = val & AMDFCH41_SMBUS_EN;
+   wdten = val & AMDFCH41_WDT_EN;
 
bus_space_write_1(sc->sc_iot, ioh, 0,
AMDFCH41_PM_DECODE_EN + 1);
@@ -282,7 +288,59 @@ piixpm_attach(struct device *parent, str
config_found(self, , iicbus_print);
}
 
+   /* Register watchdog */
+   if (wdten && bus_space_map(sc->sc_memt, AMDFCH41_WDTREG_BASE,
+   AMDFCH41_WDTREG_SIZE, 0, >sc_wdt_mh) == 0) {
+   val = bus_space_read_1(sc->sc_memt, sc->sc_wdt_mh,
+   AMDFCH41_WDTREG_CTL);
+   if (val & AMDFCH41_WDTREG_CTL_FIRED) {
+   printf("%s watchdog caused previous restart\n",
+   sc->sc_dev.dv_xname);
+   bus_space_write_1(sc->sc_memt, sc->sc_wdt_mh,
+   AMDFCH41_WDTREG_CTL,
+   val | AMDFCH41_WDTREG_CTL_FIRED);
+   }
+
+   if (val & AMDFCH41_WDTREG_CTL_DISABLED)
+   printf("%s watchdog disabled\n", sc->sc_dev.dv_xname);
+   else {
+   printf("%s watchdog found\n", sc->sc_dev.dv_xname);
+
+   /* Set 1 second counter period */
+   bus_space_write_1(sc->sc_iot, sc->sc_sb800_ioh, 0,
+   AMDFCH41_PM_DECODE_EN + 3);
+   val = bus_space_read_1(sc->sc_iot, sc->sc_sb800_ioh, 1);
+   val = (val & ~AMDFCH41_WDT_MASK) | AMDFCH41_WDT_1S;
+   bus_space_write_1(sc->sc_iot, sc->sc_sb800_ioh, 1, val);
+
+   wdog_register(piixpm_wdt_cb, sc);
+   }
+   }
+
return;
+}
+
+int
+piixpm_wdt_cb(void *arg, int period)
+{
+   struct piixpm_softc *sc = (struct piixpm_softc *)arg;
+   u_int16_t val;
+
+   val = bus_space_read_1(sc->sc_memt, sc->sc_wdt_mh, AMDFCH41_WDTREG_CTL);
+
+   if (period > 0x)
+   period = 0x;
+   if (period > 0) {
+   bus_space_write_2(sc->sc_memt, sc->sc_wdt_mh,
+   AMDFCH41_WDTREG_COUNT, period);
+   val |= AMDFCH41_WDTREG_CTL_RUN | AMDFCH41_WDTREG_CTL_TRIGGER;
+   }
+   else
+   val &= ~AMDFCH41_WDTREG_CTL_RUN;
+
+   bus_space_write_1(sc->sc_memt, sc->sc_wdt_mh, AMDFCH41_WDTREG_CTL, val);
+
+   return period;
 }
 
 int
Index: sys/dev/pci/piixreg.h
===
RCS file: /cvs/src/sys/dev/pci/piixreg.h,v
retrieving revision 1.6
diff -u -p -r1.6 piixreg.h
--- 

em(4) reset I210 wake status

[Nathanael Rensen]

When I boot an APU2 using wake-on-lan and then attempt to power off with
shutdown -hp it wakes itself up again. To prevent this it is necessary to
clear the I210 PME_STATUS flag. This is described in section 5.6.2 of the
I210 datasheet:

  The PE_WAKE_N remains asserted until the operating system either writes
  a 1b to the PMCSR.PME_Status bit or writes a 0b to the PMCSR.PME_En bit. 

In addition the WUS (wake up status) register is not automatically cleared,
as described in section 8.2.13 of the i210 datasheet:

  This register is not cleared when PE_RST_N is asserted. It is only
  cleared when LAN_PWR_GOOD is deasserted or when cleared by the software
  device driver.

Nathanael

Index: sys/dev/pci/if_em.c
===
RCS file: /cvs/src/sys/dev/pci/if_em.c,v
retrieving revision 1.344
diff -u -p -r1.344 if_em.c
--- sys/dev/pci/if_em.c 4 Feb 2020 10:59:23 -   1.344
+++ sys/dev/pci/if_em.c 5 Feb 2020 14:01:34 -
@@ -561,6 +561,9 @@ em_attach(struct device *parent, struct 
 #endif
printf(", address %s\n", ether_sprintf(sc->sc_ac.ac_enaddr));
 
+   if (sc->hw.wus)
+   printf("%s wakeup: %d\n", sc->sc_dev.dv_xname, sc->hw.wus);
+
/* Indicate SOL/IDER usage */
if (em_check_phy_reset_block(>hw))
printf("%s: PHY reset is blocked due to SOL/IDER session.\n",
Index: sys/dev/pci/if_em_hw.c
===
RCS file: /cvs/src/sys/dev/pci/if_em_hw.c,v
retrieving revision 1.106
diff -u -p -r1.106 if_em_hw.c
--- sys/dev/pci/if_em_hw.c  4 Feb 2020 10:59:23 -   1.106
+++ sys/dev/pci/if_em_hw.c  5 Feb 2020 14:01:34 -
@@ -1678,6 +1678,12 @@ em_init_hw(struct em_hw *hw)
ctrl = (ctrl & ~E1000_TXDCTL_WTHRESH) | 
E1000_TXDCTL_FULL_TX_DESC_WB;
E1000_WRITE_REG(hw, TXDCTL(1), ctrl);
+   hw->wus = E1000_READ_REG(hw, WUS);
+   if (hw->wus)
+   E1000_WRITE_REG(hw, WUS, reg_data);
+   reg_data = E1000_READ_REG(hw, WUC);
+   if (reg_data & E1000_WUC_PME_STATUS)
+   E1000_WRITE_REG(hw, WUC, reg_data);
break;
}
 
Index: sys/dev/pci/if_em_hw.h
===
RCS file: /cvs/src/sys/dev/pci/if_em_hw.h,v
retrieving revision 1.80
diff -u -p -r1.80 if_em_hw.h
--- sys/dev/pci/if_em_hw.h  4 Feb 2020 10:59:23 -   1.80
+++ sys/dev/pci/if_em_hw.h  5 Feb 2020 14:01:34 -
@@ -1482,6 +1482,7 @@ struct em_hw {
 uint16_t swfw;
 boolean_t eee_enable;
 int sw_flag;
+int wus;
 };
 
 #define E1000_EEPROM_SWDPIN0   0x0001   /* SWDPIN 0 EEPROM Value */

gpio(4) support for APU2

[Nathanael Rensen]

The diff below adds gpio(4) support to wbsio(4) for Nuvoton NCT5104D
(pcengines APU2). It is based on Matt Dainty's diff posted to this list
in November 2018:

  https://marc.info/?l=openbsd-tech=154134941027009=2

A key difference from Matt Dainty's original diff is the use of
config_search() rather than config_found() to avoid problems with
the lm78 driver mentioned in his post.

Nathanael

Index: sys/dev/isa/wbsioreg.h
===
RCS file: /cvs/src/sys/dev/isa/wbsioreg.h,v
retrieving revision 1.5
diff -u -p -r1.5 wbsioreg.h
--- sys/dev/isa/wbsioreg.h  17 Dec 2019 01:34:59 -  1.5
+++ sys/dev/isa/wbsioreg.h  29 Jan 2020 14:30:28 -
@@ -30,8 +30,15 @@
 
 /* Configuration Space Registers */
 #define WBSIO_LDN  0x07/* Logical Device Number */
+#define WBSIO_MF   0x1c/* Multi Function Selection */
+#define WBSIO_MF_UARTD (1 << 2)
+#define WBSIO_MF_UARTC (1 << 3)
+#define WBSIO_MF_GP67  (1 << 4)
 #define WBSIO_ID   0x20/* Device ID */
 #define WBSIO_REV  0x21/* Device Revision */
+#define WBSIO_CR27 0x27/* Global Option */
+#define WBSIO_SF   0x2f/* Strapping Function */
+#define WBSIO_LDN_EN   0x30/* Logical Device Enable */
 
 #define WBSIO_ID_W83627HF  0x52
 #define WBSIO_ID_W83627THF 0x82
@@ -52,8 +59,38 @@
 #define WBSIO_ID_NCT6795D  0xd3
 
 /* Logical Device Number (LDN) Assignments */
+#define WBSIO_LDN_GPIO10x07
+#define WBSIO_LDN_WDT  0x08
+#define WBSIO_LDN_GPIO20x09/* Not used */
 #define WBSIO_LDN_HM   0x0b
+#define WBSIO_LDN_GPIO30x0f
 
 /* Hardware Monitor Control Registers (LDN B) */
 #define WBSIO_HM_ADDR_MSB  0x60/* Address [15:8] */
 #define WBSIO_HM_ADDR_LSB  0x61/* Address [7:0] */
+
+/* GPIO Control Registers (LDN 7) */
+/* GPIOn registers are offset by n*4 bytes */
+#define WBSIO_GPIO_IO  0xe0/* GPIO Direction */
+#define WBSIO_GPIO_DATA0xe1/* GPIO Data */
+#define WBSIO_GPIO_INVERT  0xe2/* GPIO Invert */
+#define WBSIO_GPIO_STATUS  0xe3/* GPIO Status */
+#define WBSIO_GPIO0_EN (1 << 0)
+#define WBSIO_GPIO1_EN (1 << 1)
+#define WBSIO_GPIO6_EN (1 << 6)
+#define WBSIO_GPIO0_NPINS  8
+#define WBSIO_GPIO1_NPINS  8
+#define WBSIO_GPIO6_NPINS  1
+#define WBSIO_GPIO_NPINS   (WBSIO_GPIO0_NPINS + WBSIO_GPIO1_NPINS + \
+WBSIO_GPIO6_NPINS)
+
+/* WDT Control Registers (LDN 8) */
+#define WBSIO_WDT_ENABLE   0x30
+#define WBSIO_WDT_CONTROL  0xf0
+#define WBSIO_WDT_COUNTER  0xf1
+#define WBSIO_WDT_MINUTE   (1 << 3)
+#define WBSIO_WDT_FAST (1 << 4)
+
+/* GPIO Control Registers (LDN F) */
+/* GPIOn register is offset by n bytes */
+#define WBSIO_GPIO_PP_OD   0xe0/* GPIO Push-Pull/Open Drain */
Index: sys/dev/isa/wbsio.c
===
RCS file: /cvs/src/sys/dev/isa/wbsio.c,v
retrieving revision 1.11
diff -u -p -r1.11 wbsio.c
--- sys/dev/isa/wbsio.c 17 Dec 2019 01:34:59 -  1.11
+++ sys/dev/isa/wbsio.c 29 Jan 2020 14:30:28 -
@@ -23,11 +23,13 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
 #include 
 #include 
+#include 
 
 #ifdef WBSIO_DEBUG
 #define DPRINTF(x) printf x
@@ -40,12 +42,22 @@ struct wbsio_softc {
 
bus_space_tag_t sc_iot;
bus_space_handle_t  sc_ioh;
+
+   struct gpio_chipset_tag sc_gpio_gc;
+   gpio_pin_t  sc_gpio_pins[WBSIO_GPIO_NPINS];
 };
 
 intwbsio_probe(struct device *, void *, void *);
 void   wbsio_attach(struct device *, struct device *, void *);
+intwbsio_search_cb(struct device *, void *, void *);
 intwbsio_print(void *, const char *);
 
+intwbsio_gpio_pin_to_group(struct wbsio_softc *, int);
+intwbsio_gpio_pin_read(struct wbsio_softc *, int);
+intwbsio_gpio_read(void *, int);
+void   wbsio_gpio_write(void *, int, int);
+void   wbsio_gpio_ctl(void *, int, int);
+
 struct cfattach wbsio_ca = {
sizeof(struct wbsio_softc),
wbsio_probe,
@@ -56,6 +68,11 @@ struct cfdriver wbsio_cd = {
NULL, "wbsio", DV_DULL
 };
 
+struct wbsio_aux {
+   struct isa_attach_args ia;
+   struct gpiobus_attach_args gba;
+};
+
 static __inline void
 wbsio_conf_enable(bus_space_tag_t iot, bus_space_handle_t ioh)
 {
@@ -132,9 +149,10 @@ wbsio_attach(struct device *parent, stru
 {
struct wbsio_softc *sc = (void *)self;
struct isa_attach_args *ia = aux;
-   struct isa_attach_args nia;
-   u_int8_t devid, reg, reg0, reg1;
+   struct wbsio_aux wbsio_aux;
+   u_int8_t devid, reg, reg0, reg1, mf;
u_int16_t iobase;
+   int i, npins = 0, group, gpin;
 
/* Map ISA I/O space */
sc->sc_iot = ia->ia_iot;
@@ -219,17 +237,123 @@ 

Re: ld.so speedup (part 1)

[Nathanael Rensen]

On Mon 6, May 2019 at 00:16:57, Nathanael Rensen wrote:

> On Sun, 5 May 2019 at 06:41, Martin Pieuchot  wrote:

[snip]

> > Do I understand correctly that you're not recursing if not object is
> > inserted?
>
> Yes.
>
> You have probably also noticed that using a single recurse flag is not
> optimally efficient. It is only necessary to recurse child nodes where
>   n->data->grpsym_gen != _dl_grpsym_gen
> With my approach if any child node satisfies that then all child nodes
> are recursed.

[snip]

> What do you think about the approach below?
>
> static void
> _dl_insert_grpsym(elf_object_t *object)
> {
>   struct dep_node *n;
>
>   n = _dl_malloc(sizeof *n);
>   if (n == NULL)
>   _dl_oom();
>   n->data = object;
>   TAILQ_INSERT_TAIL(&_dl_loading_object->grpsym_list, n, next_sib);
> }
>
> void
> _dl_link_grpsym(elf_object_t *object)
> {
>   struct dep_node *n;
>
>   TAILQ_FOREACH(n, &_dl_loading_object->grpsym_list, next_sib)
>   if (n->data == object && object->grpsym_gen == _dl_grpsym_gen)
>   return; /* found, dont bother adding */
>
>   _dl_insert_grpsym(object);
>   object->grpsym_gen = _dl_grpsym_gen;
> }
>
> void
> _dl_cache_grpsym_list(elf_object_t *object)
> {
>   struct dep_node *n;
>
>   /*
>* grpsym_list is an ordered list of all child libs of the
>* _dl_loading_object with no dups. The order is equivalent
>* to a breadth-first traversal of the child list without dups.
>*/
>
>   TAILQ_FOREACH(n, >child_list, next_sib)
>   if (n->data->grpsym_gen != _dl_grpsym_gen)
>   _dl_insert_grpsym(n->data);
>
>   TAILQ_FOREACH(n, >child_list, next_sib)
>   if (n->data->grpsym_gen != _dl_grpsym_gen) {
>   object->grpsym_gen = _dl_grpsym_gen;
>   _dl_cache_grpsym_list(n->data);
>   }
> }

For the record, the approach above is wrong. It is essential that
object->grpsym_gen is updated by _dl_insert_grpsym() otherwise a tree
such as that below would result in z being added to grpsym_list twice:

  x -> y,z 
  y -> z

While processing x both children y and z are inserted without updating
grpsym_gen. Then while recursing into y, z is added a second time.

This can be fixed using a status flag as follows:

static void
_dl_insert_grpsym(elf_object_t *object)
{
struct dep_node *n;

object->grpsym_gen = _dl_grpsym_gen;
n = _dl_malloc(sizeof *n);
if (n == NULL)
_dl_oom();
n->data = object;
TAILQ_INSERT_TAIL(&_dl_loading_object->grpsym_list, n, next_sib);
}

void
_dl_link_grpsym(elf_object_t *object)
{
struct dep_node *n;

TAILQ_FOREACH(n, &_dl_loading_object->grpsym_list, next_sib)
if (n->data == object && object->grpsym_gen == _dl_grpsym_gen)
return; /* found, dont bother adding */

_dl_insert_grpsym(object);
}

void
_dl_cache_grpsym_list(elf_object_t *object)
{
struct dep_node *n;

/*
 * grpsym_list is an ordered list of all child libs of the
 * _dl_loading_object with no dups. The order is equivalent
 * to a breadth-first traversal of the child list without dups.
 */

TAILQ_FOREACH(n, >child_list, next_sib)
if (n->data->grpsym_gen != _dl_grpsym_gen) {
n->data->status |= STAT_GS_RECURSE;
_dl_insert_grpsym(n->data);
}

TAILQ_FOREACH(n, >child_list, next_sib)
if (n->data->status & STAT_GS_RECURSE) {
n->data->status &= ~STAT_GS_RECURSE;
_dl_cache_grpsym_list(n->data);
}
}

This approach maintains the integrity of the grpsym_gen mechanism ensuring
a node will not be added to the grpsym_list multiple times. The
STAT_GS_RECURSE flag is used to record the requirement to recurse for
those nodes added on this iteration. This extends the per-iteration
recurse flag of the original diff to a per-child flag without damaging
the integrity of the grpsym_gen mechanism.

For the example tree above, while processing x both y and z are inserted
and STAT_GS_RECURSE is set on both. Then when recursing y, z is not
inserted again because grpsym_gen has already been updated.

The flaw described above is not present in the original part 1 diff, only
in the variation I proposed in response to mpi@'s questions.

Nathanael

Re: ld.so speedup (part 2)

[Nathanael Rensen]

On Sun, 5 May 2019 at 06:26, Martin Pieuchot  wrote:
>
> On 27/04/19(Sat) 21:55, Nathanael Rensen wrote:
> > The diff below speeds up ld.so library intialisation where the dependency
> > tree is broad and deep, such as samba's smbd which links over 100 libraries.
> >
> > See for example https://marc.info/?l=openbsd-misc=155007285712913=2
> >
> > See https://marc.info/?l=openbsd-tech=155637285221396=2 for part 1
> > that speeds up library loading.
> >
> > The timings below are for /usr/local/sbin/smbd --version:
> >
> > Timing without either diff  : 6m45.67s real  6m45.65s user  0m00.02s system
> > Timing with part 1 diff only: 4m42.88s real  4m42.85s user  0m00.02s system
> > Timing with part 2 diff only: 2m02.61s real  2m02.60s user  0m00.01s system
> > Timing with both diffs  : 0m00.03s real  0m00.03s user  0m00.00s system
> >
> > Note that these timings are for a build of a recent samba master tree
> > (linked with kerberos) which is probably slower than the OpenBSD port.
>
> Nice numbers.  Could you explain in words what your diff is doing?  Why
> does splitting the flag help?  Is it because some ctors/initarray are
> being initialized multiple times currently?

No, the STAT_INIT_DONE flag prevents that.

> Or is it just to prevent some traversal?

Yes.

> In that case does that mean the `STAT_VISISTED' flag is removed too
> early?

Yes, STAT_VISITED is removed too early. The visited flag is set on a node
while traversing the child nodes of that node and then removed. It serves
to protect against circular dependencies, but does not prevent repeatedly
traversing through a node that appears on separate branches.

The entire tree must be traversed twice - first to initialise the
DF_1_INITFIRST libraries, and secondly to initialise the others. This
is presumably why this diff contributes roughly twice as much speedup
as the part 1 diff. To be effective in avoiding repeated traversals
the visited flag must persist throughout an entire tree traversal but
it must either be cleared between first and second traversals or a
different flag used for the second traversal.

My approach was to add a second visited flag and make them both
persistent. My rationale for why I believe the flags may be
persisted is as follows. dlopen() calls _dl_call_init() with the
newly loaded object and neither the newly loaded object nor any
newly loaded children of that object will have either visited flag
set. Already loaded children will have those flags set, but they
won't have gained any new children as a result of the dlopen().
If this reasoning is wrong then the diff is wrong and could lead to
uninitialised libraries (and an ld.so regress test should probably
be created to catch that situation).

It occurs to me as I'm writing this that perhaps it's possible to
avoid a tree traversal entirely by walking the linearised grpsym_list
in reverse and relying only on the STAT_INIT_DONE flag.
/*
 * grpsym_list is an ordered list of all child libs of the
 * _dl_loading_object with no dups. The order is equivalent
 * to a breadth-first traversal of the child list without dups.
 */
I don't think it is a true breadth-first traversal, not in the way I
understand breadth-first, but it does ensure that parent nodes appear
before child nodes. So in reverse, child nodes will appear before
parent nodes. While this is not the same as a depth-first traversal
it may be OK. There may be some specific requirements of DF_1_INITFIRST
that need to be taken into account.

Nathanael

>
> > Index: libexec/ld.so/loader.c
> > ===
> > RCS file: /cvs/src/libexec/ld.so/loader.c,v
> > retrieving revision 1.177
> > diff -u -p -p -u -r1.177 loader.c
> > --- libexec/ld.so/loader.c3 Dec 2018 05:29:56 -   1.177
> > +++ libexec/ld.so/loader.c27 Apr 2019 13:24:02 -
> > @@ -749,15 +749,15 @@ _dl_call_init_recurse(elf_object_t *obje
> >  {
> >   struct dep_node *n;
> > 
> > - object->status |= STAT_VISITED;
> > + int visited_flag = initfirst ? STAT_VISITED_1 : STAT_VISITED_2;
> > +
> > + object->status |= visited_flag;
> > 
> >   TAILQ_FOREACH(n, >child_list, next_sib) {
> > - if (n->data->status & STAT_VISITED)
> > + if (n->data->status & visited_flag)
> >   continue;
> >   _dl_call_init_recurse(n->data, initfirst);
> >   }
> > -
> > - object->status &= ~STAT_VISITED;
> > 
> >   if (object->status & STAT_INIT_DONE)
> >   return;
> > Index: libexec/ld.so/resolve.h
> > 

Re: ld.so speedup (part 1)

[Nathanael Rensen]

On Sun, 5 May 2019 at 06:41, Martin Pieuchot  wrote:
>
> On 27/04/19(Sat) 21:45, Nathanael Rensen wrote:
> > The diff below speeds up ld.so library loading where the dependency tree
> > is broad and deep, such as samba's smbd which links over 100 libraries.
> >
> > See for example https://marc.info/?l=openbsd-misc=155007285712913=2
> >
> > The timings below are for ldd /usr/local/sbin/smbd:
> >
> > Timing without diff: 2m02.50s real 2m02.48s user 0m00.02s system
> > Timing with diff   : 0m00.03s real 0m00.02s user 0m00.01s system
> >
> > Note that these timings are for a build of a recent samba master tree
> > (linked with kerberos) which is probably slower than the OpenBSD port.
> >
> > While this diff speeds up loading (e.g. ldd) a second diff (part 2) is
> > also helpful to speed up library initialisation.
>
> As for the other diff, could you explain in words what does your change
> do?

The broad intent is the same as for the other diff - avoid traversing a
node multiple times when it appears in multiple places within the tree.
This situation is more complex than the part 2 diff. More detail in
response to your specific questions below.

> Comments below :)
>
> > Index: libexec/ld.so/dlfcn.c
> > ===
> > RCS file: /cvs/src/libexec/ld.so/dlfcn.c,v
> > retrieving revision 1.102
> > diff -u -p -p -u -r1.102 dlfcn.c
> > --- libexec/ld.so/dlfcn.c 22 Oct 2018 01:59:08 -  1.102
> > +++ libexec/ld.so/dlfcn.c 27 Apr 2019 13:24:02 -
> > @@ -93,7 +93,7 @@ dlopen(const char *libname, int flags)
> >   /* if opened but grpsym_list has not been created */
> >   if (OBJECT_DLREF_CNT(object) == 1) {
> >   /* add first object manually */
> > - _dl_link_grpsym(object, 1);
> > + _dl_link_grpsym(object);
> >   _dl_cache_grpsym_list(object);
> >   }
> >   goto loaded;
> > Index: libexec/ld.so/library_subr.c
> > ===
> > RCS file: /cvs/src/libexec/ld.so/library_subr.c,v
> > retrieving revision 1.48
> > diff -u -p -p -u -r1.48 library_subr.c
> > --- libexec/ld.so/library_subr.c  28 Aug 2017 14:06:22 -  1.48
> > +++ libexec/ld.so/library_subr.c  27 Apr 2019 13:24:02 -
> > @@ -527,19 +527,13 @@ _dl_link_child(elf_object_t *dep, elf_ob
> >  static unsigned int _dl_grpsym_gen = 0;
> > 
> >  void
> > -_dl_link_grpsym(elf_object_t *object, int checklist)
> > +_dl_link_grpsym(elf_object_t *object)
> >  {
> >   struct dep_node *n;
> > 
> > - if (checklist) {
> > - TAILQ_FOREACH(n, &_dl_loading_object->grpsym_list, next_sib)
> > - if (n->data == object)
> > - return; /* found, dont bother adding */
> > - } else {
> > - if (object->grpsym_gen == _dl_grpsym_gen) {
> > + TAILQ_FOREACH(n, &_dl_loading_object->grpsym_list, next_sib)
> > + if (n->data == object)
> >   return; /* found, dont bother adding */
> > - }
> > - }
>
> Currently this loop isn't executed when being called from
> _dl_cache_grpsym_list().  Is there any risk of adding a duplicate?

I don't believe so. As far as I can tell the grpsym_gen mechanism works
as intended to avoid the overhead of the dupe-check during recursion.

> Does it buy us much to remove the lookup?

In my testing I found that the cost was negligible. It's a linear search
through a list bounded by the number of distinct child libraries, however
there may be dependency trees where it does make a noticeable difference.

The reason that my diff removes the dupe-check bypass is aesthetic. The
bypass is only used by _dl_cache_grpsym_list(). I chose to prioritise
cleaning up the _dl_link_grpsym() interface and gain obvious avoidance
of duplicates over performance (as my testing didn't find that
performance was adversely affected).

I can see an irony here - I'm proposing a speed-up diff that removes
one of the existing performance improvements. Your point is correct:
If the goal is optimal performance then the dupe-check bypass should
be restored. It can be - more later.

> >   object->grpsym_gen = _dl_grpsym_gen;
> > 
> >   n = _dl_malloc(sizeof *n);
> > @@ -572,6 +566,7 @@ _dl_cache_grpsym_list_setup(elf_object_t
> >  void
> >  _dl_cache_grpsym_list(elf_object_t *object)
> >  {
> > + int recurse = 0;
>

ld.so speedup (part 2)

[Nathanael Rensen]

The diff below speeds up ld.so library intialisation where the dependency
tree is broad and deep, such as samba's smbd which links over 100 libraries.

See for example https://marc.info/?l=openbsd-misc=155007285712913=2

See https://marc.info/?l=openbsd-tech=155637285221396=2 for part 1
that speeds up library loading.

The timings below are for /usr/local/sbin/smbd --version:

Timing without either diff  : 6m45.67s real  6m45.65s user  0m00.02s system
Timing with part 1 diff only: 4m42.88s real  4m42.85s user  0m00.02s system
Timing with part 2 diff only: 2m02.61s real  2m02.60s user  0m00.01s system
Timing with both diffs  : 0m00.03s real  0m00.03s user  0m00.00s system

Note that these timings are for a build of a recent samba master tree
(linked with kerberos) which is probably slower than the OpenBSD port.

Nathanael


Index: libexec/ld.so/loader.c
===
RCS file: /cvs/src/libexec/ld.so/loader.c,v
retrieving revision 1.177
diff -u -p -p -u -r1.177 loader.c
--- libexec/ld.so/loader.c  3 Dec 2018 05:29:56 -   1.177
+++ libexec/ld.so/loader.c  27 Apr 2019 13:24:02 -
@@ -749,15 +749,15 @@ _dl_call_init_recurse(elf_object_t *obje
 {
struct dep_node *n;
 
-   object->status |= STAT_VISITED;
+   int visited_flag = initfirst ? STAT_VISITED_1 : STAT_VISITED_2;
+
+   object->status |= visited_flag;
 
TAILQ_FOREACH(n, >child_list, next_sib) {
-   if (n->data->status & STAT_VISITED)
+   if (n->data->status & visited_flag)
continue;
_dl_call_init_recurse(n->data, initfirst);
}
-
-   object->status &= ~STAT_VISITED;
 
if (object->status & STAT_INIT_DONE)
return;
Index: libexec/ld.so/resolve.h
===
RCS file: /cvs/src/libexec/ld.so/resolve.h,v
retrieving revision 1.90
diff -u -p -p -u -r1.90 resolve.h
--- libexec/ld.so/resolve.h 21 Apr 2019 04:11:42 -  1.90
+++ libexec/ld.so/resolve.h 27 Apr 2019 13:24:02 -
@@ -125,8 +125,9 @@ struct elf_object {
 #defineSTAT_FINI_READY 0x10
 #defineSTAT_UNLOADED   0x20
 #defineSTAT_NODELETE   0x40
-#defineSTAT_VISITED0x80
+#defineSTAT_VISITED_1  0x80
 #defineSTAT_GNU_HASH   0x100
+#defineSTAT_VISITED_2  0x200
 
Elf_Phdr*phdrp;
int phdrc;

ld.so speedup (part 1)

[Nathanael Rensen]

The diff below speeds up ld.so library loading where the dependency tree
is broad and deep, such as samba's smbd which links over 100 libraries.

See for example https://marc.info/?l=openbsd-misc=155007285712913=2 

The timings below are for ldd /usr/local/sbin/smbd:

Timing without diff: 2m02.50s real 2m02.48s user 0m00.02s system
Timing with diff   : 0m00.03s real 0m00.02s user 0m00.01s system

Note that these timings are for a build of a recent samba master tree
(linked with kerberos) which is probably slower than the OpenBSD port.

While this diff speeds up loading (e.g. ldd) a second diff (part 2) is
also helpful to speed up library initialisation.

Nathanael


Index: libexec/ld.so/dlfcn.c
===
RCS file: /cvs/src/libexec/ld.so/dlfcn.c,v
retrieving revision 1.102
diff -u -p -p -u -r1.102 dlfcn.c
--- libexec/ld.so/dlfcn.c   22 Oct 2018 01:59:08 -  1.102
+++ libexec/ld.so/dlfcn.c   27 Apr 2019 13:24:02 -
@@ -93,7 +93,7 @@ dlopen(const char *libname, int flags)
/* if opened but grpsym_list has not been created */
if (OBJECT_DLREF_CNT(object) == 1) {
/* add first object manually */
-   _dl_link_grpsym(object, 1);
+   _dl_link_grpsym(object);
_dl_cache_grpsym_list(object);
}
goto loaded;
Index: libexec/ld.so/library_subr.c
===
RCS file: /cvs/src/libexec/ld.so/library_subr.c,v
retrieving revision 1.48
diff -u -p -p -u -r1.48 library_subr.c
--- libexec/ld.so/library_subr.c28 Aug 2017 14:06:22 -  1.48
+++ libexec/ld.so/library_subr.c27 Apr 2019 13:24:02 -
@@ -527,19 +527,13 @@ _dl_link_child(elf_object_t *dep, elf_ob
 static unsigned int _dl_grpsym_gen = 0;
 
 void
-_dl_link_grpsym(elf_object_t *object, int checklist)
+_dl_link_grpsym(elf_object_t *object)
 {
struct dep_node *n;
 
-   if (checklist) {
-   TAILQ_FOREACH(n, &_dl_loading_object->grpsym_list, next_sib)
-   if (n->data == object)
-   return; /* found, dont bother adding */
-   } else {
-   if (object->grpsym_gen == _dl_grpsym_gen) {
+   TAILQ_FOREACH(n, &_dl_loading_object->grpsym_list, next_sib)
+   if (n->data == object)
return; /* found, dont bother adding */
-   }
-   }
object->grpsym_gen = _dl_grpsym_gen;
 
n = _dl_malloc(sizeof *n);
@@ -572,6 +566,7 @@ _dl_cache_grpsym_list_setup(elf_object_t
 void
 _dl_cache_grpsym_list(elf_object_t *object)
 {
+   int recurse = 0;
struct dep_node *n;
 
/*
@@ -581,8 +576,12 @@ _dl_cache_grpsym_list(elf_object_t *obje
 */
 
TAILQ_FOREACH(n, >child_list, next_sib)
-   _dl_link_grpsym(n->data, 0);
+   if (n->data->grpsym_gen != _dl_grpsym_gen) {
+   _dl_link_grpsym(n->data);
+   recurse = 1;
+   }
 
-   TAILQ_FOREACH(n, >child_list, next_sib)
-   _dl_cache_grpsym_list(n->data);
+   if (recurse)
+   TAILQ_FOREACH(n, >child_list, next_sib)
+   _dl_cache_grpsym_list(n->data);
 }
Index: libexec/ld.so/loader.c
===
RCS file: /cvs/src/libexec/ld.so/loader.c,v
retrieving revision 1.177
diff -u -p -p -u -r1.177 loader.c
--- libexec/ld.so/loader.c  3 Dec 2018 05:29:56 -   1.177
+++ libexec/ld.so/loader.c  27 Apr 2019 13:24:02 -
@@ -360,7 +360,7 @@ _dl_load_dep_libs(elf_object_t *object, 
}
 
/* add first object manually */
-   _dl_link_grpsym(object, 1);
+   _dl_link_grpsym(object);
_dl_cache_grpsym_list_setup(object);
 
return(0);
@@ -543,7 +543,7 @@ _dl_boot(const char **argv, char **envp,
_dl_add_object(dyn_obj);
 
dyn_obj->refcount++;
-   _dl_link_grpsym(dyn_obj, 1);
+   _dl_link_grpsym(dyn_obj);
 
dyn_obj->status |= STAT_RELOC_DONE;
_dl_set_sod(dyn_obj->load_name, _obj->sod);
Index: libexec/ld.so/resolve.h
===
RCS file: /cvs/src/libexec/ld.so/resolve.h,v
retrieving revision 1.90
@@ -271,7 +272,7 @@ int _dl_load_dep_libs(elf_object_t *obje
 int _dl_rtld(elf_object_t *object);
 void _dl_call_init(elf_object_t *object);
 void _dl_link_child(elf_object_t *dep, elf_object_t *p);
-void _dl_link_grpsym(elf_object_t *object, int checklist);
+void _dl_link_grpsym(elf_object_t *object);
 void _dl_cache_grpsym_list(elf_object_t *object);
 void _dl_cache_grpsym_list_setup(elf_object_t *object);
 void _dl_link_grpref(elf_object_t *load_group, elf_object_t *load_object);

AMRR improvements for rt2860

[Nathanael Rensen]

I have been using an rt2860 hostap for a few years and I have discovered
that AMRR does not work properly for this driver. The symptom is that
some stations get stuck at 1 Mbps and do not progress up to faster rates.

Unlike many drivers, rt2860 does not keep the ieee80211_amrr_node on its
rt2860_node. Instead it maintains an array of ieee80211_amrr_nodes on
rt2860_softc indexed by WCID.

This approach runs into trouble when a non-active node (e.g. when in
IEEE80211_STA_COLLECT) exists with the same WCID as an active node.
If during rt2860_updatestats() the non-active node is encountered prior
to the active node, ieee80211_amrr_choose() performs the rate adjustment
against the non-active node. The counters in the ieee80211_amrr_node
structure get reset and then when the active node does show up in the
iteration it doesn't get its rate adjusted.

The approach that the diff below takes is to move the ieee80211_amrr_node
onto the rt2860_node and maintain a WCID to rt2860_node mapping on
rt2860_softc. This improves consistency with other drivers such as athn(4),
bwi(4), iwn(4), rt2560 and others, which maintain the ieee80211_amrr_node
on the device node respectively.

The diff below also introduces dedicated timers for AMRR and for scan
instead of using the RT2860 GP interrupt, which also improves consistency
with the way other drivers manage AMRR.

Nathanael

Index: rt2860.c
===
RCS file: /cvs/src/sys/dev/ic/rt2860.c,v
retrieving revision 1.90
diff -u -p -r1.90 rt2860.c
--- rt2860.c13 Apr 2016 10:49:26 -  1.90
+++ rt2860.c24 Apr 2016 04:02:37 -
@@ -84,8 +84,9 @@ void  rt2860_free_rx_ring(struct rt2860_
struct rt2860_rx_ring *);
 struct ieee80211_node *rt2860_node_alloc(struct ieee80211com *);
 intrt2860_media_change(struct ifnet *);
+void   rt2860_next_scan(void *);
 void   rt2860_iter_func(void *, struct ieee80211_node *);
-void   rt2860_updatestats(struct rt2860_softc *);
+void   rt2860_amrr_timeout(void *);
 void   rt2860_newassoc(struct ieee80211com *, struct ieee80211_node *,
int);
 void   rt2860_node_leave(struct ieee80211com *,
@@ -103,7 +104,6 @@ voidrt2860_drain_stats_fifo(struct rt2
 void   rt2860_tx_intr(struct rt2860_softc *, int);
 void   rt2860_rx_intr(struct rt2860_softc *);
 void   rt2860_tbtt_intr(struct rt2860_softc *);
-void   rt2860_gp_intr(struct rt2860_softc *);
 intrt2860_tx(struct rt2860_softc *, struct mbuf *,
struct ieee80211_node *);
 void   rt2860_start(struct ifnet *);
@@ -127,7 +127,6 @@ int rt3090_filter_calib(struct rt2860_s
uint8_t *);
 void   rt3090_rf_setup(struct rt2860_softc *);
 void   rt2860_set_leds(struct rt2860_softc *, uint16_t);
-void   rt2860_set_gp_timer(struct rt2860_softc *, int);
 void   rt2860_set_bssid(struct rt2860_softc *, const uint8_t *);
 void   rt2860_set_macaddr(struct rt2860_softc *, const uint8_t *);
 void   rt2860_updateslot(struct ieee80211com *);
@@ -202,6 +201,8 @@ rt2860_attach(void *xsc, int id)
 
sc->amrr.amrr_min_success_threshold =  1;
sc->amrr.amrr_max_success_threshold = 15;
+   timeout_set(>amrr_to, rt2860_amrr_timeout, sc);
+   timeout_set(>scan_to, rt2860_next_scan, sc);
 
/* wait for NIC to initialize */
for (ntries = 0; ntries < 100; ntries++) {
@@ -376,6 +377,9 @@ rt2860_detach(void *xsc)
struct ifnet *ifp = >sc_ic.ic_if;
int qid;
 
+   timeout_del(>scan_to);
+   timeout_del(>amrr_to);
+
ieee80211_ifdetach(ifp);/* free all nodes */
if_detach(ifp);
 
@@ -768,20 +772,40 @@ rt2860_media_change(struct ifnet *ifp)
return 0;
 }
 
+/*
+ * This function is called periodically during scanning to
+ * switch from one channel to another.
+ */
+void
+rt2860_next_scan(void *arg)
+{
+   struct rt2860_softc *sc = arg;
+   struct ieee80211com *ic = >sc_ic;
+   int s;
+
+   s = splnet();
+   if (ic->ic_state == IEEE80211_S_SCAN)
+   ieee80211_next_scan(>ic_if);
+   splx(s);
+}
+
 void
 rt2860_iter_func(void *arg, struct ieee80211_node *ni)
 {
struct rt2860_softc *sc = arg;
-   uint8_t wcid = ((struct rt2860_node *)ni)->wcid;
+   struct rt2860_node *rn = (struct rt2860_node *)ni;
 
-   ieee80211_amrr_choose(>amrr, ni, >amn[wcid]);
+   ieee80211_amrr_choose(>amrr, ni, >amn);
 }
 
 void
-rt2860_updatestats(struct rt2860_softc *sc)
+rt2860_amrr_timeout(void *arg)
 {
+   struct rt2860_softc *sc = arg;
struct ieee80211com *ic = >sc_ic;
+   int s;
 
+   s = splnet();
 #ifndef IEEE80211_STA_ONLY
/*
 * In IBSS or HostAP modes (when the hardware sends beacons), the
@@ -809,6 +833,9 @@ rt2860_updatestats(struct 

Re: preserve /etc and /var file ownership during make build

[Nathanael Rensen]

On 7 March 2016 at 08:49, Theo de Raadt  wrote:
>> During make build the distrib/sets/makeetcset script does not preserve
>> ownership when installing /etc and /var files from the base set.
>>
>> I've noticed that this leads to some discrepancies compared with a normal
>> install / upgrade.
>
>Then please list them...

/etc/examples/chio.conf
 - group: operator vs wheel

/var/yp/Makefile.main
/var/yp/Makefile.yp
/usr/share/misc/mail.hlp
/usr/share/misc/mail.tildehelp
 - group: bin vs wheel

It is any file installed by the distribution-etc-root-var target with
ownership different from root:wheel that is also included in the base set.

Nathanael

maillog mode

[Nathanael Rensen]

About four months ago the mode for maillog was changed from 600 to 640
in newsyslog.conf, but the Makefile that creates this file was not
updated.

Nathanael

Index: Makefile
===
RCS file: /cvs/src/etc/Makefile,v
retrieving revision 1.419
diff -u -p -r1.419 Makefile
--- Makefile27 Jan 2016 20:52:41 -  1.419
+++ Makefile15 Feb 2016 13:08:32 -
@@ -183,7 +183,7 @@ distribution-etc-root-var: distrib-dirs
${DESTDIR}/var/log/lastlog
${INSTALL} -c -o ${BINOWN} -g wheel -m 640 /dev/null \
${DESTDIR}/var/log/lpd-errs
-   ${INSTALL} -c -o ${BINOWN} -g wheel -m 600 /dev/null \
+   ${INSTALL} -c -o ${BINOWN} -g wheel -m 640 /dev/null \
${DESTDIR}/var/log/maillog
${INSTALL} -c -o ${BINOWN} -g wheel -m 644 /dev/null \
${DESTDIR}/var/log/messages

sis(4) induced uvm faults on net4801

[Nathanael Rensen]

tl;dr: Zero-length packets from sis(4) on net4801 result in negative
length mbufs causing uvm faults.

I have observed uvm faults shortly after bringing up a sis(4) interface on
a Soekris net4801:

uvm_fault(0xd3adbbf0, 0xd3ee5000, 0, 1) -> e
kernel: page fault trap, code=0
Stopped at  memcpy+0x13:repe movsl  (%esi),%es:(%edi)

ddb> trace
memcpy(d6025e00,1a280e,3b9aca00,2,1) at memcpy+0x13
m_copym2(d6025e00,e,3b9aca00,2) at m_copym2+0x19
bridge_m_dup(d6025e00,d3eed000,f13c1d78,d02a0d4b) at bridge_m_dup+0x2f
bridge_localbroadcast(d3eed000,d3ee0c00,f13c1dda,d6025e00) at bridge_localbroad
cast+0x47
bridge_broadcast(d3eed000,d3d3d834,f13c1dda,d6025e00,d3d3d834) at bridge_broadc
ast+0xaf
bridgeintr_frame(d3eed000,d3d3d834,d6025e00,d3ed9700) at bridgeintr_frame+0x1ed

bridge_process(d3d3d834,d6025e00,f13c1e6c,f13c1e6c,d6025c00) at bridge_process+
0x257
bridgeintr(d029312e,d3b22b20,d6025c00,f13c1ea8,d3d5c800) at bridgeintr+0x54
netintr(0,d020224c,0,f13c1efc) at netintr+0x47
softintr_dispatch(1) at softintr_dispatch+0x6c
Xsoftnet() at Xsoftnet+0x12
--- interrupt ---
0:

ddb> show mbuf 0xd6025e00
mbuf 0xd6025e00
m_type: 1   m_flags: b
m_next: 0x0 m_nextpkt: 0x0
m_data: 0xd3d42000  m_len: 4294967292
m_dat: 0xd6025e14   m_pktdat: 0xd6025e44
m_ptkhdr.ph_ifidx: 1m_pkthdr.len: -4
m_ptkhdr.ph_tags: 0x0   m_pkthdr.ph_tagsset: 0
m_pkthdr.ph_flowid: 0
m_pkthdr.csum_flags: 0
m_pkthdr.ether_vtag: 0  m_ptkhdr.ph_rtableid: 0
m_pkthdr.pf.statekey: 0x0   m_pkthdr.pf.inp 0x0
m_pkthdr.pf.qid: 0  m_pkthdr.pf.tag: 0
m_pkthdr.pf.flags: 0
m_pkthdr.pf.routed: 0   m_pkthdr.pf.prio: 3
m_ext.ext_buf: 0xd3d42000   m_ext.ext_size: 2048
m_ext.ext_free: 0xd027a717  m_ext.ext_arg: 0xd3b0050c
m_ext.ext_nextref: 0xd6025e00   m_ext.ext_prevref: 0xd6025e00

# dmesg | grep sis0
sis0 at pci0 dev 6 function 0 vendor 0x100b product 0x0020 rev 0x00, DP83816A: 
irq 10, address 00:00:24:c4:fe:3c
nsphyter0 at sis0 phy 0: DP83815 10/100 PHY, rev. 1

I traced this to the DP83816A sometimes producing a small number of zero-
length packets soon after initialisation. The sis(4) driver unconditionally
subtracts ETHER_CRC_LEN from the packet length. When the packet length is
zero this results in a negative length mbuf.

#define SIS_RXBYTES(x)  \
((letoh32((x)->sis_ctl) & SIS_CMDSTS_BUFLEN) - ETHER_CRC_LEN)

total_len = SIS_RXBYTES(cur_rx);

m->m_pkthdr.len = m->m_len = total_len;

I haven't been able to nail down the root cause of this behaviour. It is
not a recently introduced issue. For me this happens commonly enough that
it is readily reproduced, but the zero-length packets do not occur on
every boot and not every zero-length packet results in a uvm fault.

A web search didn't turn up any similar reports so perhaps there is
something unusual about my situation. Maybe having sis0 on a bridge
introduces a code path that increases the likelihood of a uvm_fault in
response to a negative length mbuf.

The descriptor cmdsts for these zero-length packets is consistently:

0xd000 = SIS_CMDSTS_OWN | SIS_CMDSTS_MORE | SIS_CMDSTS_CRC

I found the following comment in the DP83816A datasheet:

Care must be taken when setting DRTH to a value lower than the number
of bytes needed to determine if packet should be accepted or rejected.
In this case, the packet might be rejected after the bus master
operation to begin transferring the packet into memory has begun. When
this occurs, neither the OK bit or any error status bit in the
descriptor's cmdsts will be set.

sis(4) sets DRTH to 64 bytes, which is sufficient for packet identification.
In the end, maybe this is just buggy DP83816A behaviour.

In any case the following diff works around the problem for me.

Nathanael


Index: if_sisreg.h
===
RCS file: /cvs/src/sys/dev/pci/if_sisreg.h,v
retrieving revision 1.34
diff -u -p -r1.34 if_sisreg.h
--- if_sisreg.h 11 Feb 2015 21:36:02 -  1.34
+++ if_sisreg.h 6 Jan 2016 14:33:11 -
@@ -343,8 +343,7 @@ struct sis_desc {
 #define SIS_LASTDESC(x)(!(letoh32((x)->sis_ctl) & 
SIS_CMDSTS_MORE)))
 #define SIS_OWNDESC(x) (letoh32((x)->sis_ctl) & SIS_CMDSTS_OWN)
 #define SIS_INC(x, y)  (x) = ((x) == ((y)-1)) ? 0 : (x)+1
-#define SIS_RXBYTES(x) \
-   ((letoh32((x)->sis_ctl) & SIS_CMDSTS_BUFLEN) - ETHER_CRC_LEN)
+#define SIS_RXBYTES(x) (letoh32((x)->sis_ctl) & SIS_CMDSTS_BUFLEN)
 
 #define SIS_RXSTAT_COLL0x0001
 #define SIS_RXSTAT_LOOPBK  0x0002
Index: if_sis.c
===
RCS file: /cvs/src/sys/dev/pci/if_sis.c,v
retrieving revision 1.132
diff -u -p -r1.132 if_sis.c
--- if_sis.c25 Nov 2015 03:09:59 -  1.132
+++ if_sis.c6 Jan 2016 14:33:11 -
@@ -1389,6 +1389,18 @@ sis_rxeof(struct sis_softc *sc)
if_rxr_put(>sis_cdata.sis_rx_ring, 1);
 
   

pfctl(8) detect multiple root queues on one interface

[Nathanael Rensen]

pfctl(8) doesn't check that there is at most one root queue per interface.
For example:

  queue r0 on $if bandwidth 100M default
  queue r1 on $if bandwidth 100M default

# pfctl -f /etc/pf.conf
pfctl: DIOCXCOMMIT: Invalid argument

Below is a diff for pfctl(8) to detect and report this situation before
attempting to commit the queues.

Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.332
diff -u -p -r1.332 pfctl.c
--- pfctl.c 10 Dec 2015 17:27:00 -  1.332
+++ pfctl.c 11 Dec 2015 11:58:51 -
@@ -1120,6 +1120,16 @@ pfctl_add_queue(struct pfctl *pf, struct
return (1);
}
 
+   if (q->parent[0] == '\0') {
+   TAILQ_FOREACH(qi, , entries) {
+   if (strcmp(q->ifname, qi->qs.ifname))
+   continue;
+   printf("A queue is already defined on interface %s\n",
+   qi->qs.ifname);
+   return (1);
+   }
+   }
+
if ((qi = calloc(1, sizeof(*qi))) == NULL)
err(1, "calloc");
bcopy(q, >qs, sizeof(qi->qs));

Avoid -nan for ping std-dev

[Nathanael Rensen]

Sometimes floating point imprecision in the calculation of the standard
deviation of round-trip latency by ping(8) / ping6(8) leads to attempting
to evaluate the sqrt(3) of a negative value:

$ ping -c 1 www.google.com
PING www.google.com (150.101.170.166): 56 data bytes
64 bytes from 150.101.170.166: icmp_seq=0 ttl=61 time=20.623 ms
--- www.google.com ping statistics ---
1 packets transmitted, 1 packets received, 0.0% packet loss
round-trip min/avg/max/std-dev = 20.623/20.623/20.623/-nan ms

Index: ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.134
diff -u -p -r1.134 ping.c
--- ping.c  10 Nov 2015 18:36:33 -  1.134
+++ ping.c  28 Nov 2015 15:00:08 -
@@ -1004,7 +1004,7 @@ summary(int header, int insig)
/* Only display average to microseconds */
double num = nreceived + nrepeats;
double avg = tsum / num;
-   double dev = sqrt(tsumsq / num - avg * avg);
+   double dev = sqrt(fmax(0, tsumsq / num - avg * avg));
snprintf(buft, sizeof(buft),
"round-trip min/avg/max/std-dev = %.3f/%.3f/%.3f/%.3f ms\n",
tmin, avg, tmax, dev);
Index: ping6.c
===
RCS file: /cvs/src/sbin/ping6/ping6.c,v
retrieving revision 1.141
diff -u -p -r1.141 ping6.c
--- ping6.c 10 Nov 2015 18:36:33 -  1.141
+++ ping6.c 28 Nov 2015 15:03:31 -
@@ -1263,7 +1263,7 @@ summary(int signo)
/* Only display average to microseconds */
double num = nreceived + nrepeats;
double avg = tsum / num;
-   double dev = sqrt(tsumsq / num - avg * avg);
+   double dev = sqrt(fmax(0, tsumsq / num - avg * avg));
snprintf(buft, sizeof(buft),
"round-trip min/avg/max/std-dev = %.3f/%.3f/%.3f/%.3f ms\n",
tmin, avg, tmax, dev);

Redundant logic in /etc/rc.d/spamlogd

[Nathanael Rensen]

The logic to disable spamlogd when spamd is not enabled was added to
_rc_quirks() in /etc/rc.d/rc.subr. The corresponding logic that remains
in /etc/rc.d/spamlogd became ineffective when additional code was added
below it anyway.

Index: spamlogd
===
RCS file: /cvs/src/etc/rc.d/spamlogd,v
retrieving revision 1.2
diff -u -p -r1.2 spamlogd
--- spamlogd8 Aug 2011 17:13:31 -   1.2
+++ spamlogd29 Jul 2014 04:23:21 -
@@ -9,7 +9,6 @@ daemon="/usr/libexec/spamlogd"
 rc_reload=NO
 
 rc_pre() {
-   [  X"${spamd_flags}" != X"NO" -a X"${spamd_black}" = X"NO" ]
if pfctl -si | grep -q Enabled; then
ifconfig pflog0 create
if ifconfig pflog0; then

Nathanael

RFC 5071 DHCP options

[Nathanael Rensen]

I use dhcpd(8) to boot some boxes with PXELINUX. The numbered options work
but dhcpd.conf(5) is easier to maintain with names. These options are
defined in RFC 5071.

Nathanael

Index: sbin/dhclient/tables.c
===
RCS file: /cvs/src/sbin/dhclient/tables.c,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 tables.c
--- sbin/dhclient/tables.c  21 Jan 2014 03:07:50 -  1.18
+++ sbin/dhclient/tables.c  27 Aug 2015 09:13:26 -
@@ -269,9 +269,9 @@ const struct option dhcp_options[256] = 
/* 206 */ { option-206, X },
/* 207 */ { option-207, X },
/* 208 */ { option-208, X },
-   /* 209 */ { option-209, X },
-   /* 210 */ { option-210, X },
-   /* 211 */ { option-211, X },
+   /* 209 */ { config-file, t },
+   /* 210 */ { path-prefix, t },
+   /* 211 */ { reboot-time, L },
/* 212 */ { option-212, X },
/* 213 */ { option-213, X },
/* 214 */ { option-214, X },
Index: usr.sbin/dhcpd/dhcp-options.5
===
RCS file: /cvs/src/usr.sbin/dhcpd/dhcp-options.5,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 dhcp-options.5
--- usr.sbin/dhcpd/dhcp-options.5   2 Jun 2015 16:02:45 -   1.21
+++ usr.sbin/dhcpd/dhcp-options.5   27 Aug 2015 09:13:29 -
@@ -176,6 +176,10 @@ This option does the same as
 .Ic classless-static-routes ,
 but uses option code 249 instead of 121,
 since Windows XP and Windows Server 2003 ignore option 121.
+.It Ic option config-file Ar string ;
+This option specifies the configuration file for the PXELINUX boot loader.
+This is typically used in conjuction with the path-prefix option.
+See RFC 5071 for details.
 .It Ic option cookie-servers Ar ip-address Oo , Ar ip-address ... Oc ;
 The
 .Ic cookie-servers
@@ -453,6 +457,11 @@ Path MTU Discovery as defined in RFC 119
 The table is formatted as a list of 16-bit unsigned integers,
 ordered from smallest to largest.
 The minimum MTU value cannot be smaller than 68.
+.It Ic option path-prefix Ar string ;
+This option specifies the path prefix for a PXELINUX boot loader
+configuration file.
+This is typically used in conjuction with the config-file option.
+See RFC 5071 for details.
 .It Ic option perform-mask-discovery Ar flag ;
 This option specifies whether or not the client should perform subnet mask
 discovery using ICMP.
@@ -472,6 +481,10 @@ The
 .Ic pop-server
 option specifies a list of POP3 servers available to the client.
 Servers should be listed in order of preference.
+.It Ic option reboot-time Ar uint32 ;
+This option determines how long the PXELINUX boot loader waits to connect
+to a TFTP server before rebooting.
+See RFC 5071 for details.
 .It Ic option relay-agent-information Ar string ;
 This is a container option for specific agent-supplied sub-options.
 See RFC 3046 for details.
Index: usr.sbin/dhcpd/tables.c
===
RCS file: /cvs/src/usr.sbin/dhcpd/tables.c,v
retrieving revision 1.11
diff -u -p -u -p -r1.11 tables.c
--- usr.sbin/dhcpd/tables.c 27 Jun 2015 14:29:39 -  1.11
+++ usr.sbin/dhcpd/tables.c 27 Aug 2015 09:13:29 -
@@ -271,9 +271,9 @@ struct option dhcp_options[256] = {
{ option-206, X,dhcp_universe, 206 },
{ option-207, X,dhcp_universe, 207 },
{ option-208, X,dhcp_universe, 208 },
-   { option-209, X,dhcp_universe, 209 },
-   { option-210, X,dhcp_universe, 210 },
-   { option-211, X,dhcp_universe, 211 },
+   { config-file, t,   dhcp_universe, 209 },
+   { path-prefix, t,   dhcp_universe, 210 },
+   { reboot-time, L,   dhcp_universe, 211 },
{ option-212, X,dhcp_universe, 212 },
{ option-213, X,dhcp_universe, 213 },
{ option-214, X,dhcp_universe, 214 },

ntpd(8) constraint with a uniprocessor kernel

[Nathanael Rensen]

With ntpd(8) on a uniprocessor kernel (i386/amd64) I've found that poll(2)
often reacts to the SIGCHLD from a constraint child process before the
response message is received. Then constraint_check_child() closes the fd
and the response is lost.

# /usr/sbin/ntpd -dvs
ntp engine ready
constraint request to 150.101.170.174
constraint request to 150.101.170.153
constraint request to 150.101.170.146
constraint request to 150.101.170.166
constraint request to 150.101.170.159
constraint request to 150.101.170.152
constraint request to 150.101.170.160
constraint request to 150.101.170.187
no reply received in time, skipping initial time setting
^Cntp engine exiting
Terminating

The diff below improves the situation for me.

Nathanael

Index: constraint.c
===
RCS file: /cvs/src/usr.sbin/ntpd/constraint.c,v
retrieving revision 1.12
diff -u -p -r1.12 constraint.c
--- constraint.c28 May 2015 21:34:36 -  1.12
+++ constraint.c25 Jun 2015 02:11:46 -
@@ -277,7 +277,8 @@ constraint_check_child(void)
 received in time, next query %ds,
log_sockaddr((struct sockaddr *)
cstr-addr-ss), CONSTRAINT_SCAN_INTERVAL);
-   }
+   } else if (cstr-state  STATE_REPLY_RECEIVED)
+   constraint_dispatch_msg(cstr-fd);
 
if (fail || cstr-state  STATE_REPLY_RECEIVED) {
cstr-senderrors++;
@@ -372,7 +373,7 @@ constraint_remove(struct constraint *cst
 }
 
 int
-constraint_dispatch_msg(struct pollfd *pfd)
+constraint_dispatch_msg(int fd)
 {
struct imsg  imsg;
struct constraint   *cstr;
@@ -380,20 +381,17 @@ constraint_dispatch_msg(struct pollfd *p
struct timeval   tv[2];
double   offset;
 
-   if ((cstr = constraint_byfd(pfd-fd)) == NULL)
-   return (0);
-
-   if (!(pfd-revents  POLLIN))
+   if ((cstr = constraint_byfd(fd)) == NULL)
return (0);
 
if ((n = imsg_read(cstr-ibuf)) == -1 || n == 0) {
-   constraint_close(pfd-fd);
+   constraint_close(fd);
return (1);
}
 
for (;;) {
if ((n = imsg_get(cstr-ibuf, imsg)) == -1) {
-   constraint_close(pfd-fd);
+   constraint_close(fd);
return (1);
}
if (n == 0)
Index: ntp.c
===
RCS file: /cvs/src/usr.sbin/ntpd/ntp.c,v
retrieving revision 1.132
diff -u -p -r1.132 ntp.c
--- ntp.c   25 May 2015 14:58:34 -  1.132
+++ ntp.c   25 Jun 2015 02:11:46 -
@@ -425,7 +421,11 @@ ntp_main(int pipe_prnt[2], int fd_ctl, s
}
 
for (; nfds  0  j  i; j++) {
-   nfds -= constraint_dispatch_msg(pfd[j]);
+   if (pfd[j].revents) {
+   nfds--;
+   if (pfd[j].revents  POLLIN)
+   constraint_dispatch_msg(pfd[j].fd);
+   }
}
 
for (s = TAILQ_FIRST(conf-ntp_sensors); s != NULL;
Index: ntpd.h
===
RCS file: /cvs/src/usr.sbin/ntpd/ntpd.h,v
retrieving revision 1.121
diff -u -p -r1.121 ntpd.h
--- ntpd.h  20 May 2015 13:32:39 -  1.121
+++ ntpd.h  25 Jun 2015 02:11:46 -
@@ -345,7 +345,7 @@ void constraint_add(struct constraint *
 voidconstraint_remove(struct constraint *);
 int constraint_init(struct constraint *);
 int constraint_query(struct constraint *);
-int constraint_dispatch_msg(struct pollfd *);
+int constraint_dispatch_msg(int fd);
 voidconstraint_check_child(void);
 int constraint_check(double);
 voidconstraint_dns(u_int32_t, u_int8_t *, size_t);

Fix for smtpd offline enqueue

[Nathanael Rensen]

The smtpd enqueue -S option does not take an argument.

An email enqueued offline with only the -t option (e.g. vi recovery)
results in the options -S -t passed to smtpd enqueue. The -t is
ignored resulting in:

debug: smtpd: scanning offline queue...
debug: smtpd: enqueueing offline message 
/var/spool/smtpd/offline/1430502211.RW2ASq5x3b
debug: smtpd: offline scanning done
sendmail: no recipients
warn: smtpd: couldn't enqueue offline message 
/var/spool/smtpd/offline/1430502211.RW2ASq5x3b; smtpctl exited abnormally

Index: enqueue.c
===
RCS file: /cvs/src/usr.sbin/smtpd/enqueue.c,v
retrieving revision 1.91
diff -u -p -r1.91 enqueue.c
--- enqueue.c   27 Feb 2015 12:17:36 -  1.91
+++ enqueue.c   2 May 2015 04:09:25 -
@@ -186,7 +186,7 @@ enqueue(int argc, char *argv[])
save_argv = argv;
 
while ((ch = getopt(argc, argv,
-   A:B:b:E::e:F:f:iJ::L:mN:o:p:qRS:tvV:x)) != -1) {
+   A:B:b:E::e:F:f:iJ::L:mN:o:p:qRStvV:x)) != -1) {
switch (ch) {
case 'f':
fake_from = optarg;

Fix for shutdown(8) +minutes

[Nathanael Rensen]

Errant semicolon preventing shutdown(8) +minutes from working:

Index: shutdown.c
===
RCS file: /cvs/src/sbin/shutdown/shutdown.c,v
retrieving revision 1.42
diff -u -p -r1.42 shutdown.c
--- shutdown.c  18 Apr 2015 18:28:37 -  1.42
+++ shutdown.c  23 Apr 2015 01:59:48 -
@@ -440,7 +440,7 @@ getoffset(char *timearg)
const char *errstr;
 
offset = strtonum(++timearg, 0, INT_MAX, errstr);
-   if (errstr);
+   if (errstr)
badtime();
offset *= 60;
shuttime = now + offset;

TLS_READ_AGAIN and TLS_WRITE_AGAIN

[Nathanael Rensen]

The tls_init(3) man page states:

 The tls_close(), tls_read() and tls_write() functions, along with the
 tls_accept() and tls_connect() function families, have two special return
 values:

   TLS_READ_AGAIN   A read operation is necessary to continue.
   TLS_WRITE_AGAIN  A write operation is necessary to continue.

The caller should call the appropriate function or, in the case of the
tls_close() and the tls_accept() and tls_connect() function families,
repeat the call.

I find the reference to appropriate function unclear. Perhaps there is
some deeper meaning that I'm missing, but since in each case the required
action is to repeat the call it is clearer to state that directly.

I've also included a note about the non-blocking case.

Index: tls_init.3
===
RCS file: /cvs/src/lib/libtls/tls_init.3,v
retrieving revision 1.23
diff -u -p -r1.23 tls_init.3
--- tls_init.3  3 Apr 2015 22:33:43 -   1.23
+++ tls_init.3  18 Apr 2015 07:38:38 -
@@ -424,13 +424,14 @@ A read operation is necessary to continu
 A write operation is necessary to continue.
 .El
 .Pp
-The caller should call the appropriate function or, in the case of the
-.Fn tls_close
-and the
-.Fn tls_accept
-and
-.Fn tls_connect
-function families, repeat the call.
+In response to these return values the original call must be repeated
+with the same arguments.
+If the underlying socket is non-blocking the caller should first
+confirm that the socket is ready to support the required operation,
+such as by using
+.Xr poll 2
+or
+.Xr select 2 .
 .Sh ERRORS
 The
 .Fn tls_error

autoinstall(8) tweaks

[Nathanael Rensen]

A couple of autoinstall(8) tweaks that I find useful.

I find it convenient to be able to specify a path to the response file.
I also prefer to use the DHCP supplied hostname rather than the MAC
address.

Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.829
diff -u -p -r1.829 install.sub
--- install.sub 5 Apr 2015 12:37:14 -   1.829
+++ install.sub 6 Apr 2015 13:33:02 -
@@ -304,7 +304,7 @@ retrap() {
 
 # Fetch response file for autoinstall.
 get_responsefile() {
-   local _rf _ifdev _mac _mode _server _lf
+   local _rf _ifdev _mac _mode _server _lf _path
action=
 
[[ -f /auto_upgrade.conf ]]  _rf=/auto_upgrade.conf _mode=upgrade
@@ -328,16 +328,18 @@ get_responsefile() {
[[ -n $_ifdev ]]  dhclient $_ifdev || break
_lf=/var/db/dhclient.leases.$_ifdev
_server=$(sed /^ *next-server /!d;s///;s/;$//;q $_lf)
-   _mode=$(sed -E '/^ *filename 
auto_(install|upgrade);$/!d;s//\1/;q' $_lf)
+   _mode=$(sed -E '/^ *filename 
(.*\/)?auto_(install|upgrade);$/!d;s//\2/;q' $_lf)
+   _path=$(sed -E '/^ *filename (.*\/)[^/]+;$/!d;s//\1/;q' $_lf)
hostname $(sed -E '/^ *option host-name (.*);$/!d;s//\1/;q' 
$_lf)
done
 
# Fetch response file if server and mode are known, otherwise tell which
-   # one was missing. First try to fetch mac-mode.conf, then mode.conf.
+   # one was missing. First try to fetch mac-mode.conf, then
+   # hostname-mode.conf, and finally mode.conf.
if [[ -n $_server  -n $_mode ]]; then
_mac=$(ifconfig $_ifdev | sed 's/.*lladdr \(.*\)/\1/p;d')
-   for _rf in {$_mac-,}$_mode; do
-   _url=http://$_server/$_rf.conf
+   for _rf in {$_mac-,$(hostname -s)-,}$_mode; do
+   _url=http://$_server/$_path$_rf.conf
echo Fetching $_url
if ftp -Vo /ai.$_mode.conf $_url 2/dev/null; then
action=$_mode
Index: autoinstall.8
===
RCS file: /cvs/src/share/man/man8/autoinstall.8,v
retrieving revision 1.11
diff -u -p -r1.11 autoinstall.8
--- autoinstall.8   23 Oct 2014 21:33:21 -  1.11
+++ autoinstall.8   6 Apr 2015 13:31:22 -
@@ -47,22 +47,16 @@ It behaves as if the user selected '(A)u
 always fetches the response file via the netboot interface.
 .Ss Fetching the response file
 .Nm
-uses HTTP to fetch one of the files
-.Pa install.conf
-or
-.Ar MAC_address Ns - Ns Pa install.conf
-for install answers, or one of
-.Pa upgrade.conf
-or
-.Ar MAC_address Ns - Ns Pa upgrade.conf
-for upgrade answers.
-The URL used to fetch the file is constructed from information provided in
-the
+uses HTTP to fetch a response file which provides answers to
+install or upgrade questions.
+The URL used to fetch the response file is constructed from
+information provided in the
 .Xr dhcpd.conf 5
-statements
-.Ic next-server
+.Ic next-server ,
+.Ic filename
 and
-.Ic filename .
+.Ic host-name
+options.
 If the
 .Ar filename
 is
@@ -71,6 +65,7 @@ then the URLs tried are, in order:
 .Sm off
 .Bd -unfilled -offset indent
 .No http:// Ar next-server No / Ar MAC_address No -install.conf
+.No http:// Ar next-server No / Ar host-name No -install.conf
 .No http:// Ar next-server No /install.conf
 .Ed
 .Sm on
@@ -79,7 +74,7 @@ where
 .Ar MAC_address
 is a string of six hex octets separated by colons
 representing the MAC
-address of the interface being used to fetch the files.
+address of the interface being used to fetch the response file.
 .Pp
 If the
 .Ar filename
@@ -89,9 +84,15 @@ the URLs tried are, in order:
 .Sm off
 .Bd -unfilled -offset indent
 .No http:// Ar next-server No / Ar MAC_address No -upgrade.conf
+.No http:// Ar next-server No / Ar host-name No -upgrade.conf
 .No http:// Ar next-server No /upgrade.conf
 .Ed
 .Sm on
+.Pp
+If the
+.Ar filename
+includes a parent directory it will be used as a server relative
+path in the http URL to fetch the response file.
 .Pp
 On architectures where the
 .Ic filename


It's handy for the log to remain on the host as a reference. Sometimes the
email gets lost (e.g. caught in a spam trap).

Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.829
diff -u -p -r1.829 install.sub
--- install.sub 5 Apr 2015 12:37:14 -   1.829
+++ install.sub 6 Apr 2015 13:33:02 -
@@ -2305,13 +2318,12 @@ elif [[ -z $RESPFILE ]]; then
if [[ -f /ai.done ]]; then
# Generate unique filename and let rc.firsttime feed it to
# whatever mail system we have at hand by then.
-   while _lf=/mnt/var/log/ai.log.$RANDOM  test -e $_lf; do done
+   

Re: Flag to set from address in mail(1)

[Nathanael Rensen]

On Wed, 07 Jan 2015 14:31:13 -0700, Todd C. Miller wrote:

 Here's a version that does not rely on non-standard sendmail -t
 behavior.
 
  - todd

Much better. Works well for me, thanks.

Nathanael

 Index: usr.bin/mail/cmd3.c
 ===
 RCS file: /cvs/src/usr.bin/mail/cmd3.c,v
 retrieving revision 1.25
 diff -u -r1.25 cmd3.c
 --- usr.bin/mail/cmd3.c   6 Apr 2011 11:36:26 -   1.25
 +++ usr.bin/mail/cmd3.c   7 Jan 2015 18:07:06 -
 @@ -227,6 +227,7 @@
   }
   np = elide(np);
   head.h_to = np;
 + head.h_from = NULL;
   if ((head.h_subject = hfield(subject, mp)) == NULL)
   head.h_subject = hfield(subj, mp);
   head.h_subject = reedit(head.h_subject);
 @@ -619,6 +620,7 @@
   if ((head.h_subject = hfield(subject, mp)) == NULL)
   head.h_subject = hfield(subj, mp);
   head.h_subject = reedit(head.h_subject);
 + head.h_from = NULL;
   head.h_cc = NULL;
   head.h_bcc = NULL;
   head.h_smopts = NULL;
 Index: usr.bin/mail/def.h
 ===
 RCS file: /cvs/src/usr.bin/mail/def.h,v
 retrieving revision 1.13
 diff -u -r1.13 def.h
 --- usr.bin/mail/def.h25 Jun 2003 15:13:32 -  1.13
 +++ usr.bin/mail/def.h7 Jan 2015 21:08:11 -
 @@ -172,6 +172,7 @@
   */
  struct header {
   struct name *h_to;  /* Dynamic To: string */
 + char *h_from;   /* User-specified From: string */
   char *h_subject;/* Subject string */
   struct name *h_cc;  /* Carbon copies string */
   struct name *h_bcc; /* Blind carbon copies */
 Index: usr.bin/mail/extern.h
 ===
 RCS file: /cvs/src/usr.bin/mail/extern.h,v
 retrieving revision 1.27
 diff -u -r1.27 extern.h
 --- usr.bin/mail/extern.h 28 Jul 2009 16:05:04 -  1.27
 +++ usr.bin/mail/extern.h 7 Jan 2015 17:59:52 -
 @@ -163,8 +163,8 @@
  void  load(char *);
  struct var *
lookup(char *);
 -int   mail (struct name *, struct name *, struct name *, struct name *,
 -char *);
 +int   mail(struct name *, struct name *, struct name *, struct name *,
 +char *, char *);
  void  mail1(struct header *, int);
  void  makemessage(FILE *, int);
  void  mark(int);
 Index: usr.bin/mail/mail.1
 ===
 RCS file: /cvs/src/usr.bin/mail/mail.1,v
 retrieving revision 1.72
 diff -u -r1.72 mail.1
 --- usr.bin/mail/mail.1   7 Jan 2015 17:08:21 -   1.72
 +++ usr.bin/mail/mail.1   7 Jan 2015 21:25:55 -
 @@ -43,6 +43,7 @@
  .Op Fl dEIinv
  .Op Fl b Ar list
  .Op Fl c Ar list
 +.Op Fl r Ar from-addr
  .Op Fl s Ar subject
  .Ar to-addr ...
  .Ek
 @@ -109,6 +110,13 @@
  Inhibits reading
  .Pa /etc/mail.rc
  upon startup.
 +.It Fl r Ar from-addr
 +Use
 +.Ar from-addr
 +as the from address in the message and envelope.
 +Overrides any
 +.Em from
 +options in the startup files.
  .It Fl s Ar subject
  Specify subject on command line
  (only the first argument after the
 @@ -969,6 +977,19 @@
  .Nm mail
  to expand message recipient addresses, as explained in the section
  .Sx Recipient address specifications .
 +.It Ar from
 +Causes
 +.Nm mail
 +to use the specified sender address in the
 +.Dq From:
 +field of the message header.
 +A stripped down version of the address is also used in the message envelope.
 +If unset, the message will not include an explicit sender address and
 +a default value will be added by the MTA, typically
 +.Dq user@host .
 +This value can be overridden by specifying the
 +.Fl r
 +flag on the command line.
  .It Ar hold
  This option is used to hold messages in the system mailbox
  by default.
 Index: usr.bin/mail/main.c
 ===
 RCS file: /cvs/src/usr.bin/mail/main.c,v
 retrieving revision 1.26
 diff -u -r1.26 main.c
 --- usr.bin/mail/main.c   16 Dec 2014 18:37:17 -  1.26
 +++ usr.bin/mail/main.c   7 Jan 2015 21:27:04 -
 @@ -49,6 +49,7 @@
  {
   int i;
   struct name *to, *cc, *bcc, *smopts;
 + char *fromaddr;
   char *subject;
   char *ef;
   char nosrc = 0;
 @@ -77,8 +78,9 @@
   cc = NULL;
   bcc = NULL;
   smopts = NULL;
 + fromaddr = NULL;
   subject = NULL;
 - while ((i = getopt(argc, argv, EIN:b:c:dfins:u:v)) != -1) {
 + while ((i = getopt(argc, argv, EIN:b:c:dfinr:s:u:v)) != -1) {
   switch (i) {
   case 'u':
   /*
 @@ -100,6 +102,12 @@
   case 'd':
   debug++;
   break;
 + case 'r':
 + /*
 +  * Set From: address
 +  */
 + fromaddr = optarg;
 + break;
  

Re: Flag to set from address in mail(1)

[Nathanael Rensen]

On Mon, 05 Jan 2015 22:26:03 -0500, trondd wrote:

 I like this better. But I still want the set from=XXX in .mailrc and
 of course the manpage.


I would like to have this option.  The diff doesn't work, however. If
you reply to a message, it messes up the header and replyall will crash.

 set from=tro...@gmail.com
 set
...
fromtro...@gmail.com
...
 r 294
From: Replyall
To: ...@x.xxx
Subject: Re: Hi

~x
 R 294
Bus error (core dumped)

Sorry, my fault. Try the diff below.

Nathanael

Index: cmd3.c
===
RCS file: /cvs/src/usr.bin/mail/cmd3.c,v
retrieving revision 1.25
diff -u -p -r1.25 cmd3.c
--- cmd3.c  6 Apr 2011 11:36:26 -   1.25
+++ cmd3.c  6 Jan 2015 17:49:11 -
@@ -230,6 +230,7 @@ _respond(msgvec)
if ((head.h_subject = hfield(subject, mp)) == NULL)
head.h_subject = hfield(subj, mp);
head.h_subject = reedit(head.h_subject);
+   head.h_from = NULL;
if (replyto == NULL  (cp = skin(hfield(cc, mp))) != NULL) {
np = elide(extract(cp, GCC));
np = delname(np, myname);
@@ -619,6 +620,7 @@ _Respond(int *msgvec)
if ((head.h_subject = hfield(subject, mp)) == NULL)
head.h_subject = hfield(subj, mp);
head.h_subject = reedit(head.h_subject);
+   head.h_from = NULL;
head.h_cc = NULL;
head.h_bcc = NULL;
head.h_smopts = NULL;
Index: def.h
===
RCS file: /cvs/src/usr.bin/mail/def.h,v
retrieving revision 1.13
diff -u -p -r1.13 def.h
--- def.h   25 Jun 2003 15:13:32 -  1.13
+++ def.h   6 Jan 2015 17:49:11 -
@@ -173,6 +173,7 @@ struct headline {
 struct header {
struct name *h_to;  /* Dynamic To: string */
char *h_subject;/* Subject string */
+   char *h_from;   /* Sender */
struct name *h_cc;  /* Carbon copies string */
struct name *h_bcc; /* Blind carbon copies */
struct name *h_smopts;  /* Sendmail options */
Index: extern.h
===
RCS file: /cvs/src/usr.bin/mail/extern.h,v
retrieving revision 1.27
diff -u -p -r1.27 extern.h
--- extern.h28 Jul 2009 16:05:04 -  1.27
+++ extern.h6 Jan 2015 17:49:11 -
@@ -164,7 +164,7 @@ void load(char *);
 struct var *
 lookup(char *);
 int mail (struct name *, struct name *, struct name *, struct name *,
-  char *);
+  char *, char *);
 voidmail1(struct header *, int);
 voidmakemessage(FILE *, int);
 voidmark(int);
Index: mail.1
===
RCS file: /cvs/src/usr.bin/mail/mail.1,v
retrieving revision 1.70
diff -u -p -r1.70 mail.1
--- mail.1  16 Dec 2014 18:37:17 -  1.70
+++ mail.1  6 Jan 2015 17:49:11 -
@@ -42,6 +42,7 @@
 .Bk -words
 .Op Fl dEIinv
 .Op Fl b Ar list
+.Op Fl F Ar from
 .Op Fl c Ar list
 .Op Fl s Ar subject
 .Ar to-addr ...
@@ -62,6 +63,11 @@ with lines replaced by messages.
 .Pp
 The options are as follows:
 .Bl -tag -width Ds
+.It Fl F Ar from
+Pass
+.Ar from
+to the mail delivery system as the from address.
+Overrides the from option below.
 .It Fl b Ar list
 Send blind carbon copies to
 .Ar list .
@@ -965,6 +971,14 @@ Causes
 .Nm mail
 to expand message recipient addresses, as explained in the section
 .Sx Recipient address specifications .
+.It Ar from
+Causes
+.Nm mail
+to pass a from address to the mail delivery system. If unset, no from
+address will be passed and the mail delivery system will use its default
+of user at host. This will be overriden if the
+.Fl F
+flag is set.
 .It Ar hold
 This option is used to hold messages in the system mailbox
 by default.
Index: main.c
===
RCS file: /cvs/src/usr.bin/mail/main.c,v
retrieving revision 1.26
diff -u -p -r1.26 main.c
--- main.c  16 Dec 2014 18:37:17 -  1.26
+++ main.c  6 Jan 2015 17:49:11 -
@@ -50,6 +50,7 @@ main(int argc, char **argv)
int i;
struct name *to, *cc, *bcc, *smopts;
char *subject;
+   char *from;
char *ef;
char nosrc = 0;
char *rc;
@@ -78,7 +79,8 @@ main(int argc, char **argv)
bcc = NULL;
smopts = NULL;
subject = NULL;
-   while ((i = getopt(argc, argv, EIN:b:c:dfins:u:v)) != -1) {
+   from = NULL;
+   while ((i = getopt(argc, argv, EF:IN:b:c:dfins:u:v)) != -1) {
switch (i) {
case 'u':
/*
@@ -100,6 +102,9 @@ main(int argc, char **argv)
case 'd':
debug++;
break;
+   case 'F':
+   from = optarg;
+   break;
case 's':
 

Re: Flag to set from address in mail(1)

[Nathanael Rensen]

On Sun, 04 Jan 2015 05:57:38 +, Martin Brandenburg wrote:
 Since the ability to pass arbitrary arguments to sendmail has been
 removed from mail(1), I have added a variable and flag to pass a from
 address to sendmail.
 
 I considered making mail take the same arguments for this as it would
 have when it was just passing onto sendmail but decided against that
 because the code would be more complicated than necessary. But maybe
 somebody wants the compatibility?
 
 Thoughts?
 
 -- Martin Brandenburg
 
 Index: mail.1
 ===
 RCS file: /cvs/src/usr.bin/mail/mail.1,v
 retrieving revision 1.70
 diff -u -p -r1.70 mail.1
 --- mail.116 Dec 2014 18:37:17 -  1.70
 +++ mail.14 Jan 2015 05:42:08 -
 @@ -41,6 +41,7 @@
  .Nm mail
  .Bk -words
  .Op Fl dEIinv
 +.Op Fl a Ar from
  .Op Fl b Ar list
  .Op Fl c Ar list
  .Op Fl s Ar subject
 @@ -62,6 +63,11 @@ with lines replaced by messages.
  .Pp
  The options are as follows:
  .Bl -tag -width Ds
 +.It Fl a Ar from
 +Pass
 +.Ar from
 +to the mail delivery system as the from address.
 +Overrides the from option below.
  .It Fl b Ar list
  Send blind carbon copies to
  .Ar list .
 @@ -965,6 +971,14 @@ Causes
  .Nm mail
  to expand message recipient addresses, as explained in the section
  .Sx Recipient address specifications .
 +.It Ar from
 +Causes
 +.Nm mail
 +to pass a from address to the mail delivery system. If unset, no from
 +address will be passed and the mail delivery system will use its default
 +of user@host. This will be overriden if the
 +.Fl a
 +flag is set.
  .It Ar hold
  This option is used to hold messages in the system mailbox
  by default.
 Index: main.c
 ===
 RCS file: /cvs/src/usr.bin/mail/main.c,v
 retrieving revision 1.26
 diff -u -p -r1.26 main.c
 --- main.c16 Dec 2014 18:37:17 -  1.26
 +++ main.c4 Jan 2015 05:42:08 -
 @@ -49,6 +49,7 @@ main(int argc, char **argv)
  {
   int i;
   struct name *to, *cc, *bcc, *smopts;
 + char *from = NULL;
   char *subject;
   char *ef;
   char nosrc = 0;
 @@ -78,7 +79,7 @@ main(int argc, char **argv)
   bcc = NULL;
   smopts = NULL;
   subject = NULL;
 - while ((i = getopt(argc, argv, EIN:b:c:dfins:u:v)) != -1) {
 + while ((i = getopt(argc, argv, EIN:a:b:c:dfins:u:v)) != -1) {
   switch (i) {
   case 'u':
   /*
 @@ -158,6 +159,9 @@ main(int argc, char **argv)
*/
   assign(skipempty, );
   break;
 + case 'a':
 + from = optarg;
 + break;
   default:
   usage();
   /*NOTREACHED*/
 @@ -202,6 +206,8 @@ main(int argc, char **argv)
   if ((rc = getenv(MAILRC)) == 0)
   rc = ~/.mailrc;
   load(expand(rc));
 + if (from)
 + assign(from, from);
   if (!rcvmode) {
   mail(to, cc, bcc, smopts, subject);
   /*
 @@ -271,7 +277,7 @@ __dead void
  usage(void)
  {
  
 - fprintf(stderr, usage: %s [-dEIinv] [-b list] [-c list] 
 + fprintf(stderr, usage: %s [-dEIinv] [-a from] [-b list] [-c list] 
   [-s subject] to-addr ...\n, __progname);
   fprintf(stderr,%s [-dEIiNnv] -f [file]\n, __progname);
   fprintf(stderr,%s [-dEIiNnv] [-u user]\n, __progname);
 Index: send.c
 ===
 RCS file: /cvs/src/usr.bin/mail/send.c,v
 retrieving revision 1.23
 diff -u -p -r1.23 send.c
 --- send.c17 Jan 2014 18:42:30 -  1.23
 +++ send.c4 Jan 2015 05:42:08 -
 @@ -287,7 +287,11 @@ mail(struct name *to, struct name *cc, s
   head.h_subject = subject;
   head.h_cc = cc;
   head.h_bcc = bcc;
 - head.h_smopts = smopts;
 + if (value(from)) {
 + head.h_smopts = cat(smopts, nalloc(-f, 0));
 + head.h_smopts = cat(head.h_smopts, nalloc(value(from), 0));
 + } else
 + head.h_smopts = smopts;
   mail1(head, 0);
   return(0);
  }
 @@ -307,7 +311,11 @@ sendmail(void *v)
   head.h_subject = NULL;
   head.h_cc = NULL;
   head.h_bcc = NULL;
 - head.h_smopts = NULL;
 + if (value(from)) {
 + head.h_smopts = nalloc(-f, 0);
 + head.h_smopts = cat(head.h_smopts, nalloc(value(from), 0));
 + } else
 + head.h_smopts = NULL;
   mail1(head, 0);
   return(0);
  }
 

I like to be able to specify both the sender name and address.

I've been using the following approach which allows me to specify both name
and address for recipients as well as sender.

  mail -s Lunch? -F Batman bat...@bat.cave -c Robin ro...@bat.cave
-b Al alf...@wayneent.com Jim jgor...@gotham.gov

I like the symmetry of treating all these headers in the same way, and it
can 

Re: Perl build race building .bs files

[Nathanael Rensen]

On Thu, 1 Jan 2015 13:47:59 -0700, Andrew Fresh wrote:

 I had seen complaints but was unable to reproduce the problem myself.
 Nathanael Rensen sent me an email out of the blue with both the likely
 culprit as well as a patch to fix.   A few days after receiving this
 patch, another person emailed me to ask if I knew why the parallel make
 failed.  I forwarded the patch and it was reported back that it worked.

 I'd like more confirmation that this solves the problem, so if you can
 reproduce it reliably, please test it and let me know.

 The patch seems reasonable to me although I am still unable to reproduce
 the failure, but was able to successfully build a release with this patch.

 OKs? Comments?

I can't reproduce the race reliably. For me it occurred six times in around
100 builds since 2014-11-17, when MM_Unix.pm was updated.  It hasn't
occurred in 10 or so builds since I applied the diff.

I've reported the problem upstream. It sounds like there are much bigger
changes on the way for ExtUtils-MakeMaker which might resolve this.

https://github.com/Perl-Toolchain-Gang/ExtUtils-MakeMaker/issues/192

Nathanael

IEEE80211_DEBUG

[Nathanael Rensen]

The IEEE80211_DEBUG kernel option needs a little help to compile.

Index: ieee80211_pae_input.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_pae_input.c,v
retrieving revision 1.19
diff -u -p -r1.19 ieee80211_pae_input.c
--- ieee80211_pae_input.c   22 Jul 2014 11:06:10 -  1.19
+++ ieee80211_pae_input.c   23 Jul 2014 14:25:22 -
@@ -978,7 +978,7 @@ ieee80211_recv_group_msg2(struct ieee802
 
/* discard if we're not expecting this message */
if (ni-ni_rsn_gstate != RSNA_REKEYNEGOTIATING) {
-   DPRINTF((%s: unexpected in state: %d\n, ni-ni_rsn_gstate));
+   DPRINTF((unexpected in state: %d\n, ni-ni_rsn_gstate));
return;
}
if (BE_READ_8(key-replaycnt) != ni-ni_replaycnt) {

Also a plug for a previous diff that may have fallen through the cracks:

http://marc.info/?l=openbsd-techm=140421855720135

Nathanael

sysmerge auto update

[Nathanael Rensen]

Since the switch to SHA256, sysmerge(8) has stopped auto-upgrading files without
local changes.

$ diff -u a b | grep -E '^\+' | sed '1d'
+SHA256 (./etc/rc) = 
0e1cb2e6c6d11941dab14853a8ab1888f1b14fad6588fa3e6230b8ef16b62ffd

$ diff -u a b | grep -E '^\+' | sed '1d' | awk '{print $3}'
=

$ diff -u a b | sed -n 's/^+SHA256 (\(.*\)).*/\1/p'
./etc/rc

Index: sysmerge.sh
===
RCS file: /cvs/src/usr.sbin/sysmerge/sysmerge.sh,v
retrieving revision 1.142
diff -u -p -r1.142 sysmerge.sh
--- sysmerge.sh 18 Jul 2014 10:43:29 -  1.142
+++ sysmerge.sh 20 Jul 2014 07:20:45 -
@@ -194,7 +194,7 @@ sm_populate() {
fi
 
# set auto-upgradable files
-   _D=$(diff -u ${WRKDIR}/${i} ${DESTDIR}/${DBDIR}/${i} | 
grep -E '^\+' | sed '1d' | awk '{print $3}')
+   _D=$(diff -u ${WRKDIR}/${i} ${DESTDIR}/${DBDIR}/${i} | 
sed -n 's/^+SHA256 (\(.*\)).*/\1/p')
for _d in ${_D}; do
# 2/dev/null: if file got removed manually but 
is still in the sum file
CURSUM=$(cd ${DESTDIR:=/}  sha256 ${_d} 
2/dev/null)

Nathanael

tcpdump(8) not printing llc data for ieee80211

[Nathanael Rensen]

Sometimes tcpdump(8) does not print llc  higher layer data when
using -y IEEE802_11 or IEEE802_11_RADIO.

Index: print-802_11.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-802_11.c,v
retrieving revision 1.13
diff -u -p -r1.13 print-802_11.c
--- print-802_11.c  17 Jan 2013 02:53:07 -  1.13
+++ print-802_11.c  19 Jul 2014 03:48:02 -
@@ -153,7 +153,7 @@ ieee80211_data(struct ieee80211_frame *w
u_int8_t *t = (u_int8_t *)wh;
struct ieee80211_frame_addr4 *w4;
u_int datalen;
-   int data = !(wh-i_fc[1]  IEEE80211_FC0_SUBTYPE_NODATA);
+   int data = !(wh-i_fc[0]  IEEE80211_FC0_SUBTYPE_NODATA);
u_char *esrc = NULL, *edst = NULL;
 
TCHECK(*wh);

Nathanael

Modifying queues while pf is disabled

[Nathanael Rensen]

When pf is disabled (pfctl -d), pf_remove_queues() detaches hfsc from the
interface but the queues are not removed from pf_queues_active.

The next time pf rules are loaded, pf_commit_queues() calls pf_remove_queues()
and in turn hfsc_delqueue() for each queue in pf_queues_active. This fails
since hfsc has already been detached.

Example:

# echo 'queue root on em0 bandwidth 2M default'  /tmp/pf.conf
# pfctl -f /tmp/pf.conf
# pfctl -d
# sudo pfctl -f /tmp/pf.conf
pfctl: DIOCXCOMMIT: Invalid argument

Also, if queues are loaded after pf is disabled they become immediately
active rather than remaining inactive until pf is enabled. Example:

# pfctl -Fa
# pfctl -d
# pfctl -f /tmp/pf.conf
# pfctl -vs queue
  [ pkts:  0  bytes:  0  dropped pkts:  0 bytes:  0 ]
  [ qlength:   0/ 50 ]
queue root on em0 bandwidth 2M default qlimit 50
  [ pkts:180  bytes:  25352  dropped pkts:  0 bytes:  0 ]
  [ qlength:   0/ 50 ]

The diff below also allows 'pfctl -vs queue' to show zeros rather than an
error when pf is disabled.

Nathanael

Index: sys/net/pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.272
diff -u -p -r1.272 pf_ioctl.c
--- sys/net/pf_ioctl.c  22 Apr 2014 14:41:03 -  1.272
+++ sys/net/pf_ioctl.c  1 Jul 2014 12:36:08 -
@@ -591,7 +591,7 @@ pf_commit_queues(void)
struct pf_queuehead *qswap;
int error;
 
-   if ((error = pf_remove_queues(NULL)) != 0)
+   if (pf_status.running  (error = pf_remove_queues(NULL)) != 0)
return (error);
 
/* swap */
@@ -600,7 +600,9 @@ pf_commit_queues(void)
pf_queues_inactive = qswap;
pf_free_queues(pf_queues_inactive, NULL);
 
-   return (pf_create_queues());
+   if (pf_status.running  (error = pf_create_queues()) != 0)
+   return (error);
+   return (0);
 }
 
 #define PF_MD5_UPD(st, elm)\
@@ -1004,8 +1006,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
break;
}
bcopy(qs, pq-queue, sizeof(pq-queue));
-   error = hfsc_qstats(qs, pq-buf, nbytes);
-   if (error == 0)
+   pq-nbytes = 0;
+   if (pf_status.running 
+   (error = hfsc_qstats(qs, pq-buf, nbytes)) == 0)
pq-nbytes = nbytes;
break;
}

ntpd(8) losing sync

[Nathanael Rensen]

I noticed that ntpd(8) was regularly reporting it was unsynced, which
in turn impacts down stream clients.

ntpd_adjtime() in usr.sbin/ntpd/ntpd.c uses the olddelta returned by
adjtime(2) to determine whether the previous adjustment was completed.
The clock is considered to be synced only if olddelta is zero. However,
olddelta may be returned using a non-normalised zero, which is not the
zero that ntpd_adjtime() is expecting. ntpd(8) will then report that it
is unsynced, even though the previous adjustment was completed.

The non-normalised zero arises from the adjustment calculation
performed by ntp_update_second() in sys/kern/kern_tc.c. Consider a
-100usec adjustment applied by adjtime(2):
adjtimedelta = { tv_sec = -1, tv_usec = 00 }
ntp_update_second() will then calculate a non-normalised adj:
adj = { tv_sec = 0, tv_usec = -100 }
and call timersub() to apply adj to adjtimedelta. In response to the
non-normalised adj timersub() produces a non-normalised zero:
adjtimedelta = { tv_sec = -1, tv_usec = 100 }.

With the diff below I find ntpd(8) loses sync far less often.

Nathanael

Index: kern_tc.c
===
RCS file: /cvs/src/sys/kern/kern_tc.c,v
retrieving revision 1.21
diff -u -p -r1.21 kern_tc.c
--- kern_tc.c   6 Oct 2013 01:27:49 -   1.21
+++ kern_tc.c   28 Jan 2014 16:12:30 -
@@ -623,9 +623,13 @@ ntp_update_second(int64_t *adjust, time_
adj.tv_usec = -5000;
else if (adjtimedelta.tv_sec == -1)
adj.tv_usec = MAX(-5000, adjtimedelta.tv_usec - 100);
-   timersub(adjtimedelta, adj, adjtimedelta);
*adjust = ((int64_t)adj.tv_usec * 1000)  32;
*adjust += timecounter-tc_freq_adj;
+   if (adj.tv_usec  0) {
+   adj.tv_sec = -1;
+   adj.tv_usec += 100;
+   }
+   timersub(adjtimedelta, adj, adjtimedelta);
 }
 
 int

pflow(4) with optional flowsrc

[Nathanael Rensen]

Some time ago I proposed a diff to allow pflow(4) to determine the src IP
address based on the route table if flowsrc was not specified. That diff
was not accepted because having multiple places look up route tables is
undesirable.

Since then henning@ moved UDP checksum calcs into ip_output. That makes
it very simple to allow the pflow flowsrc parameter to be optional. This
diff permits the flowsrc parameter to be unspecified, as was permitted
prior to version 1.35 of if_flow.c, except that now, thanks to henning@,
it works.

Nathanael


Index: sbin/ifconfig/ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.237
diff -u -p -u -p -r1.237 ifconfig.8
--- sbin/ifconfig/ifconfig.813 Oct 2013 10:45:34 -  1.237
+++ sbin/ifconfig/ifconfig.815 Jan 2014 18:20:18 -
@@ -1224,12 +1224,11 @@ Pflow data will be sent to this address/
 Unset the receiver address and stop sending pflow data.
 .It Cm flowsrc Ar addr
 Set the source IP address for pflow packets.
-Must be defined to export pflow data.
 .Ar addr
 is the IP address used as sender of the UDP packets and may be used to
 identify the source of the data on the pflow collector.
 .It Fl flowsrc
-Unset the source address and stop sending pflow data.
+Unset the source address.
 .It Cm pflowproto Ar n
 Set the protocol version.
 The default is version 5.
Index: sbin/ifconfig/ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.280
diff -u -p -u -p -r1.280 ifconfig.c
--- sbin/ifconfig/ifconfig.c1 Dec 2013 10:05:29 -   1.280
+++ sbin/ifconfig/ifconfig.c15 Jan 2014 18:20:18 -
@@ -3879,8 +3879,9 @@ pflow_status(void)
if (ioctl(s, SIOCGETPFLOW, (caddr_t)ifr) == -1)
 return;
 
-   printf(\tpflow: sender: %s , preq.sender_ip.s_addr != INADDR_ANY ?
-   inet_ntoa(preq.sender_ip) : INVALID);
+   printf(\tpflow: );
+   if (preq.sender_ip.s_addr != INADDR_ANY)
+   printf(sender: %s , inet_ntoa(preq.sender_ip));
printf(receiver: %s:, preq.receiver_ip.s_addr != INADDR_ANY ?
inet_ntoa(preq.receiver_ip) : INVALID);
if (preq.receiver_port == 0)
Index: share/man/man4/pflow.4
===
RCS file: /cvs/src/share/man/man4/pflow.4,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 pflow.4
--- share/man/man4/pflow.4  14 Sep 2013 14:54:30 -  1.16
+++ share/man/man4/pflow.4  15 Jan 2014 18:20:18 -
@@ -42,8 +42,7 @@ Multiple
 interfaces can be created at runtime using the
 .Ic ifconfig pflow Ns Ar N Ic create
 command.
-Each interface must be configured with a flow sender IP address,
-a flow receiver IP address,
+Each interface must be configured with a flow receiver IP address
 and a flow receiver port number.
 .Pp
 Only states created by a rule marked with the
@@ -92,8 +91,6 @@ collector.
 .Cm flowdst
 defines the collector IP address and the port.
 The
-.Cm flowsrc
-IP address and
 .Cm flowdst
 IP address and port must be defined to enable the export of flows.
 .Pp
Index: sys/net/if_pflow.c
===
RCS file: /cvs/src/sys/net/if_pflow.c,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 if_pflow.c
--- sys/net/if_pflow.c  1 Nov 2013 14:34:27 -   1.38
+++ sys/net/if_pflow.c  15 Jan 2014 18:20:23 -
@@ -426,7 +426,6 @@ pflowioctl(struct ifnet *ifp, u_long cmd
if ((ifp-if_flags  IFF_UP) 
sc-sc_receiver_ip.s_addr != INADDR_ANY 
sc-sc_receiver_port != 0 
-   sc-sc_sender_ip.s_addr != INADDR_ANY 
sc-sc_sender_port != 0) {
ifp-if_flags |= IFF_RUNNING;
sc-sc_gcounter=pflowstats.pflow_flows;
@@ -506,7 +505,6 @@ pflowioctl(struct ifnet *ifp, u_long cmd
if ((ifp-if_flags  IFF_UP) 
sc-sc_receiver_ip.s_addr != INADDR_ANY 
sc-sc_receiver_port != 0 
-   sc-sc_sender_ip.s_addr != INADDR_ANY 
sc-sc_sender_port != 0) {
ifp-if_flags |= IFF_RUNNING;
sc-sc_gcounter=pflowstats.pflow_flows;

Re: 802.11 power save fixes

[Nathanael Rensen]

On 10 October 2012 17:54, Mark Kettenis mark.kette...@xs4all.nl wrote:
 Your RT2860-based AP still works reliably with power saving support
 enabled?  If so I'll probably commit that part of the diff as well.

Yes it's working well. Since enabling power saving on my RT2860 AP I
haven't noticed any problems with stations that were previously
working properly and power saving seems to be working.

I do still have a problem with one android phone. Sometimes after a
long period of inactivity (e.g. overnight) it will associate, get an
IP address, and then seems unable to receive traffic. It sends ARP
requests but appears to ignore the response. I overcome that by
disabling and re-enabling wifi on the phone. This may be a problem
with the phone rather than the AP since a second android phone is
unaffected. I haven't made a concerted effort to investigate yet, so
far it's been too tempting to toggle wifi and get on with my day. I'm
not certain it is a power save problem, and for now enabling power
save on the AP makes wifi usable on the phone where previously it was
pretty hopeless.

Thanks,

Nathanael

802.11 power save fixes

[Nathanael Rensen]

I enabled power save for an rt2860 AP using the following diff:

Index: sys/dev/ic/rt2860.c
===
RCS file: /cvs/src/sys/dev/ic/rt2860.c,v
retrieving revision 1.65
diff -u -p -r1.65 sys/dev/ic/rt2860.c
--- sys/dev/ic/rt2860.c 23 Oct 2010 14:24:54 -  1.65
+++ sys/dev/ic/rt2860.c 17 Sep 2012 12:15:41 -
@@ -303,9 +303,7 @@ rt2860_attachhook(void *xsc)
 #ifndef IEEE80211_STA_ONLY
IEEE80211_C_IBSS |  /* IBSS mode supported */
IEEE80211_C_HOSTAP |/* HostAP mode supported */
-#ifdef notyet
IEEE80211_C_APPMGT |/* HostAP power management */
-#endif
 #endif
IEEE80211_C_SHPREAMBLE |/* short preamble supported */
IEEE80211_C_SHSLOT |/* short slot time supported */

I found that some devices (android phones) would eventually fail to
associate. An ifconfig scan on the AP shows that the node would end up
in IEEE80211_STA_COLLECT status with power save enabled. Attempts by
the station to authenticate would then fail because the AP buffered
the authentication responses. The AP refuses to change the power save
status of a node that is not associated, which in turn prevents the
station from associating.

The first part of the solution is to clear the power save mode and
savedq in ieee80211_node_leave:

Index: ieee80211_node.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_node.c,v
retrieving revision 1.74
diff -u -p -r1.74 ieee80211_node.c
--- ieee80211_node.c20 Sep 2012 17:21:13 -  1.74
+++ ieee80211_node.c28 Sep 2012 13:35:52 -
@@ -1604,8 +1604,18 @@ ieee80211_node_leave(struct ieee80211com
return;
}

-   if (ni-ni_pwrsave == IEEE80211_PS_DOZE)
+   if (ni-ni_pwrsave == IEEE80211_PS_DOZE) {
ic-ic_pssta--;
+   ni-ni_pwrsave = IEEE80211_PS_AWAKE;
+   }
+
+#ifndef IEEE80211_STA_ONLY
+   if (!IF_IS_EMPTY(ni-ni_savedq)) {
+   IF_PURGE(ni-ni_savedq);
+   if (ic-ic_set_tim != NULL)
+   (*ic-ic_set_tim)(ic, ni-ni_associd, 0);
+   }
+#endif

if (ic-ic_flags  IEEE80211_F_RSNON)
ieee80211_node_leave_rsn(ic, ni);

This solves the problem providing the node leaves (either by
disassociating or by timeout) before trying to reauthenticate. However
if an associated station crashes while power save is enabled and then
attempts to reauthenticate with the power save flag set the power save
mode would become latched for that node. Normally this situation would
be avoided because a station would not set the power save flag during
authentication, but it can be avoided without relying on the behaviour
of the station by preventing nodes from transitioning directly from
IEEE80211_STA_ASSOC to IEEE80211_STA_AUTH. This has the additional
benefit of preventing a rogue station from forcing an already
associated node back to authenticated state, resulting in a DoS of an
associated station.

Index: ieee80211_proto.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_proto.c,v
retrieving revision 1.46
diff -u -p -r1.46 ieee80211_proto.c
--- ieee80211_proto.c   18 Jan 2012 14:35:56 -  1.46
+++ ieee80211_proto.c   28 Sep 2012 13:35:53 -
@@ -721,7 +721,8 @@ ieee80211_auth_open(struct ieee80211com
ether_sprintf((u_int8_t *)ni-ni_macaddr),
ni-ni_state != IEEE80211_STA_CACHE ?
newly : already);
-   ieee80211_node_newstate(ni, IEEE80211_STA_AUTH);
+   if (ni-ni_state != IEEE80211_STA_ASSOC)
+   ieee80211_node_newstate(ni, IEEE80211_STA_AUTH);
break;
 #endif /* IEEE80211_STA_ONLY */

Nathanael

acpi ioapic related diffs

[Nathanael Rensen]

I have included some diffs below that I have been accumulating.

a) sys/dev/acpi/acpi.c  sys/dev/acpi/acpicpu.c

This diff is to support ACPI in a Xen HVM. I posted this previously
(http://marc.info/?l=openbsd-techm=128120981015035w=2), but my
timing was poor - it was late in a release cycle and deemed too
intrusive at the time.

Index: sys/dev/acpi/acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.224
diff -u -p -u -p -r1.224 acpi.c
--- sys/dev/acpi/acpi.c 27 Apr 2011 20:55:42 -  1.224
+++ sys/dev/acpi/acpi.c 2 Jun 2011 14:37:44 -
@@ -610,15 +610,6 @@ acpi_attach(struct device *parent, struc
}

/*
-* Check if we are able to enable ACPI control
-*/
-   if (!sc-sc_fadt-smi_cmd ||
-   (!sc-sc_fadt-acpi_enable  !sc-sc_fadt-acpi_disable)) {
-   printf(, ACPI control unavailable\n);
-   return;
-   }
-
-   /*
 * Set up a pointer to the firmware control structure
 */
if (sc-sc_fadt-hdr_revision  3 || sc-sc_fadt-x_firmware_ctl == 0)
@@ -696,14 +687,18 @@ acpi_attach(struct device *parent, struc
 * This may prevent thermal control on some systems where
 * that actually does work
 */
-   acpi_write_pmreg(sc, ACPIREG_SMICMD, 0, sc-sc_fadt-acpi_enable);
-   idx = 0;
-   do {
-   if (idx++  ACPIEN_RETRIES) {
-   printf(, can't enable ACPI\n);
-   return;
-   }
-   } while (!(acpi_read_pmreg(sc, ACPIREG_PM1_CNT, 0)  ACPI_PM1_SCI_EN));
+   if (sc-sc_fadt-smi_cmd  sc-sc_fadt-acpi_enable) {
+   acpi_write_pmreg(sc, ACPIREG_SMICMD, 0,
+   sc-sc_fadt-acpi_enable);
+   idx = 0;
+   do {
+   if (idx++  ACPIEN_RETRIES) {
+   printf(, can't enable ACPI\n);
+   return;
+   }
+   } while (!(acpi_read_pmreg(sc, ACPIREG_PM1_CNT, 0) 
+   ACPI_PM1_SCI_EN));
+   }

printf(\n%s: tables, DEVNAME(sc));
SIMPLEQ_FOREACH(entry, sc-sc_tables, q_next) {

Index: sys/dev/acpi/acpicpu.c
===
RCS file: /cvs/src/sys/dev/acpi/acpicpu.c,v
retrieving revision 1.57
diff -u -p -u -p -r1.57 acpicpu.c
--- sys/dev/acpi/acpicpu.c  21 Jul 2010 19:35:15 -  1.57
+++ sys/dev/acpi/acpicpu.c  2 Jun 2011 14:37:44 -
@@ -394,7 +394,8 @@ acpicpu_attach(struct device *parent, st
sc-sc_flags |= FLAGS_NOPCT;
else if (sc-sc_pss_len  0) {
/* Notify BIOS we are handing p-states */
-   if (sc-sc_acpi-sc_fadt-pstate_cnt)
+   if (sc-sc_acpi-sc_fadt-smi_cmd 
+   sc-sc_acpi-sc_fadt-pstate_cnt)
acpi_write_pmreg(sc-sc_acpi, ACPIREG_SMICMD, 0,
sc-sc_acpi-sc_fadt-pstate_cnt);



b) sys/arch/i386/conf/files.i386

arch/i386/i386/hibernate_machdep.c will not compile without acpi
(requires hibernate_resume_machine etc  from
arch/i386/i386/acpi_wakecode.S).

Index: sys/arch/i386/conf/files.i386
===
RCS file: /cvs/src/sys/arch/i386/conf/files.i386,v
retrieving revision 1.203
diff -u -p -u -p -r1.203 files.i386
--- sys/arch/i386/conf/files.i386   23 May 2011 09:54:20 -  1.203
+++ sys/arch/i386/conf/files.i386   2 Jun 2011 14:37:41 -
@@ -24,7 +24,7 @@ file  arch/i386/i386/est.c!small_kernel
 file   arch/i386/i386/gdt.c
 file   arch/i386/i386/in_cksum.s   inet
 file   arch/i386/i386/machdep.c
-file   arch/i386/i386/hibernate_machdep.c
+file   arch/i386/i386/hibernate_machdep.c  acpi  !small_kernel
 file   arch/i386/i386/via.c
 file   arch/i386/i386/amd64errata.c!small_kernel
 file   arch/i386/i386/kgdb_machdep.c   kgdb


c) sys/arch/i386/pci/pci_machdep.c

pci_machdep.c will no longer compile without ioapic.

Index: sys/arch/i386/pci/pci_machdep.c
===
RCS file: /cvs/src/sys/arch/i386/pci/pci_machdep.c,v
retrieving revision 1.62
diff -u -p -u -p -r1.62 pci_machdep.c
--- sys/arch/i386/pci/pci_machdep.c 30 May 2011 19:24:28 -  1.62
+++ sys/arch/i386/pci/pci_machdep.c 2 Jun 2011 14:37:42 -
@@ -587,6 +587,7 @@ not2:
 int
 pci_intr_map_msi(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
 {
+#if NIOAPIC  0
pci_chipset_tag_t pc = pa-pa_pc;
pcitag_t tag = pa-pa_tag;

@@ -598,6 +599,9 @@ pci_intr_map_msi(struct pci_attach_args
ihp-line = APIC_INT_VIA_MSG;
ihp-pin = 0;
return 0;
+#else
+   return 1;
+#endif
 }

 int
@@ -763,6 +767,8 @@ pci_intr_establish(pci_chipset_tag_t pc,
void *ret;
  

ACPI systems without legacy mode

[Nathanael Rensen]

Some ACPI systems do not support legacy mode and do not provide an smi_cmd
port within the FADT. This is mentioned in the following:

http://www.acpi.info/DOWNLOADS/ACPIspec40a.pdf (p119, SMI_CMD)
http://wiki.osdev.org/ACPI (Switching to ACPI Mode)

OpenBSD aborts ACPI initialisation if the FADT does not provide an smi_cmd
port. The diff below permits ACPI initialisation to complete in such a case.
In my case, this allows halt -p to shut down a Xen HVM DomU.

Nathanael


Index: acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.197
diff -p -u -r1.197 acpi.c
--- acpi.c  28 Jul 2010 14:39:43 -  1.197
+++ acpi.c  7 Aug 2010 19:30:21 -
@@ -599,15 +599,6 @@ acpi_attach(struct device *parent, struc
}

/*
-* Check if we are able to enable ACPI control
-*/
-   if (!sc-sc_fadt-smi_cmd ||
-   (!sc-sc_fadt-acpi_enable  !sc-sc_fadt-acpi_disable)) {
-   printf(, ACPI control unavailable\n);
-   return;
-   }
-
-   /*
 * Set up a pointer to the firmware control structure
 */
if (sc-sc_fadt-hdr_revision  3 || sc-sc_fadt-x_firmware_ctl == 0)
@@ -685,14 +676,18 @@ acpi_attach(struct device *parent, struc
 * This may prevent thermal control on some systems where
 * that actually does work
 */
-   acpi_write_pmreg(sc, ACPIREG_SMICMD, 0, sc-sc_fadt-acpi_enable);
-   idx = 0;
-   do {
-   if (idx++  ACPIEN_RETRIES) {
-   printf(, can't enable ACPI\n);
-   return;
-   }
-   } while (!(acpi_read_pmreg(sc, ACPIREG_PM1_CNT, 0)  ACPI_PM1_SCI_EN));
+   if (sc-sc_fadt-smi_cmd  sc-sc_fadt-acpi_enable) {
+   acpi_write_pmreg(sc, ACPIREG_SMICMD, 0,
+   sc-sc_fadt-acpi_enable);
+   idx = 0;
+   do {
+   if (idx++  ACPIEN_RETRIES) {
+   printf(, can't enable ACPI\n);
+   return;
+   }
+   } while (!(acpi_read_pmreg(sc, ACPIREG_PM1_CNT, 0) 
+   ACPI_PM1_SCI_EN));
+   }

printf(\n%s: tables, DEVNAME(sc));
SIMPLEQ_FOREACH(entry, sc-sc_tables, q_next) {
Index: acpicpu.c
===
RCS file: /cvs/src/sys/dev/acpi/acpicpu.c,v
retrieving revision 1.57
diff -p -u -r1.57 acpicpu.c
--- acpicpu.c   21 Jul 2010 19:35:15 -  1.57
+++ acpicpu.c   7 Aug 2010 19:30:22 -
@@ -394,7 +394,8 @@ acpicpu_attach(struct device *parent, st
sc-sc_flags |= FLAGS_NOPCT;
else if (sc-sc_pss_len  0) {
/* Notify BIOS we are handing p-states */
-   if (sc-sc_acpi-sc_fadt-pstate_cnt)
+   if (sc-sc_acpi-sc_fadt-smi_cmd 
+   sc-sc_acpi-sc_fadt-pstate_cnt)
acpi_write_pmreg(sc-sc_acpi, ACPIREG_SMICMD, 0,
sc-sc_acpi-sc_fadt-pstate_cnt);

RT2860 GTK update

[Nathanael Rensen]

I am using a Ralink RT2860 with OpenBSD 4.7 in a Soekris net4801 in
hostap mode as a wireless access point. After a group temporal key
(GTK) update broadcasts stop being received by the attached stations.
If the GTK update reuses the same RT2860 shared key slot then the
problem goes away:

Index: ieee80211_proto.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_proto.c,v
retrieving revision 1.41
diff -u ieee80211_proto.c
--- ieee80211_proto.c   21 Nov 2009 18:09:31 -  1.41
+++ ieee80211_proto.c   5 Jun 2010 07:46:25 -
@@ -448,6 +448,7 @@

/* Swap(GM, GN) */
kid = (ic-ic_def_txkey == 1) ? 2 : 1;
+kid = ic-ic_def_txkey;
k = ic-ic_nw_keys[kid];
memset(k, 0, sizeof(*k));
k-k_id = kid;
@@ -482,6 +483,7 @@

/* install GTK */
kid = (ic-ic_def_txkey == 1) ? 2 : 1;
+kid = ic-ic_def_txkey;
if ((*ic-ic_set_key)(ic, ic-ic_bss, ic-ic_nw_keys[kid]) == 0)
ic-ic_def_txkey = kid;

Note that I'm not suggesting this diff as a fix for the issue, it's
just something I found to work around the problem.

Without the diff above, the GTK is placed initially into the RT2860
shared key slot 1, and then on the first update the new GTK is placed
into slot 2. The old GTK in slot one is not removed and nothing seems
to instruct the RT2860 to switch to the new GTK in slot 2. It may be
that the RT2860 continues to use the old GTK instead of the newly
updated one. However, if that was the case on the next update when the
GTK is placed back in slot 1 broadcasts would start working again,
which is not what I observe.

I have tried to find a proper fix for the problem, but no luck so far.
Does anyone have any suggestions?

Nathanael