Re: Add LRO counter in ix(4)
On Thu, May 18, 2023 at 12:01:44AM +0200, Alexander Bluhm wrote:
> On Tue, May 16, 2023 at 09:11:48PM +0200, Jan Klemkow wrote:
> > @@ -412,6 +412,10 @@ tcp_stats(char *name)
> > p(tcps_outhwtso, "\t\t%u output TSO packet%s hardware processed\n");
> > p(tcps_outpkttso, "\t\t%u output TSO packet%s generated\n");
> > p(tcps_outbadtso, "\t\t%u output TSO packet%s dropped\n");
> > + p(tcps_inhwlro, "\t\t%u input LRO generated packet%s from hardware\n");
> > + p(tcps_inpktlro, "\t\t%u input LRO coalesced packet%s from hardware\n");
>
> ... coalesced packet%s by hardware
done
> > + p(tcps_inbadlro, "\t\t%u input bad LRO packet%s from hardware\n");
> > +
>
> Move this down to the "packets received" section. You included it
> in "packets sent".
done
> > + /*
> > +* This function iterates over interleaved descriptors.
> > +* Thus, we reuse ph_mss as global segment counter per
> > +* TCP connection, insteat of introducing a new variable
>
> s/insteat/instead/
done
ok?
Thanks,
Jan
diff --git a/sys/dev/pci/if_ix.c b/sys/dev/pci/if_ix.c
index 4119a2416dc..924a6d63236 100644
--- a/sys/dev/pci/if_ix.c
+++ b/sys/dev/pci/if_ix.c
@@ -3214,12 +3214,23 @@ ixgbe_rxeof(struct rx_ring *rxr)
sendmp = rxbuf->fmp;
rxbuf->buf = rxbuf->fmp = NULL;
- if (sendmp != NULL) /* secondary frag */
+ if (sendmp != NULL) { /* secondary frag */
sendmp->m_pkthdr.len += mp->m_len;
- else {
+
+ /*
+* This function iterates over interleaved descriptors.
+* Thus, we reuse ph_mss as global segment counter per
+* TCP connection, instead of introducing a new variable
+* in m_pkthdr.
+*/
+ if (rsccnt)
+ sendmp->m_pkthdr.ph_mss += rsccnt - 1;
+ } else {
/* first desc of a non-ps chain */
sendmp = mp;
sendmp->m_pkthdr.len = mp->m_len;
+ if (rsccnt)
+ sendmp->m_pkthdr.ph_mss = rsccnt - 1;
#if NVLAN > 0
if (sc->vlan_stripping && staterr & IXGBE_RXD_STAT_VP) {
sendmp->m_pkthdr.ether_vtag = vtag;
@@ -3241,6 +3252,21 @@ ixgbe_rxeof(struct rx_ring *rxr)
SET(sendmp->m_pkthdr.csum_flags, M_FLOWID);
}
+ if (sendmp->m_pkthdr.ph_mss == 1)
+ sendmp->m_pkthdr.ph_mss = 0;
+
+ if (sendmp->m_pkthdr.ph_mss > 0) {
+ struct ether_extracted ext;
+ uint16_t pkts = sendmp->m_pkthdr.ph_mss;
+
+ ether_extract_headers(sendmp, &ext);
+ if (ext.tcp)
+ tcpstat_inc(tcps_inhwlro);
+ else
+ tcpstat_inc(tcps_inbadlro);
+ tcpstat_add(tcps_inpktlro, pkts);
+ }
+
ml_enqueue(&ml, sendmp);
}
next_desc:
diff --git a/sys/netinet/tcp_usrreq.c b/sys/netinet/tcp_usrreq.c
index 120e3cc5ea7..3970636cde1 100644
--- a/sys/netinet/tcp_usrreq.c
+++ b/sys/netinet/tcp_usrreq.c
@@ -1340,6 +1340,9 @@ tcp_sysctl_tcpstat(void *oldp, size_t *oldlenp, void
*newp)
ASSIGN(tcps_outhwtso);
ASSIGN(tcps_outpkttso);
ASSIGN(tcps_outbadtso);
+ ASSIGN(tcps_inhwlro);
+ ASSIGN(tcps_inpktlro);
+ ASSIGN(tcps_inbadlro);
#undef ASSIGN
diff --git a/sys/netinet/tcp_var.h b/sys/netinet/tcp_var.h
index 0a9630d719f..e706fedd0e7 100644
--- a/sys/netinet/tcp_var.h
+++ b/sys/netinet/tcp_var.h
@@ -447,6 +447,9 @@ struct tcpstat {
u_int32_t tcps_outhwtso;/* output tso processed by hardware */
u_int32_t tcps_outpkttso; /* packets generated by tso */
u_int32_t tcps_outbadtso; /* output tso failed, packet dropped */
+ u_int32_t tcps_inhwlro; /* input lro from hardware */
+ u_int32_t tcps_inpktlro;/* packets coalessed by hardware lro */
+ u_int32_t tcps_inbadlro;/* input bad lro packets from hardware
*/
};
/*
@@ -625,6 +628,9 @@ enum tcpstat_counters {
tcps_outhwtso,
tcps_outpkttso,
tcps_outbadtso,
+ tcps_inhwlro,
+ tcps_inpktlro,
+ tcps_inbadlro,
tcps_ncounters,
};
diff --git a/usr.bin/netstat/inet.c b/usr.bin/netstat/inet.c
index e04355ed078..6db14673b97 100644
--- a/usr.bin/netstat/inet.c
+++ b/usr.bin/netstat/inet.c
@@ -439,6 +439,9 @@ tcp_stats(char *name)
p(tcps_inswcsum, "\t\t%u packet%s software-checksu
Re: Unlock ip6_sysctl()
On Thu, May 18, 2023 at 01:56:13AM +0300, Vitaliy Makkoveev wrote: > > On 18 May 2023, at 01:14, Alexander Bluhm wrote: > > > > On Wed, May 17, 2023 at 12:46:02PM +0300, Vitaliy Makkoveev wrote: > >> Introduce `ip6_soiikey_lock' rwlock(9) to protect `ip6_soiikey'. It > >> accessed only by ip6_sysctl_soiikey() and ip6_sysctl() is the only > >> ip6_sysctl_soiikey() caller so context switch is allowed here. Also > >> remove unused `oldkey' from ip6_sysctl_soiikey(). > > > > rwlock or mutex? As sysctl mlocks userland memory, copyin/copyout > > cannot sleep here. So it should be safe to use a mutex. > > It is locked because context switch is strictly unwanted when you > do copyin/copyout within foreach loops of kernel lock protected > lists like ps_list. Really this lock is the kind of a hack. But > since rwlocks already introduced context switch to sysctl() paths, > we use sysctl_lock rwlock to prevent many processes to > simultaneously lock large amount of memory. This is another hack > in this layer. Guess, both of them will gone in the future, so > copyin/copyout will sleep. So rwlock is better. > > > > > And why is this ip6_soiikey in the kernel anyway? I guess it is > > from a time when address configuration was done in the kernel. > > Could slaacd(8) just read /etc/soii.key? > > It could, but this should be implemented in slaacd first. And this > work is not related to (*pr_sysctl)() unlocking. > Please stop changing the locking inside sysctl. It seems that the last change introduced a regression with the result that systems lock up. Until that is solved nothing else should be changed in this code. -- :wq Claudio
Re: Unlock ip6_sysctl()
On 2023-05-18 00:14 +02, Alexander Bluhm wrote: > And why is this ip6_soiikey in the kernel anyway? I guess it is > from a time when address configuration was done in the kernel. > Could slaacd(8) just read /etc/soii.key? Originally we implemented RFC 7217 for link-local addresses, too. The value of doing that is IMO not very high and it turned out that there are ISPs out there that calculate the clients link-local address from the MAC address and route a /64 (or whatever) to that link-local address. If we calculate a different link-local address IPv6 is broken. It's pretty difficult to debug this so we removed support for it in the kernel. p.s. Furthermore I think link-local addresses should be generated by userland. -- In my defence, I have been left unsupervised.
install.sub: two little ergonomic tweaks
Hello devs!
First of all, my compliment.
The installer is already quite ergonomic (for a CLI ;) ).
But there are the following two little diff(1)s standing
between it and its perfection IMAO.
--- distrib/miniroot/install.sub.orig Thu May 18 12:37:52 2023
+++ distrib/miniroot/install.subThu May 18 12:44:49 2023
@@ -1220,3 +1220,3 @@
ask_until "IPv6 address for $_if? (or 'autoconf' or 'none')" \
- "${_addr:-none}"
+ "${_addr:-autoconf}"
case $resp in
I personally enable IPv6 everywhere,
even if I have only link-local addresses.
If I got SLAAC, nice for my OpenBSD clients
and the clients of my OpenBSD servers.
Win-win. If not, I haven't lost anything.
In the worst case I have to do specific config,
but then the default doesn't matter anyway.
The only reason against this could be a permit-default pf.conf.
But such shouldn't be done and this is the installer after all.
One writes pf.conf after the installer or can -in extreme case-
still type "none" here (which is shorter to type).
I know that you folks like not to surprise users.
But IMAO default-enabling IPv6 *on new installs* isn't a surprise
(in 2023 when IIRC some US gov orgs already sell their whole IPv4s).
In case you don't agree with me:
What about a shortcut "a" (= autoconf)
for IPv[46] address (like below)?
--- distrib/miniroot/install.sub.orig Thu May 18 12:37:52 2023
+++ distrib/miniroot/install.subThu May 18 12:44:49 2023
@@ -2306,15 +2306,15 @@
[[ $START_SSHD == y ]] || return
if [[ -z $ADMIN ]]; then
echo "Since no user was setup, root logins via sshd(8) might be
useful."
fi
echo "WARNING: root is targeted by password guessing attacks, pubkeys are
safer."
while :; do
- ask "Allow root ssh login? (yes, no, prohibit-password)" no
+ ask "Allow root ssh login? (yes, no, (p)rohibit-password)" no
_resp=$resp
case $_resp in
y|yes) SSHD_ENABLEROOT=yes
;;
n|no) SSHD_ENABLEROOT=no
;;
w|p|without-password|prohibit-password)
Originally I wanted to do the same thing as above here.
I.e. to change the default no -> prohibit-password
which isn't less secure IMAO until you explicitly set auth. keys.
But then I've discovered the "p" shortcut (I'm showing you via diff(1) -U7).
IMAO showing it as I patched wouldn't harm anyone.
ok?
Re: install.sub: two little ergonomic tweaks
Alexander Klimov wrote: > --- distrib/miniroot/install.sub.orig Thu May 18 12:37:52 2023 > +++ distrib/miniroot/install.subThu May 18 12:44:49 2023 > @@ -2306,15 +2306,15 @@ > [[ $START_SSHD == y ]] || return > > if [[ -z $ADMIN ]]; then > echo "Since no user was setup, root logins via sshd(8) might > be useful." > fi > echo "WARNING: root is targeted by password guessing attacks, pubkeys > are safer." > while :; do > - ask "Allow root ssh login? (yes, no, prohibit-password)" no > + ask "Allow root ssh login? (yes, no, (p)rohibit-password)" no > _resp=$resp > case $_resp in > y|yes) SSHD_ENABLEROOT=yes > ;; > n|no) SSHD_ENABLEROOT=no > ;; > w|p|without-password|prohibit-password) > > Originally I wanted to do the same thing as above here. > I.e. to change the default no -> prohibit-password > which isn't less secure IMAO until you explicitly set auth. keys. > But then I've discovered the "p" shortcut (I'm showing you via diff(1) -U7). > IMAO showing it as I patched wouldn't harm anyone. This should not be neccessary. Almost all the prompts in the installer allow shortcut answers, as long as it is unambigious. Users can assume this, and learn it quickly. Therefore we do NOT chang all the "yes" to "(y)es". I'd be shocked if you haven't discovered this on your own. What would be interesting, is to hear of a bug where the case statements are not handling a short form. You'll see in the code above that we match "y|yes", we do not match "ye". That is also intentional. But if someone forgets to add a "n", that would be bad, and we would want to fix it.
Re: install.sub: two little ergonomic tweaks
Alexander Klimov wrote:
> First of all, my compliment.
> The installer is already quite ergonomic (for a CLI ;) ).
> But there are the following two little diff(1)s standing
> between it and its perfection IMAO.
>
>
> --- distrib/miniroot/install.sub.orig Thu May 18 12:37:52 2023
> +++ distrib/miniroot/install.subThu May 18 12:44:49 2023
> @@ -1220,3 +1220,3 @@
> ask_until "IPv6 address for $_if? (or 'autoconf' or 'none')" \
> - "${_addr:-none}"
> + "${_addr:-autoconf}"
> case $resp in
>
> I personally enable IPv6 everywhere,
> even if I have only link-local addresses.
> If I got SLAAC, nice for my OpenBSD clients
> and the clients of my OpenBSD servers.
> Win-win. If not, I haven't lost anything.
> In the worst case I have to do specific config,
> but then the default doesn't matter anyway.
>
> The only reason against this could be a permit-default pf.conf.
> But such shouldn't be done and this is the installer after all.
> One writes pf.conf after the installer or can -in extreme case-
> still type "none" here (which is shorter to type).
> I know that you folks like not to surprise users.
> But IMAO default-enabling IPv6 *on new installs* isn't a surprise
> (in 2023 when IIRC some US gov orgs already sell their whole IPv4s).
I disagree. I believe network participation should be opt-in. Minimal
network configuration is encouraged, but the minimum is v4, not v6.
v6 is still not natural. It is a surprise. I think it will be too easy
for users to turn it on in the wrong places, and since OpenBSD
automatically learns DNS configuration from such v6 network configuration
edges, this is a hazard. Configurating v6 must be intentional.
> In case you don't agree with me:
> What about a shortcut "a" (= autoconf)
> for IPv[46] address (like below)?
That would make sense. It would also need to be done for v4. Diff would
be something like this, not tested, there might be other pieces missing to
do it perfectly.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1244
diff -u -p -u -r1.1244 install.sub
--- install.sub 2 May 2023 15:55:58 - 1.1244
+++ install.sub 18 May 2023 14:01:01 -
@@ -1105,7 +1105,7 @@ v4_config() {
case $resp in
none) return
;;
- autoconf|dhcp)
+ a|autoconf|dhcp)
dhcp_request $_if
echo "inet autoconf" >>$_hn
return
@@ -1222,7 +1222,7 @@ v6_config() {
case $resp in
none) return
;;
- autoconf)
+ a|autoconf)
ifconfig $_if inet6 autoconf up
echo "inet6 autoconf" >>$_hn
return
user: handle paths with whitespace / metacharacters
The way user(8) runs commands via system(3) is fragile. It does
not correctly handle paths with whitespace or shell metacharacters.
Rather than try to quote everything (which is also fragile) I think
it is safest to just exec the commands directly.
OK?
- todd
Index: usr.sbin/user/user.c
===
RCS file: /cvs/src/usr.sbin/user/user.c,v
retrieving revision 1.130
diff -u -p -u -r1.130 user.c
--- usr.sbin/user/user.c16 May 2023 21:28:46 - 1.130
+++ usr.sbin/user/user.c18 May 2023 15:10:52 -
@@ -31,6 +31,7 @@
#include
#include
+#include
#include
#include
@@ -42,7 +43,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
@@ -103,6 +104,10 @@ enum {
F_ACCTUNLOCK= 0x1
};
+/* bit flags for runas() */
+#define RUNAS_DUP_DEVNULL 0x01
+#define RUNAS_IGNORE_EXITVAL 0x02
+
#define CONFFILE "/etc/usermgmt.conf"
#define _PATH_NONEXISTENT "/nonexistent"
@@ -179,8 +184,6 @@ enum {
static int adduser(char *, user_t *);
static int append_group(char *, int, const char **);
-static int asystem(const char *, ...)
- __attribute__((__format__(__printf__, 1, 2)));
static int copydotfiles(char *, char *);
static int creategid(char *, gid_t, const char *);
static int getnextgid(uid_t *, uid_t, uid_t);
@@ -214,30 +217,77 @@ strsave(char **cpp, const char *s)
err(1, NULL);
}
-/* a replacement for system(3) */
+/* run the given command with optional arguments as the specified uid */
static int
-asystem(const char *fmt, ...)
+runas(const char *path, const char *const argv[], uid_t uid, int flags)
{
- va_list vp;
- charbuf[MaxCommandLen];
- int ret;
+ int argc, status, ret = 0;
+ char buf[MaxCommandLen];
+ pid_t child;
- va_start(vp, fmt);
- (void) vsnprintf(buf, sizeof(buf), fmt, vp);
- va_end(vp);
- if (verbose) {
- printf("Command: %s\n", buf);
+ strlcpy(buf, path, sizeof(buf));
+ for (argc = 1; argv[argc] != NULL; argc++) {
+ strlcat(buf, " ", sizeof(buf));
+ strlcat(buf, argv[argc], sizeof(buf));
}
- if ((ret = system(buf)) != 0) {
- warnx("[Warning] can't system `%s'", buf);
+ if (verbose)
+ printf("Command: %s\n", buf);
+
+ child = fork();
+ switch (child) {
+ case -1:
+ err(EXIT_FAILURE, "fork");
+ case 0:
+ if (flags & RUNAS_DUP_DEVNULL) {
+ /* Redirect output to /dev/null if possible. */
+ int dev_null = open(_PATH_DEVNULL, O_RDWR);
+ if (dev_null != -1) {
+ dup2(dev_null, STDOUT_FILENO);
+ dup2(dev_null, STDERR_FILENO);
+ if (dev_null > STDERR_FILENO)
+ close(dev_null);
+ } else {
+ warn("%s", _PATH_DEVNULL);
+ }
+ }
+ if (uid != -1) {
+ if (setresuid(uid, uid, uid) == -1)
+ warn("setresuid(%u, %u, %u)", uid, uid, uid);
+ }
+ execv(path, (char **)argv);
+ warn("%s", buf);
+ _exit(EXIT_FAILURE);
+ default:
+ while (waitpid(child, &status, 0) == -1) {
+ if (errno != EINTR)
+ err(EXIT_FAILURE, "waitpid");
+ }
+ if (WIFSIGNALED(status)) {
+ ret = WTERMSIG(status);
+ warnx("[Warning] `%s' killed by signal %d", buf, ret);
+ ret |= 128;
+ } else {
+ if (!(flags & RUNAS_IGNORE_EXITVAL))
+ ret = WEXITSTATUS(status);
+ if (ret != 0)
+ warnx("[Warning] unable to run `%s'", buf);
+ }
+ return ret;
}
- return ret;
+}
+
+/* run the given command with optional arguments */
+static int
+run(const char *path, const char *const argv[])
+{
+ return runas(path, argv, -1, false);
}
/* remove a users home directory, returning 1 for success (ie, no problems
encountered) */
static int
removehomedir(const char *user, uid_t uid, const char *dir)
{
+ const char *rm_argv[] = { "rm", "-rf", dir, NULL };
struct stat st;
/* userid not root? */
@@ -263,11 +313,9 @@ removehomedir(const char *user, uid_t ui
return 0;
}
- (void) seteuid(uid);
- /* we add the "|| true" to keep asystem() quiet if there is a non-zero
exit status. */
- (void) asystem("%s -rf %s > /dev/null 2>&1 || true", RM, dir);
- (void) seteuid(0);
Re: user: handle paths with whitespace / metacharacters
On 2023/05/18 09:11:59 -0600, Todd C. Miller wrote:
> The way user(8) runs commands via system(3) is fragile. It does
> not correctly handle paths with whitespace or shell metacharacters.
> Rather than try to quote everything (which is also fragile) I think
> it is safest to just exec the commands directly.
when I saw that the previous change was to remove the only "cd xyz &&
cmd" I hoped that the follow up was to use exec instead of system :)
> OK?
verified that it works as intended. -v is actually nicer now, since
it logs the commands it executes (which is expected as per man page
but not as per previous behaviour.)
for what is worth ok op@ (with or without nits below)
> - todd
>
> Index: usr.sbin/user/user.c
> ===
> RCS file: /cvs/src/usr.sbin/user/user.c,v
> retrieving revision 1.130
> diff -u -p -u -r1.130 user.c
> --- usr.sbin/user/user.c 16 May 2023 21:28:46 - 1.130
> +++ usr.sbin/user/user.c 18 May 2023 15:10:52 -
> @@ -31,6 +31,7 @@
>
> #include
> #include
> +#include
>
> #include
> #include
> @@ -42,7 +43,7 @@
> #include
> #include
> #include
> -#include
> +#include
existing code just used 0 and 1 (e.g. creategid.) lone zeros as
arguments are not really readable, but still I can't make myself
liking stdbool. not that my tastes matters, but it seems to be used
very sparsingly in usr.sbin
> #include
> #include
> #include
> @@ -103,6 +104,10 @@ enum {
> F_ACCTUNLOCK= 0x1
> };
>
> +/* bit flags for runas() */
> +#define RUNAS_DUP_DEVNULL0x01
> +#define RUNAS_IGNORE_EXITVAL 0x02
another nit: the other flags are defined within an enums.
> [...]
> -/* a replacement for system(3) */
> +/* run the given command with optional arguments as the specified uid */
> static int
> -asystem(const char *fmt, ...)
> +runas(const char *path, const char *const argv[], uid_t uid, int flags)
> {
> [...]
> + default:
> + while (waitpid(child, &status, 0) == -1) {
> + if (errno != EINTR)
> + err(EXIT_FAILURE, "waitpid");
> + }
> + if (WIFSIGNALED(status)) {
> + ret = WTERMSIG(status);
> + warnx("[Warning] `%s' killed by signal %d", buf, ret);
> + ret |= 128;
> + } else {
> + if (!(flags & RUNAS_IGNORE_EXITVAL))
> + ret = WEXITSTATUS(status);
> + if (ret != 0)
> + warnx("[Warning] unable to run `%s'", buf);
more than "unable to run" I'd say "failed to run" or "command
failed" / "exited with nonzero status".
"unable to run" makes me think that it failed to exec the
program, rather than it failed at runtime.
> + }
> + return ret;
> }
> - return ret;
> +}
> +
> +/* run the given command with optional arguments */
> +static int
> +run(const char *path, const char *const argv[])
> +{
> + return runas(path, argv, -1, false);
> }
Re: ix hardware tso
Alexander Bluhm wrote: > we cannot do hardware TSO in a bridge. Maybe we could if all > bridge members support it. Just a note that I've discussed another "if all bridge members" case with dlg; switch drivers eventually knowing to do bridge offloading. //Peter
Re: user: handle paths with whitespace / metacharacters
On Thu, 18 May 2023 18:13:57 +0200, Omar Polo wrote:
> existing code just used 0 and 1 (e.g. creategid.) lone zeros as
> arguments are not really readable, but still I can't make myself
> liking stdbool. not that my tastes matters, but it seems to be used
> very sparsingly in usr.sbin
This was leftover from before I added bit flags to runas().
It is not actually needed now so I have removed it.
> > #include
> > #include
> > #include
> > @@ -103,6 +104,10 @@ enum {
> > F_ACCTUNLOCK= 0x1
> > };
> >
> > +/* bit flags for runas() */
> > +#define RUNAS_DUP_DEVNULL 0x01
> > +#define RUNAS_IGNORE_EXITVAL 0x02
>
> another nit: the other flags are defined within an enums.
Sure, no reason to be inconsistent.
> > [...]
> > -/* a replacement for system(3) */
> > +/* run the given command with optional arguments as the specified uid */
> > static int
> > -asystem(const char *fmt, ...)
> > +runas(const char *path, const char *const argv[], uid_t uid, int flags)
> > {
> > [...]
> > + default:
> > + while (waitpid(child, &status, 0) == -1) {
> > + if (errno != EINTR)
> > + err(EXIT_FAILURE, "waitpid");
> > + }
> > + if (WIFSIGNALED(status)) {
> > + ret = WTERMSIG(status);
> > + warnx("[Warning] `%s' killed by signal %d", buf, ret);
> > + ret |= 128;
> > + } else {
> > + if (!(flags & RUNAS_IGNORE_EXITVAL))
> > + ret = WEXITSTATUS(status);
> > + if (ret != 0)
> > + warnx("[Warning] unable to run `%s'", buf);
>
> more than "unable to run" I'd say "failed to run" or "command
> failed" / "exited with nonzero status".
How about "failed with status N"? It can be useful to have the
exit status since that may indicate the source of the error (though
perhaps not so much in this case).
Updated diff follows.
- todd
Index: usr.sbin/user/user.c
===
RCS file: /cvs/src/usr.sbin/user/user.c,v
retrieving revision 1.130
diff -u -p -u -r1.130 user.c
--- usr.sbin/user/user.c16 May 2023 21:28:46 - 1.130
+++ usr.sbin/user/user.c18 May 2023 17:24:01 -
@@ -31,6 +31,7 @@
#include
#include
+#include
#include
#include
@@ -42,7 +43,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -103,6 +103,12 @@ enum {
F_ACCTUNLOCK= 0x1
};
+/* flags for runas() */
+enum {
+ RUNAS_DUP_DEVNULL = 0x01,
+ RUNAS_IGNORE_EXITVAL= 0x02
+};
+
#define CONFFILE "/etc/usermgmt.conf"
#define _PATH_NONEXISTENT "/nonexistent"
@@ -179,8 +185,6 @@ enum {
static int adduser(char *, user_t *);
static int append_group(char *, int, const char **);
-static int asystem(const char *, ...)
- __attribute__((__format__(__printf__, 1, 2)));
static int copydotfiles(char *, char *);
static int creategid(char *, gid_t, const char *);
static int getnextgid(uid_t *, uid_t, uid_t);
@@ -214,30 +218,79 @@ strsave(char **cpp, const char *s)
err(1, NULL);
}
-/* a replacement for system(3) */
+/* run the given command with optional arguments as the specified uid */
static int
-asystem(const char *fmt, ...)
+runas(const char *path, const char *const argv[], uid_t uid, int flags)
{
- va_list vp;
- charbuf[MaxCommandLen];
- int ret;
+ int argc, status, ret = 0;
+ char buf[MaxCommandLen];
+ pid_t child;
- va_start(vp, fmt);
- (void) vsnprintf(buf, sizeof(buf), fmt, vp);
- va_end(vp);
- if (verbose) {
- printf("Command: %s\n", buf);
+ strlcpy(buf, path, sizeof(buf));
+ for (argc = 1; argv[argc] != NULL; argc++) {
+ strlcat(buf, " ", sizeof(buf));
+ strlcat(buf, argv[argc], sizeof(buf));
}
- if ((ret = system(buf)) != 0) {
- warnx("[Warning] can't system `%s'", buf);
+ if (verbose)
+ printf("Command: %s\n", buf);
+
+ child = fork();
+ switch (child) {
+ case -1:
+ err(EXIT_FAILURE, "fork");
+ case 0:
+ if (flags & RUNAS_DUP_DEVNULL) {
+ /* Redirect output to /dev/null if possible. */
+ int dev_null = open(_PATH_DEVNULL, O_RDWR);
+ if (dev_null != -1) {
+ dup2(dev_null, STDOUT_FILENO);
+ dup2(dev_null, STDERR_FILENO);
+ if (dev_null > STDERR_FILENO)
+ close(dev_null);
+ } else {
+ warn("%s", _PATH_DEVNULL);
+ }
+ }
+ if (uid != -1) {
+ if (setresuid(uid, uid, uid) == -1)
+
Re: install.sub: two little ergonomic tweaks
On 18.05.23 15:56, Theo de Raadt wrote: Alexander Klimov wrote: --- distrib/miniroot/install.sub.orig Thu May 18 12:37:52 2023 +++ distrib/miniroot/install.subThu May 18 12:44:49 2023 @@ -2306,15 +2306,15 @@ [[ $START_SSHD == y ]] || return if [[ -z $ADMIN ]]; then echo "Since no user was setup, root logins via sshd(8) might be useful." fi echo "WARNING: root is targeted by password guessing attacks, pubkeys are safer." while :; do - ask "Allow root ssh login? (yes, no, prohibit-password)" no + ask "Allow root ssh login? (yes, no, (p)rohibit-password)" no _resp=$resp case $_resp in y|yes) SSHD_ENABLEROOT=yes ;; n|no) SSHD_ENABLEROOT=no ;; w|p|without-password|prohibit-password) Originally I wanted to do the same thing as above here. I.e. to change the default no -> prohibit-password which isn't less secure IMAO until you explicitly set auth. keys. But then I've discovered the "p" shortcut (I'm showing you via diff(1) -U7). IMAO showing it as I patched wouldn't harm anyone. This should not be neccessary. Almost all the prompts in the installer allow shortcut answers, as long as it is unambigious. Users can assume this, and learn it quickly. Therefore we do NOT chang all the "yes" to "(y)es". I'd be shocked if you haven't discovered this on your own. Wait a second! (Yes, one of my biggest talents is to "oversee elephants"TM, but not this time.) E.g. I use the (C)ustom layout which *clearly indicates* its one-(C)har shortcut. Other prompts, like my diff(1)ed one, *do not*. Change my mind. Now, given that I've not overseen yet another elephant right now, if I as a user see some "(C)ustom layout" stuff on the one hand and some "yes"/"autoconf" stuff on the other hand, how should I even think of the latter being shortcuttable? apt(8) and IIRC yum(8) at least write e.g. "[Y/n]" indicating something (the default in this case) by an upper-case. Just let me know in case you change your mind. I.e. if you'd accept a s/yes/(y)es/ etc. patch from me. What would be interesting, is to hear of a bug where the case statements are not handling a short form. You'll see in the code above that we match "y|yes", we do not match "ye". That is also intentional. But if someone forgets to add a "n", that would be bad, and we would want to fix it.
Re: install.sub: two little ergonomic tweaks
Alexander Klimov wrote: > Wait a second! > (Yes, one of my biggest talents is to "oversee elephants"TM, but not this > time.) > E.g. I use the (C)ustom layout which *clearly indicates* its one-(C)har > shortcut. > Other prompts, like my diff(1)ed one, *do not*. > > Change my mind. > > Now, given that I've not overseen yet another elephant right now, > if I as a user see some "(C)ustom layout" stuff on the one hand > and some "yes"/"autoconf" stuff on the other hand, > how should I even think of the latter being shortcuttable? > apt(8) and IIRC yum(8) at least write e.g. "[Y/n]" > indicating something (the default in this case) by an upper-case. > > Just let me know in case you change your mind. > I.e. if you'd accept a s/yes/(y)es/ etc. patch from me. No.
Re: user: handle paths with whitespace / metacharacters
On 2023/05/18 11:28:58 -0600, Todd C. Miller wrote:
> On Thu, 18 May 2023 18:13:57 +0200, Omar Polo wrote:
> > > + if (ret != 0)
> > > + warnx("[Warning] unable to run `%s'", buf);
> >
> > more than "unable to run" I'd say "failed to run" or "command
> > failed" / "exited with nonzero status".
>
> How about "failed with status N"? It can be useful to have the
> exit status since that may indicate the source of the error (though
> perhaps not so much in this case).
I couldn't have come up with something better.
> Updated diff follows.
ok for me.
Thanks!
net_tq_barriers()
this is a tiny slice off a big pfsync diff i've been working on. when
you bring pfsync down i need it to wait until all the work it's been
doing in the network stack has finished, which means i need a barrier
for all the network taskqs. that's what this implements.
a barrier per taskq would mean iterating over the taskqs and waiting for
a barrier on each one. by using a refcnt and shoving a task onto each of
them in one go, i only have to wait for the slowest one, not all
of them in series.
ok?
Index: if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.696
diff -u -p -r1.696 if.c
--- if.c14 May 2023 01:46:53 - 1.696
+++ if.c19 May 2023 03:50:10 -
@@ -3481,3 +3481,19 @@ net_tq(unsigned int ifindex)
return (sn->sn_taskq);
}
+
+void
+net_tq_barriers(const char *wmesg)
+{
+ struct task barriers[NET_TASKQ];
+ struct refcnt r = REFCNT_INITIALIZER();
+ int i;
+
+ for (i = 0; i < nitems(barriers); i++) {
+ task_set(&barriers[i], (void (*)(void *))refcnt_rele_wake, &r);
+ refcnt_take(&r);
+ task_add(softnets[i].sn_taskq, &barriers[i]);
+ }
+
+ refcnt_finalize(&r, wmesg);
+}
Index: if.h
===
RCS file: /cvs/src/sys/net/if.h,v
retrieving revision 1.212
diff -u -p -r1.212 if.h
--- if.h15 May 2023 16:34:56 - 1.212
+++ if.h19 May 2023 03:50:10 -
@@ -560,6 +560,7 @@ int if_congested(void);
__dead voidunhandled_af(int);
intif_setlladdr(struct ifnet *, const uint8_t *);
struct taskq * net_tq(unsigned int);
+void net_tq_barriers(const char *);
#endif /* _KERNEL */
Re: net_tq_barriers()
On Fri, May 19, 2023 at 01:56:38PM +1000, David Gwynne wrote:
> this is a tiny slice off a big pfsync diff i've been working on. when
> you bring pfsync down i need it to wait until all the work it's been
> doing in the network stack has finished, which means i need a barrier
> for all the network taskqs. that's what this implements.
>
> a barrier per taskq would mean iterating over the taskqs and waiting for
> a barrier on each one. by using a refcnt and shoving a task onto each of
> them in one go, i only have to wait for the slowest one, not all
> of them in series.
>
> ok?
Not sure if it matters here but wouldn't it be even better to insert this
barrier on the head of the task queue? At least I think you just want one
task run to be done.
Now 'ifconfig pfsync0 down' is not a hot path so it does not matter but
such barriers have the tendency to end up in unexpected places.
Running all tasks in parallel is a good compromise right now.
OK claudio@
> Index: if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.696
> diff -u -p -r1.696 if.c
> --- if.c 14 May 2023 01:46:53 - 1.696
> +++ if.c 19 May 2023 03:50:10 -
> @@ -3481,3 +3481,19 @@ net_tq(unsigned int ifindex)
>
> return (sn->sn_taskq);
> }
> +
> +void
> +net_tq_barriers(const char *wmesg)
> +{
> + struct task barriers[NET_TASKQ];
> + struct refcnt r = REFCNT_INITIALIZER();
> + int i;
> +
> + for (i = 0; i < nitems(barriers); i++) {
> + task_set(&barriers[i], (void (*)(void *))refcnt_rele_wake, &r);
> + refcnt_take(&r);
> + task_add(softnets[i].sn_taskq, &barriers[i]);
> + }
> +
> + refcnt_finalize(&r, wmesg);
> +}
> Index: if.h
> ===
> RCS file: /cvs/src/sys/net/if.h,v
> retrieving revision 1.212
> diff -u -p -r1.212 if.h
> --- if.h 15 May 2023 16:34:56 - 1.212
> +++ if.h 19 May 2023 03:50:10 -
> @@ -560,6 +560,7 @@ int if_congested(void);
> __dead void unhandled_af(int);
> int if_setlladdr(struct ifnet *, const uint8_t *);
> struct taskq * net_tq(unsigned int);
> +void net_tq_barriers(const char *);
>
> #endif /* _KERNEL */
>
>
--
:wq Claudio
ypldap: try servers until one succeeds
We sometimes run into situations where one of the three servers a ypldap
can talk to will accept a TCP connection but won't do TLS properly, or won't
perform LDAP searches. ypldap currently only tries servers until one accepts
the connection, so when this happens, it is less successful at updating than
it could be.
The diff below adjusts the ldap update code so it tries servers until it
either successfully queries one or it runs out of addresses to try.
If a server breaks after returning partial results, the ldap process will
still send what it got to the main process. If the ldap process then gets
full results from another server, those will overwrite the partial results,
and if it doesn't, the main process will discard the partial results when it
gets a 'trash update' message from the ldap process.
While here, the diff also adds the server address to log messages about
servers not working, so it's easier to figure out what's going wrong.
ok?
Index: ldapclient.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ldapclient.c,v
retrieving revision 1.46
diff -u -p -r1.46 ldapclient.c
--- ldapclient.c13 Oct 2022 04:55:33 - 1.46
+++ ldapclient.c3 Feb 2023 03:58:17 -
@@ -53,50 +53,10 @@ int client_build_req(struct idm *, struc
int, int);
intclient_search_idm(struct env *, struct idm *, struct aldap *,
char **, char *, int, int, enum imsg_type);
-intclient_try_idm(struct env *, struct idm *);
+intclient_try_idm(struct env *, struct idm *, struct ypldap_addr *);
void client_addr_init(struct idm *);
intclient_addr_free(struct idm *);
-struct aldap *client_aldap_open(struct ypldap_addr_list *);
-
-/*
- * dummy wrapper to provide aldap_init with its fd's.
- */
-struct aldap *
-client_aldap_open(struct ypldap_addr_list *addr)
-{
- int fd = -1;
- struct ypldap_addr *p;
- struct aldap*al;
-
- TAILQ_FOREACH(p, addr, next) {
- char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV];
- struct sockaddr *sa = (struct sockaddr *)&p->ss;
-
- if (getnameinfo(sa, SA_LEN(sa), hbuf, sizeof(hbuf), sbuf,
- sizeof(sbuf), NI_NUMERICHOST | NI_NUMERICSERV))
- errx(1, "could not get numeric hostname");
-
- if ((fd = socket(sa->sa_family, SOCK_STREAM, 0)) == -1)
- return NULL;
-
- if (connect(fd, sa, SA_LEN(sa)) == 0)
- break;
-
- log_warn("connect to %s port %s failed", hbuf, sbuf);
- close(fd);
- fd = -1;
- }
-
- if (fd == -1)
- return NULL;
-
- al = aldap_init(fd);
- if (al == NULL)
- close(fd);
- return al;
-}
-
void
client_addr_init(struct idm *idm)
{
@@ -241,8 +201,12 @@ client_dispatch_dns(int fd, short events
}
TAILQ_FOREACH(idm, &env->sc_idms, idm_entry) {
- if (client_try_idm(env, idm) == -1)
- idm->idm_state = STATE_LDAP_FAIL;
+ TAILQ_FOREACH(h, &idm->idm_addr, next) {
+ if (client_try_idm(env, idm, h) == -1)
+ idm->idm_state = STATE_LDAP_FAIL;
+ else
+ break;
+ }
if (idm->idm_state < STATE_LDAP_DONE)
wait_cnt++;
@@ -585,17 +549,36 @@ fail:
}
int
-client_try_idm(struct env *env, struct idm *idm)
+client_try_idm(struct env *env, struct idm *idm, struct ypldap_addr *addr)
{
const char *where;
+ char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV];
char*attrs[ATTR_MAX+1];
+ int fd = -1;
int i, j;
+ struct sockaddr *sa = (struct sockaddr *)&addr->ss;
struct aldap_message*m;
struct aldap*al;
+ if (getnameinfo(sa, SA_LEN(sa), hbuf, sizeof(hbuf), sbuf,
+ sizeof(sbuf), NI_NUMERICHOST | NI_NUMERICSERV))
+ errx(1, "could not get numeric hostname");
+
where = "connect";
- if ((al = client_aldap_open(&idm->idm_addr)) == NULL)
+ if ((fd = socket(sa->sa_family, SOCK_STREAM, 0)) == -1)
+ return (-1);
+
+ if (connect(fd, sa, SA_LEN(sa)) != 0) {
+ log_warn("connect to %s port %s failed", hbuf, sbuf);
+ close(fd);
+ return (-1);
+ }
+
+ al = aldap_init(fd);
+ if (al == NULL) {
+ close(fd);
return (-1);
+ }
if (idm->idm_flags & F_STARTTLS) {
log_debug("requesting starttls");
@@ -625,8 +608,8 @@ client_try_idm(struct env *env, struct i
if (aldap_tls(al, idm->idm_tls_config, idm->i
Re: smtpd: some fatal -> fatalx
On Tue, May 16, 2023 at 02:51:44PM +0200, Omar Polo wrote: > while debugging a pebkac in -portable, I noticed that in various > places we use fatal() for libtls failures. errno doesn't generally > contains anything useful after libtls functions, and in most it's > explicitly cleared to avoid misuse. > > just to provide a quick example, with `listen on ... ciphers foobar': > > % doas smtpd -d > info: OpenSMTPD 7.0.0 starting > dispatcher: no ciphers for 'foobar': No such file or directory > smtpd: process dispatcher socket closed > > So change most of them to fatalx which doesn't append errno. While > here I'm also logging the actual error, via tls_config_error() or > tls_error(), that before was missing. > > tls_config_new(), tls_server() and tls_client() failures are still > logged with fatal(), which I believe it's correct. > > ok? > make sense, ok giovanni@ Cheers Giovanni
