Implement VFS read clustering for MSDOSFS, third try

[Stefan Fritsch]

Hi,

this is another try at making read clustering work for msdosfs.

Last year, mpi@ implemented VFS read clustering for MSDOSFS in

sys/msdosfs/denode.h 1.28
sys/msdosfs/msdosfs_vnops.c 1.105

This caused regressions when doing seeks past the end of the file and had.
to be reverted. Then I tried to fix that in

denode.h,v 1.31
msdosfs_vnops.c,v 1.114 

But that caused different problems and had to be reverted, too. I have 
another test for that that I will commit soon.

It seems that while netbsd uses DEV_BSIZE as base for the logical block 
numbers even in files, openbsd is more like freebsd and uses 
mnt_stat.f_iosize in that case. However, if the file system does accesses 
for metadata on the disk, it has to use DEV_BSIZE for accesses. So the 
fixes in 1.31/1.114 were wrong and the following diff is again based on 
1.28/1.105 with some fixes in extendfile() to make seeks past the end of 
the file work.


Testers/reviews/OKs welcome. Especially people who need msdosfs for 
booting via uefi. In any case, I won't commit until I get home again in a 
week.

If my analysis with the block sizes is correct, I will add some 
clarifications to the bread/bwrite man page later.

Cheers,
Stefan


