pci(4): PCIOCSETVGA: tsleep(9) -> tsleep_nsec(9)
Just an INFSLP, easy.
ok?
Index: pci/pci.c
===
RCS file: /cvs/src/sys/dev/pci/pci.c,v
retrieving revision 1.114
diff -u -p -r1.114 pci.c
--- pci/pci.c 25 Jun 2019 16:46:32 - 1.114
+++ pci/pci.c 15 Jan 2020 06:51:29 -
@@ -1419,8 +1419,8 @@ pciioctl(dev_t dev, u_long cmd, caddr_t
while (pci_vga_proc != p && pci_vga_proc != NULL) {
if (vga->pv_lock == PCI_VGA_TRYLOCK)
return (EBUSY);
- error = tsleep(_vga_proc, PLOCK | PCATCH,
- "vgalk", 0);
+ error = tsleep_nsec(_vga_proc, PLOCK | PCATCH,
+ "vgalk", INFSLP);
if (error)
return (error);
}
xbf(4): tsleep(9) -> tsleep_nsec(9)
Given the SCSI_NOSLEEP split here I think the simplest thing we can do is ask to sleep as much as we delay(9). The question is: if you *could* poll in 10us intervals here with tsleep_nsec(9), would you want to? If so, then this works. If not, what is a more appropriate interval? Index: pv/xbf.c === RCS file: /cvs/src/sys/dev/pv/xbf.c,v retrieving revision 1.32 diff -u -p -r1.32 xbf.c --- pv/xbf.c17 Jul 2017 10:30:03 - 1.32 +++ pv/xbf.c15 Jan 2020 06:20:25 - @@ -738,7 +738,7 @@ xbf_poll_cmd(struct scsi_xfer *xs) if (ISSET(xs->flags, SCSI_NOSLEEP)) delay(10); else - tsleep(xs, PRIBIO, "xbfpoll", 1); + tsleep_nsec(xs, PRIBIO, "xbfpoll", USEC_TO_NSEC(10)); xbf_intr(xs->sc_link->adapter_softc); } while(--timo > 0);
hvn(4): *sleep(9) -> *sleep_nsec(9)
First, the tsleep(9) calls. These are easy. If cold != 0 we
delay(9), so just do the equivalent thing with tsleep_nsec(9).
Next, the msleep(9). If I understand correctly, in hvn_alloc_cmd()
we're waiting to pull something off a freelist. The freelist,
sc->sc_cntl_fq, is protexted by the mutex sc->sc_cntl_fqlck. The
mutex ensures we can't miss the wakeup(9) from hvn_free_cmd(), so
I think we can just INFSLP and await the wakeup(9).
ok?
Index: pv/if_hvn.c
===
RCS file: /cvs/src/sys/dev/pv/if_hvn.c,v
retrieving revision 1.40
diff -u -p -r1.40 if_hvn.c
--- pv/if_hvn.c 17 May 2018 12:32:33 - 1.40
+++ pv/if_hvn.c 15 Jan 2020 06:15:33 -
@@ -1049,7 +1049,8 @@ hvn_nvs_cmd(struct hvn_softc *sc, void *
if (cold)
delay(1000);
else
- tsleep(cmd, PRIBIO, "nvsout", 1);
+ tsleep_nsec(cmd, PRIBIO, "nvsout",
+ USEC_TO_NSEC(1000));
} else if (rv) {
DPRINTF("%s: NVSP operation %u send error %d\n",
sc->sc_dev.dv_xname, hdr->nvs_type, rv);
@@ -1070,7 +1071,8 @@ hvn_nvs_cmd(struct hvn_softc *sc, void *
if (cold)
delay(1000);
else
- tsleep(sc, PRIBIO | PCATCH, "nvscmd", 1);
+ tsleep_nsec(sc, PRIBIO | PCATCH, "nvscmd",
+ USEC_TO_NSEC(1000));
s = splnet();
hvn_nvs_intr(sc);
splx(s);
@@ -1123,8 +1125,8 @@ hvn_alloc_cmd(struct hvn_softc *sc)
mtx_enter(>sc_cntl_fqlck);
while ((rc = TAILQ_FIRST(>sc_cntl_fq)) == NULL)
- msleep(>sc_cntl_fq, >sc_cntl_fqlck,
- PRIBIO, "nvsalloc", 1);
+ msleep_nsec(>sc_cntl_fq, >sc_cntl_fqlck,
+ PRIBIO, "nvsalloc", INFSLP);
TAILQ_REMOVE(>sc_cntl_fq, rc, rc_entry);
mtx_leave(>sc_cntl_fqlck);
return (rc);
@@ -1367,7 +1369,8 @@ hvn_rndis_cmd(struct hvn_softc *sc, stru
if (cold)
delay(100);
else
- tsleep(rc, PRIBIO, "rndisout", 1);
+ tsleep_nsec(rc, PRIBIO, "rndisout",
+ USEC_TO_NSEC(100));
} else if (rv) {
DPRINTF("%s: RNDIS operation %u send error %d\n",
sc->sc_dev.dv_xname, hdr->rm_type, rv);
@@ -1389,7 +1392,8 @@ hvn_rndis_cmd(struct hvn_softc *sc, stru
if (cold)
delay(1000);
else
- tsleep(rc, PRIBIO | PCATCH, "rndiscmd", 1);
+ tsleep_nsec(rc, PRIBIO | PCATCH, "rndiscmd",
+ USEC_TO_NSEC(1000));
s = splnet();
hvn_nvs_intr(sc);
splx(s);
Re: umb(4) WIP diff and questions
On Tue Jan 14, 2020 at 12:40 PM, Theo de Raadt wrote: > > > > Channeling a conversation from 15 years ago: "How about wpakeyfile" > > > > > Another consideration is... many of these passwords are locked to narrow > usage cases, so does it really matter all that much? > > Right, seems like I should leave ifconfig(8) alone for the moment. I'll carry on with umb and the suggestions from Stefan and Claudio though if that's OK?
Re: umb(4) WIP diff and questions
Hi Stefan, On Tue Jan 14, 2020 at 2:40 PM, Stefan Sperling wrote: > <... lots of useful stuff ...> > That was exactly the sort of thing I was looking for. Thanks! It was seeing your device drivers presentation on Youtube a week or so ago that originally inspired me to get stuck in, so thanks for that, too. Lee.
aac(4): aac_wait_intr: intended timeout unit?
Here's another tsleep(9) -> tsleep_nsec(9) conversion.
The first chunk is easy: ticks to seconds.
The second chunk looks fishy, though.
aac_wait_command() calls tsleep(9) with a timeout in ticks, but the
only caller of aac_wait_command() uses a scsi_xfer.timeout as the
timeout, and if I'm reading scsiconf.h correctly scsi_xfer.timeout is
in milliseconds.
Is this a typo? What is the intended unit for aac_wait_intr()?
Related: can scsi_xfer.timeout be zero?
Index: ic/aac.c
===
RCS file: /cvs/src/sys/dev/ic/aac.c,v
retrieving revision 1.71
diff -u -p -r1.71 aac.c
--- ic/aac.c5 Oct 2019 20:41:27 - 1.71
+++ ic/aac.c15 Jan 2020 01:03:21 -
@@ -573,8 +573,8 @@ aac_command_thread(void *arg)
("%s: command thread sleeping\n",
sc->aac_dev.dv_xname));
AAC_LOCK_RELEASE(>aac_io_lock);
- retval = tsleep(sc->aifthread, PRIBIO, "aifthd",
- AAC_PERIODIC_INTERVAL * hz);
+ retval = tsleep_nsec(sc->aifthread, PRIBIO, "aifthd",
+ SEC_TO_NSEC(AAC_PERIODIC_INTERVAL));
AAC_LOCK_ACQUIRE(>aac_io_lock);
}
@@ -844,7 +844,7 @@ aac_bio_complete(struct aac_command *cm)
* spam the memory of a command that has been recycled.
*/
int
-aac_wait_command(struct aac_command *cm, int timeout)
+aac_wait_command(struct aac_command *cm, int msecs)
{
struct aac_softc *sc = cm->cm_sc;
int error = 0;
@@ -860,7 +860,7 @@ aac_wait_command(struct aac_command *cm,
AAC_DPRINTF(AAC_D_MISC, ("%s: sleeping until command done\n",
sc->aac_dev.dv_xname));
AAC_LOCK_RELEASE(>aac_io_lock);
- error = tsleep(cm, PRIBIO, "aacwait", timeout);
+ error = tsleep_nsec(cm, PRIBIO, "aacwait", MSEC_TO_NSEC(msecs));
AAC_LOCK_ACQUIRE(>aac_io_lock);
}
return (error);
Re: route: better error message for getaddrinfo() failure
On Wed, Jan 15, 2020 at 01:26:53AM +0100, Klemens Nanni wrote:
> Replace the very same error message strlcpy() uses earlier. This makes
> it easier to distinguish and does not hide different errors behind the
> same generic one.
>
> For example, it turns
>
> # route -qn add -inet6 fec0::% -prefixlen 10 ::1 -reject
> route: fec0::%: bad value
>
> into
>
> # ./obj/route -qn add -inet6 fec0::% -prefixlen 10 ::1 -reject
> route: fec0::%: no address associated with name
>
> Feedback? OK?
Error messages should be unique so you know where to debug.
OK bluhm@
> Index: route.c
> ===
> RCS file: /cvs/src/sbin/route/route.c,v
> retrieving revision 1.246
> diff -u -p -r1.246 route.c
> --- route.c 22 Nov 2019 15:28:05 - 1.246
> +++ route.c 15 Jan 2020 00:20:29 -
> @@ -859,6 +859,7 @@ getaddr(int which, int af, char *s, stru
> sizeof("::::::255:255:255:255/128")
> ];
> char *sep;
> + int error;
>
> if (strlcpy(buf, s, sizeof buf) >= sizeof buf) {
> errx(1, "%s: bad value", s);
> @@ -871,10 +872,12 @@ getaddr(int which, int af, char *s, stru
> hints.ai_family = afamily; /*AF_INET6*/
> hints.ai_flags = AI_NUMERICHOST;
> hints.ai_socktype = SOCK_DGRAM; /*dummy*/
> - if (getaddrinfo(buf, "0", , ) != 0) {
> + error = getaddrinfo(buf, "0", , );
> + if (error) {
> hints.ai_flags = 0;
> - if (getaddrinfo(buf, "0", , ) != 0)
> - errx(1, "%s: bad value", s);
> + error = getaddrinfo(buf, "0", , );
> + if (error)
> + errx(1, "%s: %s", s, gai_strerror(error));
> }
> if (res->ai_next)
> errx(1, "%s: resolved to multiple values", s);
lpt(4): timeout_add(9) -> timeout_add_msec(9), tsleep(9) -> tsleep_nsec(9)
Ticks to milliseconds.
I've changed "tic" to "msecs" in the backoff loop to make the units
more obvious. I went with 10ms as the starting sleep. A perfect
conversion would start at something like (tick / 1000), but obviously
we're trying to move away from that sort of thing. Absent a better
idea, 10ms seems safe.
Everything else here is straightforward.
ok?
Index: ic/lpt.c
===
RCS file: /cvs/src/sys/dev/ic/lpt.c,v
retrieving revision 1.14
diff -u -p -r1.14 lpt.c
--- ic/lpt.c11 May 2015 02:01:01 - 1.14
+++ ic/lpt.c15 Jan 2020 00:33:47 -
@@ -71,8 +71,8 @@
#include "lpt.h"
-#defineTIMEOUT hz*16 /* wait up to 16 seconds for a ready */
-#defineSTEPhz/4
+#defineTIMEOUT 16000 /* wait up to 16 seconds for a ready */
+#defineSTEP250 /* 1/4 seconds */
#defineLPTPRI (PZERO+8)
#defineLPT_BSIZE 1024
@@ -199,7 +199,8 @@ lptopen(dev_t dev, int flag, int mode, s
}
/* wait 1/4 second, give up if we get a signal */
- error = tsleep((caddr_t)sc, LPTPRI | PCATCH, "lptopen", STEP);
+ error = tsleep_nsec(sc, LPTPRI | PCATCH, "lptopen",
+ MSEC_TO_NSEC(STEP));
if (sc->sc_state == 0)
return (EIO);
if (error != EWOULDBLOCK) {
@@ -256,7 +257,7 @@ lptwakeup(void *arg)
splx(s);
if (sc->sc_state != 0)
- timeout_add(>sc_wakeup_tmo, STEP);
+ timeout_add_msec(>sc_wakeup_tmo, STEP);
}
/*
@@ -293,7 +294,7 @@ lptpushbytes(struct lpt_softc *sc)
int error;
if (sc->sc_flags & LPT_NOINTR) {
- int spin, tic;
+ int msecs, spin;
u_int8_t control = sc->sc_control;
while (sc->sc_count > 0) {
@@ -303,16 +304,17 @@ lptpushbytes(struct lpt_softc *sc)
while (NOT_READY()) {
if (++spin < sc->sc_spinmax)
continue;
- tic = 0;
+ msecs = 10;
/* adapt busy-wait algorithm */
sc->sc_spinmax++;
while (NOT_READY_ERR()) {
/* exponential backoff */
- tic = tic + tic + 1;
- if (tic > TIMEOUT)
- tic = TIMEOUT;
- error = tsleep((caddr_t)sc,
- LPTPRI | PCATCH, "lptpsh", tic);
+ msecs = msecs + msecs + 1;
+ if (msecs > TIMEOUT)
+ msecs = TIMEOUT;
+ error = tsleep_nsec(sc,
+ LPTPRI | PCATCH, "lptpsh",
+ MSEC_TO_NSEC(msecs));
if (sc->sc_state == 0)
error = EIO;
if (error != EWOULDBLOCK)
@@ -345,8 +347,8 @@ lptpushbytes(struct lpt_softc *sc)
}
if (sc->sc_state == 0)
return (EIO);
- error = tsleep((caddr_t)sc, LPTPRI | PCATCH,
- "lptwrite2", 0);
+ error = tsleep_nsec(sc, LPTPRI | PCATCH,
+ "lptwrite2", INFSLP);
if (sc->sc_state == 0)
error = EIO;
if (error)
route: better error message for getaddrinfo() failure
Replace the very same error message strlcpy() uses earlier. This makes
it easier to distinguish and does not hide different errors behind the
same generic one.
For example, it turns
# route -qn add -inet6 fec0::% -prefixlen 10 ::1 -reject
route: fec0::%: bad value
into
# ./obj/route -qn add -inet6 fec0::% -prefixlen 10 ::1 -reject
route: fec0::%: no address associated with name
Feedback? OK?
Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.246
diff -u -p -r1.246 route.c
--- route.c 22 Nov 2019 15:28:05 - 1.246
+++ route.c 15 Jan 2020 00:20:29 -
@@ -859,6 +859,7 @@ getaddr(int which, int af, char *s, stru
sizeof("::::::255:255:255:255/128")
];
char *sep;
+ int error;
if (strlcpy(buf, s, sizeof buf) >= sizeof buf) {
errx(1, "%s: bad value", s);
@@ -871,10 +872,12 @@ getaddr(int which, int af, char *s, stru
hints.ai_family = afamily; /*AF_INET6*/
hints.ai_flags = AI_NUMERICHOST;
hints.ai_socktype = SOCK_DGRAM; /*dummy*/
- if (getaddrinfo(buf, "0", , ) != 0) {
+ error = getaddrinfo(buf, "0", , );
+ if (error) {
hints.ai_flags = 0;
- if (getaddrinfo(buf, "0", , ) != 0)
- errx(1, "%s: bad value", s);
+ error = getaddrinfo(buf, "0", , );
+ if (error)
+ errx(1, "%s: %s", s, gai_strerror(error));
}
if (res->ai_next)
errx(1, "%s: resolved to multiple values", s);
Re: netstart: IPv6 routes: do not redirect already quiet stdout
On Tue, Jan 14, 2020 at 11:54:32PM +0100, Klemens Nanni wrote: > Came here while testing an IPv6 related sys/net/rtsock.c diff: all > invocations use `-q' and route(8) says > >-q Suppress all output. > > so the redirection is duplicate. If route still prints to standard > output despite the quiet flag I want to see such a bug and fix it. > > Note that this does not involve standard error which is neither effected > by `-q' nor redirected. > > OK? OK bluhm@ > Index: netstart > === > RCS file: /cvs/src/etc/netstart,v > retrieving revision 1.201 > diff -u -p -r1.201 netstart > --- netstart 25 Oct 2019 06:01:27 - 1.201 > +++ netstart 14 Jan 2020 22:46:22 - > @@ -254,26 +254,26 @@ if ifconfig lo0 inet6 >/dev/null 2>&1; t > ip6kernel=YES > > # Disallow link-local unicast dest without outgoing scope identifiers. > - route -qn add -inet6 fe80:: -prefixlen 10 ::1 -reject >/dev/null > + route -qn add -inet6 fe80:: -prefixlen 10 ::1 -reject > > # Disallow site-local unicast dest without outgoing scope identifiers. > # If you configure site-locals without scope id (it is permissible > # config for routers that are not on scope boundary), you may want > # to comment the line out. > - route -qn add -inet6 fec0:: -prefixlen 10 ::1 -reject >/dev/null > + route -qn add -inet6 fec0:: -prefixlen 10 ::1 -reject > > # Disallow "internal" addresses to appear on the wire. > - route -qn add -inet6 :::0.0.0.0 -prefixlen 96 ::1 -reject >/dev/null > + route -qn add -inet6 :::0.0.0.0 -prefixlen 96 ::1 -reject > > # Disallow packets to malicious 6to4 prefix. > - route -qn add -inet6 2002:e000:: -prefixlen 20 ::1 -reject >/dev/null > - route -qn add -inet6 2002:7f00:: -prefixlen 24 ::1 -reject >/dev/null > - route -qn add -inet6 2002::: -prefixlen 24 ::1 -reject >/dev/null > - route -qn add -inet6 2002:ff00:: -prefixlen 24 ::1 -reject >/dev/null > + route -qn add -inet6 2002:e000:: -prefixlen 20 ::1 -reject > + route -qn add -inet6 2002:7f00:: -prefixlen 24 ::1 -reject > + route -qn add -inet6 2002::: -prefixlen 24 ::1 -reject > + route -qn add -inet6 2002:ff00:: -prefixlen 24 ::1 -reject > > # Disallow packets without scope identifier. > - route -qn add -inet6 ff01:: -prefixlen 16 ::1 -reject >/dev/null > - route -qn add -inet6 ff02:: -prefixlen 16 ::1 -reject >/dev/null > + route -qn add -inet6 ff01:: -prefixlen 16 ::1 -reject > + route -qn add -inet6 ff02:: -prefixlen 16 ::1 -reject > > # Completely disallow packets to IPv4 compatible prefix. > # > @@ -290,7 +290,7 @@ if ifconfig lo0 inet6 >/dev/null 2>&1; t > # > # Due to rare use of IPv4 compatible addresses, and security issues > # with it, we disable it by default. > - route -qn add -inet6 ::0.0.0.0 -prefixlen 96 ::1 -reject >/dev/null > + route -qn add -inet6 ::0.0.0.0 -prefixlen 96 ::1 -reject > else > ip6kernel=NO > fi
Re: umb(4) WIP diff and questions
On Tue, Jan 14, 2020 at 5:11 PM Stefan Sperling wrote:
> On Tue, Jan 14, 2020 at 12:34:29PM -0700, Theo de Raadt wrote:
> > Channeling a conversation from 15 years ago: "How about wpakeyfile"
>
> ifconfig wpakeyfile would be trivial to add if we really want it.
>
But how will hostname.if will work when using join in netstart, one would
need to:
# cat /etc/hostname.iwm0
join ssid1 wpakeyfile /etc/wpa/ssd1-wpa.key
join ssd2 wpakeyfile /etc/wpa/ssd2-wpa.key
[etc...]
?
>
> The downside is loss of unveil, here handled the same way as for the
> bridge rulesfile. Looks like unveil(argv[i], "r") is considered bad
> practice even for an 'i' that should contain a path?
>
> diff a7540b3fac3fd3a71fd4134709ac4d4f71a3b5a4 /usr/src
> blob - 3fb0780ba7cf1333894f5c3485a95e71885fbd6d
> file + sbin/ifconfig/ifconfig.8
> --- sbin/ifconfig/ifconfig.8
> +++ sbin/ifconfig/ifconfig.8
> @@ -940,6 +940,7 @@ will begin advertising as master.
> .Op Cm wpaciphers Ar cipher,cipher,...
> .Op Cm wpagroupcipher Ar cipher
> .Op Oo Fl Oc Ns Cm wpakey Ar passphrase | hexkey
> +.Op Cm wpakeyfile Ar path
> .Op Cm wpaprotos Ar proto,proto,...
> .Ek
> .nr nS 0
> @@ -990,6 +991,7 @@ the
> .Cm join
> list will record
> .Cm wpakey ,
> +.Cm wpakeyfile ,
> .Cm wpaprotos ,
> or
> .Cm nwkey
> @@ -1209,6 +1211,8 @@ The default value is
> .Dq psk
> can only be used if a pre-shared key is configured using the
> .Cm wpakey
> +or
> +.Cm wpakeyfile
> option.
> .It Cm wpaciphers Ar cipher,cipher,...
> Set the comma-separated list of allowed pairwise ciphers.
> @@ -1268,6 +1272,10 @@ or
> option must first be specified, since
> .Nm
> will hash the nwid along with the passphrase to create the key.
> +.It Cm wpakeyfile Ar path
> +Set the WPA key contained in the file at the specified
> +.Ar path .
> +Trailing whitespace is ignored.
> .It Cm -wpakey
> Delete the pre-shared WPA key and disable WPA.
> .It Cm wpaprotos Ar proto,proto,...
> blob - f242d72cd73e8d50ccf1dd3d96ac62e35fe7025b
> file + sbin/ifconfig/ifconfig.c
> --- sbin/ifconfig/ifconfig.c
> +++ sbin/ifconfig/ifconfig.c
> @@ -63,6 +63,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -106,6 +107,7 @@
> #include
> #include
> #include
> +#include
>
> #ifndef SMALL
> #include
> @@ -211,6 +213,7 @@ voidsetifwpaakms(const char *, int);
> void setifwpaciphers(const char *, int);
> void setifwpagroupcipher(const char *, int);
> void setifwpakey(const char *, int);
> +void setifwpakeyfile(const char *, int);
> void setifchan(const char *, int);
> void setifscan(const char *, int);
> void setifnwflag(const char *, int);
> @@ -415,6 +418,7 @@ const structcmd {
> { "wpagroupcipher", NEXTARG,0,
> setifwpagroupcipher },
> { "wpaprotos", NEXTARG,0, setifwpaprotos },
> { "wpakey", NEXTARG,0, setifwpakey },
> + { "wpakeyfile", NEXTARG,0, setifwpakeyfile },
> { "-wpakey",-1, 0, setifwpakey },
> { "chan", NEXTARG0, 0, setifchan },
> { "-chan", -1, 0, setifchan },
> @@ -728,7 +732,7 @@ main(int argc, char *argv[])
> int create = 0;
> int Cflag = 0;
> int gflag = 0;
> - int found_rulefile = 0;
> + int found_rulefile = 0, found_wpakeyfile = 0, wpafileidx = 0;
> int i;
>
> /* If no args at all, print all interfaces. */
> @@ -785,9 +789,13 @@ main(int argc, char *argv[])
> found_rulefile = 1;
> break;
> }
> + if (strcmp(argv[i], "wpakeyfile") == 0) {
> + found_wpakeyfile = 1;
> + break;
> + }
> }
>
> - if (!found_rulefile) {
> + if (!found_rulefile && !found_wpakeyfile) {
> if (unveil(_PATH_RESCONF, "r") == -1)
> err(1, "unveil");
> if (unveil(_PATH_HOSTS, "r") == -1)
> @@ -2240,6 +2248,40 @@ setifwpakey(const char *val, int d)
> wpa.i_enabled = psk.i_enabled;
> if (ioctl(sock, SIOCS80211WPAPARMS, (caddr_t)) == -1)
> err(1, "SIOCS80211WPAPARMS");
> +}
> +
> +void
> +setifwpakeyfile(const char *val, int d)
> +{
> + char *wpakey;
> + int fd;
> + struct stat sb;
> + ssize_t n;
> +
> + fd = open(val, O_RDONLY);
> + if (fd == -1)
> + err(1, "open %s", val);
> +
> + if (fstat(fd, ) == -1)
> + err(1, "fstat %s", val);
> +
> + wpakey = malloc(sb.st_size);
> + if (wpakey == NULL)
> + err(1, "malloc");
> +
> + n = read(fd, wpakey, sb.st_size);
> + if (n == -1)
> + err(1, "read %s", val);
> + if (n != sb.st_size)
> + errx(1, "failed to read from file %s", val);
> + close(fd);
> +
Re: umb(4) WIP diff and questions
On Tue, Jan 14, 2020 at 12:34:29PM -0700, Theo de Raadt wrote:
> Channeling a conversation from 15 years ago: "How about wpakeyfile"
ifconfig wpakeyfile would be trivial to add if we really want it.
The downside is loss of unveil, here handled the same way as for the
bridge rulesfile. Looks like unveil(argv[i], "r") is considered bad
practice even for an 'i' that should contain a path?
diff a7540b3fac3fd3a71fd4134709ac4d4f71a3b5a4 /usr/src
blob - 3fb0780ba7cf1333894f5c3485a95e71885fbd6d
file + sbin/ifconfig/ifconfig.8
--- sbin/ifconfig/ifconfig.8
+++ sbin/ifconfig/ifconfig.8
@@ -940,6 +940,7 @@ will begin advertising as master.
.Op Cm wpaciphers Ar cipher,cipher,...
.Op Cm wpagroupcipher Ar cipher
.Op Oo Fl Oc Ns Cm wpakey Ar passphrase | hexkey
+.Op Cm wpakeyfile Ar path
.Op Cm wpaprotos Ar proto,proto,...
.Ek
.nr nS 0
@@ -990,6 +991,7 @@ the
.Cm join
list will record
.Cm wpakey ,
+.Cm wpakeyfile ,
.Cm wpaprotos ,
or
.Cm nwkey
@@ -1209,6 +1211,8 @@ The default value is
.Dq psk
can only be used if a pre-shared key is configured using the
.Cm wpakey
+or
+.Cm wpakeyfile
option.
.It Cm wpaciphers Ar cipher,cipher,...
Set the comma-separated list of allowed pairwise ciphers.
@@ -1268,6 +1272,10 @@ or
option must first be specified, since
.Nm
will hash the nwid along with the passphrase to create the key.
+.It Cm wpakeyfile Ar path
+Set the WPA key contained in the file at the specified
+.Ar path .
+Trailing whitespace is ignored.
.It Cm -wpakey
Delete the pre-shared WPA key and disable WPA.
.It Cm wpaprotos Ar proto,proto,...
blob - f242d72cd73e8d50ccf1dd3d96ac62e35fe7025b
file + sbin/ifconfig/ifconfig.c
--- sbin/ifconfig/ifconfig.c
+++ sbin/ifconfig/ifconfig.c
@@ -63,6 +63,7 @@
#include
#include
#include
+#include
#include
#include
@@ -106,6 +107,7 @@
#include
#include
#include
+#include
#ifndef SMALL
#include
@@ -211,6 +213,7 @@ voidsetifwpaakms(const char *, int);
void setifwpaciphers(const char *, int);
void setifwpagroupcipher(const char *, int);
void setifwpakey(const char *, int);
+void setifwpakeyfile(const char *, int);
void setifchan(const char *, int);
void setifscan(const char *, int);
void setifnwflag(const char *, int);
@@ -415,6 +418,7 @@ const structcmd {
{ "wpagroupcipher", NEXTARG,0, setifwpagroupcipher },
{ "wpaprotos", NEXTARG,0, setifwpaprotos },
{ "wpakey", NEXTARG,0, setifwpakey },
+ { "wpakeyfile", NEXTARG,0, setifwpakeyfile },
{ "-wpakey",-1, 0, setifwpakey },
{ "chan", NEXTARG0, 0, setifchan },
{ "-chan", -1, 0, setifchan },
@@ -728,7 +732,7 @@ main(int argc, char *argv[])
int create = 0;
int Cflag = 0;
int gflag = 0;
- int found_rulefile = 0;
+ int found_rulefile = 0, found_wpakeyfile = 0, wpafileidx = 0;
int i;
/* If no args at all, print all interfaces. */
@@ -785,9 +789,13 @@ main(int argc, char *argv[])
found_rulefile = 1;
break;
}
+ if (strcmp(argv[i], "wpakeyfile") == 0) {
+ found_wpakeyfile = 1;
+ break;
+ }
}
- if (!found_rulefile) {
+ if (!found_rulefile && !found_wpakeyfile) {
if (unveil(_PATH_RESCONF, "r") == -1)
err(1, "unveil");
if (unveil(_PATH_HOSTS, "r") == -1)
@@ -2240,6 +2248,40 @@ setifwpakey(const char *val, int d)
wpa.i_enabled = psk.i_enabled;
if (ioctl(sock, SIOCS80211WPAPARMS, (caddr_t)) == -1)
err(1, "SIOCS80211WPAPARMS");
+}
+
+void
+setifwpakeyfile(const char *val, int d)
+{
+ char *wpakey;
+ int fd;
+ struct stat sb;
+ ssize_t n;
+
+ fd = open(val, O_RDONLY);
+ if (fd == -1)
+ err(1, "open %s", val);
+
+ if (fstat(fd, ) == -1)
+ err(1, "fstat %s", val);
+
+ wpakey = malloc(sb.st_size);
+ if (wpakey == NULL)
+ err(1, "malloc");
+
+ n = read(fd, wpakey, sb.st_size);
+ if (n == -1)
+ err(1, "read %s", val);
+ if (n != sb.st_size)
+ errx(1, "failed to read from file %s", val);
+ close(fd);
+
+ while (n > 0 && isspace(wpakey[n - 1])) {
+ wpakey[n - 1] = '\0';
+ n--;
+ }
+
+ setifwpakey(wpakey, d);
}
void
netstart: IPv6 routes: do not redirect already quiet stdout
Came here while testing an IPv6 related sys/net/rtsock.c diff: all invocations use `-q' and route(8) says -q Suppress all output. so the redirection is duplicate. If route still prints to standard output despite the quiet flag I want to see such a bug and fix it. Note that this does not involve standard error which is neither effected by `-q' nor redirected. OK? Index: netstart === RCS file: /cvs/src/etc/netstart,v retrieving revision 1.201 diff -u -p -r1.201 netstart --- netstart25 Oct 2019 06:01:27 - 1.201 +++ netstart14 Jan 2020 22:46:22 - @@ -254,26 +254,26 @@ if ifconfig lo0 inet6 >/dev/null 2>&1; t ip6kernel=YES # Disallow link-local unicast dest without outgoing scope identifiers. - route -qn add -inet6 fe80:: -prefixlen 10 ::1 -reject >/dev/null + route -qn add -inet6 fe80:: -prefixlen 10 ::1 -reject # Disallow site-local unicast dest without outgoing scope identifiers. # If you configure site-locals without scope id (it is permissible # config for routers that are not on scope boundary), you may want # to comment the line out. - route -qn add -inet6 fec0:: -prefixlen 10 ::1 -reject >/dev/null + route -qn add -inet6 fec0:: -prefixlen 10 ::1 -reject # Disallow "internal" addresses to appear on the wire. - route -qn add -inet6 :::0.0.0.0 -prefixlen 96 ::1 -reject >/dev/null + route -qn add -inet6 :::0.0.0.0 -prefixlen 96 ::1 -reject # Disallow packets to malicious 6to4 prefix. - route -qn add -inet6 2002:e000:: -prefixlen 20 ::1 -reject >/dev/null - route -qn add -inet6 2002:7f00:: -prefixlen 24 ::1 -reject >/dev/null - route -qn add -inet6 2002::: -prefixlen 24 ::1 -reject >/dev/null - route -qn add -inet6 2002:ff00:: -prefixlen 24 ::1 -reject >/dev/null + route -qn add -inet6 2002:e000:: -prefixlen 20 ::1 -reject + route -qn add -inet6 2002:7f00:: -prefixlen 24 ::1 -reject + route -qn add -inet6 2002::: -prefixlen 24 ::1 -reject + route -qn add -inet6 2002:ff00:: -prefixlen 24 ::1 -reject # Disallow packets without scope identifier. - route -qn add -inet6 ff01:: -prefixlen 16 ::1 -reject >/dev/null - route -qn add -inet6 ff02:: -prefixlen 16 ::1 -reject >/dev/null + route -qn add -inet6 ff01:: -prefixlen 16 ::1 -reject + route -qn add -inet6 ff02:: -prefixlen 16 ::1 -reject # Completely disallow packets to IPv4 compatible prefix. # @@ -290,7 +290,7 @@ if ifconfig lo0 inet6 >/dev/null 2>&1; t # # Due to rare use of IPv4 compatible addresses, and security issues # with it, we disable it by default. - route -qn add -inet6 ::0.0.0.0 -prefixlen 96 ::1 -reject >/dev/null + route -qn add -inet6 ::0.0.0.0 -prefixlen 96 ::1 -reject else ip6kernel=NO fi
Re: qle(4): tsleep(9) -> tsleep_nsec(9)
On Mon, Jan 13, 2020 at 06:32:00PM -0600, Scott Cheloha wrote:
> These sleeps don't have units, though in practice they are 1 second.
> Just prior in the code I see a delay(9) of 100 microseconds. Is the
> 100 ticks here a typo? What is a reasonable sleep duration for this
> hardware?
Both of these are inside loops that terminate when the hardware (actually the
fibrechannel switch the hardware is connected to, in this case) responsds.
The tsleep length is arbitrary, so one second is as good as anything else.
ok jmatthew@
>
> Index: pci/qle.c
> ===
> RCS file: /cvs/src/sys/dev/pci/qle.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 qle.c
> --- pci/qle.c 31 Dec 2019 22:57:07 - 1.48
> +++ pci/qle.c 14 Jan 2020 00:29:34 -
> @@ -1886,7 +1886,8 @@ qle_ct_pass_through(struct qle_softc *sc
> if (qle_read_isr(sc, , ) != 0)
> qle_handle_intr(sc, isr, info);
> } else {
> - tsleep(sc->sc_scratch, PRIBIO, "qle_fabric", 100);
> + tsleep_nsec(sc->sc_scratch, PRIBIO, "qle_fabric",
> + SEC_TO_NSEC(1));
> }
> }
> if (rv == 0)
> @@ -2014,7 +2015,8 @@ qle_fabric_plogx(struct qle_softc *sc, s
> if (qle_read_isr(sc, , ) != 0)
> qle_handle_intr(sc, isr, info);
> } else {
> - tsleep(sc->sc_scratch, PRIBIO, "qle_fabric", 100);
> + tsleep_nsec(sc->sc_scratch, PRIBIO, "qle_fabric",
> + SEC_TO_NSEC(1));
> }
> }
> sc->sc_fabric_pending = 0;
Re: iked(8): get rid of IPv6 flow and -6 flag?
On 2020/01/14 21:48, Stuart Henderson wrote: > On 2020/01/14 21:03, Tobias Heider wrote: > > On Tue, Jan 14, 2020 at 09:17:11AM -0700, Theo de Raadt wrote: > > > Stuart Henderson wrote: > > > > > > > On 2020/01/13 20:51, Klemens Nanni wrote: > > > > > I'm in favour of removing the option and OK with your diff, but simply > > > > > removing it is probably a bad idea given its nature. > > > > > > > > > > What about printing a deprecation warning so that users can safely > > > > > adjust their rcctl flags instead of running into "iked(failed)" on the > > > > > next snapshot. > > > > > > > > Yes please make -6 a noop or a warning rather than an error. Sometimes > > > > breakage is unavoidable, but this isn't one of those cases. > > > > > > I agree. > > > > > > > Makes sense. I added a warning and a notice in current.html. > > > > ok? > > > > Index: www/faq/current.html > > === > > RCS file: /cvs/www/faq/current.html,v > > retrieving revision 1.1017 > > diff -u -p -r1.1017 current.html > > --- www/faq/current.html31 Dec 2019 02:18:01 - 1.1017 > > +++ www/faq/current.html14 Jan 2020 19:32:25 - > > @@ -135,6 +135,12 @@ or they can be rebuilt from ports. > > > > > > +2020/1/14 - iked(8) automatic IPv6 blocking removed > > > > + > > +https://man.openbsd.org/iked.8;>iked(8) no longer > > automatically adds > > +an IPv6 blocking IPsec flow. > > +The -6 option is deprecated and should be removed from > > + > href="https://man.openbsd.org/rc.conf.local.8;>/etc/rc.conf.local. > > How about this? > > > Index: current.html > === > RCS file: /cvs/www/faq/current.html,v > retrieving revision 1.1017 > diff -u -p -r1.1017 current.html > --- current.html 31 Dec 2019 02:18:01 - 1.1017 > +++ current.html 14 Jan 2020 21:47:35 - > @@ -136,6 +136,33 @@ or they can be rebuilt from ports. > --> > > > +2020/1/14 - iked(8) automatic IPv6 blocking removed > + > +https://man.openbsd.org/iked.8;>iked(8) no longer automatically > +blocks unencrypted outbound IPv6 packets. > +This feature was intended to avoid accidental leakage, but in practice was > +found to mostly be a cause of misconfiguration. > +The -6 flag was used to disable this feature but is now no > longer > +needed and should be removed from +href="https://man.openbsd.org/rc.conf.local.8;>/etc/rc.conf.local > +if used. > + > +Instead, if you would like to explicitly block these packets, add the > following Actually, on reading it back now I've posted it, "instead" is bad here, with the previous sentence it makes it seem like this is something to do if you *did* use -6, when actually it's something to do if you *didn't* use -6 and want to keep the feature. ... So here's some reordering that works better: Index: current.html === RCS file: /cvs/www/faq/current.html,v retrieving revision 1.1017 diff -u -p -r1.1017 current.html --- current.html31 Dec 2019 02:18:01 - 1.1017 +++ current.html14 Jan 2020 21:53:31 - @@ -136,6 +136,34 @@ or they can be rebuilt from ports. --> +2020/1/14 - iked(8) automatic IPv6 blocking removed + +https://man.openbsd.org/iked.8;>iked(8) no longer automatically +blocks unencrypted outbound IPv6 packets. +This feature was intended to avoid accidental leakage, but in practice was +found to mostly be a cause of misconfiguration. +Instead, if you would like to explicitly block these packets, add the following +line to https://man.openbsd.org/ipsec.conf.5;>/etc/ipsec.conf +(not iked.conf): + + +flow esp out from ::/0 to ::/0 type deny + + +and enable loading it with + + +# rcctl enable ipsec # to load at boot +# ipsecctl -f /etc/ipsec.conf # to load immediately + + +If you previously used https://man.openbsd.org/iked.8;>iked(8)'s +-6 flag to disable this feature, it is no longer needed and should +be removed from https://man.openbsd.org/rc.conf.local.8;>/etc/rc.conf.local +if used. + +
Re: iked(8): get rid of IPv6 flow and -6 flag?
On 2020/01/14 21:03, Tobias Heider wrote: > On Tue, Jan 14, 2020 at 09:17:11AM -0700, Theo de Raadt wrote: > > Stuart Henderson wrote: > > > > > On 2020/01/13 20:51, Klemens Nanni wrote: > > > > I'm in favour of removing the option and OK with your diff, but simply > > > > removing it is probably a bad idea given its nature. > > > > > > > > What about printing a deprecation warning so that users can safely > > > > adjust their rcctl flags instead of running into "iked(failed)" on the > > > > next snapshot. > > > > > > Yes please make -6 a noop or a warning rather than an error. Sometimes > > > breakage is unavoidable, but this isn't one of those cases. > > > > I agree. > > > > Makes sense. I added a warning and a notice in current.html. > > ok? > > Index: www/faq/current.html > === > RCS file: /cvs/www/faq/current.html,v > retrieving revision 1.1017 > diff -u -p -r1.1017 current.html > --- www/faq/current.html 31 Dec 2019 02:18:01 - 1.1017 > +++ www/faq/current.html 14 Jan 2020 19:32:25 - > @@ -135,6 +135,12 @@ or they can be rebuilt from ports. > > > +2020/1/14 - iked(8) automatic IPv6 blocking removed > + > +https://man.openbsd.org/iked.8;>iked(8) no longer automatically > adds > +an IPv6 blocking IPsec flow. > +The -6 option is deprecated and should be removed from > + href="https://man.openbsd.org/rc.conf.local.8;>/etc/rc.conf.local. How about this? Index: current.html === RCS file: /cvs/www/faq/current.html,v retrieving revision 1.1017 diff -u -p -r1.1017 current.html --- current.html31 Dec 2019 02:18:01 - 1.1017 +++ current.html14 Jan 2020 21:47:35 - @@ -136,6 +136,33 @@ or they can be rebuilt from ports. --> +2020/1/14 - iked(8) automatic IPv6 blocking removed + +https://man.openbsd.org/iked.8;>iked(8) no longer automatically +blocks unencrypted outbound IPv6 packets. +This feature was intended to avoid accidental leakage, but in practice was +found to mostly be a cause of misconfiguration. +The -6 flag was used to disable this feature but is now no longer +needed and should be removed from https://man.openbsd.org/rc.conf.local.8;>/etc/rc.conf.local +if used. + +Instead, if you would like to explicitly block these packets, add the following +line to https://man.openbsd.org/ipsec.conf.5;>/etc/ipsec.conf +(not iked.conf): + + +flow esp out from ::/0 to ::/0 type deny + + +and enable loading it with + + +# rcctl enable ipsec # to load at boot +# ipsecctl -f /etc/ipsec.conf # to load immediately + + +
Re: iked(8): get rid of IPv6 flow and -6 flag?
On Tue, Jan 14, 2020 at 09:03:04PM +0100, Tobias Heider wrote: > Makes sense. I added a warning and a notice in current.html. OK kn
Re: in httpd, use the correct configured server config
Thanks for the diff, commited. Sebastian Benoit(be...@openbsd.org) on 2020.01.14 21:14:44 +0100: > seems sensible. > > ok benno@ > > > Tracey Emery(tra...@traceyemery.net) on 2020.01.14 13:08:03 -0700: > > Hello, > > > > In the server_response function of httpd, the if comparison to > > srv_conf->maxrequests is using the wrong value. The value is derived from > > the > > first server configuration in httpd.conf, since we still don't know which > > server > > name the client is requesting. > > > > This small diff moves srv_conf->maxrequests usage in server_response to > > where we finally know which configured server config to actually use. > > This ensures we are not using the first configured server config in the > > queue. > > > > Thanks, > > > > Tracey > > > > -- > > > > Tracey Emery > > > > diff e80014c68b2561493718bbcef6e7fcb172d7f885 /usr/src > > blob - 326daa6a687ff7159adc744f66b38e6ea9e266eb > > file + usr.sbin/httpd/server_http.c > > --- usr.sbin/httpd/server_http.c > > +++ usr.sbin/httpd/server_http.c > > @@ -1232,13 +1232,6 @@ server_response(struct httpd *httpd, struct client > > *cl > > clt->clt_persist = 0; > > } > > > > - if (clt->clt_persist >= srv_conf->maxrequests) > > - clt->clt_persist = 0; > > - > > - /* pipelining should end after the first "idempotent" method */ > > - if (clt->clt_pipelining && clt->clt_toread > 0) > > - clt->clt_persist = 0; > > - > > /* > > * Do we have a Host header and matching configuration? > > * XXX the Host can also appear in the URL path. > > @@ -1291,6 +1284,13 @@ server_response(struct httpd *httpd, struct client > > *cl > > goto fail; > > srv_conf = clt->clt_srv_conf; > > } > > + > > + if (clt->clt_persist >= srv_conf->maxrequests) > > + clt->clt_persist = 0; > > + > > + /* pipelining should end after the first "idempotent" method */ > > + if (clt->clt_pipelining && clt->clt_toread > 0) > > + clt->clt_persist = 0; > > > > if ((desc->http_host = strdup(hostname)) == NULL) > > goto fail; > > >
Re: in httpd, use the correct configured server config
seems sensible. ok benno@ Tracey Emery(tra...@traceyemery.net) on 2020.01.14 13:08:03 -0700: > Hello, > > In the server_response function of httpd, the if comparison to > srv_conf->maxrequests is using the wrong value. The value is derived from the > first server configuration in httpd.conf, since we still don't know which > server > name the client is requesting. > > This small diff moves srv_conf->maxrequests usage in server_response to > where we finally know which configured server config to actually use. > This ensures we are not using the first configured server config in the > queue. > > Thanks, > > Tracey > > -- > > Tracey Emery > > diff e80014c68b2561493718bbcef6e7fcb172d7f885 /usr/src > blob - 326daa6a687ff7159adc744f66b38e6ea9e266eb > file + usr.sbin/httpd/server_http.c > --- usr.sbin/httpd/server_http.c > +++ usr.sbin/httpd/server_http.c > @@ -1232,13 +1232,6 @@ server_response(struct httpd *httpd, struct client *cl > clt->clt_persist = 0; > } > > - if (clt->clt_persist >= srv_conf->maxrequests) > - clt->clt_persist = 0; > - > - /* pipelining should end after the first "idempotent" method */ > - if (clt->clt_pipelining && clt->clt_toread > 0) > - clt->clt_persist = 0; > - > /* >* Do we have a Host header and matching configuration? >* XXX the Host can also appear in the URL path. > @@ -1291,6 +1284,13 @@ server_response(struct httpd *httpd, struct client *cl > goto fail; > srv_conf = clt->clt_srv_conf; > } > + > + if (clt->clt_persist >= srv_conf->maxrequests) > + clt->clt_persist = 0; > + > + /* pipelining should end after the first "idempotent" method */ > + if (clt->clt_pipelining && clt->clt_toread > 0) > + clt->clt_persist = 0; > > if ((desc->http_host = strdup(hostname)) == NULL) > goto fail; >
in httpd, use the correct configured server config
Hello, In the server_response function of httpd, the if comparison to srv_conf->maxrequests is using the wrong value. The value is derived from the first server configuration in httpd.conf, since we still don't know which server name the client is requesting. This small diff moves srv_conf->maxrequests usage in server_response to where we finally know which configured server config to actually use. This ensures we are not using the first configured server config in the queue. Thanks, Tracey -- Tracey Emery diff e80014c68b2561493718bbcef6e7fcb172d7f885 /usr/src blob - 326daa6a687ff7159adc744f66b38e6ea9e266eb file + usr.sbin/httpd/server_http.c --- usr.sbin/httpd/server_http.c +++ usr.sbin/httpd/server_http.c @@ -1232,13 +1232,6 @@ server_response(struct httpd *httpd, struct client *cl clt->clt_persist = 0; } - if (clt->clt_persist >= srv_conf->maxrequests) - clt->clt_persist = 0; - - /* pipelining should end after the first "idempotent" method */ - if (clt->clt_pipelining && clt->clt_toread > 0) - clt->clt_persist = 0; - /* * Do we have a Host header and matching configuration? * XXX the Host can also appear in the URL path. @@ -1291,6 +1284,13 @@ server_response(struct httpd *httpd, struct client *cl goto fail; srv_conf = clt->clt_srv_conf; } + + if (clt->clt_persist >= srv_conf->maxrequests) + clt->clt_persist = 0; + + /* pipelining should end after the first "idempotent" method */ + if (clt->clt_pipelining && clt->clt_toread > 0) + clt->clt_persist = 0; if ((desc->http_host = strdup(hostname)) == NULL) goto fail;
Re: iked(8): get rid of IPv6 flow and -6 flag?
On Tue, Jan 14, 2020 at 09:17:11AM -0700, Theo de Raadt wrote: > Stuart Henderson wrote: > > > On 2020/01/13 20:51, Klemens Nanni wrote: > > > I'm in favour of removing the option and OK with your diff, but simply > > > removing it is probably a bad idea given its nature. > > > > > > What about printing a deprecation warning so that users can safely > > > adjust their rcctl flags instead of running into "iked(failed)" on the > > > next snapshot. > > > > Yes please make -6 a noop or a warning rather than an error. Sometimes > > breakage is unavoidable, but this isn't one of those cases. > > I agree. > Makes sense. I added a warning and a notice in current.html. ok? Index: www/faq/current.html === RCS file: /cvs/www/faq/current.html,v retrieving revision 1.1017 diff -u -p -r1.1017 current.html --- www/faq/current.html31 Dec 2019 02:18:01 - 1.1017 +++ www/faq/current.html14 Jan 2020 19:32:25 - @@ -135,6 +135,12 @@ or they can be rebuilt from ports. +2020/1/14 - iked(8) automatic IPv6 blocking removed + +https://man.openbsd.org/iked.8;>iked(8) no longer automatically adds +an IPv6 blocking IPsec flow. +The -6 option is deprecated and should be removed from +https://man.openbsd.org/rc.conf.local.8;>/etc/rc.conf.local.
Re: umb(4) WIP diff and questions
Theo de Raadt wrote: > Stuart Henderson wrote: > > > On 2020/01/14 10:27, Theo de Raadt wrote: > > > Unfortunate part of this diff is that the password is (very > > > momentarily) visible with ps(1) in the root-run ifconfig argv[] array. > > > It's a tight race, but still it is visible. > > > > > > People do run "sh /etc/netstart umb0" to activate the interface > > > during multiuser. > > > > > > If the password is truly sensitive, it should be placed in a file, > > > and the ifconfig's extension should be changed to read the file. > > > > That's not unique to umb though, it's been a problem forever with carp, > > pppoe and especially wlan interfaces. > > When creating new versions of the same problem... that's a great time > to reconsider the old solutions. > > > Another fix would be to accept > > ifconfig options on stdin, which is more convenient for quick runtime > > changes than two steps of writing to a file then pointing ifconfig at > > it, and changing netstart to use it would improve things for existing > > users without them needing to touch any config files. > > I think using the shell is silly, because what if there are two such > options? Then you can't seperate the stdin. > > Additionally, most of the time when creating such temporary configuration, > the pattern is that if it works you'll want to apply it permanent. > > Another thought is to create both versions of the option. One on the > commandline, and one pointing at a file. > > Channeling a conversation from 15 years ago: "How about wpakeyfile" Another consideration is... many of these passwords are locked to narrow usage cases, so does it really matter all that much?
Re: diff: simplify globbing in ftpd(8)
On Mon, Jan 13, 2020 at 10:30:11AM -0700, Todd C. Miller wrote:
> On Mon, 13 Jan 2020 13:45:55 +0100, Jan Klemkow wrote:
>
> > This diff simplifies globbing for the ftpd(8) ls and stat command. And
> > it avoids to glob for the parameters "-lgA".
> >
> > Due, we just pass a single string to the ftpd_ls() function, we don't
> > need to provide infrastructure for a dynamic list of arguments.
>
> This conflicts with the diff to support NLIST arguments from SASANO
> Takayoshi. I think we can support this by adding a flag to ftpd_ls()
> that indicates whether it is a long list or not.
Here is an improved version of the diff, but as I explaind here [1]
without argument injection. I stopped argument injection by adding
"--" before client paramenters for ls(1).
[1]: https://marc.info/?l=openbsd-tech=157900764005335=2
OK?
Thanks,
Jan
Index: extern.h
===
RCS file: /cvs/src/libexec/ftpd/extern.h,v
retrieving revision 1.20
diff -u -p -r1.20 extern.h
--- extern.h8 May 2019 23:56:48 - 1.20
+++ extern.h13 Jan 2020 11:03:39 -
@@ -68,7 +68,7 @@ void delete(char *);
void dologout(int);
void fatal(char *);
intftpd_pclose(FILE *, pid_t);
-FILE *ftpd_ls(char *, char *, pid_t *);
+FILE *ftpd_ls(const char *, pid_t *);
int get_line(char *, int, FILE *);
void ftpdlogwtmp(char *, char *, char *);
void lreply(int, const char *, ...);
Index: ftpd.c
===
RCS file: /cvs/src/libexec/ftpd/ftpd.c,v
retrieving revision 1.228
diff -u -p -r1.228 ftpd.c
--- ftpd.c 3 Jul 2019 03:24:04 - 1.228
+++ ftpd.c 13 Jan 2020 11:15:31 -
@@ -1124,7 +1124,7 @@ retrieve(enum ret_cmd cmd, char *name)
fin = fopen(name, "r");
st.st_size = 0;
} else {
- fin = ftpd_ls("-lgA", name, );
+ fin = ftpd_ls(name, );
st.st_size = -1;
st.st_blksize = BUFSIZ;
}
@@ -1730,7 +1730,7 @@ statfilecmd(char *filename)
int c;
int atstart;
pid_t pid;
- fin = ftpd_ls("-lgA", filename, );
+ fin = ftpd_ls(filename, );
if (fin == NULL) {
reply(451, "Local resource failure");
return;
Index: popen.c
===
RCS file: /cvs/src/libexec/ftpd/popen.c,v
retrieving revision 1.28
diff -u -p -r1.28 popen.c
--- popen.c 28 Jun 2019 13:32:53 - 1.28
+++ popen.c 14 Jan 2020 18:54:52 -
@@ -60,51 +60,45 @@
#define MAX_GARGV 1000
FILE *
-ftpd_ls(char *arg, char *path, pid_t *pidptr)
+ftpd_ls(const char *path, pid_t *pidptr)
{
char *cp;
FILE *iop;
- int argc = 0, gargc, pdes[2];
+ int argc = 0, pdes[2];
pid_t pid;
- char **pop, *argv[MAX_ARGV], *gargv[MAX_GARGV];
+ char **pop, *argv[MAX_ARGV];
if (pipe(pdes) == -1)
return (NULL);
/* break up string into pieces */
argv[argc++] = "/bin/ls";
- if (arg != NULL)
- argv[argc++] = arg;
- if (path != NULL)
- argv[argc++] = path;
- argv[argc] = NULL;
- argv[MAX_ARGV-1] = NULL;
+ argv[argc++] = "-lgA";
+ argv[argc++] = "--";
- /* glob each piece */
- gargv[0] = argv[0];
- for (gargc = argc = 1; argv[argc]; argc++) {
+ /* glob that path */
+ if (path != NULL) {
glob_t gl;
memset(, 0, sizeof(gl));
- if (glob(argv[argc],
+ if (glob(path,
GLOB_BRACE|GLOB_NOCHECK|GLOB_QUOTE|GLOB_TILDE|GLOB_LIMIT,
NULL, )) {
- if (gargc < MAX_GARGV-1) {
- gargv[gargc++] = strdup(argv[argc]);
- if (gargv[gargc -1] == NULL)
- fatal ("Out of memory.");
- }
+ argv[argc++] = strdup(path);
+ if (argv[argc -1] == NULL)
+ fatal ("Out of memory.");
} else if (gl.gl_pathc > 0) {
- for (pop = gl.gl_pathv; *pop && gargc < MAX_GARGV-1;
pop++) {
- gargv[gargc++] = strdup(*pop);
- if (gargv[gargc - 1] == NULL)
+ for (pop = gl.gl_pathv; *pop && argc < MAX_GARGV-1;
pop++) {
+ argv[argc++] = strdup(*pop);
+ if (argv[argc - 1] == NULL)
fatal ("Out of memory.");
}
}
globfree();
}
- gargv[gargc] = NULL;
+ argv[argc] = NULL;
+ argv[MAX_ARGV-1] = NULL;
iop = NULL;
@@ -128,15 +122,15 @@ ftpd_ls(char *arg, char *path, pid_t
Re: umb(4) WIP diff and questions
Stuart Henderson wrote: > On 2020/01/14 10:27, Theo de Raadt wrote: > > Unfortunate part of this diff is that the password is (very > > momentarily) visible with ps(1) in the root-run ifconfig argv[] array. > > It's a tight race, but still it is visible. > > > > People do run "sh /etc/netstart umb0" to activate the interface > > during multiuser. > > > > If the password is truly sensitive, it should be placed in a file, > > and the ifconfig's extension should be changed to read the file. > > That's not unique to umb though, it's been a problem forever with carp, > pppoe and especially wlan interfaces. When creating new versions of the same problem... that's a great time to reconsider the old solutions. > Another fix would be to accept > ifconfig options on stdin, which is more convenient for quick runtime > changes than two steps of writing to a file then pointing ifconfig at > it, and changing netstart to use it would improve things for existing > users without them needing to touch any config files. I think using the shell is silly, because what if there are two such options? Then you can't seperate the stdin. Additionally, most of the time when creating such temporary configuration, the pattern is that if it works you'll want to apply it permanent. Another thought is to create both versions of the option. One on the commandline, and one pointing at a file. Channeling a conversation from 15 years ago: "How about wpakeyfile"
Re: umb(4) WIP diff and questions
On 2020/01/14 10:27, Theo de Raadt wrote: > Unfortunate part of this diff is that the password is (very > momentarily) visible with ps(1) in the root-run ifconfig argv[] array. > It's a tight race, but still it is visible. > > People do run "sh /etc/netstart umb0" to activate the interface > during multiuser. > > If the password is truly sensitive, it should be placed in a file, > and the ifconfig's extension should be changed to read the file. That's not unique to umb though, it's been a problem forever with carp, pppoe and especially wlan interfaces. Another fix would be to accept ifconfig options on stdin, which is more convenient for quick runtime changes than two steps of writing to a file then pointing ifconfig at it, and changing netstart to use it would improve things for existing users without them needing to touch any config files.
Re: apply changes immediately to join'd essids
On Tue, Jan 14, 2020 at 06:12:27PM +0100, Peter Hessler wrote:
> Updated diff
>
>
Look good. Thanks!
ok stsp@
> Index: net80211/ieee80211_ioctl.c
> ===
> RCS file: /home/cvs/openbsd/src/sys/net80211/ieee80211_ioctl.c,v
> retrieving revision 1.78
> diff -u -p -u -p -r1.78 ieee80211_ioctl.c
> --- net80211/ieee80211_ioctl.c13 Jan 2020 09:57:25 - 1.78
> +++ net80211/ieee80211_ioctl.c14 Jan 2020 13:52:18 -
> @@ -512,6 +512,8 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
> case SIOCS80211JOIN:
> if ((error = suser(curproc)) != 0)
> break;
> + if (ic->ic_opmode != IEEE80211_M_STA)
> + break;
> if ((error = copyin(ifr->ifr_data, , sizeof(join))) != 0)
> break;
> if (join.i_len > IEEE80211_NWID_LEN) {
> @@ -543,7 +545,13 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
> if (ic->ic_des_esslen == join.i_len &&
> memcmp(join.i_nwid, ic->ic_des_essid,
> join.i_len) == 0) {
> + struct ieee80211_node *ni;
> +
> ieee80211_deselect_ess(ic);
> + ni = ieee80211_find_node(ic,
> + ic->ic_bss->ni_bssid);
> + if (ni != NULL)
> + ieee80211_free_node(ic, ni);
> error = ENETRESET;
> }
> /* save nwid for auto-join */
> Index: net80211/ieee80211_node.c
> ===
> RCS file: /home/cvs/openbsd/src/sys/net80211/ieee80211_node.c,v
> retrieving revision 1.178
> diff -u -p -u -p -r1.178 ieee80211_node.c
> --- net80211/ieee80211_node.c 29 Dec 2019 14:00:36 - 1.178
> +++ net80211/ieee80211_node.c 14 Jan 2020 13:53:55 -
> @@ -72,7 +72,6 @@ int ieee80211_ess_is_better(struct ieee8
> void ieee80211_node_set_timeouts(struct ieee80211_node *);
> void ieee80211_setup_node(struct ieee80211com *, struct ieee80211_node *,
> const u_int8_t *);
> -void ieee80211_free_node(struct ieee80211com *, struct ieee80211_node *);
> struct ieee80211_node *ieee80211_alloc_node_helper(struct ieee80211com *);
> void ieee80211_node_switch_bss(struct ieee80211com *, struct ieee80211_node
> *);
> void ieee80211_node_addba_request(struct ieee80211_node *, int);
> Index: net80211/ieee80211_node.h
> ===
> RCS file: /home/cvs/openbsd/src/sys/net80211/ieee80211_node.h,v
> retrieving revision 1.84
> diff -u -p -u -p -r1.84 ieee80211_node.h
> --- net80211/ieee80211_node.h 29 Dec 2019 13:49:22 - 1.84
> +++ net80211/ieee80211_node.h 14 Jan 2020 13:54:18 -
> @@ -533,6 +533,7 @@ void ieee80211_create_ibss(struct ieee80
> struct ieee80211_channel *);
> void ieee80211_notify_dtim(struct ieee80211com *);
> void ieee80211_set_tim(struct ieee80211com *, int, int);
> +void ieee80211_free_node(struct ieee80211com *, struct ieee80211_node *);
>
> int ieee80211_node_cmp(const struct ieee80211_node *,
> const struct ieee80211_node *);
>
>
> --
> This sentence contradicts itself -- no actually it doesn't.
> -- Hofstadter
>
>
Re: acpivout: try to consistently adjust brightness by 5%
On Mon, Jan 13, 2020 at 12:20:10PM +0100, Patrick Wildt wrote:
> The problem is that the last two values are 67 and 100. If you go
> 5% down, it's 95. The nearest will still be 100. The code then
> realizes that it's the same level as before, and does nlevel--.
> But nlevel-- is 99, and not 67, because nlevel is the value and
> not the index of the bcl array. So in essence the change needed
> is to decrease the index, not the value, and then look up the value.
Disucssing this over dinner again, patrick and I independently came up
with the very same diff below.
It makes acpivout_find_brightness() return an index instead a level for
the selected brightness.
Just works on my X230; I can go through every level with the function
keys and verify with xbacklight(1) that I am indeed not skipping any of
the 16 levels in either direction.
OK?
Index: acpivout.c
===
RCS file: /cvs/src/sys/dev/acpi/acpivout.c,v
retrieving revision 1.16
diff -u -p -r1.16 acpivout.c
--- acpivout.c 14 Dec 2019 10:57:48 - 1.16
+++ acpivout.c 14 Jan 2020 18:50:23 -
@@ -165,7 +165,7 @@ acpivout_brightness_cycle(struct acpivou
void
acpivout_brightness_step(struct acpivout_softc *sc, int dir)
{
- int level, nlevel;
+ int level, nindex;
if (sc->sc_bcl_len == 0)
return;
@@ -173,17 +173,17 @@ acpivout_brightness_step(struct acpivout
if (level == -1)
return;
- nlevel = acpivout_find_brightness(sc, level + (dir * BRIGHTNESS_STEP));
- if (nlevel == level) {
- if (dir == 1 && (nlevel + 1 < sc->sc_bcl_len))
- nlevel++;
- else if (dir == -1 && (nlevel - 1 >= 0))
- nlevel--;
+ nindex = acpivout_find_brightness(sc, level + (dir * BRIGHTNESS_STEP));
+ if (sc->sc_bcl[nindex] == level) {
+ if (dir == 1 && (nindex + 1 < sc->sc_bcl_len))
+ nindex++;
+ else if (dir == -1 && (nindex - 1 >= 0))
+ nindex--;
}
- if (nlevel == level)
+ if (sc->sc_bcl[nindex] == level)
return;
- acpivout_set_brightness(sc, nlevel);
+ acpivout_set_brightness(sc, sc->sc_bcl[nindex]);
}
void
@@ -219,14 +219,14 @@ acpivout_find_brightness(struct acpivout
for (i = 0; i < sc->sc_bcl_len - 1; i++) {
mid = sc->sc_bcl[i] + (sc->sc_bcl[i + 1] - sc->sc_bcl[i]) / 2;
if (sc->sc_bcl[i] <= level && level <= mid)
- return sc->sc_bcl[i];
+ return i;
if (mid < level && level <= sc->sc_bcl[i + 1])
- return sc->sc_bcl[i + 1];
+ return i + 1;
}
if (level < sc->sc_bcl[0])
- return sc->sc_bcl[0];
+ return 0;
else
- return sc->sc_bcl[i];
+ return i;
}
void
@@ -321,7 +321,7 @@ int
acpivout_set_param(struct wsdisplay_param *dp)
{
struct acpivout_softc *sc = NULL;
- int i, exact;
+ int i, nindex;
switch (dp->param) {
case WSDISPLAYIO_PARAM_BRIGHTNESS:
@@ -335,8 +335,8 @@ acpivout_set_param(struct wsdisplay_para
}
if (sc != NULL && sc->sc_bcl_len != 0) {
rw_enter_write(>sc_acpi->sc_lck);
- exact = acpivout_find_brightness(sc, dp->curval);
- acpivout_set_brightness(sc, exact);
+ nindex = acpivout_find_brightness(sc, dp->curval);
+ acpivout_set_brightness(sc, sc->sc_bcl[nindex]);
rw_exit_write(>sc_acpi->sc_lck);
return 0;
}
Re: nfs mp vnode list remove safe
On Tue, Jan 14, 2020 at 01:32:50PM -0500, Ted Unangst wrote:
> Alexander Bluhm wrote:
> > loop:
> > - TAILQ_FOREACH_SAFE(vp, >mnt_vnodelist, v_mntvnodes, nvp) {
> > + TAILQ_FOREACH(vp, >mnt_vnodelist, v_mntvnodes) {
> > if (vp->v_mount != mp) /* Paranoia */
> > goto loop;
>
> that looks like an infinite loop? if there's a bad vnode on the list, it'll
> still be there next time around.
Yes, that is stupid. We have it in multiple loops. Should be a
kassert. Here it was added at Sat May 5 17:07:46 1990 UTC:
https://svnweb.freebsd.org/csrg/sys/kern/vfs_subr.c?r1=41420=41421;
That was the oldest commit I have found. From there it spreaded
over the tree. Have to check whether a kassert fits everywhere.
bluhm
Re: nfs mp vnode list remove safe
Alexander Bluhm wrote:
> Hi,
>
> There is no remove and no sleep in this loop. So _SAFE is unnecessary.
>
> ok?
sure, with one bonus note.
>
> bluhm
>
> Index: nfs/nfs_subs.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_subs.c,v
> retrieving revision 1.141
> diff -u -p -r1.141 nfs_subs.c
> --- nfs/nfs_subs.c10 Jan 2020 10:33:35 - 1.141
> +++ nfs/nfs_subs.c13 Jan 2020 18:02:54 -
> @@ -1509,16 +1509,16 @@ netaddr_match(int family, union nethosta
> void
> nfs_clearcommit(struct mount *mp)
> {
> - struct vnode *vp, *nvp;
> - struct buf *bp, *nbp;
> + struct vnode *vp;
> + struct buf *bp;
> int s;
>
> s = splbio();
> loop:
> - TAILQ_FOREACH_SAFE(vp, >mnt_vnodelist, v_mntvnodes, nvp) {
> + TAILQ_FOREACH(vp, >mnt_vnodelist, v_mntvnodes) {
> if (vp->v_mount != mp) /* Paranoia */
> goto loop;
that looks like an infinite loop? if there's a bad vnode on the list, it'll
still be there next time around.
Re: add DIOCRADDADDRS ioctl to kern_pledge pf
On Tue, Jan 14, 2020 at 11:05:38AM -0700, Theo de Raadt wrote: > Some of the pledges (such as "pf") exist to support a cluster of > programs -- not just 1 program -- and improve their security by limiting > what they can do. So that when the program gets subverted due something > on it's input, the damage it can do is limited. > Additionally, the list of operations permitted is kept very small, so that > the switch() case in the kernel don't turn into a bloated fiasco. OK. > Your proposal supports 1 program. Which isn't even in base. There is > no way I'm going accept this change. OK, I see. Yes it was selfish of me. > Beyond that I need to once again point out a major missed step: > > Your proposal is to permit DIOCRADDADDRS in all the current programs > using "pf". But you did not assessment to determine if there is a > downside to giving them such access. You simply wanted to barrel along > forward, to support your one little program. > > Ignoring the impact of your changes on the rest of the ecosystem is > totally nuts. Thanks for pointing that out. I did take a look at one program in base though and noticed it wasn't pledged. But yes I barrelled along. Sorry. Regards, -peter
Re: add DIOCRADDADDRS ioctl to kern_pledge pf
> I'm in the process of building a program that adds IP addresses to a table, > from the network, It is HMAC'ed. > > I was stopped by a pledge, it seems it was not configured. Here is the > ktrace snippet: > > 40051 table-server CALL open(0xbb705fb11f6,0x2) > 40051 table-server NAMI "/dev/pf" > 40051 table-server RET open 4 > 40051 table-server CALL kbind(0x7f7a2b08,24,0x2de4af929c6b5090) > 40051 table-server RET kbind 0 > 40051 table-server CALL ioctl(4,DIOCRADDTABLES,0x7f7a32a8) > 40051 table-server RET ioctl 0 > 40051 table-server CALL kbind(0x7f7a2b08,24,0x2de4af929c6b5090) > 40051 table-server RET kbind 0 > 40051 table-server CALL ioctl(4,DIOCRADDADDRS,0x7f7a32a8) > 40051 table-server PLDG ioctl, "tty", errno 1 Operation not permitted > 40051 table-server PSIG SIGABRT SIG_DFL > 40051 table-server NAMI "table-server.core" > > Here is a patch to consider, it compiles but I haven't tested it yet because > I'm unsure if there is a reason why this DIOCR* was left out. Some of the pledges (such as "pf") exist to support a cluster of programs -- not just 1 program -- and improve their security by limiting what they can do. So that when the program gets subverted due something on it's input, the damage it can do is limited. Additionally, the list of operations permitted is kept very small, so that the switch() case in the kernel don't turn into a bloated fiasco. Your proposal supports 1 program. Which isn't even in base. There is no way I'm going accept this change. Beyond that I need to once again point out a major missed step: Your proposal is to permit DIOCRADDADDRS in all the current programs using "pf". But you did not assessment to determine if there is a downside to giving them such access. You simply wanted to barrel along forward, to support your one little program. Ignoring the impact of your changes on the rest of the ecosystem is totally nuts.
add DIOCRADDADDRS ioctl to kern_pledge pf
Hi, I'm in the process of building a program that adds IP addresses to a table, from the network, It is HMAC'ed. I was stopped by a pledge, it seems it was not configured. Here is the ktrace snippet: 40051 table-server CALL open(0xbb705fb11f6,0x2) 40051 table-server NAMI "/dev/pf" 40051 table-server RET open 4 40051 table-server CALL kbind(0x7f7a2b08,24,0x2de4af929c6b5090) 40051 table-server RET kbind 0 40051 table-server CALL ioctl(4,DIOCRADDTABLES,0x7f7a32a8) 40051 table-server RET ioctl 0 40051 table-server CALL kbind(0x7f7a2b08,24,0x2de4af929c6b5090) 40051 table-server RET kbind 0 40051 table-server CALL ioctl(4,DIOCRADDADDRS,0x7f7a32a8) 40051 table-server PLDG ioctl, "tty", errno 1 Operation not permitted 40051 table-server PSIG SIGABRT SIG_DFL 40051 table-server NAMI "table-server.core" Here is a patch to consider, it compiles but I haven't tested it yet because I'm unsure if there is a reason why this DIOCR* was left out. I'm guessing, if the patch is OK, I'll have to leave the pledge out for 6.6 which is what this is intended for. Sad, but OK, at least there is unveil. Index: kern_pledge.c === RCS file: /cvs/src/sys/kern/kern_pledge.c,v retrieving revision 1.256 diff -u -p -u -r1.256 kern_pledge.c --- kern_pledge.c 8 Dec 2019 23:08:59 - 1.256 +++ kern_pledge.c 14 Jan 2020 17:51:19 - @@ -1205,6 +1205,7 @@ pledge_ioctl(struct proc *p, long com, s case DIOCADDRULE: case DIOCGETSTATUS: case DIOCNATLOOK: + case DIOCRADDADDRS: case DIOCRADDTABLES: case DIOCRCLRADDRS: case DIOCRCLRTABLES: Cheers, -peter
Re: umb(4) WIP diff and questions
Unfortunate part of this diff is that the password is (very momentarily) visible with ps(1) in the root-run ifconfig argv[] array. It's a tight race, but still it is visible. People do run "sh /etc/netstart umb0" to activate the interface during multiuser. If the password is truly sensitive, it should be placed in a file, and the ifconfig's extension should be changed to read the file.
Re: Towards splitting SCHED_LOCK()
Mark Kettenis wrote: > > Date: Tue, 14 Jan 2020 10:34:05 +1100 > > From: Jonathan Gray > > > > On Mon, Jan 13, 2020 at 05:02:12PM +0100, Martin Pieuchot wrote: > > > I'm facing a lock ordering issue while profiling the scheduler which > > > cannot be solved without using a different lock for the global sleepqueue > > > and the runqueues. > > > > > > The SCHED_LOCK() currently protects both data structures as well as the > > > related fields in 'struct proc'. One of this fields is `p_wchan' which > > > is obviously related to the global sleepqueue. > > > > > > The cleaning diff below introduces a new function, awake(), that unify > > > the multiple uses of the following idiom: > > > > > > if (p->p_stat == SSLEEP) > > > setrunnable(p); > > > else > > > unsleep(p); > > > > > > This is generally done after checking if the thread `p' is on the > > > sleepqueue. > > > > Why not name it wakeup_proc() instead or something like endsleep()? > > > > awake() does not describe the action that is being done and > > if (awake()) reads like it could be > > if (p->p_stat != SSLEEP && p->p_stat != SSTOP) > > Must say I had reservations about the "awake" name as well, but jsg@ > nails it here since it both a verb and an adjective which creates > confusion. Both names suggester here are find I think. If you want an active verb that avoids the adjective confusion, you can consider awaken()
Re: apply changes immediately to join'd essids
On 2020 Jan 14 (Tue) at 13:11:57 +0100 (+0100), Stefan Sperling wrote:
:On Mon, Jan 13, 2020 at 10:38:35PM +0100, Peter Hessler wrote:
:> On 2020 Jan 12 (Sun) at 21:39:19 +0100 (+0100), Peter Hessler wrote:
:> :When we change attributes for a join essid, we should apply the change
:> :immediately instead of waiting to (randomly) switch away and switch
:> :back.
:>
:> And if we are connected to an AP, remove the node from the cache so we
:> can properly reconnect.
:>
:> OK?
:>
:>
...
:This should call ieee80211_free_node() instead. We should also make
:sure this code runs in station opmode (IEEE80211_M_STA) only.
:
...
Updated diff
Index: net80211/ieee80211_ioctl.c
===
RCS file: /home/cvs/openbsd/src/sys/net80211/ieee80211_ioctl.c,v
retrieving revision 1.78
diff -u -p -u -p -r1.78 ieee80211_ioctl.c
--- net80211/ieee80211_ioctl.c 13 Jan 2020 09:57:25 - 1.78
+++ net80211/ieee80211_ioctl.c 14 Jan 2020 13:52:18 -
@@ -512,6 +512,8 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
case SIOCS80211JOIN:
if ((error = suser(curproc)) != 0)
break;
+ if (ic->ic_opmode != IEEE80211_M_STA)
+ break;
if ((error = copyin(ifr->ifr_data, , sizeof(join))) != 0)
break;
if (join.i_len > IEEE80211_NWID_LEN) {
@@ -543,7 +545,13 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
if (ic->ic_des_esslen == join.i_len &&
memcmp(join.i_nwid, ic->ic_des_essid,
join.i_len) == 0) {
+ struct ieee80211_node *ni;
+
ieee80211_deselect_ess(ic);
+ ni = ieee80211_find_node(ic,
+ ic->ic_bss->ni_bssid);
+ if (ni != NULL)
+ ieee80211_free_node(ic, ni);
error = ENETRESET;
}
/* save nwid for auto-join */
Index: net80211/ieee80211_node.c
===
RCS file: /home/cvs/openbsd/src/sys/net80211/ieee80211_node.c,v
retrieving revision 1.178
diff -u -p -u -p -r1.178 ieee80211_node.c
--- net80211/ieee80211_node.c 29 Dec 2019 14:00:36 - 1.178
+++ net80211/ieee80211_node.c 14 Jan 2020 13:53:55 -
@@ -72,7 +72,6 @@ int ieee80211_ess_is_better(struct ieee8
void ieee80211_node_set_timeouts(struct ieee80211_node *);
void ieee80211_setup_node(struct ieee80211com *, struct ieee80211_node *,
const u_int8_t *);
-void ieee80211_free_node(struct ieee80211com *, struct ieee80211_node *);
struct ieee80211_node *ieee80211_alloc_node_helper(struct ieee80211com *);
void ieee80211_node_switch_bss(struct ieee80211com *, struct ieee80211_node *);
void ieee80211_node_addba_request(struct ieee80211_node *, int);
Index: net80211/ieee80211_node.h
===
RCS file: /home/cvs/openbsd/src/sys/net80211/ieee80211_node.h,v
retrieving revision 1.84
diff -u -p -u -p -r1.84 ieee80211_node.h
--- net80211/ieee80211_node.h 29 Dec 2019 13:49:22 - 1.84
+++ net80211/ieee80211_node.h 14 Jan 2020 13:54:18 -
@@ -533,6 +533,7 @@ void ieee80211_create_ibss(struct ieee80
struct ieee80211_channel *);
void ieee80211_notify_dtim(struct ieee80211com *);
void ieee80211_set_tim(struct ieee80211com *, int, int);
+void ieee80211_free_node(struct ieee80211com *, struct ieee80211_node *);
int ieee80211_node_cmp(const struct ieee80211_node *,
const struct ieee80211_node *);
--
This sentence contradicts itself -- no actually it doesn't.
-- Hofstadter
Re: umb(4) WIP diff and questions
On Tue, Jan 14, 2020 at 02:40:45PM +0100, Stefan Sperling wrote: > On Tue, Jan 14, 2020 at 09:51:05PM +0900, leeb wrote: > > Hello again tech@ > > > > I've included diffs of what I've got so far at the bottom > > of this mail, but first a couple of questions: > > > > - Using the full 510-character limits for username and > > passphrase specified in the MBIM spec, kernel compilation > > fails due to tripping the 2047-byte stack frame warning > > when compiling the driver. That's the reason for the '100' > > magic numbers that I put in there temporarily. > > > > Any hints on the best way to handle this would be appreciated. > > I would allocate mp dynamically instead of storing it on the stack. > > In umb_ioctl(), you could change mp to a pointer type: > > struct umb_parameter *mp; > > The ioctl is allowed to sleep until memory is available (M_WAITOK), so > this allocation cannot fail and you don't need to check for NULL: > > mp = malloc(sizeof(*mp), M_DEVBUF, M_ZERO | M_WAITOK); > > free mp before umb_ioctl returns: > > free(mp, M_DEVBUF, sizeof(*mp)); > > See 'man 9 malloc' for details. > > > - I included the username/passphrase fields in the ifconfig > > output, as it seems to me that APN settings are generally > > made public so end-users can configure their own devices > > If needed I'll take it out, or perhaps change it to display > > '*set*' (or similar) if you think it should be hidden? > > Genrally, the kernel should not return credentials to userland. > So these shouldn't just be hidden or obscured in ifconfig. Rather, > umb_ioctl should not copy such data out to any userland program. > > This rule also applies to wifi and carp interfaces, for instance. > > > A couple of other points: > > > > - Wireless providers where I am all seem to require a > > username/password with the APN. So I'm unable to confirm > > whether or not this breaks non-authenticated connections. > > > > - I've been building my kernel using the documented > > procedure, and have no problems there. I ran through a > > base system build and that (eventually) completed OK too. > > When compiling ifconfig(8), I've been doing 'make includes' > > in /usr/src (after installing and booting my new kernel), > > and using 'make ifconfig' and 'make install' in > > /usr/src/sbin/ifconfig. Is this OK? or should I be doing > > something else instead? > > You should verify that 'make release' works after having completed a full > system build. The ramdisks use a special build of ifconfig so a check that > this still compiles is required. See 'man release' for details. > Since the credentials should not be passed back to userland I would not add them to struct umb_parameter but instead to struct umb_softc. Then you don't need to use struct umb_parameter for the ioctl and instead could just pass the (utf16) string to the kernel. Apart form that it looks good. -- :wq Claudio
Re: Towards splitting SCHED_LOCK()
> Date: Tue, 14 Jan 2020 10:34:05 +1100
> From: Jonathan Gray
>
> On Mon, Jan 13, 2020 at 05:02:12PM +0100, Martin Pieuchot wrote:
> > I'm facing a lock ordering issue while profiling the scheduler which
> > cannot be solved without using a different lock for the global sleepqueue
> > and the runqueues.
> >
> > The SCHED_LOCK() currently protects both data structures as well as the
> > related fields in 'struct proc'. One of this fields is `p_wchan' which
> > is obviously related to the global sleepqueue.
> >
> > The cleaning diff below introduces a new function, awake(), that unify
> > the multiple uses of the following idiom:
> >
> > if (p->p_stat == SSLEEP)
> > setrunnable(p);
> > else
> > unsleep(p);
> >
> > This is generally done after checking if the thread `p' is on the
> > sleepqueue.
>
> Why not name it wakeup_proc() instead or something like endsleep()?
>
> awake() does not describe the action that is being done and
> if (awake()) reads like it could be
> if (p->p_stat != SSLEEP && p->p_stat != SSTOP)
Must say I had reservations about the "awake" name as well, but jsg@
nails it here since it both a verb and an adjective which creates
confusion. Both names suggester here are find I think.
> > This diff introduces a change in behavior in the Linux compat:
> > wake_up_process() will now return 1 if the thread was stopped and not
> > sleeping. This should be fine since the only place this value is
> > checked is with the combination of task_asleep().
> >
> > While here I removed two unnecessary `p_wchan' check before unsleep().
> >
> > ok?
> >
> > Index: dev/pci/drm/drm_linux.c
> > ===
> > RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
> > retrieving revision 1.55
> > diff -u -p -r1.55 drm_linux.c
> > --- dev/pci/drm/drm_linux.c 5 Jan 2020 13:46:02 - 1.55
> > +++ dev/pci/drm/drm_linux.c 13 Jan 2020 15:34:44 -
> > @@ -115,20 +115,8 @@ schedule_timeout(long timeout)
> > int
> > wake_up_process(struct proc *p)
> > {
> > - int s, r = 0;
> > -
> > - SCHED_LOCK(s);
> > atomic_cas_ptr(_proc, p, NULL);
> > - if (p->p_wchan) {
> > - if (p->p_stat == SSLEEP) {
> > - setrunnable(p);
> > - r = 1;
> > - } else
> > - unsleep(p);
> > - }
> > - SCHED_UNLOCK(s);
> > -
> > - return r;
> > + return awake(p, NULL);
> > }
> >
> > void
> > Index: kern/sys_generic.c
> > ===
> > RCS file: /cvs/src/sys/kern/sys_generic.c,v
> > retrieving revision 1.127
> > diff -u -p -r1.127 sys_generic.c
> > --- kern/sys_generic.c 8 Jan 2020 16:27:41 - 1.127
> > +++ kern/sys_generic.c 13 Jan 2020 15:35:22 -
> > @@ -767,7 +767,6 @@ void
> > selwakeup(struct selinfo *sip)
> > {
> > struct proc *p;
> > - int s;
> >
> > KNOTE(>si_note, NOTE_SUBMIT);
> > if (sip->si_seltid == 0)
> > @@ -780,15 +779,10 @@ selwakeup(struct selinfo *sip)
> > p = tfind(sip->si_seltid);
> > sip->si_seltid = 0;
> > if (p != NULL) {
> > - SCHED_LOCK(s);
> > - if (p->p_wchan == (caddr_t)) {
> > - if (p->p_stat == SSLEEP)
> > - setrunnable(p);
> > - else
> > - unsleep(p);
> > + if (awake(p, )) {
> > + /* nothing else to do */
> > } else if (p->p_flag & P_SELECT)
> > atomic_clearbits_int(>p_flag, P_SELECT);
> > - SCHED_UNLOCK(s);
> > }
> > }
> >
> > Index: kern/kern_synch.c
> > ===
> > RCS file: /cvs/src/sys/kern/kern_synch.c,v
> > retrieving revision 1.156
> > diff -u -p -r1.156 kern_synch.c
> > --- kern/kern_synch.c 12 Jan 2020 00:01:12 - 1.156
> > +++ kern/kern_synch.c 13 Jan 2020 15:41:00 -
> > @@ -469,8 +469,7 @@ sleep_setup_signal(struct sleep_state *s
> > */
> > atomic_setbits_int(>p_flag, P_SINTR);
> > if (p->p_p->ps_single != NULL || (sls->sls_sig = CURSIG(p)) != 0) {
> > - if (p->p_wchan)
> > - unsleep(p);
> > + unsleep(p);
> > p->p_stat = SONPROC;
> > sls->sls_do_sleep = 0;
> > } else if (p->p_wchan == 0) {
> > @@ -505,6 +504,25 @@ sleep_finish_signal(struct sleep_state *
> > return (error);
> > }
> >
> > +int
> > +awake(struct proc *p, const volatile void *chan)
> > +{
> > + int s, awakened = 0;
> > +
> > + SCHED_LOCK(s);
> > + if (p->p_wchan != NULL &&
> > + ((chan == NULL) || (p->p_wchan == chan))) {
> > + awakened = 1;
> > + if (p->p_stat == SSLEEP)
> > + setrunnable(p);
> > + else
> > + unsleep(p);
> > + }
> > + SCHED_UNLOCK(s);
> > +
> > + return awakened;
> > +}
> > +
> > /*
> >
Re: Towards splitting SCHED_LOCK()
On 14/01/20(Tue) 10:34, Jonathan Gray wrote:
> On Mon, Jan 13, 2020 at 05:02:12PM +0100, Martin Pieuchot wrote:
> > I'm facing a lock ordering issue while profiling the scheduler which
> > cannot be solved without using a different lock for the global sleepqueue
> > and the runqueues.
> >
> > The SCHED_LOCK() currently protects both data structures as well as the
> > related fields in 'struct proc'. One of this fields is `p_wchan' which
> > is obviously related to the global sleepqueue.
> >
> > The cleaning diff below introduces a new function, awake(), that unify
> > the multiple uses of the following idiom:
> >
> > if (p->p_stat == SSLEEP)
> > setrunnable(p);
> > else
> > unsleep(p);
> >
> > This is generally done after checking if the thread `p' is on the
> > sleepqueue.
>
> Why not name it wakeup_proc() instead or something like endsleep()?
>
> awake() does not describe the action that is being done and
> if (awake()) reads like it could be
> if (p->p_stat != SSLEEP && p->p_stat != SSTOP)
Sure, updated diff that also keep the SCHED_LOCK() in endtsleep() as
pointed out by visa@. The reason is that sleep_finish_timeout() is
executed under the SCHED_LOCK() and we want to ensure the P_TIMEOUT
flag is set and check under this lock to avoid races.
Index: dev/pci/drm/drm_linux.c
===
RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
retrieving revision 1.55
diff -u -p -r1.55 drm_linux.c
--- dev/pci/drm/drm_linux.c 5 Jan 2020 13:46:02 - 1.55
+++ dev/pci/drm/drm_linux.c 14 Jan 2020 16:22:38 -
@@ -115,20 +115,8 @@ schedule_timeout(long timeout)
int
wake_up_process(struct proc *p)
{
- int s, r = 0;
-
- SCHED_LOCK(s);
atomic_cas_ptr(_proc, p, NULL);
- if (p->p_wchan) {
- if (p->p_stat == SSLEEP) {
- setrunnable(p);
- r = 1;
- } else
- unsleep(p);
- }
- SCHED_UNLOCK(s);
-
- return r;
+ return wakeup_proc(p, NULL);
}
void
Index: kern/kern_sig.c
===
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.241
diff -u -p -r1.241 kern_sig.c
--- kern/kern_sig.c 14 Jan 2020 08:52:18 - 1.241
+++ kern/kern_sig.c 14 Jan 2020 16:20:42 -
@@ -1109,7 +1109,7 @@ ptsignal(struct proc *p, int signum, enu
* runnable and can look at the signal. But don't make
* the process runnable, leave it stopped.
*/
- if (p->p_wchan && p->p_flag & P_SINTR)
+ if (p->p_flag & P_SINTR)
unsleep(p);
goto out;
Index: kern/kern_synch.c
===
RCS file: /cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.157
diff -u -p -r1.157 kern_synch.c
--- kern/kern_synch.c 14 Jan 2020 08:52:18 - 1.157
+++ kern/kern_synch.c 14 Jan 2020 16:23:02 -
@@ -469,8 +469,7 @@ sleep_setup_signal(struct sleep_state *s
*/
atomic_setbits_int(>p_flag, P_SINTR);
if (p->p_p->ps_single != NULL || (sls->sls_sig = CURSIG(p)) != 0) {
- if (p->p_wchan)
- unsleep(p);
+ unsleep(p);
p->p_stat = SONPROC;
sls->sls_do_sleep = 0;
} else if (p->p_wchan == 0) {
@@ -505,6 +504,25 @@ sleep_finish_signal(struct sleep_state *
return (error);
}
+int
+wakeup_proc(struct proc *p, const volatile void *chan)
+{
+ int s, awakened = 0;
+
+ SCHED_LOCK(s);
+ if (p->p_wchan != NULL &&
+ ((chan == NULL) || (p->p_wchan == chan))) {
+ awakened = 1;
+ if (p->p_stat == SSLEEP)
+ setrunnable(p);
+ else
+ unsleep(p);
+ }
+ SCHED_UNLOCK(s);
+
+ return awakened;
+}
+
/*
* Implement timeout for tsleep.
* If process hasn't been awakened (wchan non-zero),
@@ -518,13 +536,8 @@ endtsleep(void *arg)
int s;
SCHED_LOCK(s);
- if (p->p_wchan) {
- if (p->p_stat == SSLEEP)
- setrunnable(p);
- else
- unsleep(p);
+ if (wakeup_proc(p, NULL))
atomic_setbits_int(>p_flag, P_TIMEOUT);
- }
SCHED_UNLOCK(s);
}
@@ -536,7 +549,7 @@ unsleep(struct proc *p)
{
SCHED_ASSERT_LOCKED();
- if (p->p_wchan) {
+ if (p->p_wchan != NULL) {
TAILQ_REMOVE([LOOKUP(p->p_wchan)], p, p_runq);
p->p_wchan = NULL;
}
@@ -570,13 +583,8 @@ wakeup_n(const volatile void *ident, int
if (p->p_stat != SSLEEP && p->p_stat != SSTOP)
panic("wakeup: p_stat is %d", (int)p->p_stat);
#endif
- if (p->p_wchan ==
Re: iked(8): get rid of IPv6 flow and -6 flag?
On Jan 13, 2020, at 11:55 AM, Tobias Heider wrote: > > Hi, > > iked by default blocks all IPv6 traffic on a host unless any > of the configured policies use v6. This was originally meant > as a measure to prevent VPN leakage for people who did not > think of IPv6 when configuring IPsec. With the -6 flag > set, iked does not install this IPv6 blocking flow. > > I think we should discuss whether we can remove the flow > (and the -6 flag) as I constantly hear people complaining > that it broke their setups and I don't think anyone > expects some seemingly unrelated program breaking IPv6. Ah, THAT's why iked nuked IPv6 on my router when I enabled it. I am strongly in favor of this proposal, with the subsequent recommendations to make it a warning instead of an error. - Dave
Re: Please test: kill `p_priority'
On 13/01/20(Mon) 21:31, Martin Pieuchot wrote:
> I'd like hardclock() to be free of SCHED_LOCK(), the diff below brings
> us one step away from that goal. Please test with your favorite
> benchmark and report any regression :o)
>
> The reason for moving the SCHED_LOCK() away from hardclock() is because
> it will allow us to re-commit the diff moving accounting out of the
> SCHED_LOCK(). In the end we shrink the SCHED_LOCK() which is generally
> a good thing(tm).
>
> `p_priority' is not very well named. It's currently a placeholder for
> three values:
>
> - the sleeping priority, corresponding to the value passed to tsleep(9)
> which will be later used by setrunnable() to place the thread on the
> appropriate runqueue.
>
> - the per-CPU runqueue priority which is used to add/remove a thread
> to/from a runqueue.
>
> - the scheduling priority, also named `p_usrpri', calculated from the
> estimated amount of CPU time used.
>
>
> The diff below splits the current `p_priority' into two fields `p_runpri'
> and `p_slppri'. The important part is the schedclock() chunk:
>
> @@ -519,8 +515,6 @@ schedclock(struct proc *p)
> SCHED_LOCK(s);
> newcpu = ESTCPULIM(p->p_estcpu + 1);
> setpriority(p, newcpu, p->p_p->ps_nice);
> - if (p->p_priority >= PUSER)
> - p->p_priority = p->p_usrpri;
> SCHED_UNLOCK(s);
>
> `p_priority' had to be updated because it was overwritten during each
> tsleep()/setrunnable() cycle. The same happened in schedcpu():
>
> schedcpu: reseting priority for zerothread(44701) 127 -> 90
> schedclock: reseting priority for zerothread(44701) 127 -> 91
>
> Getting rid of this assignment means we can then protect `p_estcpu' and
> `p_usrpri' with a custom rer-thread lock.
>
> With this division, `p_runpri' becomes part of the scheduler fields while
> `p_slppri' is obviously part of the global sleepqueue.
>
> The rest of the diff is quite straightforward, including the userland
> compatibility bits.
>
> Tests, comments and oks welcome :o)
Updated diff that changes getpriority() into a macro allowing libkvm to
build as reported by anton@.
Index: arch/sparc64/sparc64/db_interface.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/db_interface.c,v
retrieving revision 1.54
diff -u -p -r1.54 db_interface.c
--- arch/sparc64/sparc64/db_interface.c 7 Nov 2019 14:44:53 - 1.54
+++ arch/sparc64/sparc64/db_interface.c 14 Jan 2020 16:02:08 -
@@ -958,10 +958,10 @@ db_proc_cmd(addr, have_addr, count, modi
return;
}
db_printf("process %p:", p);
- db_printf("pid:%d vmspace:%p pmap:%p ctx:%x wchan:%p pri:%d upri:%d\n",
+ db_printf("pid:%d vmspace:%p pmap:%p ctx:%x wchan:%p rpri:%d upri:%d\n",
p->p_p->ps_pid, p->p_vmspace, p->p_vmspace->vm_map.pmap,
p->p_vmspace->vm_map.pmap->pm_ctx,
- p->p_wchan, p->p_priority, p->p_usrpri);
+ p->p_wchan, p->p_runpri, p->p_usrpri);
db_printf("maxsaddr:%p ssiz:%dpg or %llxB\n",
p->p_vmspace->vm_maxsaddr, p->p_vmspace->vm_ssize,
(unsigned long long)ptoa(p->p_vmspace->vm_ssize));
Index: dev/pci/drm/i915/intel_breadcrumbs.c
===
RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_breadcrumbs.c,v
retrieving revision 1.4
diff -u -p -r1.4 intel_breadcrumbs.c
--- dev/pci/drm/i915/intel_breadcrumbs.c10 Jul 2019 07:56:30 -
1.4
+++ dev/pci/drm/i915/intel_breadcrumbs.c14 Jan 2020 16:02:08 -
@@ -451,7 +451,7 @@ static bool __intel_engine_add_wait(stru
#ifdef __linux__
if (wait->tsk->prio > to_wait(parent)->tsk->prio) {
#else
- if (wait->tsk->p_priority >
to_wait(parent)->tsk->p_priority) {
+ if (wait->tsk->p_usrpri >
to_wait(parent)->tsk->p_usrpri) {
#endif
p = >rb_right;
first = false;
@@ -538,7 +538,7 @@ static inline bool chain_wakeup(struct r
#else
static inline bool chain_wakeup(struct rb_node *rb, int priority)
{
- return rb && to_wait(rb)->tsk->p_priority <= priority;
+ return rb && to_wait(rb)->tsk->p_usrpri <= priority;
}
#endif
@@ -558,7 +558,7 @@ static inline int wakeup_priority(struct
if (p == b->signaler)
return INT_MIN;
else
- return p->p_priority;
+ return p->p_usrpri;
}
#endif
Index: kern/kern_fork.c
===
RCS file: /cvs/src/sys/kern/kern_fork.c,v
retrieving revision 1.220
diff -u -p -r1.220 kern_fork.c
--- kern/kern_fork.c6 Jan 2020 10:25:10 - 1.220
+++ kern/kern_fork.c14 Jan 2020 16:02:08 -
@@ -312,7 +312,7
Re: iked(8): get rid of IPv6 flow and -6 flag?
Stuart Henderson wrote: > On 2020/01/13 20:51, Klemens Nanni wrote: > > I'm in favour of removing the option and OK with your diff, but simply > > removing it is probably a bad idea given its nature. > > > > What about printing a deprecation warning so that users can safely > > adjust their rcctl flags instead of running into "iked(failed)" on the > > next snapshot. > > Yes please make -6 a noop or a warning rather than an error. Sometimes > breakage is unavoidable, but this isn't one of those cases. I agree.
Re: pfctl - allow recursive flush operations [ 5/5 ]
Thanks, OK kn
Re: pfctl - allow recursive flush operations [ 5/5 ]
Hello
> > >
> > > This reads simpler and clearer to me, what do you think?
> >
> > OK, I'll buy if (...) from you, but '+ 1' must stay there,
> > because it is for a '\0' terminator. So let's go for this:
> > len = strlen(pr->name) + 1;
> > if (pr->path[0])
> > len += strlen(pr->path) + 1;
>
> I think this should use asprintf() instead.
>
and you are right.
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 9c5778f4f97..16251d2e289 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2173,7 +2173,6 @@ int
pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void *warg)
{
struct pfr_anchoritem *pfra;
- size_t len;
struct pfr_anchors *anchors;
anchors = (struct pfr_anchors *) warg;
@@ -2182,19 +2181,15 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void
*warg)
if (pfra == NULL)
err(1, "%s", __func__);
- len = strlen(pr->name) + 1;
+ pfra->pfra_anchorname = NULL;
if (pr->path[0])
- len += strlen(pr->path) + 1;
+ asprintf(>pfra_anchorname, "%s/%s", pr->path, pr->name);
+ else
+ asprintf(>pfra_anchorname, "%s", pr->name);
- pfra->pfra_anchorname = malloc(len);
if (pfra->pfra_anchorname == NULL)
err(1, "%s", __func__);
- if (pr->path[0])
- snprintf(pfra->pfra_anchorname, len, "%s/%s",
- pr->path, pr->name);
- else
- snprintf(pfra->pfra_anchorname, len, "%s", pr->name);
SLIST_INSERT_HEAD(anchors, pfra, pfra_sle);
Re: pfctl - allow recursive flush operations [ 5/5 ]
On Tue, Jan 14, 2020 at 04:32:41PM +0100, Alexandr Nedvedicky wrote: > Hello, > > On Tue, Jan 14, 2020 at 03:02:09PM +0100, Klemens Nanni wrote: > > On Mon, Jan 13, 2020 at 10:47:27PM +0100, Alexandr Nedvedicky wrote: > > > @@ -2182,7 +2182,7 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr, > > > void *warg) > > > if (pfra == NULL) > > > err(1, "%s", __func__); > > > > > > - len = strlen(pr->path) + 1; > > > + len = (pr->path[0] == '\0') ? 0 : strlen(pr->path) + 1; > > > len += strlen(pr->name) + 1; > > pr->name is always used and only pr->path is optional. With this diff > > you're always adding one with the name altough it is only required if > > path is given (for the "/" delimiter). > > > > len = strlen(pr->name); > > if (pr->path[0]) > > len += strlen(pr->path) + 1; > > > > This reads simpler and clearer to me, what do you think? > > OK, I'll buy if (...) from you, but '+ 1' must stay there, > because it is for a '\0' terminator. So let's go for this: > len = strlen(pr->name) + 1; > if (pr->path[0]) > len += strlen(pr->path) + 1; I think this should use asprintf() instead.
Re: pfctl - allow recursive flush operations [ 5/5 ]
Hello,
On Tue, Jan 14, 2020 at 03:02:09PM +0100, Klemens Nanni wrote:
> On Mon, Jan 13, 2020 at 10:47:27PM +0100, Alexandr Nedvedicky wrote:
> > @@ -2182,7 +2182,7 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr,
> > void *warg)
> > if (pfra == NULL)
> > err(1, "%s", __func__);
> >
> > - len = strlen(pr->path) + 1;
> > + len = (pr->path[0] == '\0') ? 0 : strlen(pr->path) + 1;
> > len += strlen(pr->name) + 1;
> pr->name is always used and only pr->path is optional. With this diff
> you're always adding one with the name altough it is only required if
> path is given (for the "/" delimiter).
>
> len = strlen(pr->name);
> if (pr->path[0])
> len += strlen(pr->path) + 1;
>
> This reads simpler and clearer to me, what do you think?
OK, I'll buy if (...) from you, but '+ 1' must stay there,
because it is for a '\0' terminator. So let's go for this:
len = strlen(pr->name) + 1;
if (pr->path[0])
len += strlen(pr->path) + 1;
>
> > pfra->pfra_anchorname = malloc(len);
> > if (pfra->pfra_anchorname == NULL)
> > @@ -2312,7 +2312,8 @@ pfctl_recurse(int dev, int opts, const char
> > *anchorname,
> >
> > anchors = pfctl_get_anchors(dev, anchorname, opts);
> > /*
> > -* don't let pfctl_clear_*() function to fail with exit
> > +* don't let pfctl_clear_*() function to fail with exit, also make
> > +* pfctl_clear_tables(),(which is passed via walkf argument, quiet.
> Missing space after comma, extraneous parenthese as well.
>
> > */
> > opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
my comment was misleading. I also want to make pfctl_clear_rules()
silent. But setting PF_OPT_QUIET flag in pfctl_call_() functions,
where we need it makes sense.
>
> Below is a comment according to style(9) that seems fitting for IGNFAIL
> alone (leaving QUIET out now due to above); it's more of a "why" rather
> than a "what" so the semantics behind IGNFAIL become clearer since it
> isn't used or documented anywhere else.
>
> /*
>* While traversing the list, pfctl_clear_*() must always return
>* so that failures on one anchor do not prevent clearing others.
>*/
> opts |= PF_OPT_IGNFAIL;
comment reads far better.
diff below brings suggested changes to 5/5 patch.
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 67dae4cdad9..9c5778f4f97 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2182,8 +2182,10 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void
*warg)
if (pfra == NULL)
err(1, "%s", __func__);
- len = (pr->path[0] == '\0') ? 0 : strlen(pr->path) + 1;
- len += strlen(pr->name) + 1;
+ len = strlen(pr->name) + 1;
+ if (pr->path[0])
+ len += strlen(pr->path) + 1;
+
pfra->pfra_anchorname = malloc(len);
if (pfra->pfra_anchorname == NULL)
err(1, "%s", __func__);
@@ -2281,6 +2283,11 @@ pfctl_get_anchors(int dev, const char *anchor, int opts)
int
pfctl_call_cleartables(int dev, int opts, struct pfr_anchoritem *pfra)
{
+ /*
+* PF_OPT_QUIET makes pfctl_clear_tables() to stop printing number of
+* tables cleared for given anchor.
+*/
+ opts |= PF_OPT_QUIET;
return ((pfctl_clear_tables(pfra->pfra_anchorname, opts) == -1) ?
1 : 0);
}
@@ -2288,6 +2295,11 @@ pfctl_call_cleartables(int dev, int opts, struct
pfr_anchoritem *pfra)
int
pfctl_call_clearrules(int dev, int opts, struct pfr_anchoritem *pfra)
{
+ /*
+* PF_OPT_QUIET makes pfctl_clear_rules() to stop printing a 'rules
+* cleared' message for every anchor it deletes.
+*/
+ opts |= PF_OPT_QUIET;
return (pfctl_clear_rules(dev, opts, pfra->pfra_anchorname));
}
@@ -2297,7 +2309,7 @@ pfctl_call_clearanchors(int dev, int opts, struct
pfr_anchoritem *pfra)
int rv = 0;
rv |= pfctl_call_cleartables(dev, opts, pfra);
- rv |= pfctl_clear_rules(dev, opts, pfra->pfra_anchorname);
+ rv |= pfctl_call_clearrules(dev, opts, pfra);
return (rv);
}
@@ -2312,10 +2324,10 @@ pfctl_recurse(int dev, int opts, const char *anchorname,
anchors = pfctl_get_anchors(dev, anchorname, opts);
/*
-* don't let pfctl_clear_*() function to fail with exit, also make
-* pfctl_clear_tables(),(which is passed via walkf argument, quiet.
+* While traversing the list, pfctl_clear_*() must always return
+* so that failures on one anchor do not prevent clearing others.
*/
- opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
+ opts |= PF_OPT_IGNFAIL;
printf("Removing:\n");
SLIST_FOREACH_SAFE(pfra, anchors, pfra_sle, pfra_save) {
printf(" %s\n", (*pfra->pfra_anchorname == '\0') ?
Re: Towards splitting SCHED_LOCK()
On Mon, Jan 13, 2020 at 05:02:12PM +0100, Martin Pieuchot wrote:
> I'm facing a lock ordering issue while profiling the scheduler which
> cannot be solved without using a different lock for the global sleepqueue
> and the runqueues.
>
> The SCHED_LOCK() currently protects both data structures as well as the
> related fields in 'struct proc'. One of this fields is `p_wchan' which
> is obviously related to the global sleepqueue.
>
> The cleaning diff below introduces a new function, awake(), that unify
> the multiple uses of the following idiom:
>
> if (p->p_stat == SSLEEP)
> setrunnable(p);
> else
> unsleep(p);
>
> This is generally done after checking if the thread `p' is on the
> sleepqueue.
Why not name it wakeup_proc() instead or something like endsleep()?
awake() does not describe the action that is being done and
if (awake()) reads like it could be
if (p->p_stat != SSLEEP && p->p_stat != SSTOP)
>
> This diff introduces a change in behavior in the Linux compat:
> wake_up_process() will now return 1 if the thread was stopped and not
> sleeping. This should be fine since the only place this value is
> checked is with the combination of task_asleep().
>
> While here I removed two unnecessary `p_wchan' check before unsleep().
>
> ok?
>
> Index: dev/pci/drm/drm_linux.c
> ===
> RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
> retrieving revision 1.55
> diff -u -p -r1.55 drm_linux.c
> --- dev/pci/drm/drm_linux.c 5 Jan 2020 13:46:02 - 1.55
> +++ dev/pci/drm/drm_linux.c 13 Jan 2020 15:34:44 -
> @@ -115,20 +115,8 @@ schedule_timeout(long timeout)
> int
> wake_up_process(struct proc *p)
> {
> - int s, r = 0;
> -
> - SCHED_LOCK(s);
> atomic_cas_ptr(_proc, p, NULL);
> - if (p->p_wchan) {
> - if (p->p_stat == SSLEEP) {
> - setrunnable(p);
> - r = 1;
> - } else
> - unsleep(p);
> - }
> - SCHED_UNLOCK(s);
> -
> - return r;
> + return awake(p, NULL);
> }
>
> void
> Index: kern/sys_generic.c
> ===
> RCS file: /cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.127
> diff -u -p -r1.127 sys_generic.c
> --- kern/sys_generic.c8 Jan 2020 16:27:41 - 1.127
> +++ kern/sys_generic.c13 Jan 2020 15:35:22 -
> @@ -767,7 +767,6 @@ void
> selwakeup(struct selinfo *sip)
> {
> struct proc *p;
> - int s;
>
> KNOTE(>si_note, NOTE_SUBMIT);
> if (sip->si_seltid == 0)
> @@ -780,15 +779,10 @@ selwakeup(struct selinfo *sip)
> p = tfind(sip->si_seltid);
> sip->si_seltid = 0;
> if (p != NULL) {
> - SCHED_LOCK(s);
> - if (p->p_wchan == (caddr_t)) {
> - if (p->p_stat == SSLEEP)
> - setrunnable(p);
> - else
> - unsleep(p);
> + if (awake(p, )) {
> + /* nothing else to do */
> } else if (p->p_flag & P_SELECT)
> atomic_clearbits_int(>p_flag, P_SELECT);
> - SCHED_UNLOCK(s);
> }
> }
>
> Index: kern/kern_synch.c
> ===
> RCS file: /cvs/src/sys/kern/kern_synch.c,v
> retrieving revision 1.156
> diff -u -p -r1.156 kern_synch.c
> --- kern/kern_synch.c 12 Jan 2020 00:01:12 - 1.156
> +++ kern/kern_synch.c 13 Jan 2020 15:41:00 -
> @@ -469,8 +469,7 @@ sleep_setup_signal(struct sleep_state *s
>*/
> atomic_setbits_int(>p_flag, P_SINTR);
> if (p->p_p->ps_single != NULL || (sls->sls_sig = CURSIG(p)) != 0) {
> - if (p->p_wchan)
> - unsleep(p);
> + unsleep(p);
> p->p_stat = SONPROC;
> sls->sls_do_sleep = 0;
> } else if (p->p_wchan == 0) {
> @@ -505,6 +504,25 @@ sleep_finish_signal(struct sleep_state *
> return (error);
> }
>
> +int
> +awake(struct proc *p, const volatile void *chan)
> +{
> + int s, awakened = 0;
> +
> + SCHED_LOCK(s);
> + if (p->p_wchan != NULL &&
> +((chan == NULL) || (p->p_wchan == chan))) {
> + awakened = 1;
> + if (p->p_stat == SSLEEP)
> + setrunnable(p);
> + else
> + unsleep(p);
> + }
> + SCHED_UNLOCK(s);
> +
> + return awakened;
> +}
> +
> /*
> * Implement timeout for tsleep.
> * If process hasn't been awakened (wchan non-zero),
> @@ -515,17 +533,9 @@ void
> endtsleep(void *arg)
> {
> struct proc *p = arg;
> - int s;
>
> - SCHED_LOCK(s);
> - if (p->p_wchan) {
> - if (p->p_stat == SSLEEP)
> - setrunnable(p);
> - else
> - unsleep(p);
> + if (awake(p, NULL))
>
nfs mp vnode list remove safe
Hi,
There is no remove and no sleep in this loop. So _SAFE is unnecessary.
ok?
bluhm
Index: nfs/nfs_subs.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/nfs/nfs_subs.c,v
retrieving revision 1.141
diff -u -p -r1.141 nfs_subs.c
--- nfs/nfs_subs.c 10 Jan 2020 10:33:35 - 1.141
+++ nfs/nfs_subs.c 13 Jan 2020 18:02:54 -
@@ -1509,16 +1509,16 @@ netaddr_match(int family, union nethosta
void
nfs_clearcommit(struct mount *mp)
{
- struct vnode *vp, *nvp;
- struct buf *bp, *nbp;
+ struct vnode *vp;
+ struct buf *bp;
int s;
s = splbio();
loop:
- TAILQ_FOREACH_SAFE(vp, >mnt_vnodelist, v_mntvnodes, nvp) {
+ TAILQ_FOREACH(vp, >mnt_vnodelist, v_mntvnodes) {
if (vp->v_mount != mp) /* Paranoia */
goto loop;
- LIST_FOREACH_SAFE(bp, >v_dirtyblkhd, b_vnbufs, nbp) {
+ LIST_FOREACH(bp, >v_dirtyblkhd, b_vnbufs) {
if ((bp->b_flags & (B_BUSY | B_DELWRI | B_NEEDCOMMIT))
== (B_DELWRI | B_NEEDCOMMIT))
bp->b_flags &= ~B_NEEDCOMMIT;
Re: iked(8): get rid of IPv6 flow and -6 flag?
On 2020/01/13 23:31, Sebastian Benoit wrote: > Alexander Bluhm(alexander.bl...@gmx.net) on 2020.01.13 18:19:31 +0100: > > On Mon, Jan 13, 2020 at 05:55:06PM +0100, Tobias Heider wrote: > > > I think we should discuss whether we can remove the flow > > > (and the -6 flag) as I constantly hear people complaining > > > that it broke their setups and I don't think anyone > > > expects some seemingly unrelated program breaking IPv6. > > > > A missing -6 flag on the iked command line, is a very unexpected > > way to break your IPv6 setup. So we should remove that. > > > > OK bluhm@ > > > > If there is demand for such a feature, we could create an option > > in the example/iked.conf that shows how to disable IPv6. > > And perhaps one to disable IPv4 for the IPv6 hipser :-) > > I'm ok with getting rid of it, but as kn@ suggests please with disable and > warning. Please add comment /* XXX remove during OpenBSD 6.7-current */ > A current.html note is needed. > I somehow doubt anybody will actually see a warning - is there a real need to schedule it for removal rather than just keep it indefinitely?
Re: iked(8): get rid of IPv6 flow and -6 flag?
Alexander Bluhm(alexander.bl...@gmx.net) on 2020.01.13 18:19:31 +0100: > On Mon, Jan 13, 2020 at 05:55:06PM +0100, Tobias Heider wrote: > > I think we should discuss whether we can remove the flow > > (and the -6 flag) as I constantly hear people complaining > > that it broke their setups and I don't think anyone > > expects some seemingly unrelated program breaking IPv6. > > A missing -6 flag on the iked command line, is a very unexpected > way to break your IPv6 setup. So we should remove that. > > OK bluhm@ > > If there is demand for such a feature, we could create an option > in the example/iked.conf that shows how to disable IPv6. > And perhaps one to disable IPv4 for the IPv6 hipser :-) I'm ok with getting rid of it, but as kn@ suggests please with disable and warning. Please add comment /* XXX remove during OpenBSD 6.7-current */ A current.html note is needed.
Re: acpivout: try to consistently adjust brightness by 5%
On Mon, Jan 13, 2020 at 12:20:10PM +0100, Patrick Wildt wrote:
> The problem is that the last two values are 67 and 100. If you go
> 5% down, it's 95. The nearest will still be 100. The code then
> realizes that it's the same level as before, and does nlevel--.
> But nlevel-- is 99, and not 67, because nlevel is the value and
> not the index of the bcl array. So in essence the change needed
> is to decrease the index, not the value, and then look up the value.
That's what happens, but the problem isn't that my machine is lacking
levels between 67 and 100.
Rather, acpivout_find_brightness() seems to linear scale in levels
(which presumably is the case on newer machines), but my machine's
levels scale exponentially.
Diff below comments on this and hilights the two relevant checks:
Given an initial brightness of 100% (the last level), pressing the
function key to decrease the level eventually calls
acpivout_find_brightness() with `level = 100 + (-1 * 5) = 95', it then
iterates over the levels starting with the smallest.
Reaching the second last level (67 on my machine), the loop's body does
mid = 67 + (100 - 67) / 2 = 83;
if (67 <= 95 && 95 <= 83)
return 67;
if (83 <= 95 and 95 <= 100)
return 100;
So for requesting level below 100 it always returns 100 itself because
the next level has a value of 67 which is way out of the 5% threshold
this algorithm assumes.
I suppose a better heuristic is required to make both linearly and
exponentially scaling machines happy, but this is currently too finicky
for me so I just reverted 1.14 in my tree.
Index: acpivout.c
===
RCS file: /cvs/src/sys/dev/acpi/acpivout.c,v
retrieving revision 1.16
diff -u -p -r1.16 acpivout.c
--- acpivout.c 14 Dec 2019 10:57:48 - 1.16
+++ acpivout.c 14 Jan 2020 14:19:08 -
@@ -218,6 +218,11 @@ acpivout_find_brightness(struct acpivout
for (i = 0; i < sc->sc_bcl_len - 1; i++) {
mid = sc->sc_bcl[i] + (sc->sc_bcl[i + 1] - sc->sc_bcl[i]) / 2;
+ /*
+* XXX these checks assume levels to be on a linear scale,
+* but some hardware provides exponentially scaled brightness
+* levels (ThinkPad X230, T420).
+*/
if (sc->sc_bcl[i] <= level && level <= mid)
return sc->sc_bcl[i];
if (mid < level && level <= sc->sc_bcl[i + 1])
qle(4): tsleep(9) -> tsleep_nsec(9)
These sleeps don't have units, though in practice they are 1 second.
Just prior in the code I see a delay(9) of 100 microseconds. Is the
100 ticks here a typo? What is a reasonable sleep duration for this
hardware?
Index: pci/qle.c
===
RCS file: /cvs/src/sys/dev/pci/qle.c,v
retrieving revision 1.48
diff -u -p -r1.48 qle.c
--- pci/qle.c 31 Dec 2019 22:57:07 - 1.48
+++ pci/qle.c 14 Jan 2020 00:29:34 -
@@ -1886,7 +1886,8 @@ qle_ct_pass_through(struct qle_softc *sc
if (qle_read_isr(sc, , ) != 0)
qle_handle_intr(sc, isr, info);
} else {
- tsleep(sc->sc_scratch, PRIBIO, "qle_fabric", 100);
+ tsleep_nsec(sc->sc_scratch, PRIBIO, "qle_fabric",
+ SEC_TO_NSEC(1));
}
}
if (rv == 0)
@@ -2014,7 +2015,8 @@ qle_fabric_plogx(struct qle_softc *sc, s
if (qle_read_isr(sc, , ) != 0)
qle_handle_intr(sc, isr, info);
} else {
- tsleep(sc->sc_scratch, PRIBIO, "qle_fabric", 100);
+ tsleep_nsec(sc->sc_scratch, PRIBIO, "qle_fabric",
+ SEC_TO_NSEC(1));
}
}
sc->sc_fabric_pending = 0;
Re: iked(8): get rid of IPv6 flow and -6 flag?
On 2020/01/13 18:19, Alexander Bluhm wrote: > On Mon, Jan 13, 2020 at 05:55:06PM +0100, Tobias Heider wrote: > > I think we should discuss whether we can remove the flow > > (and the -6 flag) as I constantly hear people complaining > > that it broke their setups and I don't think anyone > > expects some seemingly unrelated program breaking IPv6. > > A missing -6 flag on the iked command line, is a very unexpected > way to break your IPv6 setup. So we should remove that. > > OK bluhm@ > > If there is demand for such a feature, we could create an option > in the example/iked.conf that shows how to disable IPv6. > And perhaps one to disable IPv4 for the IPv6 hipser :-) It would need to be in ipsec.conf - iked.conf doesn't allow setting manual flows. On 2020/01/13 20:51, Klemens Nanni wrote: > I'm in favour of removing the option and OK with your diff, but simply > removing it is probably a bad idea given its nature. > > What about printing a deprecation warning so that users can safely > adjust their rcctl flags instead of running into "iked(failed)" on the > next snapshot. Yes please make -6 a noop or a warning rather than an error. Sometimes breakage is unavoidable, but this isn't one of those cases.
Re: httpd(8): patch to allow FastCGI chroots in sub-directories
I like the idea. Unfortunately the diff does not apply.
On Thu, Jan 09, 2020 at 06:10:24AM +0100, Nazar Zhuk wrote:
> httpd(8) expects FastCGI processes to have the same chroot as httpd. I
> propose a feature that allows multiple FastCGI processes chrooted in
> separate directories under /var/www (/var/www/site1, /var/www/site2, etc.)
> This would better isolate multiple applications.
>
> Configuration:
>
> fastcgi strip
>
> strips path components from the beginning of DOCUMENT_ROOT and
> SCRIPT_FILENAME. So the FastCGI server gets /script instead of
> /siteX/script.
>
> I tested this with php-fpm.
>
> Please consider including in httpd(8).
>
>
> Index: httpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/httpd/httpd.conf.5,v
> retrieving revision 1.107
> diff -u -p -u -r1.107 httpd.conf.5
> --- httpd.conf.5 8 May 2019 21:46:56 - 1.107
> +++ httpd.conf.5 9 Jan 2020 05:01:57 -
> @@ -300,6 +300,10 @@ Alternatively if
> the FastCGI handler is listening on a TCP socket,
> .Ar socket
> starts with a colon followed by the TCP port number.
> +.It Ic strip Ar number
> +Strip
> +.Ar number
> +path components from the beginning of DOCUMENT_ROOT and SCRIPT_FILENAME
> before sending them to the FastCGI server. This allows FastCGI server chroot
> to be a directory under httpd chroot.
> .It Ic param Ar variable value
> Sets a variable that will be sent to the FastCGI server.
> Each statement defines one variable.
> Index: httpd.h
> ===
> RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
> retrieving revision 1.145
> diff -u -p -u -r1.145 httpd.h
> --- httpd.h 8 May 2019 19:57:45 - 1.145
> +++ httpd.h 9 Jan 2020 05:01:57 -
> @@ -547,6 +547,7 @@ struct server_config {
> uint8_t hsts_flags;
>
> struct server_fcgiparams fcgiparams;
> + int fcgistrip;
>
> TAILQ_ENTRY(server_config) entry;
> };
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/httpd/parse.y,v
> retrieving revision 1.113
> diff -u -p -u -r1.113 parse.y
> --- parse.y 28 Jun 2019 13:32:47 - 1.113
> +++ parse.y 9 Jan 2020 05:01:58 -
> @@ -689,6 +689,13 @@ fcgiflags: SOCKET STRING {
> param->name, param->value);
> TAILQ_INSERT_HEAD(_conf->fcgiparams, param, entry);
> }
> + | STRIP NUMBER {
> + if ($2 < 0 || $2 > INT_MAX) {
> + yyerror("invalid fastcgi strip number");
> + YYERROR;
> + }
> + srv_conf->fcgistrip = $2;
> + }
> ;
>
> connection : CONNECTION '{' optnl conflags_l '}'
> Index: server_fcgi.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/server_fcgi.c,v
> retrieving revision 1.80
> diff -u -p -u -r1.80 server_fcgi.c
> --- server_fcgi.c 8 May 2019 21:41:06 - 1.80
> +++ server_fcgi.c 9 Jan 2020 05:01:58 -
> @@ -241,7 +241,9 @@ server_fcgi(struct httpd *env, struct cl
> errstr = "failed to encode param";
> goto fail;
> }
> - if (fcgi_add_param(, "SCRIPT_FILENAME", script, clt) == -1) {
> + if (fcgi_add_param(, "SCRIPT_FILENAME",
> + server_root_strip(script, srv_conf->fcgistrip),
> + clt) == -1) {
> errstr = "failed to encode param";
> goto fail;
> }
> @@ -257,7 +259,8 @@ server_fcgi(struct httpd *env, struct cl
> goto fail;
> }
>
> - if (fcgi_add_param(, "DOCUMENT_ROOT", srv_conf->root,
> + if (fcgi_add_param(, "DOCUMENT_ROOT",
> + server_root_strip(srv_conf->root, srv_conf->fcgistrip),
> clt) == -1) {
> errstr = "failed to encode param";
> goto fail;
>
--
I'm not entirely sure you are real.
Re: pfctl - allow recursive flush operations [ 5/5 ]
On Mon, Jan 13, 2020 at 10:47:27PM +0100, Alexandr Nedvedicky wrote: > @@ -2182,7 +2182,7 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void > *warg) > if (pfra == NULL) > err(1, "%s", __func__); > > - len = strlen(pr->path) + 1; > + len = (pr->path[0] == '\0') ? 0 : strlen(pr->path) + 1; > len += strlen(pr->name) + 1; pr->name is always used and only pr->path is optional. With this diff you're always adding one with the name altough it is only required if path is given (for the "/" delimiter). len = strlen(pr->name); if (pr->path[0]) len += strlen(pr->path) + 1; This reads simpler and clearer to me, what do you think? > pfra->pfra_anchorname = malloc(len); > if (pfra->pfra_anchorname == NULL) > @@ -2312,7 +2312,8 @@ pfctl_recurse(int dev, int opts, const char *anchorname, > > anchors = pfctl_get_anchors(dev, anchorname, opts); > /* > - * don't let pfctl_clear_*() function to fail with exit > + * don't let pfctl_clear_*() function to fail with exit, also make > + * pfctl_clear_tables(),(which is passed via walkf argument, quiet. Missing space after comma, extraneous parenthese as well. >*/ > opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET; Here you add QUIET for all possible walkf() candidates, but only pfctl_clear_tables() --that is pfctl_table()-- needs it: why not adjusting this function alone? Does adding QUIET to the table code have side effects in existing code paths not related to your recursion work? On the contrary: might there be side effects adding QUIET for all functions here regardless of whether `-q' or `-v' have been passed on the command line? Below is a comment according to style(9) that seems fitting for IGNFAIL alone (leaving QUIET out now due to above); it's more of a "why" rather than a "what" so the semantics behind IGNFAIL become clearer since it isn't used or documented anywhere else. /* * While traversing the list, pfctl_clear_*() must always return * so that failures on one anchor do not prevent clearing others. */ opts |= PF_OPT_IGNFAIL;
Re: umb(4) authentication
| > > | > > This email should be gold plated and moved to a permanent location in | > > the FAQ on how to help OpenBSD and how to request new features. | > > | > | > Please refrain from ridiculing people with good intentions. | > | I originally read Anders' mail as complimentary, but now I'm not so sure? | | Lee. | Hi Lee, Just ignore this Anders person. If a developer who works in this area doesn't reply in the next couple of days, you could either send the diffs to tech@ or reach out to a recent committer to the same area of code, you can see who committed via cvs or https://cvsweb.openbsd.org/src/ Brett.
Re: umb(4) authentication
On 2020/01/14 00:54, leeb wrote: > On Mon, Jan 13, 2020 at 04:42:22PM +0100, Martijn van Duren wrote: > > On 1/13/20 4:30 PM, Anders Andersson wrote: > > > On Mon, Jan 13, 2020 at 3:00 PM leeb wrote: > > >> > > >> Hello, > > >> > > > >> > > >> Thanks, > > >> > > >> Lee. > > > > > > This email should be gold plated and moved to a permanent location in > > > the FAQ on how to help OpenBSD and how to request new features. > > > > > Lee is obviously new to the community and doesn't know the workflow well > > enough. He tries to be polite and make sure he doesn't clutter the list. > > If you actually read his mail you would've seen that he has a diff and > > it works for him, so it's obviously not a dumb feature request. > > > > Please refrain from ridiculing people with good intentions. > > > > I originally read Anders' mail as complimentary, but now I'm not so sure? Yes, it was - this is a good example! : Now, this is my first attempt at OpenBSD development, and : writing my changes raised a few questions. So, does someone : want to spare a few minutes off-list, or should I just : clutter up tech@ ? tech@ is good because it means that other readers or people finding this in list archives can benefit too from the questions/answers too, and if it's something more general then people involved in documentation may be able to identify something that's missing and worth adding.
mii(4): tsleep(9) -> tsleep_nsec(9)
Ticks to milliseconds. Original sleep is half a second, so 500ms.
ok?
Index: mii/mii_physubr.c
===
RCS file: /cvs/src/sys/dev/mii/mii_physubr.c,v
retrieving revision 1.45
diff -u -p -r1.45 mii_physubr.c
--- mii/mii_physubr.c 11 Sep 2015 13:02:28 - 1.45
+++ mii/mii_physubr.c 14 Jan 2020 00:41:45 -
@@ -199,7 +199,7 @@ mii_phy_auto(struct mii_softc *sc, int w
*/
if (sc->mii_flags & MIIF_AUTOTSLEEP) {
sc->mii_flags |= MIIF_DOINGAUTO;
- tsleep(>mii_flags, PZERO, "miiaut", hz >> 1);
+ tsleep_nsec(>mii_flags, PZERO, "miiaut", MSEC_TO_NSEC(500));
mii_phy_auto_timeout(sc);
} else if ((sc->mii_flags & MIIF_DOINGAUTO) == 0) {
sc->mii_flags |= MIIF_DOINGAUTO;
Re: umb(4) WIP diff and questions
On Tue, Jan 14, 2020 at 09:51:05PM +0900, leeb wrote: > Hello again tech@ > > I've included diffs of what I've got so far at the bottom > of this mail, but first a couple of questions: > > - Using the full 510-character limits for username and > passphrase specified in the MBIM spec, kernel compilation > fails due to tripping the 2047-byte stack frame warning > when compiling the driver. That's the reason for the '100' > magic numbers that I put in there temporarily. > > Any hints on the best way to handle this would be appreciated. I would allocate mp dynamically instead of storing it on the stack. In umb_ioctl(), you could change mp to a pointer type: struct umb_parameter *mp; The ioctl is allowed to sleep until memory is available (M_WAITOK), so this allocation cannot fail and you don't need to check for NULL: mp = malloc(sizeof(*mp), M_DEVBUF, M_ZERO | M_WAITOK); free mp before umb_ioctl returns: free(mp, M_DEVBUF, sizeof(*mp)); See 'man 9 malloc' for details. > - I included the username/passphrase fields in the ifconfig > output, as it seems to me that APN settings are generally > made public so end-users can configure their own devices > If needed I'll take it out, or perhaps change it to display > '*set*' (or similar) if you think it should be hidden? Genrally, the kernel should not return credentials to userland. So these shouldn't just be hidden or obscured in ifconfig. Rather, umb_ioctl should not copy such data out to any userland program. This rule also applies to wifi and carp interfaces, for instance. > A couple of other points: > > - Wireless providers where I am all seem to require a > username/password with the APN. So I'm unable to confirm > whether or not this breaks non-authenticated connections. > > - I've been building my kernel using the documented > procedure, and have no problems there. I ran through a > base system build and that (eventually) completed OK too. > When compiling ifconfig(8), I've been doing 'make includes' > in /usr/src (after installing and booting my new kernel), > and using 'make ifconfig' and 'make install' in > /usr/src/sbin/ifconfig. Is this OK? or should I be doing > something else instead? You should verify that 'make release' works after having completed a full system build. The ramdisks use a special build of ifconfig so a check that this still compiles is required. See 'man release' for details.
Please test: kill `p_priority'
I'd like hardclock() to be free of SCHED_LOCK(), the diff below brings
us one step away from that goal. Please test with your favorite
benchmark and report any regression :o)
The reason for moving the SCHED_LOCK() away from hardclock() is because
it will allow us to re-commit the diff moving accounting out of the
SCHED_LOCK(). In the end we shrink the SCHED_LOCK() which is generally
a good thing(tm).
`p_priority' is not very well named. It's currently a placeholder for
three values:
- the sleeping priority, corresponding to the value passed to tsleep(9)
which will be later used by setrunnable() to place the thread on the
appropriate runqueue.
- the per-CPU runqueue priority which is used to add/remove a thread
to/from a runqueue.
- the scheduling priority, also named `p_usrpri', calculated from the
estimated amount of CPU time used.
The diff below splits the current `p_priority' into two fields `p_runpri'
and `p_slppri'. The important part is the schedclock() chunk:
@@ -519,8 +515,6 @@ schedclock(struct proc *p)
SCHED_LOCK(s);
newcpu = ESTCPULIM(p->p_estcpu + 1);
setpriority(p, newcpu, p->p_p->ps_nice);
- if (p->p_priority >= PUSER)
- p->p_priority = p->p_usrpri;
SCHED_UNLOCK(s);
`p_priority' had to be updated because it was overwritten during each
tsleep()/setrunnable() cycle. The same happened in schedcpu():
schedcpu: reseting priority for zerothread(44701) 127 -> 90
schedclock: reseting priority for zerothread(44701) 127 -> 91
Getting rid of this assignment means we can then protect `p_estcpu' and
`p_usrpri' with a custom rer-thread lock.
With this division, `p_runpri' becomes part of the scheduler fields while
`p_slppri' is obviously part of the global sleepqueue.
The rest of the diff is quite straightforward, including the userland
compatibility bits.
Tests, comments and oks welcome :o)
Index: arch/sparc64/sparc64/db_interface.c
===
RCS file: /cvs/src/sys/arch/sparc64/sparc64/db_interface.c,v
retrieving revision 1.54
diff -u -p -r1.54 db_interface.c
--- arch/sparc64/sparc64/db_interface.c 7 Nov 2019 14:44:53 - 1.54
+++ arch/sparc64/sparc64/db_interface.c 13 Jan 2020 18:04:23 -
@@ -958,10 +958,10 @@ db_proc_cmd(addr, have_addr, count, modi
return;
}
db_printf("process %p:", p);
- db_printf("pid:%d vmspace:%p pmap:%p ctx:%x wchan:%p pri:%d upri:%d\n",
+ db_printf("pid:%d vmspace:%p pmap:%p ctx:%x wchan:%p rpri:%d upri:%d\n",
p->p_p->ps_pid, p->p_vmspace, p->p_vmspace->vm_map.pmap,
p->p_vmspace->vm_map.pmap->pm_ctx,
- p->p_wchan, p->p_priority, p->p_usrpri);
+ p->p_wchan, p->p_runpri, p->p_usrpri);
db_printf("maxsaddr:%p ssiz:%dpg or %llxB\n",
p->p_vmspace->vm_maxsaddr, p->p_vmspace->vm_ssize,
(unsigned long long)ptoa(p->p_vmspace->vm_ssize));
Index: dev/pci/drm/i915/intel_breadcrumbs.c
===
RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_breadcrumbs.c,v
retrieving revision 1.4
diff -u -p -r1.4 intel_breadcrumbs.c
--- dev/pci/drm/i915/intel_breadcrumbs.c10 Jul 2019 07:56:30 -
1.4
+++ dev/pci/drm/i915/intel_breadcrumbs.c13 Jan 2020 17:40:41 -
@@ -451,7 +451,7 @@ static bool __intel_engine_add_wait(stru
#ifdef __linux__
if (wait->tsk->prio > to_wait(parent)->tsk->prio) {
#else
- if (wait->tsk->p_priority >
to_wait(parent)->tsk->p_priority) {
+ if (wait->tsk->p_usrpri >
to_wait(parent)->tsk->p_usrpri) {
#endif
p = >rb_right;
first = false;
@@ -538,7 +538,7 @@ static inline bool chain_wakeup(struct r
#else
static inline bool chain_wakeup(struct rb_node *rb, int priority)
{
- return rb && to_wait(rb)->tsk->p_priority <= priority;
+ return rb && to_wait(rb)->tsk->p_usrpri <= priority;
}
#endif
@@ -558,7 +558,7 @@ static inline int wakeup_priority(struct
if (p == b->signaler)
return INT_MIN;
else
- return p->p_priority;
+ return p->p_usrpri;
}
#endif
Index: kern/kern_fork.c
===
RCS file: /cvs/src/sys/kern/kern_fork.c,v
retrieving revision 1.220
diff -u -p -r1.220 kern_fork.c
--- kern/kern_fork.c6 Jan 2020 10:25:10 - 1.220
+++ kern/kern_fork.c13 Jan 2020 18:04:35 -
@@ -312,7 +312,7 @@ fork_thread_start(struct proc *p, struct
SCHED_LOCK(s);
ci = sched_choosecpu_fork(parent, flags);
- setrunqueue(ci, p, p->p_priority);
+ setrunqueue(ci, p, p->p_usrpri);
SCHED_UNLOCK(s);
}
MAKE: simplify compat handling
The convoluted logic that resets must_make does not make any sense to me.
It's just as simple to set built_status to ABORTED directly.
Note that in the compat make case, there are two instances of using
must_make left, one in arch.c, which I have yet to understand, and one
in node_failure/list_parents.
diff --git a/compat.c b/compat.c
index fd78d78..9173f44 100644
--- a/compat.c
+++ b/compat.c
@@ -116,9 +116,7 @@ CompatMake(void *gnp, /* The node to make */
* transformations the suffix module thinks are necessary.
* Once that's done, we can descend and make all our children.
* If any of them has an error but the -k flag was given,
-* our 'must_make' field will be set false again. This is our
-* signal to not attempt to do anything but abort our
-* parent as well. */
+* we will abort. */
gn->must_make = true;
gn->built_status = BUILDING;
/* note that, in case we have siblings, we only check all
@@ -132,10 +130,9 @@ CompatMake(void *gnp, /* The node to make */
sib = sib->sibling;
} while (sib != gn);
- if (!gn->must_make) {
+ if (gn->built_status == ABORTED) {
Error("Build for %s aborted", gn->name);
- gn->built_status = ABORTED;
- pgn->must_make = false;
+ pgn->built_status = ABORTED;
return;
}
@@ -233,7 +230,7 @@ CompatMake(void *gnp, /* The node to make */
Make_TimeStamp(pgn, gn);
}
} else if (keepgoing)
- pgn->must_make = false;
+ pgn->built_status = ABORTED;
else {
print_errors();
exit(1);
@@ -242,12 +239,12 @@ CompatMake(void *gnp, /* The node to make */
case ERROR:
/* Already had an error when making this beastie. Tell the
* parent to abort. */
- pgn->must_make = false;
+ pgn->built_status = ABORTED;
break;
case BUILDING:
Error("Graph cycles through %s", gn->name);
gn->built_status = ERROR;
- pgn->must_make = false;
+ pgn->built_status = ABORTED;
break;
case REBUILT:
if ((gn->type & OP_EXEC) == 0) {
Re: pfctl - allow recursive flush operations [ 5/5 ]
Hello,
> > + unsigned int len;
> strlen(3) returns size_t, malloc(3) takes it.
makes sense>
>
> > + struct pfr_anchors *anchors;
> > +
> > + anchors = (struct pfr_anchors *) warg;
> > +
> > + pfra = malloc(sizeof(*pfra));
> > + if (pfra == NULL)
> > + err(1, "%s", __func__);
> > +
> > + len = strlen(pr->path) + 1;
> > + len += strlen(pr->name) + 1;
> > + pfra->pfra_anchorname = malloc(len);
> > + if (pfra->pfra_anchorname == NULL)
> > + err(1, "%s", __func__);
> You're always allocating for path and name, but that is too much in the
> else branch. I think this part can be improved.
you are right, I've opted for ternary operator when
calculating len for pr->path.
>
>
> > +int
> > +pfctl_recurse(int dev, int opts, const char *anchorname,
> > +int(*walkf)(int, int, struct pfr_anchoritem *))
> > +{
> > + int rv = 0;
> > + struct pfr_anchors *anchors;
> > + struct pfr_anchoritem *pfra, *pfra_save;
> > +
> > + anchors = pfctl_get_anchors(dev, anchorname, opts);
> > + /*
> > +* don't let pfctl_clear_*() function to fail with exit
> > +*/
> > + opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
> Why QUIET as well?
the pfctl_clear_tables() is not that silent by default.
we pass pfctl_clear_tables() via walkf argument. I've updated
the comment above to make explain purpose of PF_OPT_QUIET.
diff below updates patch 5/5.
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index cbf17d01b7d..67dae4cdad9 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2173,7 +2173,7 @@ int
pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void *warg)
{
struct pfr_anchoritem *pfra;
- unsigned int len;
+ size_t len;
struct pfr_anchors *anchors;
anchors = (struct pfr_anchors *) warg;
@@ -2182,7 +2182,7 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void
*warg)
if (pfra == NULL)
err(1, "%s", __func__);
- len = strlen(pr->path) + 1;
+ len = (pr->path[0] == '\0') ? 0 : strlen(pr->path) + 1;
len += strlen(pr->name) + 1;
pfra->pfra_anchorname = malloc(len);
if (pfra->pfra_anchorname == NULL)
@@ -2312,7 +2312,8 @@ pfctl_recurse(int dev, int opts, const char *anchorname,
anchors = pfctl_get_anchors(dev, anchorname, opts);
/*
-* don't let pfctl_clear_*() function to fail with exit
+* don't let pfctl_clear_*() function to fail with exit, also make
+* pfctl_clear_tables(),(which is passed via walkf argument, quiet.
*/
opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
printf("Removing:\n");
tripping assert in pf_state_key_link_reverse()
Hello,
Some time ago I've tripped over ASSERT() in pf_state_key_link_reverse(), when
testing my changes to pfsync. I was using HP 710 systems with 8 cores I got
from Hrvoje. The panic happened rarely when running performance tesst over 10Gb
interfaces.
At time of panic the firewall was running with 'pass all' rule only. The rule
creates state for inbound and outbound packets. At time of panic my box was
was running a custom kernel (compiled with WITH_PF_LOCK option and multiple
input net tasks). My explanation how I could trip the assert is as follows:
client sends SYN packet, which creates two states.
now assume the server delays a response, so the response
crosses with retransmitted SYN in firewall. In this case
PF will be running two instances of pf_state_key_link_reverse().
In this case there is one winner and one looser, which trips assert.
the fix in patch below is straightforward. Use atomic ops to link keys, while
keeping same asserts in place, when either atomic op fails.
OK?
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/pf.c b/sys/net/pf.c
index ebe339921fa..738759e556c 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -7398,11 +7398,20 @@ pf_inp_unlink(struct inpcb *inp)
void
pf_state_key_link_reverse(struct pf_state_key *sk, struct pf_state_key *skrev)
{
- /* Note that sk and skrev may be equal, then we refcount twice. */
- KASSERT(sk->reverse == NULL);
- KASSERT(skrev->reverse == NULL);
- sk->reverse = pf_state_key_ref(skrev);
- skrev->reverse = pf_state_key_ref(sk);
+ struct pf_state_key *old_reverse;
+
+ old_reverse = atomic_cas_ptr(>reverse, NULL, skrev);
+ if (old_reverse != NULL)
+ KASSERT(old_reverse == skrev);
+ else
+ pf_state_key_ref(skrev);
+
+
+ old_reverse = atomic_cas_ptr(>reverse, NULL, sk);
+ if (old_reverse != NULL)
+ KASSERT(old_reverse == sk);
+ else
+ pf_state_key_ref(sk);
}
#if NPFLOG > 0
Re: apply changes immediately to join'd essids
On Mon, Jan 13, 2020 at 10:38:35PM +0100, Peter Hessler wrote:
> On 2020 Jan 12 (Sun) at 21:39:19 +0100 (+0100), Peter Hessler wrote:
> :When we change attributes for a join essid, we should apply the change
> :immediately instead of waiting to (randomly) switch away and switch
> :back.
>
> And if we are connected to an AP, remove the node from the cache so we
> can properly reconnect.
>
> OK?
>
>
> Index: net80211/ieee80211_ioctl.c
> ===
> RCS file: /home/cvs/openbsd/src/sys/net80211/ieee80211_ioctl.c,v
> retrieving revision 1.78
> diff -u -p -u -p -r1.78 ieee80211_ioctl.c
> --- net80211/ieee80211_ioctl.c13 Jan 2020 09:57:25 - 1.78
> +++ net80211/ieee80211_ioctl.c13 Jan 2020 16:16:18 -
> @@ -543,7 +543,13 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
> if (ic->ic_des_esslen == join.i_len &&
> memcmp(join.i_nwid, ic->ic_des_essid,
> join.i_len) == 0) {
> + struct ieee80211_node *ni;
> +
> ieee80211_deselect_ess(ic);
> + ni = ieee80211_find_node(ic,
> + ic->ic_bss->ni_bssid);
> + if (ni != NULL)
> + ieee80211_release_node(ic, ni);
This should call ieee80211_free_node() instead. We should also make
sure this code runs in station opmode (IEEE80211_M_STA) only.
> error = ENETRESET;
> }
> /* save nwid for auto-join */
>
Re: [PATCH] src/libexec/ftpd: fix nlist with -option does not work
On Mon, Nov 18, 2019 at 02:56:46PM +0900, SASANO Takayoshi wrote: > At least OpenBSD-6.5 and 6.6's ftpd does not work NLIST command with > any -option like this. > > > ftp> nlist > 150 Opening ASCII mode data connection for 'file list'. > uaa > _sysupgrade > 226 Transfer complete. > ftp> nlist -LF > 550 -LF: No such file or directory. > ftp> > > > Here is the remedy, ok? I don't like the idea to let the client call custom options of ls(1). It seems to be secure, but no one knows what options will implemented in ls(1) in the future. Also the FTP RFC does not mention custom options, as far as I can see. It's just possible to do that, because traditional ftp daemons (like ours) call ls(1). I'm more interested in avoiding option insertion by put a "--" before the clients parameters. bye, Jan
Re: apply changes immediately to join'd essids
On 2020 Jan 12 (Sun) at 21:39:19 +0100 (+0100), Peter Hessler wrote:
:When we change attributes for a join essid, we should apply the change
:immediately instead of waiting to (randomly) switch away and switch
:back.
And if we are connected to an AP, remove the node from the cache so we
can properly reconnect.
OK?
Index: net80211/ieee80211_ioctl.c
===
RCS file: /home/cvs/openbsd/src/sys/net80211/ieee80211_ioctl.c,v
retrieving revision 1.78
diff -u -p -u -p -r1.78 ieee80211_ioctl.c
--- net80211/ieee80211_ioctl.c 13 Jan 2020 09:57:25 - 1.78
+++ net80211/ieee80211_ioctl.c 13 Jan 2020 16:16:18 -
@@ -543,7 +543,13 @@ ieee80211_ioctl(struct ifnet *ifp, u_lon
if (ic->ic_des_esslen == join.i_len &&
memcmp(join.i_nwid, ic->ic_des_essid,
join.i_len) == 0) {
+ struct ieee80211_node *ni;
+
ieee80211_deselect_ess(ic);
+ ni = ieee80211_find_node(ic,
+ ic->ic_bss->ni_bssid);
+ if (ni != NULL)
+ ieee80211_release_node(ic, ni);
error = ENETRESET;
}
/* save nwid for auto-join */
umb(4) WIP diff and questions
Hello again tech@
I've included diffs of what I've got so far at the bottom
of this mail, but first a couple of questions:
- Using the full 510-character limits for username and
passphrase specified in the MBIM spec, kernel compilation
fails due to tripping the 2047-byte stack frame warning
when compiling the driver. That's the reason for the '100'
magic numbers that I put in there temporarily.
Any hints on the best way to handle this would be appreciated.
- I included the username/passphrase fields in the ifconfig
output, as it seems to me that APN settings are generally
made public so end-users can configure their own devices
If needed I'll take it out, or perhaps change it to display
'*set*' (or similar) if you think it should be hidden?
A couple of other points:
- Wireless providers where I am all seem to require a
username/password with the APN. So I'm unable to confirm
whether or not this breaks non-authenticated connections.
- I've been building my kernel using the documented
procedure, and have no problems there. I ran through a
base system build and that (eventually) completed OK too.
When compiling ifconfig(8), I've been doing 'make includes'
in /usr/src (after installing and booting my new kernel),
and using 'make ifconfig' and 'make install' in
/usr/src/sbin/ifconfig. Is this OK? or should I be doing
something else instead?
Thanks,
Lee.
Index: sbin/ifconfig/ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.343
diff -u -p -u -p -r1.343 ifconfig.8
--- sbin/ifconfig/ifconfig.810 Nov 2019 09:10:44 - 1.343
+++ sbin/ifconfig/ifconfig.814 Jan 2020 11:56:06 -
@@ -1914,6 +1914,9 @@ Clear the virtual network identifier.
.Op Cm pin Ar pin
.Op Cm puk Ar puk Ar newpin
.Op Oo Fl Oc Ns Cm roaming
+.Op Oo Fl Oc Ns Cm umbuser Ar user
+.Op Oo Fl Oc Ns Cm umbpasswd Ar password
+.Op Oo Fl Oc Ns Cm umbauth Ar authmethod
.Ek
.nr nS 0
.Pp
@@ -1960,6 +1963,26 @@ to validate the request.
Enable data roaming.
.It Cm -roaming
Disable data roaming.
+.It Cm umbuser Ar user
+Set the authentication username.
+.It Cm -umbuser
+Clear the current username.
+.It Cm umbpasswd Ar password
+Set the authentication password.
+.It Cm -umbpasswd
+Clear the current password.
+.It Cm umbauth Ar authmethod
+Set the authentication method. Valid methods are:
+.Ar none ,
+.Ar pap ,
+.Ar chap ,
+.Ar mschap .
+Specifying an invalid method will cause a value of
+.Ar none
+to be used.
+.It Cm -umbauth
+Clear the authentication method. Equivalent to
+.Cm umbauth none .
.It Cm up
As soon as the interface is marked as "up", the
.Xr umb 4
Index: sbin/ifconfig/ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.417
diff -u -p -u -p -r1.417 ifconfig.c
--- sbin/ifconfig/ifconfig.c27 Dec 2019 14:34:46 - 1.417
+++ sbin/ifconfig/ifconfig.c14 Jan 2020 11:56:06 -
@@ -339,6 +339,9 @@ voidumb_chgpin(const char *, const char
void umb_puk(const char *, const char *);
void umb_pinop(int, int, const char *, const char *);
void umb_apn(const char *, int);
+void umb_user(const char *, int);
+void umb_passwd(const char *, int);
+void umb_authentication(const char *, int);
void umb_setclass(const char *, int);
void umb_roaming(const char *, int);
void utf16_to_char(uint16_t *, int, char *, size_t);
@@ -587,6 +590,12 @@ const struct cmd {
{ "puk",NEXTARG2, 0, NULL, umb_puk },
{ "apn",NEXTARG,0, umb_apn },
{ "-apn", -1, 0, umb_apn },
+ { "umbuser",NEXTARG,0, umb_user },
+ { "-umbuser", -1, 0, umb_user },
+ { "umbpasswd", NEXTARG,0, umb_passwd },
+ { "-umbpasswd", -1, 0, umb_passwd },
+ { "umbauth",NEXTARG,0, umb_authentication },
+ { "-umbauth", -1, 0, umb_authentication },
{ "class", NEXTARG0, 0, umb_setclass },
{ "-class", -1, 0, umb_setclass },
{ "roaming",1, 0, umb_roaming },
@@ -5628,6 +5637,7 @@ setifpriority(const char *id, int param)
const struct umb_valdescr umb_regstate[] = MBIM_REGSTATE_DESCRIPTIONS;
const struct umb_valdescr umb_dataclass[] = MBIM_DATACLASS_DESCRIPTIONS;
+const struct umb_valdescr umb_authprot[] = MBIM_AUTHPROT_DESCRIPTIONS;
const struct umb_valdescr umb_simstate[] = MBIM_SIMSTATE_DESCRIPTIONS;
const struct umb_valdescr umb_istate[] = UMB_INTERNAL_STATE_DESCRIPTIONS;
const struct umb_valdescr umb_pktstate[] = MBIM_PKTSRV_STATE_DESCRIPTIONS;
@@ -5665,6 +5675,9 @@ umb_status(void)
char iccid[UMB_ICCID_MAXLEN+1];
char