diff --git a/sys/msdosfs/denode.h b/sys/msdosfs/denode.h
index 38ae2027a63..4e521c6d5b8 100644
--- a/sys/msdosfs/denode.h
+++ b/sys/msdosfs/denode.h
@@ -142,7 +142,6 @@ struct denode {
struct vnode *de_devvp; /* vnode of blk dev we live on */
uint32_t de_flag;   /* flag bits */
dev_t de_dev;   /* device where direntry lives */
-   daddr_t de_lastr;
uint32_t de_dirclust;   /* cluster of the directory file containing 
this entry */
uint32_t de_diroffset;  /* offset of this entry in the directory 
cluster */
uint32_t de_fndoffset;  /* offset of found dir entry */
diff --git a/sys/msdosfs/msdosfs_fat.c b/sys/msdosfs/msdosfs_fat.c
index 9e4675fa7e5..bee7c07f4d7 100644
--- a/sys/msdosfs/msdosfs_fat.c
+++ b/sys/msdosfs/msdosfs_fat.c
@@ -1021,14 +1021,12 @@ extendfile(struct denode *dep, uint32_t count, struct 
buf **bpp, uint32_t *ncp,
bp = getblk(pmp->pm_devvp, cntobn(pmp, 
cn++),
pmp->pm_bpcluster, 0, 0);
else {
-   bp = getblk(DETOV(dep), de_cn2bn(pmp, 
frcn++),
+   bp = getblk(DETOV(dep), frcn++,
pmp->pm_bpcluster, 0, 0);
/*
 * Do the bmap now, as in msdosfs_write
 */
-   if (pcbmap(dep,
-   de_bn2cn(pmp, bp->b_lblkno),
-   >b_blkno, 0, 0))
+   if (pcbmap(dep, bp->b_lblkno, 
>b_blkno, 0, 0))
bp->b_blkno = -1;
if (bp->b_blkno == -1)
panic("extendfile: pcbmap");
diff --git a/sys/msdosfs/msdosfs_vnops.c b/sys/msdosfs/msdosfs_vnops.c
index 8452e9be80c..cadaf78920d 100644
--- a/sys/msdosfs/msdosfs_vnops.c
+++ b/sys/msdosfs/msdosfs_vnops.c
@@ -77,13 +77,13 @@
 
 static uint32_t fileidhash(uint64_t);
 
+int msdosfs_bmaparray(struct vnode *, daddr_t, daddr_t *, int *);
 int msdosfs_kqfilter(void *);
 int filt_msdosfsread(struct knote *, long);
 int filt_msdosfswrite(struct knote *, long);
 int filt_msdosfsvnode(struct knote *, long);
 void filt_msdosfsdetach(struct knote *);
 
-
 /*
  * Some general notes:
  *
@@ -509,18 +509,14 @@ int
 msdosfs_read(void *v)
 {
struct vop_read_args *ap = v;
-   int error = 0;
-   uint32_t diff;
-   int blsize;
-   int isadir;
-   uint32_t n;
-   long on;
-   daddr_t lbn, rablock, rablkno;
-   struct buf *bp;
struct vnode *vp = ap->a_vp;
struct denode *dep = VTODE(vp);
struct msdosfsmount *pmp = dep->de_pmp;
struct uio *uio = ap->a_uio;
+   int isadir, error = 0;
+   uint32_t n, diff, size, on;
+   struct buf *bp;
+   daddr_t cn, bn;
 
/*
 * If they didn't ask for any data, then we are done.
@@ -535,7 +531,8 @@ msdosfs_read(void *v)
if (uio->uio_offset >= dep->de_FileSize)
return (0);
 
-   lbn = de_cluster(pmp, uio->uio_offset);
+   cn = de_cluster(pmp, uio->uio_offset);
+   size = pmp->pm_bpcluster;
on = uio->uio_offset & pmp->pm_crbomask;
n = ulmin(pmp->pm_bpcluster - on, uio->uio_resid);
 
@@ -548,31 +545,23 @@ msdosfs_read(void *v)
if (diff < n)
n = diff;
 
-   /* convert cluster # to block # if a directory */
-   if 

Re: ksh(1) history lines allocation

[Jeremie Courreges-Anglas]

On Tue, Aug 15 2017, Rob Pierce  wrote:

[...]

> I was able to reproduce the problem with a HISTSIZE of 10 which at 125000
> entries rendered my system unusable. With the patch I am running fine with a
> HISTSIZE of 12 and have come back several times after hitting the 1.25x
> threshold.
>
> Regression tests pass.

Thanks for your feedback.  As I said privately, I did a bad job at
analyzing how to fix the slowness of afree().  With latest commit to
alloc.c, switching history lines allocation to strdup(3)/free(3) is not
needed any more; also it is probably better for us to use the same
allocation pattern everywhere.

Thanks,
-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

More docs for buffer cache, block sizes, etc.

[Stefan Fritsch]

Hi,

here are some comments / man page updates for things I have learned in my 
adventures through msdosfs and vfs_bio.c. I have also added the 
bread_cluster function to buffercache(9). It would be nice if someone who 
knows the buffer/disk stuff could review this.

Hint for the paragraph on block sizes: In ufs/ffs/ffs_vfsops.c there is:

sbp->f_iosize = fs->fs_bsize;

Then look at the use of fs_bsize in ufs/ffs/fs.h

Cheers,
Stefan

diff --git a/share/man/man9/buffercache.9 b/share/man/man9/buffercache.9
index 84df0c06513..0c4c8f1ad96 100644
--- a/share/man/man9/buffercache.9
+++ b/share/man/man9/buffercache.9
@@ -108,6 +108,7 @@
 .Sh NAME
 .Nm buffercache ,
 .Nm bread ,
+.Nm bread_cluster ,
 .Nm breadn ,
 .Nm bwrite ,
 .Nm bawrite ,
@@ -126,6 +127,9 @@
 .Fn bread "struct vnode *vp" "daddr_t blkno" "int size" \
 "struct buf **bpp"
 .Ft int
+.Fn bread_cluster "struct vnode *vp" "daddr_t blkno" "int size" \
+"struct buf **bpp"
+.Ft int
 .Fn breadn "struct vnode *vp" "daddr_t blkno" "int size" \
 "daddr_t rablks[]" "int rasizes[]" "int nrablks" \
 "struct buf **bpp"
@@ -163,6 +167,11 @@ In addition to describing a cached block, a
 .Em buf
 structure is also used to describe an I/O request as a part of
 the disk driver interface.
+.Pp
+The block size used for logical block numbers depends on the type of the
+given vnode.
+For file vnodes, this is f_iosize of the underlying filesystem.
+For block device vnodes, this will usually be DEV_BSIZE.
 .\" XXX struct buf, B_ flags, MP locks, etc.
 .\" XXX free list, hash queue, etc.
 .\" 
@@ -184,6 +193,10 @@ to allocate a buffer with enough pages for
 .Fa size
 and reads the specified disk block into it.
 .Pp
+.Fn bread
+always returns a buffer, even if it returns an error due to an I/O
+error.
+.Pp
 The buffer returned by
 .Fn bread
 is marked as busy.
@@ -222,6 +235,30 @@ and
 The read-ahead blocks aren't returned, but are available in cache for
 future accesses.
 .\" - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+.It Xo
+.Fo bread_cluster
+.Fa "vp"
+.Fa "blkno"
+.Fa "size"
+.Fa "bpp"
+.Fc
+.Xc
+Read a block of size
+.Fa "size"
+corresponding to
+.Fa vp
+and
+.Fa blkno ,
+with readahead.
+If neither the first block, nor a part of the next MAXBSIZE bytes is already
+in the buffer cache,
+.Fn bread_cluster
+will perform a read-ahead of MAXBSIZE bytes in a single I/O operation.
+This is currently more efficient than
+.Fn breadn .
+The read-ahead data isn't returned, but is available in cache for
+future accesses.
+.\" - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 .It Fn bwrite "bp"
 Write a block.
 Start I/O for write using
diff --git a/sys/kern/vfs_bio.c b/sys/kern/vfs_bio.c
index 88adfeff237..4804d370f49 100644
--- a/sys/kern/vfs_bio.c
+++ b/sys/kern/vfs_bio.c
@@ -568,6 +568,12 @@ bread_cluster_callback(struct buf *bp)
}
 }
 
+/*
+ * Read-ahead multiple disk blocks, but make sure only one (big) I/O
+ * request is sent to the disk.
+ * XXX This should probably be dropped and breadn should instead be optimized
+ * XXX to do fewer I/O requests.
+ */
 int
 bread_cluster(struct vnode *vp, daddr_t blkno, int size, struct buf **rbpp)
 {
@@ -1023,6 +1029,9 @@ geteblk(size_t size)
 
 /*
  * Allocate a buffer.
+ * If vp is given, put it into the buffer cache for that vnode.
+ * If size != 0, allocate memory and call buf_map().
+ * If there is already a buffer for the given vnode/blkno, return NULL.
  */
 struct buf *
 buf_get(struct vnode *vp, daddr_t blkno, size_t size)

hfsc_deferred race

[Mike Belopuhov]

Hi,

I've just triggered an assert in hfsc_deferred (a callout) on an
MP kernel running on an SP virtual machine:

  panic: kernel diagnostic assertion "HFSC_ENABLED(ifq)" failed: file 
"/home/mike/src/openbsd/sys/net/hfsc.c", line 950
  Stopped at  db_enter+0x9:   leave
  TIDPIDUID PRFLAGS PFLAGS  CPU  COMMAND
  *247463  28420  0 0x3  00  pfctl
  db_enter() at db_enter+0x9
  
panic(817f78f0,4,81a3ffc0,8110c140,800c2060,fff
  f81598b1c) at panic+0x102
  __assert(81769d93,817d7350,3b6,817d72bd) at 
__assert+0x
  35
  hfsc_deferred(800c2060) at hfsc_deferred+0x9e
  timeout_run(8004adc8) at timeout_run+0x4c
  softclock(0) at softclock+0x146
  softintr_dispatch(0) at softintr_dispatch+0x9f
  Xsoftclock() at Xsoftclock+0x1f
  --- interrupt ---
  end of kernel
  end trace frame: 0x728d481974c08548, count: 7
  0x2cfe9c031c9:
  https://www.openbsd.org/ddb.html describes the minimum info required in bug
  reports.  Insufficient info makes it difficult to find and fix bugs.
  ddb{0}> ps
 PID TID   PPIDUID  S   FLAGS  WAIT  COMMAND
  *28420  247463   5000  0  7 0x3pfctl


pfctl runs in the loop reloading the ruleset.  So at some point we
disable HFSC on the interface but lose a race with hfsc_deferred
before re-enabling it.

IFQ has a mechanism to lock the underlying object and I believe this
is the right tool for this job.  Any other ideas?

I don't think it's a good idea to hold the mutex (ifq_q_enter and
ifq_q_leave effectively lock and unlock it) during the ifq_start,
so we have to make a concession and run the ifq_start before knowing
whether or not HFSC is attached.  IMO, it's a small price to pay to
avoide clutter.  Kernel lock assertion is pointless at this point.

OK?

diff --git sys/net/hfsc.c sys/net/hfsc.c
index 410bea733c6..3c5b6f6ef78 100644
--- sys/net/hfsc.c
+++ sys/net/hfsc.c
@@ -944,20 +944,19 @@ hfsc_deferred(void *arg)
 {
struct ifnet *ifp = arg;
struct ifqueue *ifq = >if_snd;
struct hfsc_if *hif;
 
-   KERNEL_ASSERT_LOCKED();
-   KASSERT(HFSC_ENABLED(ifq));
-
if (!ifq_empty(ifq))
ifq_start(ifq);
 
-   hif = ifq->ifq_q;
-
+   hif = ifq_q_enter(>if_snd, ifq_hfsc_ops);
+   if (hif == NULL)
+   return;
/* XXX HRTIMER nearest virtual/fit time is likely less than 1/HZ. */
timeout_add(>hif_defer, 1);
+   ifq_q_leave(>if_snd, hif);
 }
 
 void
 hfsc_cl_purge(struct hfsc_if *hif, struct hfsc_class *cl, struct mbuf_list *ml)
 {

Skipping amd errata on hypervisors?

[Stefan Fritsch]

I have got a report that openbsd panics on boot with qemu -cpu Opteron_G3 
(but Opteron_G2 works).

kernel: protection fault trap, code=0
Stopped at  amd64_errata_setmsr+0x14:   rdmsr
ddb{0}> >> OpenBSD/amd64 BOOT 3.33
boot>

Qemu does not implement all the secret errata MSRs. Does it make sense to 
simply skip all errata processing if we detect a hypervisor?


diff --git a/sys/arch/amd64/amd64/identcpu.c b/sys/arch/amd64/amd64/identcpu.c
index a448b885ba7..371c0c8ff48 100644
--- a/sys/arch/amd64/amd64/identcpu.c
+++ b/sys/arch/amd64/amd64/identcpu.c
@@ -708,7 +708,7 @@ identifycpu(struct cpu_info *ci)
}
 #endif
 
-   if (!strcmp(cpu_vendor, "AuthenticAMD"))
+   if (!strcmp(cpu_vendor, "AuthenticAMD") && !ISSET(cpu_ecxfeature, 
CPUIDECX_HV))
amd64_errata(ci);
 
if (CPU_IS_PRIMARY(ci) && !strcmp(cpu_vendor, "CentaurHauls")) {

Re: Skipping amd errata on hypervisors?

[Theo de Raadt]

>I have got a report that openbsd panics on boot with qemu -cpu Opteron_G3 
>(but Opteron_G2 works).
>
>kernel: protection fault trap, code=0
>Stopped at  amd64_errata_setmsr+0x14:   rdmsr
>ddb{0}> >> OpenBSD/amd64 BOOT 3.33
>boot>
>
>Qemu does not implement all the secret errata MSRs. Does it make sense to 
>simply skip all errata processing if we detect a hypervisor?

doctor doctor it hurts when i apply the crazy options

so don't apply the crazy options!

If all software at all levels had to permanently deal with current,
future, and past bugs in all other software, eventually our universe
would collapse in upon itself.

Re: Implement VFS read clustering for MSDOSFS, third try

[Martijn Rijkeboer]

On 08/15/17 17:20, Stefan Fritsch wrote:
> Hi,
> 
> this is another try at making read clustering work for msdosfs.
> 
> Last year, mpi@ implemented VFS read clustering for MSDOSFS in
> 
> sys/msdosfs/denode.h 1.28
> sys/msdosfs/msdosfs_vnops.c 1.105
> 
> This caused regressions when doing seeks past the end of the file and had.
> to be reverted. Then I tried to fix that in
> 
> denode.h,v 1.31
> msdosfs_vnops.c,v 1.114 
> 
> But that caused different problems and had to be reverted, too. I have 
> another test for that that I will commit soon.
> 
> It seems that while netbsd uses DEV_BSIZE as base for the logical block 
> numbers even in files, openbsd is more like freebsd and uses 
> mnt_stat.f_iosize in that case. However, if the file system does accesses 
> for metadata on the disk, it has to use DEV_BSIZE for accesses. So the 
> fixes in 1.31/1.114 were wrong and the following diff is again based on 
> 1.28/1.105 with some fixes in extendfile() to make seeks past the end of 
> the file work.
> 
> 
> Testers/reviews/OKs welcome. Especially people who need msdosfs for 
> booting via uefi. In any case, I won't commit until I get home again in a 
> week.
> 
> If my analysis with the block sizes is correct, I will add some 
> clarifications to the bread/bwrite man page later.

Since I was the one who raised the second problem [0], I tried this
patch and everything seems to work fine. When I copy a binary file
on a msdos (uefi) partition (running the kernel with the patch) the
checksum remains the same:

# cp refind_x64.efi bootx64.efi
# ls -l refind_x64.efi bootx64.efi
-rw-r--r--  1 root  wheel  230856 Aug 15 19:49 bootx64.efi
-rw-r--r--  1 root  wheel  230856 Mar  9 11:09 refind_x64.efi
# sha256 refind_x64.efi bootx64.efi
SHA256 (refind_x64.efi) =
968596f40353ab8140867e71928fcd4c0bc2f39397aba8324f7255bcd57f0913
SHA256 (bootx64.efi) =
968596f40353ab8140867e71928fcd4c0bc2f39397aba8324f7255bcd57f0913


Kind regards,


Martijn Rijkeboer


[0] http://marc.info/?l=openbsd-tech=149725838018944=2

Re: [patch] Add -z and -Z to apmd for automatic suspend/hibernate

[Jesper Wallin]

On Mon, Aug 14, 2017 at 05:12:05PM +0200, Klemens Nanni wrote:
> Personally I'd also prefer having this in apmd(8) rather than some other
> daemon or script. Some comments:
> 
> You should pass optarg instead of errstr to error(). Either ways error()
> will still append since it uses err(3). This leads to
> 
>   $ obj/apmd -dz0
>   apmd: invalid percent: too small: Result too large
> 
> apmd.8's SYNOPSIS does not reflect your changes.

Oh, I completely forgot about the SYNOPSIS, my apologies.

I also went with tedu@'s suggestion for errc(3), but only in the
getopt-loop, as I would most likely do more harm than good trying to
rewrite error().

The syslog(3) line, when suspending, was too long and corrected now.


Jesper Wallin


Index: usr.sbin/apmd/apmd.8
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.8,v
retrieving revision 1.47
diff -u -p -r1.47 apmd.8
--- usr.sbin/apmd/apmd.812 Feb 2015 14:03:49 -  1.47
+++ usr.sbin/apmd/apmd.815 Aug 2017 14:57:31 -
@@ -38,6 +38,8 @@
 .Op Fl f Ar devname
 .Op Fl S Ar sockname
 .Op Fl t Ar seconds
+.Op Fl z Ar percent
+.Op Fl Z Ar percent
 .Sh DESCRIPTION
 .Nm
 monitors the advanced power management device,
@@ -113,6 +115,20 @@ The polling rate defaults to
 once per 10 minutes, but may be specified using the
 .Fl t
 command-line flag.
+.It Fl z Ar percent
+Automatically suspend the system if no AC is connected and the
+estimated battery life is equal or below
+.Ar percent .
+.It Fl Z Ar percent
+Automatically hibernate the system if no AC is connected and the
+estimated battery life is equal or below
+.Ar percent .
+.Pp
+If both 
+.Fl Z
+and
+.Fl z
+are specified, the first one will supersede the other.
 .El
 .Pp
 When a client requests a suspend or stand-by state,
Index: usr.sbin/apmd/apmd.c
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.79
diff -u -p -r1.79 apmd.c
--- usr.sbin/apmd/apmd.c16 Nov 2015 17:35:05 -  1.79
+++ usr.sbin/apmd/apmd.c15 Aug 2017 14:57:31 -
@@ -56,6 +56,9 @@
 #define TRUE 1
 #define FALSE 0
 
+#define AUTO_SUSPEND 1
+#define AUTO_HIBERNATE 2
+
 const char apmdev[] = _PATH_APM_CTLDEV;
 const char sockfile[] = _PATH_APM_SOCKET;
 
@@ -94,8 +97,8 @@ void
 usage(void)
 {
fprintf(stderr,
-   "usage: %s [-AadHLs] [-f devname] [-S sockname] [-t seconds]\n",
-   __progname);
+   "usage: %s [-AadHLs] [-f devname] [-S sockname] [-t seconds] "
+   "[-z percent] [-Z percent]\n", __progname);
exit(1);
 }
 
@@ -348,6 +351,8 @@ main(int argc, char *argv[])
 {
const char *fname = apmdev;
int ctl_fd, sock_fd, ch, suspends, standbys, hibernates, resumes;
+   int autoaction = 0;
+   int autolimit = 0;
int statonly = 0;
int powerstatus = 0, powerbak = 0, powerchange = 0;
int noacsleep = 0;
@@ -355,13 +360,14 @@ main(int argc, char *argv[])
struct apm_power_info pinfo;
time_t apmtimeout = 0;
const char *sockname = sockfile;
+   const char *errstr;
int kq, nchanges;
struct kevent ev[2];
int ncpu_mib[2] = { CTL_HW, HW_NCPU };
int ncpu;
size_t ncpu_sz = sizeof(ncpu);
 
-   while ((ch = getopt(argc, argv, "aACdHLsf:t:S:")) != -1)
+   while ((ch = getopt(argc, argv, "aACdHLsf:t:S:z:Z:")) != -1)
switch(ch) {
case 'a':
noacsleep = 1;
@@ -402,6 +408,20 @@ main(int argc, char *argv[])
doperf = PERF_MANUAL;
setperfpolicy("high");
break;
+   case 'z':
+   autoaction = AUTO_SUSPEND;
+   autolimit = strtonum(optarg, 1, 100, );
+   if (errstr != NULL)
+   errc(1, EINVAL, "%s percentage: %s", errstr,
+   optarg);
+   break;
+   case 'Z':
+   autoaction = AUTO_HIBERNATE;
+   autolimit = strtonum(optarg, 1, 100, );
+   if (errstr != NULL)
+   errc(1, EINVAL, "%s percentage: %s", errstr,
+   optarg);
+   break;
case '?':
default:
usage();
@@ -479,6 +499,21 @@ main(int argc, char *argv[])
if (powerstatus != powerbak) {
powerstatus = powerbak;
powerchange = 1;
+   }
+
+   if (!powerstatus && autoaction &&
+   autolimit > (int)pinfo.battery_life) {
+   syslog(LOG_NOTICE,
+   "estimated battery life %d%%, "
+   

Re: Skipping amd errata on hypervisors?

[Mike Larkin]

On Tue, Aug 15, 2017 at 05:39:29PM +0200, Stefan Fritsch wrote:
> I have got a report that openbsd panics on boot with qemu -cpu Opteron_G3 
> (but Opteron_G2 works).
> 
> kernel: protection fault trap, code=0
> Stopped at  amd64_errata_setmsr+0x14:   rdmsr
> ddb{0}> >> OpenBSD/amd64 BOOT 3.33
> boot>
> 
> Qemu does not implement all the secret errata MSRs. Does it make sense to 
> simply skip all errata processing if we detect a hypervisor?
> 
> 
> diff --git a/sys/arch/amd64/amd64/identcpu.c b/sys/arch/amd64/amd64/identcpu.c
> index a448b885ba7..371c0c8ff48 100644
> --- a/sys/arch/amd64/amd64/identcpu.c
> +++ b/sys/arch/amd64/amd64/identcpu.c
> @@ -708,7 +708,7 @@ identifycpu(struct cpu_info *ci)
>   }
>  #endif
>  
> - if (!strcmp(cpu_vendor, "AuthenticAMD"))
> + if (!strcmp(cpu_vendor, "AuthenticAMD") && !ISSET(cpu_ecxfeature, 
> CPUIDECX_HV))
>   amd64_errata(ci);
>  
>   if (CPU_IS_PRIMARY(ci) && !strcmp(cpu_vendor, "CentaurHauls")) {
> 

I think this is an upstream bug. If they claim to emulate a G3 Opteron, they
should be faithfully emulating it.

Re: ifstated: stop tracking interface indexes

[Rob Pierce]

On Tue, Aug 15, 2017 at 02:37:22PM -0400, Jeremie Courreges-Anglas wrote:
> On Tue, Aug 15 2017, Rob Pierce  wrote:
> > On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
> >> On Mon, Aug 14 2017, Rob Pierce  wrote:
> >> > ifstated currently tracks and maintains the index of each monitored 
> >> > interface
> >> > and does not maintain interface names. This means we need to re-index on
> >> > interface departure and arrival.
> >> >
> >> > The following diff moves away from indexes to names. Indexes are still 
> >> > required,
> >> > but easily obtained dynamically as needed. This helps simplify the next 
> >> > diff
> >> > that will provide support for interface departure and arrival.
> >> >
> >> > Suggested by deraadt.
> >> >
> >> > No intended functional change. Regress tests pass.
> >> >
> >> > Ok?
> >> 
> >> The idea looks sound to me, however I would keep the "interface" symbol
> >> in parse.y (your diff doesn't remove all "interface" references btw).
> >> 
> >> The current code checks the existence of the interface at startup.  If
> >> the interface doesn't exists, you get a syntax error.  This could happen
> >> because of a missing interface (an interesting case), or because of
> >> a typo.  Whether or not we're erroring out, it is nice to print
> >> a diagnostic message.
> >> 
> >> I'm not sure this change was intended, so here's a tentative diff that
> >> that keeps the existing behavior.  Regress tests pass.
> >
> > Yes, I see that now. That change was not intended. Thanks!
> >
> > Your diff looks good.
> >
> > Ok?
> 
> Please use four spaces, not three, for second level indents; see
> style(9).
> 
> There's a check that should be fixed, please see below.

Ok, will do. Not sure why I started using three spaces for 2nd level indents...

> With those items addressed, ok jca@
> 
> It feels a bit weird to reject unknown interface names at startup but to
> cope with departed/joining interfaces at runtime.  But I guess we'll
> have to live with this.

I don't disagree. I am most concerned about handling the departure condition
more appropriately, which is (in my opinion) currently broken.

As discussed, it is an option to do the right thing on departure (i.e. demote
with a state change to down) and then fail hard, which would mirror the
(current) startup behaviour more closely (i.e. fail on startup due to the
interfaces not being present).

I suppose we could also allow startup with unknown interfaces (present in the
configuration file) to occur (which would be a change in behaviour), and simply
mark those interfaces as down and immediately demote but keep running, like we
do for an interface that is present at startup but in a down state.

This diff does not yet introduce interface departure and arrival, and with
your updates will not change any behaviour, and I think this change makes
sense regardless of which approach is taken in the next step.

Thank you for your thorough review and comments. I will update my diff
accordingly and wait a few days for any further comments before committing.

Regards,

Rob

> >> Index: ifstated.c
> >> ===
> >> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
> >> retrieving revision 1.59
> >> diff -u -p -r1.59 ifstated.c
> >> --- ifstated.c 14 Aug 2017 03:15:28 -  1.59
> >> +++ ifstated.c 15 Aug 2017 03:04:47 -
> >> @@ -61,8 +61,8 @@ void external_handler(int, short, void 
> >>  void  external_exec(struct ifsd_external *, int);
> >>  void  check_external_status(struct ifsd_state *);
> >>  void  external_evtimer_setup(struct ifsd_state *, int);
> >> -void  scan_ifstate(int, int, int);
> >> -int   scan_ifstate_single(int, int, struct ifsd_state *);
> >> +void  scan_ifstate(const char *, int, int);
> >> +int   scan_ifstate_single(const char *, int, struct 
> >> ifsd_state *);
> >>  void  fetch_ifstate(int);
> >>  __dead void   usage(void);
> >>  void  adjust_expressions(struct ifsd_expression_list *, int);
> >> @@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
> >>char msg[2048];
> >>struct rt_msghdr *rtm = (struct rt_msghdr *)
> >>struct if_msghdr ifm;
> >> +  char ifnamebuf[IFNAMSIZ];
> >> +  char *ifname;
> >>ssize_t len;
> >>  
> >>if ((len = read(fd, msg, sizeof(msg))) == -1) {
> >> @@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
> >>switch (rtm->rtm_type) {
> >>case RTM_IFINFO:
> >>memcpy(, rtm, sizeof(ifm));
> >> -  scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
> >> +  ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
> >> +  /* ifname is NULL on interface departure */
> >> +  if (ifname != NULL)
> >> +  scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
> >>

Re: ksh(1) history lines allocation

[Rob Pierce]

On Tue, Aug 15, 2017 at 02:03:43PM -0400, Jeremie Courreges-Anglas wrote:
> On Tue, Aug 15 2017, Rob Pierce  wrote:
> 
> [...]
> 
> > I was able to reproduce the problem with a HISTSIZE of 10 which at 
> > 125000
> > entries rendered my system unusable. With the patch I am running fine with a
> > HISTSIZE of 12 and have come back several times after hitting the 1.25x
> > threshold.
> >
> > Regression tests pass.
> 
> Thanks for your feedback.  As I said privately, I did a bad job at
> analyzing how to fix the slowness of afree().  With latest commit to
> alloc.c, switching history lines allocation to strdup(3)/free(3) is not
> needed any more; also it is probably better for us to use the same
> allocation pattern everywhere.

Sounds good. Your latest commit passed my local tests with a large HISTSIZE.

Regards,

Rob

> Thanks,
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ifstated: stop tracking interface indexes

[Jeremie Courreges-Anglas]

On Tue, Aug 15 2017, Rob Pierce  wrote:
> On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
>> On Mon, Aug 14 2017, Rob Pierce  wrote:
>> > ifstated currently tracks and maintains the index of each monitored 
>> > interface
>> > and does not maintain interface names. This means we need to re-index on
>> > interface departure and arrival.
>> >
>> > The following diff moves away from indexes to names. Indexes are still 
>> > required,
>> > but easily obtained dynamically as needed. This helps simplify the next 
>> > diff
>> > that will provide support for interface departure and arrival.
>> >
>> > Suggested by deraadt.
>> >
>> > No intended functional change. Regress tests pass.
>> >
>> > Ok?
>> 
>> The idea looks sound to me, however I would keep the "interface" symbol
>> in parse.y (your diff doesn't remove all "interface" references btw).
>> 
>> The current code checks the existence of the interface at startup.  If
>> the interface doesn't exists, you get a syntax error.  This could happen
>> because of a missing interface (an interesting case), or because of
>> a typo.  Whether or not we're erroring out, it is nice to print
>> a diagnostic message.
>> 
>> I'm not sure this change was intended, so here's a tentative diff that
>> that keeps the existing behavior.  Regress tests pass.
>
> Yes, I see that now. That change was not intended. Thanks!
>
> Your diff looks good.
>
> Ok?

Please use four spaces, not three, for second level indents; see
style(9).

There's a check that should be fixed, please see below.

With those items addressed, ok jca@

It feels a bit weird to reject unknown interface names at startup but to
cope with departed/joining interfaces at runtime.  But I guess we'll
have to live with this.

>> Index: ifstated.c
>> ===
>> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
>> retrieving revision 1.59
>> diff -u -p -r1.59 ifstated.c
>> --- ifstated.c   14 Aug 2017 03:15:28 -  1.59
>> +++ ifstated.c   15 Aug 2017 03:04:47 -
>> @@ -61,8 +61,8 @@ void   external_handler(int, short, void 
>>  voidexternal_exec(struct ifsd_external *, int);
>>  voidcheck_external_status(struct ifsd_state *);
>>  voidexternal_evtimer_setup(struct ifsd_state *, int);
>> -voidscan_ifstate(int, int, int);
>> -int scan_ifstate_single(int, int, struct ifsd_state *);
>> +voidscan_ifstate(const char *, int, int);
>> +int scan_ifstate_single(const char *, int, struct ifsd_state *);
>>  voidfetch_ifstate(int);
>>  __dead void usage(void);
>>  voidadjust_expressions(struct ifsd_expression_list *, int);
>> @@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
>>  char msg[2048];
>>  struct rt_msghdr *rtm = (struct rt_msghdr *)
>>  struct if_msghdr ifm;
>> +char ifnamebuf[IFNAMSIZ];
>> +char *ifname;
>>  ssize_t len;
>>  
>>  if ((len = read(fd, msg, sizeof(msg))) == -1) {
>> @@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
>>  switch (rtm->rtm_type) {
>>  case RTM_IFINFO:
>>  memcpy(, rtm, sizeof(ifm));
>> -scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
>> +ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
>> +/* ifname is NULL on interface departure */
>> +if (ifname != NULL)
>> +scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
>>  break;
>>  case RTM_DESYNC:
>>  fetch_ifstate(1);
>> @@ -431,7 +436,7 @@ external_evtimer_setup(struct ifsd_state
>>  #define LINK_STATE_IS_DOWN(_s)  (!LINK_STATE_IS_UP((_s)))
>>  
>>  int
>> -scan_ifstate_single(int ifindex, int s, struct ifsd_state *state)
>> +scan_ifstate_single(const char *ifname, int s, struct ifsd_state *state)
>>  {
>>  struct ifsd_ifstate *ifstate;
>>  struct ifsd_expression_list expressions;
>> @@ -440,7 +445,7 @@ scan_ifstate_single(int ifindex, int s, 
>>  TAILQ_INIT();
>>  
>>  TAILQ_FOREACH(ifstate, >interface_states, entries) {
>> -if (ifstate->ifindex == ifindex) {
>> +if (strcmp(ifstate->ifname, ifname) == 0) {
>>  if (ifstate->prevstate != s &&
>>  (ifstate->prevstate != -1 || !opt_inhibit)) {
>>  struct ifsd_expression *expression;
>> @@ -472,15 +477,15 @@ scan_ifstate_single(int ifindex, int s, 
>>  }
>>  
>>  void
>> -scan_ifstate(int ifindex, int s, int do_eval)
>> +scan_ifstate(const char *ifname, int s, int do_eval)
>>  {
>>  struct ifsd_state *state;
>>  int cur_eval = 0;
>>  
>> -if (scan_ifstate_single(ifindex, s, >initstate) && do_eval)
>> +if (scan_ifstate_single(ifname, s, >initstate) && do_eval)
>>  eval_state(>initstate);
>>  

Re: [patch] Add -z and -Z to apmd for automatic suspend/hibernate

[Ingo Schwarze]

Hi Ted,

Ted Unangst wrote on Tue, Aug 15, 2017 at 07:42:39PM -0400:
> Jesper Wallin wrote:
>> On Sun, Aug 13, 2017 at 09:52:22AM +0200, Martijn van Duren wrote:

>>> I've also been bitten by this a couple of times, but you can also solve
>>> this via the sensorsd framework, which is how I've done it.

>> Yeah, someone on IRC also suggested sensorsd or even ksh and a cronjob.
>> I personally find it a bit too ducttapey though, especially for a
>> feature one would expect on a laptop. This also saves me from running an
>> extra daemon just in case my battery runs out.

> as regards the sensorsd approach, i've known about that possibility since
> forever, and think about it every time i find my laptop has a dead battery,
> but have never quite been motivated enough to set it up. i've heard the same
> from several others. to me this suggests a certain usability hurdle.

Recently, i looked at sensorsd because my laptop has a power supply with
a cable that keeps falling out, and the machine itself has no LEDs.

So when the cable falls out again, my screen turns red now.  When
configuring that, i noticed and fixed some inaccuracies and fuzzy
wordings in the manual pages, but i agree that this is still less
intuitive than the average OpenBSD utility.

Would adding something like the following to sensorsd.conf(5) EXAMPLES
lower the barrier?  It does look hackish, but i think that's due to the
design of sensorsd and hard to implement more cleanly.

  schwarze@isnote $ cat /etc/sensorsd.conf
  hw.sensors.acpiac0.indicator0:low=1:command=/etc/sensorsd/acpiac %2

  schwarze@isnote $ cat /etc/sensorsd/acpibat
  #!/bin/sh
  export XAUTHORITY=/home/schwarze/.Xauthority
  if [ "X$1" = "X2" ]; then
xsetroot -display :0 -def
  else
xsetroot -display :0 -solid red
  fi
  exit 0

Yours,
  Ingo

Re: llvm - xor return pointers

[Ori Bernstein]

On Sat, 22 Jul 2017 02:25:29 -0400
Todd Mortimer  wrote:

> xor [rsp], rsp
> 
> at the start of each function, and before every RET.

Wouldn't this break with alloca() or C99 VLAs?
%rbp may work better, if the frame pointer is
retained.

-- 
Ori Bernstein 

Re: CID 1452909: Use of untrusted scalar value (pf_table.c)

[Jonathan Gray]

On Tue, Aug 15, 2017 at 02:40:32PM +0200, Mike Belopuhov wrote:
> Hi,
> 
> Coverity has discovered that we're blindly trusting the value
> of pfra_type that we read from the userland supplied pfr_addr
> and use it to index an array of pools in pfr_create_kentry.
> 
> I suggest to do two things: add a check in pfr_validate_addr
> that is called after every copyin and also perform the check
> in pfr_create_kentry before we attempt to use the value.
> 
> OK?

ok jsg@

Default TLS configuration of httpd / libtls

[Andreas Bartelt]

After responding to a question on misc@ ( 
http://marc.info/?l=openbsd-misc=150280482525307=2 ), I've noticed 
that the part of my response with regard to default-enabled TLS cipher 
suites on current was wrong. I was testing with an ECDSA-based instead 
of an RSA-based certificate which renders the set of enabled cipher 
suites significantly shorter.


In case an RSA-based certificate is used with httpd on current, the 
following cipher suites are currently enabled by default:

TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
OLD_TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
TLS_RSA_WITH_AES_256_GCM_SHA384
TLS_RSA_WITH_AES_256_CBC_SHA256
TLS_RSA_WITH_AES_256_CBC_SHA
TLS_RSA_WITH_CAMELLIA_256_CBC_SHA256
TLS_RSA_WITH_CAMELLIA_256_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
TLS_RSA_WITH_AES_128_GCM_SHA256
TLS_RSA_WITH_AES_128_CBC_SHA256
TLS_RSA_WITH_AES_128_CBC_SHA
TLS_RSA_WITH_CAMELLIA_128_CBC_SHA256
TLS_RSA_WITH_CAMELLIA_128_CBC_SHA
TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA

I am surprised why the set of default-enabled cipher suites is so large. 
According to httpd.conf(5), it maps to "HIGH:!aNULL" which corresponds 
to "compat" in libtls's tls_config_set_ciphers(). I then did some more 
tests on the available cipher suite keywords/profiles in libtls (the 
following discussion is based on tests with an RSA-based certificate).


tls_config_set_ciphers defines the following 4 profiles:
1) secure / default
since RFC 7905 is over a year old now, shouldn't 
OLD_TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 be removed from this set?


2) compat
3) legacy

These two profiles currently only differ by one cipher suite which is 
TLS_RSA_WITH_3DES_EDE_CBC_SHA in 'legacy'. However, 'compat' also 
contains a 3DES cipher suite which is 
TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA -- I guess this one should not be 
classified as "HIGH" and only be listed in 'legacy' (since security 
level <128 bits) but not in 'compat'.


It's not clear to me at which point in time CAMELLIA cipher suites have 
been added to 'compat' and 'legacy'. There was a previous discussion on 
tech@ about adding them to libssl/libtls as an option but not enabling 
them by default ( http://marc.info/?l=openbsd-tech=147213883819627=2 
). I don't find a commit which explicitly enabled CAMELLIA cipher suites 
by default -- did they accidentally sneak into httpd's default TLS 
config due to the use of openssl's ciphers strings?


In case an ECDSA-based certificate is used, the 'compat' profile 
implicitly corresponds to ECDHE-only cipher suites (since all 
ECDSA-based cipher suites provide forward secrecy). Wouldn't it make 
sense to also remove all RSA key transport based cipher suites from 
'compat' in the case of RSA-based certificates? In case RSA key 
transport cipher suites would remain in 'legacy', this would also 
provide better differentiation between these two profiles.


4) insecure / all

Since this profile includes RC4 and DES cipher suites, I guess we all 
agree that these should never be used. In case someone actually wants to 
enable dangerous stuff in libtls, direct use of openssl's ALL ciphers 
string could still be used in order to provide the same damage. Is there 
a sufficiently good reason to keep these keywords in libtls at all?


Best regards
Andreas

Re: [patch] Add -z and -Z to apmd for automatic suspend/hibernate

[Ted Unangst]

Jesper Wallin wrote:
> On Mon, Aug 14, 2017 at 05:12:05PM +0200, Klemens Nanni wrote:
> > Personally I'd also prefer having this in apmd(8) rather than some other
> > daemon or script. Some comments:
> > 
> > You should pass optarg instead of errstr to error(). Either ways error()
> > will still append since it uses err(3). This leads to
> > 
> > $ obj/apmd -dz0
> > apmd: invalid percent: too small: Result too large
> > 
> > apmd.8's SYNOPSIS does not reflect your changes.
> 
> Oh, I completely forgot about the SYNOPSIS, my apologies.
> 
> I also went with tedu@'s suggestion for errc(3), but only in the
> getopt-loop, as I would most likely do more harm than good trying to
> rewrite error().
> 
> The syslog(3) line, when suspending, was too long and corrected now.

this looks good. will commit soon.

(one nit, i'll correct, is man page options are upper case first.)

Re: [patch] Add -z and -Z to apmd for automatic suspend/hibernate

[Ted Unangst]

Jesper Wallin wrote:
> On Sun, Aug 13, 2017 at 09:52:22AM +0200, Martijn van Duren wrote:
> > I've also been bitten by this a couple of times, but you can also solve
> > this via the sensorsd framework, which is how I've done it.
> 
> Yeah, someone on IRC also suggested sensorsd or even ksh and a cronjob.
> I personally find it a bit too ducttapey though, especially for a
> feature one would expect on a laptop. This also saves me from running an
> extra daemon just in case my battery runs out.

as regards the sensorsd approach, i've known about that possibility since
forever, and think about it every time i find my laptop has a dead battery,
but have never quite been motivated enough to set it up. i've heard the same
from several others. to me this suggests a certain usability hurdle. sensor
framework is great to make sure your scsi enclosure doesn't melt down and
sends you a page, but it's a little too diy for something as simple as
suspend. after all, we could eliminate lidsuspend/action and rely entirely on
sensors for that too, but that'd be silly.

Re: llvm - xor return pointers

[Todd Mortimer]

On Tue, Aug 15, 2017 at 05:12:39PM -0700, Ori Bernstein wrote:
> On Sat, 22 Jul 2017 02:25:29 -0400
> Todd Mortimer  wrote:
> 
> > xor [rsp], rsp
> > 
> > at the start of each function, and before every RET.
> 
> Wouldn't this break with alloca() or C99 VLAs?
> %rbp may work better, if the frame pointer is
> retained.

If alloca or C99 VLAs would break with this, than RET is broken with
alloca and C99 VLAs. At the time of the RET, %rsp *must* point to the
return address, or else RET will pop the wrong thing. So as long as we
insert the xor as the first instruction in the function preamble, then
we will xor the return pointer before alloca or VLAs have an opportunity
to modify %rsp, so when we later RET, we can xor it back.

Of course, I am happy to be proven wrong. If you can demonstrate a test
case where this breaks things, then please send it along and we can see
about fixing it. 

Cheers,
Todd

Re: [patch] Add -z and -Z to apmd for automatic suspend/hibernate

[Craig Skinner]

Hi Jesper/all,

On Sun, 13 Aug 2017 14:13:42 +0200 Jesper Wallin wrote:
> 
> ... someone on IRC also suggested sensorsd or even ksh and a
> cronjob. I personally find it a bit too ducttapey though, especially
> for a feature one would expect on a laptop.


For what its worth, below is an unpriv duck tape cron ksh script,
which I've been meaning to port & package up... (It is very reliable.)

If the battery status is critical, it syslogs & wall(1)s.

When disaster seems iminent, it starts a delayed shutdown(8),
which it later kills if power is plugged.


$ crontab -l | fgrep batt-crit
*/5 *   *   *   *   ~/bin/batt-crit


$ cat ~/bin/batt-crit
#!/bin/ksh
#
#   $Id: batt-crit,v 1.14 2016/05/13 13:22:58 craig Exp $
#
#-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
#
# Copyright (c) 2015-2016 Craig R. Skinner 
#
# Permission to use, copy, modify, and distribute this software for any
# purpose with or without fee is hereby granted, provided that the above
# copyright notice and this permission notice appear in all copies.
#
# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
#
#-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
#


# A/C charger ! disconnected
[[ $(apm -a) -ne 0 ]] && exit


# Battery status ! critical
[[ $(apm -b) -eq 2 ]] || exit 0


alias logger='logger -p daemon.crit'
apm | tr -s '\n' ' ' | logger


batt_lvl=$(apm -l)
[[ ${batt_lvl} -gt 15 ]] && exit
apm | wall
[[ ${batt_lvl} -gt 5 ]] && exit


warning='battery charge level critically low'
shutdown -hp +${batt_lvl} ${warning}
print "shutdown -hp +${batt_lvl} ${warning}" | logger


renice -n 20 $$ > /dev/null

count=100
until [[ ${count} -eq 0 ]]
do
sleep 20
[[ $(apm -a) -eq 0 ]] ||
{
pgrep -l -f shutdown &&
{
print 'killing...'
pkill -l shutdown
break
}
}
let count--
done

make get_last_resort_ifid() truely random

[Florian Obser]

Rename in6_get_rand_ifi() to get_last_resort_ifid() and delete the old
get_last_resort_ifid() function because eww.
Also if your system is so constraint that you end up in
get_last_resort_ifid() you don't deserve a random ifid that stays
stable over reboots.
Simplify code a bit since get_ifid() can no longer fail.
It couldn't fail before either because that code path was #if 0'ed.

OK?

diff --git netinet6/in6.h netinet6/in6.h
index 0caae1f586a..d80bff21370 100644
--- netinet6/in6.h
+++ netinet6/in6.h
@@ -418,7 +418,6 @@ voidin6_proto_cksum_out(struct mbuf *, struct ifnet 
*);
 intin6_localaddr(struct in6_addr *);
 intin6_addrscope(struct in6_addr *);
 struct in6_ifaddr *in6_ifawithscope(struct ifnet *, struct in6_addr *, u_int);
-void   in6_get_rand_ifid(struct ifnet *, struct in6_addr *);
 intin6_mask2len(struct in6_addr *, u_char *);
 intin6_nam2sin6(const struct mbuf *, struct sockaddr_in6 **);
 
diff --git netinet6/in6_ifattach.c netinet6/in6_ifattach.c
index 89acde9c6a4..a8abf5fa695 100644
--- netinet6/in6_ifattach.c
+++ netinet6/in6_ifattach.c
@@ -56,9 +56,9 @@
 #include 
 #endif
 
-int get_last_resort_ifid(struct ifnet *, struct in6_addr *);
+void get_last_resort_ifid(struct ifnet *, struct in6_addr *);
 int get_hw_ifid(struct ifnet *, struct in6_addr *);
-int get_ifid(struct ifnet *, struct in6_addr *);
+void get_ifid(struct ifnet *, struct in6_addr *);
 int in6_ifattach_loopback(struct ifnet *);
 
 #define EUI64_GBIT 0x01
@@ -72,52 +72,13 @@ int in6_ifattach_loopback(struct ifnet *);
 #define IFID_LOCAL(in6)(!EUI64_LOCAL(in6))
 #define IFID_UNIVERSAL(in6)(!EUI64_UNIVERSAL(in6))
 
-/*
- * Generate a last-resort interface identifier, when the machine has no
- * IEEE802/EUI64 address sources.
- * The goal here is to get an interface identifier that is
- * (1) random enough and (2) does not change across reboot.
- * We currently use SHA512(hostname) for it.
- *
- * in6 - upper 64bits are preserved
- */
-int
-get_last_resort_ifid(struct ifnet *ifp, struct in6_addr *in6)
-{
-   SHA2_CTX ctx;
-   u_int8_t digest[SHA512_DIGEST_LENGTH];
-
-#if 0
-   /* we need at least several letters as seed for ifid */
-   if (hostnamelen < 3)
-   return -1;
-#endif
-
-   /* generate 8 bytes of pseudo-random value. */
-   SHA512Init();
-   SHA512Update(, hostname, hostnamelen);
-   SHA512Final(digest, );
-
-   /* assumes sizeof(digest) > sizeof(ifid) */
-   bcopy(digest, >s6_addr[8], 8);
-
-   /* make sure to set "u" bit to local, and "g" bit to individual. */
-   in6->s6_addr[8] &= ~EUI64_GBIT; /* g bit to "individual" */
-   in6->s6_addr[8] |= EUI64_UBIT;  /* u bit to "local" */
-
-   /* convert EUI64 into IPv6 interface identifier */
-   EUI64_TO_IFID(in6);
-
-   return 0;
-}
-
 /*
  * Generate a random interface identifier.
  *
  * in6 - upper 64bits are preserved
  */
 void
-in6_get_rand_ifid(struct ifnet *ifp, struct in6_addr *in6)
+get_last_resort_ifid(struct ifnet *ifp, struct in6_addr *in6)
 {
arc4random_buf(>s6_addr32[2], 8);
 
@@ -235,7 +196,7 @@ get_hw_ifid(struct ifnet *ifp, struct in6_addr *in6)
  * available on ifp0, borrow interface identifier from other information
  * sources.
  */
-int
+void
 get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
 {
struct ifnet *ifp;
@@ -267,22 +228,15 @@ get_ifid(struct ifnet *ifp0, struct in6_addr *in6)
}
 
/* last resort: get from random number source */
-   if (get_last_resort_ifid(ifp, in6) == 0) {
-   nd6log((LOG_DEBUG,
-   "%s: interface identifier generated by random number\n",
-   ifp0->if_xname));
-   goto success;
-   }
-
-   printf("%s: failed to get interface identifier\n", ifp0->if_xname);
-   return -1;
-
+   get_last_resort_ifid(ifp, in6);
+   nd6log((LOG_DEBUG,
+   "%s: interface identifier generated by random number\n",
+   ifp0->if_xname));
 success:
nd6log((LOG_INFO, "%s: ifid: %02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x\n",
ifp0->if_xname, in6->s6_addr[8], in6->s6_addr[9], in6->s6_addr[10],
in6->s6_addr[11], in6->s6_addr[12], in6->s6_addr[13],
in6->s6_addr[14], in6->s6_addr[15]));
-   return 0;
 }
 
 /*
@@ -318,13 +272,8 @@ in6_ifattach_linklocal(struct ifnet *ifp, struct in6_addr 
*ifid)
ifra.ifra_addr.sin6_addr.s6_addr32[1] = 0;
ifra.ifra_addr.sin6_addr.s6_addr[8] &= ~EUI64_GBIT;
ifra.ifra_addr.sin6_addr.s6_addr[8] |= EUI64_UBIT;
-   } else {
-   if (get_ifid(ifp, _addr.sin6_addr) != 0) {
-   nd6log((LOG_ERR,
-   "%s: no ifid available\n", ifp->if_xname));
-   return (-1);
-   }
-   }
+   } else
+   get_ifid(ifp, _addr.sin6_addr);
 
ifra.ifra_prefixmask.sin6_len = sizeof(struct 

Re: ifstated: stop tracking interface indexes

[Rob Pierce]

On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
> On Mon, Aug 14 2017, Rob Pierce  wrote:
> > ifstated currently tracks and maintains the index of each monitored 
> > interface
> > and does not maintain interface names. This means we need to re-index on
> > interface departure and arrival.
> >
> > The following diff moves away from indexes to names. Indexes are still 
> > required,
> > but easily obtained dynamically as needed. This helps simplify the next diff
> > that will provide support for interface departure and arrival.
> >
> > Suggested by deraadt.
> >
> > No intended functional change. Regress tests pass.
> >
> > Ok?
> 
> The idea looks sound to me, however I would keep the "interface" symbol
> in parse.y (your diff doesn't remove all "interface" references btw).
> 
> The current code checks the existence of the interface at startup.  If
> the interface doesn't exists, you get a syntax error.  This could happen
> because of a missing interface (an interesting case), or because of
> a typo.  Whether or not we're erroring out, it is nice to print
> a diagnostic message.
> 
> I'm not sure this change was intended, so here's a tentative diff that
> that keeps the existing behavior.  Regress tests pass.

Yes, I see that now. That change was not intended. Thanks!

Your diff looks good.

Ok?

> Index: ifstated.c
> ===
> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 ifstated.c
> --- ifstated.c14 Aug 2017 03:15:28 -  1.59
> +++ ifstated.c15 Aug 2017 03:04:47 -
> @@ -61,8 +61,8 @@ voidexternal_handler(int, short, void 
>  void external_exec(struct ifsd_external *, int);
>  void check_external_status(struct ifsd_state *);
>  void external_evtimer_setup(struct ifsd_state *, int);
> -void scan_ifstate(int, int, int);
> -int  scan_ifstate_single(int, int, struct ifsd_state *);
> +void scan_ifstate(const char *, int, int);
> +int  scan_ifstate_single(const char *, int, struct ifsd_state *);
>  void fetch_ifstate(int);
>  __dead void  usage(void);
>  void adjust_expressions(struct ifsd_expression_list *, int);
> @@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
>   char msg[2048];
>   struct rt_msghdr *rtm = (struct rt_msghdr *)
>   struct if_msghdr ifm;
> + char ifnamebuf[IFNAMSIZ];
> + char *ifname;
>   ssize_t len;
>  
>   if ((len = read(fd, msg, sizeof(msg))) == -1) {
> @@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
>   switch (rtm->rtm_type) {
>   case RTM_IFINFO:
>   memcpy(, rtm, sizeof(ifm));
> - scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
> + ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
> + /* ifname is NULL on interface departure */
> + if (ifname != NULL)
> + scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
>   break;
>   case RTM_DESYNC:
>   fetch_ifstate(1);
> @@ -431,7 +436,7 @@ external_evtimer_setup(struct ifsd_state
>  #define  LINK_STATE_IS_DOWN(_s)  (!LINK_STATE_IS_UP((_s)))
>  
>  int
> -scan_ifstate_single(int ifindex, int s, struct ifsd_state *state)
> +scan_ifstate_single(const char *ifname, int s, struct ifsd_state *state)
>  {
>   struct ifsd_ifstate *ifstate;
>   struct ifsd_expression_list expressions;
> @@ -440,7 +445,7 @@ scan_ifstate_single(int ifindex, int s, 
>   TAILQ_INIT();
>  
>   TAILQ_FOREACH(ifstate, >interface_states, entries) {
> - if (ifstate->ifindex == ifindex) {
> + if (strcmp(ifstate->ifname, ifname) == 0) {
>   if (ifstate->prevstate != s &&
>   (ifstate->prevstate != -1 || !opt_inhibit)) {
>   struct ifsd_expression *expression;
> @@ -472,15 +477,15 @@ scan_ifstate_single(int ifindex, int s, 
>  }
>  
>  void
> -scan_ifstate(int ifindex, int s, int do_eval)
> +scan_ifstate(const char *ifname, int s, int do_eval)
>  {
>   struct ifsd_state *state;
>   int cur_eval = 0;
>  
> - if (scan_ifstate_single(ifindex, s, >initstate) && do_eval)
> + if (scan_ifstate_single(ifname, s, >initstate) && do_eval)
>   eval_state(>initstate);
>   TAILQ_FOREACH(state, >states, entries) {
> - if (scan_ifstate_single(ifindex, s, state) &&
> + if (scan_ifstate_single(ifname, s, state) &&
>   (do_eval && state == conf->curstate))
>   cur_eval = 1;
>   }
> @@ -619,8 +624,8 @@ fetch_ifstate(int do_eval)
>   for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
>   if (ifa->ifa_addr->sa_family == AF_LINK) {
>   struct if_data *ifdata = ifa->ifa_data;
> - 

CID 1452909: Use of untrusted scalar value (pf_table.c)

[Mike Belopuhov]

Hi,

Coverity has discovered that we're blindly trusting the value
of pfra_type that we read from the userland supplied pfr_addr
and use it to index an array of pools in pfr_create_kentry.

I suggest to do two things: add a check in pfr_validate_addr
that is called after every copyin and also perform the check
in pfr_create_kentry before we attempt to use the value.

OK?

P.S.
What does 'k' table and entry prefix stand for in pf_table.c?
Kernel?

diff --git sys/net/pf_table.c sys/net/pf_table.c
index 7666ec7013c..985c673b5cb 100644
--- sys/net/pf_table.c
+++ sys/net/pf_table.c
@@ -741,10 +741,12 @@ pfr_validate_addr(struct pfr_addr *ad)
return (-1);
if (ad->pfra_not && ad->pfra_not != 1)
return (-1);
if (ad->pfra_fback)
return (-1);
+   if (ad->pfra_type >= PFRKE_MAX)
+   return (-1);
return (0);
 }
 
 void
 pfr_enqueue_addrs(struct pfr_ktable *kt, struct pfr_kentryworkq *workq,
@@ -820,10 +822,13 @@ pfr_lookup_addr(struct pfr_ktable *kt, struct pfr_addr 
*ad, int exact)
 struct pfr_kentry *
 pfr_create_kentry(struct pfr_addr *ad)
 {
struct pfr_kentry_all   *ke;
 
+   if (ad->pfra_type >= PFRKE_MAX)
+   panic("unknown pfra_type %d", ad->pfra_type);
+
ke = pool_get(_kentry_pl[ad->pfra_type], PR_NOWAIT | PR_ZERO);
if (ke == NULL)
return (NULL);
 
ke->pfrke_type = ad->pfra_type;
@@ -842,13 +847,10 @@ pfr_create_kentry(struct pfr_addr *ad)
if (ad->pfra_ifname[0])
ke->pfrke_rkif = pfi_kif_get(ad->pfra_ifname);
if (ke->pfrke_rkif)
pfi_kif_ref(ke->pfrke_rkif, PFI_KIF_REF_ROUTE);
break;
-   default:
-   panic("unknown pfrke_type %d", ke->pfrke_type);
-   break;
}
 
switch (ad->pfra_af) {
case AF_INET:
FILLIN_SIN(ke->pfrke_sa.sin, ad->pfra_ip4addr);