Re: rpki-client and non-existing files
Claudio Jeker wrote: > On Wed, Apr 01, 2020 at 01:06:21PM +0200, Claudio Jeker wrote: > > Currently rpki-client logs missing files like this: > > > > rpki-client: ...trace: error:02FFF002:system library:func(4095):No such > > file or directory > > rpki-client: ...trace: error:20FFF080:BIO routines:CRYPTO_internal:no such > > file > > rpki-client: > > rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: > > BIO_new_file > > > > Yes, you need to read the errors in reverse and even then the errors are > > just hard to read. > > > > This ugly format is mostly to blame on the error stack of OpenSSL. > > As a workaround I switched to using fopen() and then BIO_new_fd() > > which does the same thing but allows me to get a nice error from fopen(): > > > > rpki-client: > > rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: fopen: > > No such file or directory > > > > Any opinions? > > This diff removes the fopen: from the warn string: > > rpki-client: > rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: No such > file or directory > > This is more in form with e.g. > > rpki-client: > rpki-repo.registro.br/repo/D81aiXpDAv5WBmgE8oEpfordjGP62otn2fHrhaL4cgby/0/3137372e3133302e302e302f32302d3234203d3e203238323630.roa: > CRL has expired thank you, it was driving me crazy.
Re: rpki-client and non-existing files
On Wed, Apr 01, 2020 at 09:42:42PM +0200, Sebastian Benoit wrote:
> ok
>
> you remove the "if (verbose > 0)" in the cms_parse_validate() case on
> purpose?
Yes, since we use rpki-client in cron with the magic -n prefix it would be
nice to have enough verbosity to know why the process failed without
having to run rpki-client -v. So I kind of walked back from the
rpki-client must be silent by default unless a bad error happens case.
> Claudio Jeker(cje...@diehard.n-r-g.com) on 2020.04.01 16:33:44 +0200:
> > On Wed, Apr 01, 2020 at 01:06:21PM +0200, Claudio Jeker wrote:
> > > Currently rpki-client logs missing files like this:
> > >
> > > rpki-client: ...trace: error:02FFF002:system library:func(4095):No such
> > > file or directory
> > > rpki-client: ...trace: error:20FFF080:BIO routines:CRYPTO_internal:no
> > > such file
> > > rpki-client:
> > > rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft:
> > > BIO_new_file
> > >
> > > Yes, you need to read the errors in reverse and even then the errors are
> > > just hard to read.
> > >
> > > This ugly format is mostly to blame on the error stack of OpenSSL.
> > > As a workaround I switched to using fopen() and then BIO_new_fd()
> > > which does the same thing but allows me to get a nice error from fopen():
> > >
> > > rpki-client:
> > > rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft:
> > > fopen: No such file or directory
> > >
> > > Any opinions?
> >
> > This diff removes the fopen: from the warn string:
> >
> > rpki-client:
> > rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: No
> > such file or directory
> >
> > This is more in form with e.g.
> >
> > rpki-client:
> > rpki-repo.registro.br/repo/D81aiXpDAv5WBmgE8oEpfordjGP62otn2fHrhaL4cgby/0/3137372e3133302e302e302f32302d3234203d3e203238323630.roa:
> > CRL has expired
> >
> > --
> > :wq Claudio
> >
> > Index: cert.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> > retrieving revision 1.14
> > diff -u -p -r1.14 cert.c
> > --- cert.c 26 Feb 2020 02:35:08 - 1.14
> > +++ cert.c 1 Apr 2020 14:28:29 -
> > @@ -930,12 +930,18 @@ cert_parse_inner(X509 **xp, const char *
> > ASN1_OBJECT *obj;
> > struct parse p;
> > BIO *bio = NULL, *shamd;
> > + FILE*f;
> > EVP_MD *md;
> > char mdbuf[EVP_MAX_MD_SIZE];
> >
> > *xp = NULL;
> >
> > - if ((bio = BIO_new_file(fn, "rb")) == NULL) {
> > + if ((f = fopen(fn, "rb")) == NULL) {
> > + warn("%s", fn);
> > + return NULL;
> > + }
> > +
> > + if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
> > if (verbose > 0)
> > cryptowarnx("%s: BIO_new_file", fn);
> > return NULL;
> > Index: cms.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
> > retrieving revision 1.6
> > diff -u -p -r1.6 cms.c
> > --- cms.c 29 Nov 2019 05:14:11 - 1.6
> > +++ cms.c 1 Apr 2020 14:28:34 -
> > @@ -42,6 +42,7 @@ cms_parse_validate(X509 **xp, const char
> > ASN1_OCTET_STRING **os = NULL;
> > BIO *bio = NULL, *shamd;
> > CMS_ContentInfo *cms;
> > + FILE*f;
> > char buf[128], mdbuf[EVP_MAX_MD_SIZE];
> > int rc = 0, sz;
> > STACK_OF(X509) *certs = NULL;
> > @@ -55,10 +56,13 @@ cms_parse_validate(X509 **xp, const char
> > * This is usually fopen() failure, so let it pass through to
> > * the handler, which will in turn ignore the entity.
> > */
> > + if ((f = fopen(fn, "rb")) == NULL) {
> > + warn("%s", fn);
> > + return NULL;
> > + }
> >
> > - if ((bio = BIO_new_file(fn, "rb")) == NULL) {
> > - if (verbose > 0)
> > - cryptowarnx("%s: BIO_new_file", fn);
> > + if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
> > + cryptowarnx("%s: BIO_new_fp", fn);
> > return NULL;
> > }
> >
> > Index: crl.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
> > retrieving revision 1.7
> > diff -u -p -r1.7 crl.c
> > --- crl.c 29 Nov 2019 04:40:04 - 1.7
> > +++ crl.c 1 Apr 2020 14:28:41 -
> > @@ -36,10 +36,16 @@ crl_parse(const char *fn, const unsigned
> > int rc = 0, sz;
> > X509_CRL*x = NULL;
> > BIO *bio = NULL, *shamd;
> > + FILE*f;
> > EVP_MD *md;
> > char mdbuf[EVP_MAX_MD_SIZE];
> >
> > - if ((bio = BIO_new_file(fn, "rb")) == NULL) {
> > + if ((f = fopen(fn, "rb")) == NULL) {
> > + warn("%s", fn);
> > + return NULL;
> > + }
> > +
> > + if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
Re: rpki-client and non-existing files
ok
you remove the "if (verbose > 0)" in the cms_parse_validate() case on
purpose?
Claudio Jeker(cje...@diehard.n-r-g.com) on 2020.04.01 16:33:44 +0200:
> On Wed, Apr 01, 2020 at 01:06:21PM +0200, Claudio Jeker wrote:
> > Currently rpki-client logs missing files like this:
> >
> > rpki-client: ...trace: error:02FFF002:system library:func(4095):No such
> > file or directory
> > rpki-client: ...trace: error:20FFF080:BIO routines:CRYPTO_internal:no such
> > file
> > rpki-client:
> > rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft:
> > BIO_new_file
> >
> > Yes, you need to read the errors in reverse and even then the errors are
> > just hard to read.
> >
> > This ugly format is mostly to blame on the error stack of OpenSSL.
> > As a workaround I switched to using fopen() and then BIO_new_fd()
> > which does the same thing but allows me to get a nice error from fopen():
> >
> > rpki-client:
> > rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: fopen:
> > No such file or directory
> >
> > Any opinions?
>
> This diff removes the fopen: from the warn string:
>
> rpki-client:
> rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: No such
> file or directory
>
> This is more in form with e.g.
>
> rpki-client:
> rpki-repo.registro.br/repo/D81aiXpDAv5WBmgE8oEpfordjGP62otn2fHrhaL4cgby/0/3137372e3133302e302e302f32302d3234203d3e203238323630.roa:
> CRL has expired
>
> --
> :wq Claudio
>
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 cert.c
> --- cert.c26 Feb 2020 02:35:08 - 1.14
> +++ cert.c1 Apr 2020 14:28:29 -
> @@ -930,12 +930,18 @@ cert_parse_inner(X509 **xp, const char *
> ASN1_OBJECT *obj;
> struct parse p;
> BIO *bio = NULL, *shamd;
> + FILE*f;
> EVP_MD *md;
> char mdbuf[EVP_MAX_MD_SIZE];
>
> *xp = NULL;
>
> - if ((bio = BIO_new_file(fn, "rb")) == NULL) {
> + if ((f = fopen(fn, "rb")) == NULL) {
> + warn("%s", fn);
> + return NULL;
> + }
> +
> + if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
> if (verbose > 0)
> cryptowarnx("%s: BIO_new_file", fn);
> return NULL;
> Index: cms.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 cms.c
> --- cms.c 29 Nov 2019 05:14:11 - 1.6
> +++ cms.c 1 Apr 2020 14:28:34 -
> @@ -42,6 +42,7 @@ cms_parse_validate(X509 **xp, const char
> ASN1_OCTET_STRING **os = NULL;
> BIO *bio = NULL, *shamd;
> CMS_ContentInfo *cms;
> + FILE*f;
> char buf[128], mdbuf[EVP_MAX_MD_SIZE];
> int rc = 0, sz;
> STACK_OF(X509) *certs = NULL;
> @@ -55,10 +56,13 @@ cms_parse_validate(X509 **xp, const char
>* This is usually fopen() failure, so let it pass through to
>* the handler, which will in turn ignore the entity.
>*/
> + if ((f = fopen(fn, "rb")) == NULL) {
> + warn("%s", fn);
> + return NULL;
> + }
>
> - if ((bio = BIO_new_file(fn, "rb")) == NULL) {
> - if (verbose > 0)
> - cryptowarnx("%s: BIO_new_file", fn);
> + if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
> + cryptowarnx("%s: BIO_new_fp", fn);
> return NULL;
> }
>
> Index: crl.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 crl.c
> --- crl.c 29 Nov 2019 04:40:04 - 1.7
> +++ crl.c 1 Apr 2020 14:28:41 -
> @@ -36,10 +36,16 @@ crl_parse(const char *fn, const unsigned
> int rc = 0, sz;
> X509_CRL*x = NULL;
> BIO *bio = NULL, *shamd;
> + FILE*f;
> EVP_MD *md;
> char mdbuf[EVP_MAX_MD_SIZE];
>
> - if ((bio = BIO_new_file(fn, "rb")) == NULL) {
> + if ((f = fopen(fn, "rb")) == NULL) {
> + warn("%s", fn);
> + return NULL;
> + }
> +
> + if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
> if (verbose > 0)
> cryptowarnx("%s: BIO_new_file", fn);
> return NULL;
>
ospf6d: update to connected routes
Handle connected routes as ospfd(8) does.
(diff to ospf6d and ospf6ctl)
Index: ospf6ctl/ospf6ctl.c
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/ospf6ctl.c,v
retrieving revision 1.50
diff -u -p -r1.50 ospf6ctl.c
--- ospf6ctl/ospf6ctl.c 26 May 2019 09:27:09 - 1.50
+++ ospf6ctl/ospf6ctl.c 1 Apr 2020 18:16:12 -
@@ -1103,10 +1103,10 @@ show_rib_msg(struct imsg *imsg)
errx(1, "Invalid route type");
}
- printf("%-20s %-17s %-12s %-9s %-7d %s\n", dstnet,
+ printf("%-20s %-16s%s %-12s %-9s %-7d %s\n", dstnet,
log_in6addr_scope(>nexthop, rt->ifindex),
- path_type_name(rt->p_type), dst_type_name(rt->d_type),
- rt->cost,
+ rt->connected ? "C" : " ", path_type_name(rt->p_type),
+ dst_type_name(rt->d_type), rt->cost,
rt->uptime == 0 ? "-" : fmt_timeframe_core(rt->uptime));
free(dstnet);
break;
Index: ospf6d/ospf6d.h
===
RCS file: /cvs/src/usr.sbin/ospf6d/ospf6d.h,v
retrieving revision 1.45
diff -u -p -r1.45 ospf6d.h
--- ospf6d/ospf6d.h 21 Jan 2020 20:38:52 - 1.45
+++ ospf6d/ospf6d.h 1 Apr 2020 18:16:12 -
@@ -483,6 +483,7 @@ struct ctl_rt {
enum dst_typed_type;
u_int8_t flags;
u_int8_t prefixlen;
+ u_int8_t connected;
};
struct ctl_sum {
Index: ospf6d/rde.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/rde.c,v
retrieving revision 1.85
diff -u -p -r1.85 rde.c
--- ospf6d/rde.c29 Mar 2020 11:59:11 - 1.85
+++ ospf6d/rde.c1 Apr 2020 18:16:12 -
@@ -886,6 +886,9 @@ rde_send_change_kroute(struct rt_node *r
TAILQ_FOREACH(rn, >nexthop, entry) {
if (rn->invalid)
continue;
+ if (rn->connected)
+ /* skip self-originated routes */
+ continue;
krcount++;
bzero(, sizeof(kr));
@@ -899,8 +902,12 @@ rde_send_change_kroute(struct rt_node *r
kr.ext_tag = r->ext_tag;
imsg_add(wbuf, , sizeof(kr));
}
- if (krcount == 0)
- fatalx("rde_send_change_kroute: no valid nexthop found");
+ if (krcount == 0) {
+ /* no valid nexthop or self originated, so remove */
+ ibuf_free(wbuf);
+ rde_send_delete_kroute(r);
+ return;
+ }
imsg_close(_main->ibuf, wbuf);
imsg_event_add(iev_main);
Index: ospf6d/rde_spf.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/rde_spf.c,v
retrieving revision 1.27
diff -u -p -r1.27 rde_spf.c
--- ospf6d/rde_spf.c29 Mar 2020 11:59:11 - 1.27
+++ ospf6d/rde_spf.c1 Apr 2020 18:16:12 -
@@ -897,7 +897,9 @@ rt_nexthop_add(struct rt_node *r, struct
rn->ifindex = vn->ifindex;
rn->adv_rtr.s_addr = adv_rtr.s_addr;
rn->uptime = now.tv_sec;
- rn->connected = vn->prev == spf_root;
+ rn->connected = (type == LSA_TYPE_NETWORK &&
+ vn->prev == spf_root) ||
+ (IN6_IS_ADDR_UNSPECIFIED(>nexthop));
rn->invalid = 0;
r->invalid = 0;
@@ -952,21 +954,24 @@ rt_dump(struct in_addr area, pid_t pid,
fatalx("rt_dump: invalid RIB type");
}
+ memset(, 0, sizeof(rtctl));
+ rtctl.prefix = r->prefix;
+ rtctl.area.s_addr = r->area.s_addr;
+ rtctl.cost = r->cost;
+ rtctl.cost2 = r->cost2;
+ rtctl.p_type = r->p_type;
+ rtctl.d_type = r->d_type;
+ rtctl.flags = r->flags;
+ rtctl.prefixlen = r->prefixlen;
+
TAILQ_FOREACH(rn, >nexthop, entry) {
if (rn->invalid)
continue;
- rtctl.prefix = r->prefix;
+ rtctl.connected = rn->connected;
rtctl.nexthop = rn->nexthop;
rtctl.ifindex = rn->ifindex;
- rtctl.area.s_addr = r->area.s_addr;
rtctl.adv_rtr.s_addr = rn->adv_rtr.s_addr;
- rtctl.cost = r->cost;
- rtctl.cost2 = r->cost2;
- rtctl.p_type = r->p_type;
- rtctl.d_type = r->d_type;
- rtctl.flags = r->flags;
- rtctl.prefixlen = r->prefixlen;
rtctl.uptime = now.tv_sec - rn->uptime;
libossaudio: start using sndio
ping!
FWIW, the diff below affects the following ports:
- emulators/gambatte
- gstreamer-0.10 mixer plugin
- sysutils/conky
- sysutils/gkrellm
- sysutil/tpb
- telephony/iaxclient
- anything depending on gstreamer, ex xfce4-mixer
If you use one of above ports and want to test, just apply this diff
to src/lib/libossaudio in base, rebuild it and reinstall it. The ABI
is not changing, so no need to rebuild or update ports.
- Forwarded message from Alexandre Ratchov -
Date: Tue, 17 Mar 2020 12:27:40 +0100
From: Alexandre Ratchov
To: po...@openbsd.org
Subject: libossaudio: important diff to test
User-Agent: Mutt/1.11.4 (2019-03-13)
Hi,
This diff makes libossaudio use sndio instead of the kernel mixer(4)
interface. I post it to ports@ as it's only used by few ports.
Switching libossaudio to sndio implies that programs using it will
always use the right device (instead of the first one) and will always
see at least the sndiod master level control.
With this diff, libossaudio will expose the following controls:
- output.level, sndiod master level, always present
- hw/output.level, hardware output level, present on certain devices only
- hw/input.level, hardware input level, present on certain devices only
OK?
-- Alexandre
Index: lib/libossaudio/Makefile
===
RCS file: /cvs/src/lib/libossaudio/Makefile,v
retrieving revision 1.5
diff -u -p -u -p -r1.5 Makefile
--- lib/libossaudio/Makefile16 Jul 2014 20:02:17 - 1.5
+++ lib/libossaudio/Makefile22 Feb 2020 16:07:05 -
@@ -4,9 +4,11 @@
LIB= ossaudio
MAN= ossaudio.3
-SRCS= ossaudio.c
+SRCS= ossaudio.c aucat.c debug.c sioctl.c sioctl_aucat.c sioctl_sun.c
CPPFLAGS+= -I${.CURDIR}
+
+.PATH: ${.CURDIR}/../libsndio
includes:
@cd ${.CURDIR}; cmp -s soundcard.h ${DESTDIR}/usr/include/soundcard.h
|| \
Index: lib/libossaudio/ossaudio.c
===
RCS file: /cvs/src/lib/libossaudio/ossaudio.c,v
retrieving revision 1.20
diff -u -p -u -p -r1.20 ossaudio.c
--- lib/libossaudio/ossaudio.c 28 Jun 2019 13:32:42 - 1.20
+++ lib/libossaudio/ossaudio.c 22 Feb 2020 16:07:05 -
@@ -36,26 +36,38 @@
#include
#include
#include
-#include
-#include
#include
-
+#include
+#include
+#include
+#include
#include "soundcard.h"
-#undef ioctl
-#define GET_DEV(com) ((com) & 0xff)
+#ifdef DEBUG
+#define DPRINTF(...) do { fprintf(stderr, __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(...) do {} while (0)
+#endif
-#define TO_OSSVOL(x) (((x) * 100 + 127) / 255)
-#define FROM_OSSVOL(x) x) > 100 ? 100 : (x)) * 255 + 50) / 100)
+#define GET_DEV(com) ((com) & 0xff)
+#define INTARG (*(int*)argp)
-static struct audiodevinfo *getdevinfo(int);
+struct control {
+ struct control *next;
+ int type; /* one of SOUND_MIXER_xxx */
+ int chan; /* 0 -> left, 1 -> right, -1 -> mono */
+ int addr; /* sioctl control id */
+ int value; /* current value */
+ int max;
+};
static int mixer_ioctl(int, unsigned long, void *);
-static int opaque_to_enum(struct audiodevinfo *di, audio_mixer_name_t *label,
int opq);
-static int enum_to_ord(struct audiodevinfo *di, int enm);
-static int enum_to_mask(struct audiodevinfo *di, int enm);
-#define INTARG (*(int*)argp)
+static int initialized;
+static struct control *controls;
+static struct sioctl_hdl *hdl;
+static char *dev_name = SIO_DEVANY;
+static struct pollfd *pfds;
int
_oss_ioctl(int fd, unsigned long com, ...)
@@ -71,201 +83,163 @@ _oss_ioctl(int fd, unsigned long com, ..
else if (IOCGROUP(com) == 'M')
return mixer_ioctl(fd, com, argp);
else
- return ioctl(fd, com, argp);
+ return (ioctl)(fd, com, argp);
}
-/* If the mixer device should have more than MAX_MIXER_DEVS devices
- * some will not be available to Linux */
-#define MAX_MIXER_DEVS 64
-struct audiodevinfo {
- int done;
- dev_t dev;
- ino_t ino;
- int16_t devmap[SOUND_MIXER_NRDEVICES],
- rdevmap[MAX_MIXER_DEVS];
- char names[MAX_MIXER_DEVS][MAX_AUDIO_DEV_LEN];
- int enum2opaque[MAX_MIXER_DEVS];
-u_long devmask, recmask, stereomask;
- u_long caps, recsource;
-};
-
-static int
-opaque_to_enum(struct audiodevinfo *di, audio_mixer_name_t *label, int opq)
+/*
+ * new control
+ */
+static void
+mixer_ondesc(void *unused, struct sioctl_desc *d, int val)
{
- int i, o;
+ struct control *i, **pi;
+ int type;
- for (i = 0; i < MAX_MIXER_DEVS; i++) {
- o = di->enum2opaque[i];
- if (o == opq)
- break;
- if (o == -1 && label != NULL &&
- !strncmp(di->names[i], label->name, sizeof di->names[i])) {
-
Re: pselect(2) bug or feature?
The select(2) family of syscalls was designed to work this way (block when
file descriptors are not available, even when they cannot become available).
For example:
select(0, NULL, NULL, NULL, NULL);
blocks forever.
In fact in the very old days (before usleep, nanosleep, clock_nanosleep),
select(2) was the only way to get sub-second sleeps from the kernel, and
the idiom was similar to the above:
void
usleep(int usec) {
struct timeval tv;
tv.tv_sec = 0;
tv.tv_usec = usec;
select(0, NULL, NULL, NULL, );
}
Best,
christos
> On Apr 1, 2020, at 8:19 AM, Theo de Raadt wrote:
>
> The regress test is wrong.
>
> The first parameter to pselect has to tell it which bits to look at.
> It should be something like getdtablesize(), or fd+1, or FD_SETSIZE.
>
> And then there's all the handwaving about fd_set overflow, meaning what
> if fd >= FD_SETSIZE. I've never heard of an exploitable one though.
>
> Martin Pieuchot wrote:
>
>> While working on moving pselect(2) outside of the KERNEL_LOCK() I found
>> this surprising {p,}select(2) behavior exposed by the libc/sys/t_select.c
>> regression test. Should it be considered as a bug or feature?
>>
>> The above mentioned test does the following (simplified):
>>
>> fd = open("/dev/null", O_RDONLY);
>> FD_ZERO();
>> FD_SET(fd, );
>> pselect(1, , NULL, NULL, NULL, );
>>
>> Note that in most environments, open(2) will return a value higher than
>> 1 which means the value written by FD_SET() will never be checked by the
>> current implementation of {p,}select(2). In this case, even if it isn't
>> waiting for any condition, the current implementation will block.
>>
>> To put it differently, the code above is an obfuscated way to write:
>>
>> sigsuspend();
>>
>> If we agree that this is not what the user wants to do, I'm suggesting
>> the diff below which makes {p,}select(2) return EINVAL if no bit has
>> been found in the given set up to `nfds'.
>>
>> Independently from our decision, I'd suggest to rename the variable
>> name `nfds' in the syscall documentation because it can lead to this
>> kind of confusion.
>>
>> Finally if we accept this change the regression test will have to be
>> adapted because polling "/dev/null" for reading always return true so
>> pselect(2) wont block waiting for a signal. Any suggestion?
>>
>> Index: sys/kern/sys_generic.c
>> ===
>> RCS file: /cvs/src/sys/kern/sys_generic.c,v
>> retrieving revision 1.131
>> diff -u -p -r1.131 sys_generic.c
>> --- sys/kern/sys_generic.c 20 Mar 2020 04:11:05 - 1.131
>> +++ sys/kern/sys_generic.c 1 Apr 2020 10:27:51 -
>> @@ -703,7 +703,7 @@ selscan(struct proc *p, fd_set *ibits, f
>> {
>> caddr_t cibits = (caddr_t)ibits, cobits = (caddr_t)obits;
>> struct filedesc *fdp = p->p_fd;
>> -int msk, i, j, fd;
>> +int msk, i, j, fd, foundfds = 0;
>> fd_mask bits;
>> struct file *fp;
>> int n = 0;
>> @@ -719,6 +719,7 @@ selscan(struct proc *p, fd_set *ibits, f
>> bits &= ~(1 << j);
>> if ((fp = fd_getfile(fdp, fd)) == NULL)
>> return (EBADF);
>> +foundfds++;
>> if ((*fp->f_ops->fo_poll)(fp, flag[msk], p)) {
>> FD_SET(fd, pobits);
>> n++;
>> @@ -727,6 +728,8 @@ selscan(struct proc *p, fd_set *ibits, f
>> }
>> }
>> }
>> +if (foundfds == 0)
>> +return (EINVAL);
>> *retval = n;
>> return (0);
>> }
>> Index: lib/libc/sys/select.2
>> ===
>> RCS file: /cvs/src/lib/libc/sys/select.2,v
>> retrieving revision 1.42
>> diff -u -p -r1.42 select.2
>> --- lib/libc/sys/select.217 Sep 2016 01:01:42 - 1.42
>> +++ lib/libc/sys/select.21 Apr 2020 10:35:29 -
>> @@ -188,7 +188,7 @@ The specified time limit is invalid.
>> One of its components is negative or too large.
>> .It Bq Er EINVAL
>> .Fa nfds
>> -was less than 0.
>> +was smaller than the smallest descriptor specified in the sets.
>> .El
>> .Sh SEE ALSO
>> .Xr accept 2 ,
>>
signature.asc
Description: Message signed with OpenPGP
Re: pselect(2) bug or feature?
On Wed, 01 Apr 2020 13:53:53 +0200, Claudio Jeker wrote: > I think this is actually a feature. The standard does not mention that you > have to listen on something and it would allow to use select() as a > sleep() replacement. I would expect that pselect() should behave the same > way. I agree that this is a feature. POSIX also specifies EINVAL if nfds is larger than FD_SETSIZE but we support arrays of fd_set larger than FD_SETSIZE so that is not an error in our implementation. - todd
Re: rpki-client and non-existing files
On Wed, Apr 01, 2020 at 01:06:21PM +0200, Claudio Jeker wrote:
> Currently rpki-client logs missing files like this:
>
> rpki-client: ...trace: error:02FFF002:system library:func(4095):No such file
> or directory
> rpki-client: ...trace: error:20FFF080:BIO routines:CRYPTO_internal:no such
> file
> rpki-client:
> rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft:
> BIO_new_file
>
> Yes, you need to read the errors in reverse and even then the errors are
> just hard to read.
>
> This ugly format is mostly to blame on the error stack of OpenSSL.
> As a workaround I switched to using fopen() and then BIO_new_fd()
> which does the same thing but allows me to get a nice error from fopen():
>
> rpki-client:
> rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: fopen:
> No such file or directory
>
> Any opinions?
This diff removes the fopen: from the warn string:
rpki-client:
rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: No such
file or directory
This is more in form with e.g.
rpki-client:
rpki-repo.registro.br/repo/D81aiXpDAv5WBmgE8oEpfordjGP62otn2fHrhaL4cgby/0/3137372e3133302e302e302f32302d3234203d3e203238323630.roa:
CRL has expired
--
:wq Claudio
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.14
diff -u -p -r1.14 cert.c
--- cert.c 26 Feb 2020 02:35:08 - 1.14
+++ cert.c 1 Apr 2020 14:28:29 -
@@ -930,12 +930,18 @@ cert_parse_inner(X509 **xp, const char *
ASN1_OBJECT *obj;
struct parse p;
BIO *bio = NULL, *shamd;
+ FILE*f;
EVP_MD *md;
char mdbuf[EVP_MAX_MD_SIZE];
*xp = NULL;
- if ((bio = BIO_new_file(fn, "rb")) == NULL) {
+ if ((f = fopen(fn, "rb")) == NULL) {
+ warn("%s", fn);
+ return NULL;
+ }
+
+ if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
if (verbose > 0)
cryptowarnx("%s: BIO_new_file", fn);
return NULL;
Index: cms.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
retrieving revision 1.6
diff -u -p -r1.6 cms.c
--- cms.c 29 Nov 2019 05:14:11 - 1.6
+++ cms.c 1 Apr 2020 14:28:34 -
@@ -42,6 +42,7 @@ cms_parse_validate(X509 **xp, const char
ASN1_OCTET_STRING **os = NULL;
BIO *bio = NULL, *shamd;
CMS_ContentInfo *cms;
+ FILE*f;
char buf[128], mdbuf[EVP_MAX_MD_SIZE];
int rc = 0, sz;
STACK_OF(X509) *certs = NULL;
@@ -55,10 +56,13 @@ cms_parse_validate(X509 **xp, const char
* This is usually fopen() failure, so let it pass through to
* the handler, which will in turn ignore the entity.
*/
+ if ((f = fopen(fn, "rb")) == NULL) {
+ warn("%s", fn);
+ return NULL;
+ }
- if ((bio = BIO_new_file(fn, "rb")) == NULL) {
- if (verbose > 0)
- cryptowarnx("%s: BIO_new_file", fn);
+ if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
+ cryptowarnx("%s: BIO_new_fp", fn);
return NULL;
}
Index: crl.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
retrieving revision 1.7
diff -u -p -r1.7 crl.c
--- crl.c 29 Nov 2019 04:40:04 - 1.7
+++ crl.c 1 Apr 2020 14:28:41 -
@@ -36,10 +36,16 @@ crl_parse(const char *fn, const unsigned
int rc = 0, sz;
X509_CRL*x = NULL;
BIO *bio = NULL, *shamd;
+ FILE*f;
EVP_MD *md;
char mdbuf[EVP_MAX_MD_SIZE];
- if ((bio = BIO_new_file(fn, "rb")) == NULL) {
+ if ((f = fopen(fn, "rb")) == NULL) {
+ warn("%s", fn);
+ return NULL;
+ }
+
+ if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
if (verbose > 0)
cryptowarnx("%s: BIO_new_file", fn);
return NULL;
Re: pselect(2) bug or feature?
The regress test is wrong.
The first parameter to pselect has to tell it which bits to look at.
It should be something like getdtablesize(), or fd+1, or FD_SETSIZE.
And then there's all the handwaving about fd_set overflow, meaning what
if fd >= FD_SETSIZE. I've never heard of an exploitable one though.
Martin Pieuchot wrote:
> While working on moving pselect(2) outside of the KERNEL_LOCK() I found
> this surprising {p,}select(2) behavior exposed by the libc/sys/t_select.c
> regression test. Should it be considered as a bug or feature?
>
> The above mentioned test does the following (simplified):
>
> fd = open("/dev/null", O_RDONLY);
> FD_ZERO();
> FD_SET(fd, );
> pselect(1, , NULL, NULL, NULL, );
>
> Note that in most environments, open(2) will return a value higher than
> 1 which means the value written by FD_SET() will never be checked by the
> current implementation of {p,}select(2). In this case, even if it isn't
> waiting for any condition, the current implementation will block.
>
> To put it differently, the code above is an obfuscated way to write:
>
> sigsuspend();
>
> If we agree that this is not what the user wants to do, I'm suggesting
> the diff below which makes {p,}select(2) return EINVAL if no bit has
> been found in the given set up to `nfds'.
>
> Independently from our decision, I'd suggest to rename the variable
> name `nfds' in the syscall documentation because it can lead to this
> kind of confusion.
>
> Finally if we accept this change the regression test will have to be
> adapted because polling "/dev/null" for reading always return true so
> pselect(2) wont block waiting for a signal. Any suggestion?
>
> Index: sys/kern/sys_generic.c
> ===
> RCS file: /cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.131
> diff -u -p -r1.131 sys_generic.c
> --- sys/kern/sys_generic.c20 Mar 2020 04:11:05 - 1.131
> +++ sys/kern/sys_generic.c1 Apr 2020 10:27:51 -
> @@ -703,7 +703,7 @@ selscan(struct proc *p, fd_set *ibits, f
> {
> caddr_t cibits = (caddr_t)ibits, cobits = (caddr_t)obits;
> struct filedesc *fdp = p->p_fd;
> - int msk, i, j, fd;
> + int msk, i, j, fd, foundfds = 0;
> fd_mask bits;
> struct file *fp;
> int n = 0;
> @@ -719,6 +719,7 @@ selscan(struct proc *p, fd_set *ibits, f
> bits &= ~(1 << j);
> if ((fp = fd_getfile(fdp, fd)) == NULL)
> return (EBADF);
> + foundfds++;
> if ((*fp->f_ops->fo_poll)(fp, flag[msk], p)) {
> FD_SET(fd, pobits);
> n++;
> @@ -727,6 +728,8 @@ selscan(struct proc *p, fd_set *ibits, f
> }
> }
> }
> + if (foundfds == 0)
> + return (EINVAL);
> *retval = n;
> return (0);
> }
> Index: lib/libc/sys/select.2
> ===
> RCS file: /cvs/src/lib/libc/sys/select.2,v
> retrieving revision 1.42
> diff -u -p -r1.42 select.2
> --- lib/libc/sys/select.2 17 Sep 2016 01:01:42 - 1.42
> +++ lib/libc/sys/select.2 1 Apr 2020 10:35:29 -
> @@ -188,7 +188,7 @@ The specified time limit is invalid.
> One of its components is negative or too large.
> .It Bq Er EINVAL
> .Fa nfds
> -was less than 0.
> +was smaller than the smallest descriptor specified in the sets.
> .El
> .Sh SEE ALSO
> .Xr accept 2 ,
>
Re: [PATCH] dired-jump for mg
George Koehler writes:
> C-x C-j doesn't work for me: it does show the dired buffer, but
> doesn't jump to the file that I was editing.
Looks like that was because I called gotofile before showbuffer. I
changed that before submitting it, and I remember it seeming to work,
but I guess I was mistaken :/ This patch should fix that though.
--
Philip K.
? dired-jump.patch
cvs server: Diffing .
Index: def.h
===
RCS file: /cvs/src/usr.bin/mg/def.h,v
retrieving revision 1.166
diff -u -p -r1.166 def.h
--- def.h 9 Feb 2020 10:13:13 - 1.166
+++ def.h 1 Apr 2020 11:42:45 -
@@ -361,6 +361,7 @@ int ask_makedir(void);
/* dired.c */
struct buffer *dired_(char *);
+int dired_jump(int, int);
int do_dired(char *);
/* file.c X */
Index: dired.c
===
RCS file: /cvs/src/usr.bin/mg/dired.c,v
retrieving revision 1.93
diff -u -p -r1.93 dired.c
--- dired.c 11 Jul 2019 18:20:18 - 1.93
+++ dired.c 1 Apr 2020 11:42:45 -
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -53,6 +54,7 @@ static int d_refreshbuffer(int, int);
static int d_filevisitalt(int, int);
static int d_gotofile(int, int);
static void reaper(int);
+static int gotofile(char*);
static struct buffer *refreshbuffer(struct buffer *);
static int createlist(struct buffer *);
static void redelete(struct buffer *);
@@ -205,6 +207,7 @@ void
dired_init(void)
{
funmap_add(dired, "dired", 1);
+ funmap_add(dired_jump, "dired-jump", 1);
funmap_add(d_create_directory, "dired-create-directory", 1);
funmap_add(d_copy, "dired-do-copy", 1);
funmap_add(d_expunge, "dired-do-flagged-delete", 0);
@@ -257,6 +260,36 @@ dired(int f, int n)
/* ARGSUSED */
int
+dired_jump(int f, int n)
+{
+ char dname[NFILEN], *fname;
+ struct buffer *bp;
+ intlen, ret;
+
+ if (getbufcwd(dname, sizeof(dname)) != TRUE)
+ return (FALSE);
+
+ fname = curbp->b_fname;
+
+ if ((bp = dired_(dname)) == FALSE)
+ return (FALSE);
+ curbp = bp;
+
+ ret = showbuffer(bp, curwp, WFFULL | WFMODE);
+ if (ret != (TRUE))
+ return ret;
+
+ if (fname[0]) {
+ len = strlen(fname);
+ if (fname[len-1] != '/')
+ gotofile(basename(fname));
+ }
+
+ return (TRUE);
+}
+
+/* ARGSUSED */
+int
d_otherwindow(int f, int n)
{
char dname[NFILEN], *bufp, *slash;
@@ -1079,35 +1112,15 @@ createlist(struct buffer *bp)
}
int
-d_gotofile(int f, int n)
+gotofile(char *fname)
{
struct line *lp, *nlp;
- struct buffer *curbp;
- size_t lenfpath;
- char fpath[NFILEN], fname[NFILEN];
- char *p, *fpth, *fnp = NULL;
+ char *p;
int tmp;
- if (getbufcwd(fpath, sizeof(fpath)) != TRUE)
- fpath[0] = '\0';
- lenfpath = strlen(fpath);
- fnp = eread("Goto file: ", fpath, NFILEN,
- EFNEW | EFCR | EFFILE | EFDEF);
- if (fnp == NULL)
- return (ABORT);
- else if (fnp[0] == '\0')
- return (FALSE);
-
- fpth = adjustname(fpath, TRUE); /* Removes last '/' if */
- if (strlen(fpth) == lenfpath - 1) { /* directory, hence -1. */
- ewprintf("No file to find"); /* Current directory given so */
- return (TRUE); /* return at present location. */
- }
- (void)xbasename(fname, fpth, NFILEN);
- curbp = curwp->w_bufp;
tmp = 0;
- for (lp = bfirstlp(curbp); lp != curbp->b_headp; lp = nlp) {
- tmp++;
+ for (lp = bfirstlp(curbp); lp != curbp->b_headp; lp = nlp) {
+ tmp++;
if ((p = findfname(lp, p)) == NULL) {
nlp = lforw(lp);
continue;
@@ -1128,6 +1141,34 @@ d_gotofile(int f, int n)
ewprintf("");
return (TRUE);
}
+}
+
+int
+d_gotofile(int f, int n)
+{
+ size_t lenfpath;
+ char fpath[NFILEN], fname[NFILEN];
+ char *fpth, *fnp = NULL;
+
+ if (getbufcwd(fpath, sizeof(fpath)) != TRUE)
+ fpath[0] = '\0';
+ lenfpath = strlen(fpath);
+ fnp = eread("Goto file: ", fpath, NFILEN,
+ EFNEW | EFCR | EFFILE | EFDEF);
+ if (fnp == NULL)
+ return (ABORT);
+ else if (fnp[0] == '\0')
+ return (FALSE);
+
+ fpth = adjustname(fpath, TRUE); /* Removes last '/' if */
+ if (strlen(fpth) == lenfpath - 1) { /* directory, hence -1. */
+ ewprintf("No file to find"); /* Current directory given so */
+ return (TRUE); /* return at present location. */
+ }
+ (void)xbasename(fname, fpth, NFILEN);
+ curbp = curwp->w_bufp;
+
+ return gotofile(fname);
}
/*
Index: keymap.c
===
RCS file: /cvs/src/usr.bin/mg/keymap.c,v
retrieving revision 1.58
diff -u -p -r1.58 keymap.c
--- keymap.c 29 Dec 2015 19:44:32 - 1.58
+++ keymap.c 1 Apr 2020 11:42:45 -
@@ -129,6 +129,10 @@ static PF cXcB[] = {
ctrlg /* ^G */
};
+static PF cXcJ[] = {
+ dired_jump /* ^J */
+};
+
static PF cXcL[] = {
lowerregion, /* ^L */
rescan, /* ^M */
@@ -189,13 +193,16 @@ static PF cXcar[] = {
undo /* u */
};
-struct KEYMAPE (6) cXmap = {
- 6,
- 6,
+struct KEYMAPE (7) cXmap = {
+ 7,
+ 7,
rescan,
{
{
CCHR('B'), CCHR('G'),
Re: pselect(2) bug or feature?
On Wed, Apr 01, 2020 at 01:08:10PM +0200, Martin Pieuchot wrote:
> While working on moving pselect(2) outside of the KERNEL_LOCK() I found
> this surprising {p,}select(2) behavior exposed by the libc/sys/t_select.c
> regression test. Should it be considered as a bug or feature?
I think this is actually a feature. The standard does not mention that you
have to listen on something and it would allow to use select() as a
sleep() replacement. I would expect that pselect() should behave the same
way.
poll() does the same thing (you are allowed to pass a 0 poll fds).
> The above mentioned test does the following (simplified):
>
> fd = open("/dev/null", O_RDONLY);
> FD_ZERO();
> FD_SET(fd, );
> pselect(1, , NULL, NULL, NULL, );
>
> Note that in most environments, open(2) will return a value higher than
> 1 which means the value written by FD_SET() will never be checked by the
> current implementation of {p,}select(2). In this case, even if it isn't
> waiting for any condition, the current implementation will block.
>
> To put it differently, the code above is an obfuscated way to write:
>
> sigsuspend();
>
> If we agree that this is not what the user wants to do, I'm suggesting
> the diff below which makes {p,}select(2) return EINVAL if no bit has
> been found in the given set up to `nfds'.
>
> Independently from our decision, I'd suggest to rename the variable
> name `nfds' in the syscall documentation because it can lead to this
> kind of confusion.
>
> Finally if we accept this change the regression test will have to be
> adapted because polling "/dev/null" for reading always return true so
> pselect(2) wont block waiting for a signal. Any suggestion?
>
> Index: sys/kern/sys_generic.c
> ===
> RCS file: /cvs/src/sys/kern/sys_generic.c,v
> retrieving revision 1.131
> diff -u -p -r1.131 sys_generic.c
> --- sys/kern/sys_generic.c20 Mar 2020 04:11:05 - 1.131
> +++ sys/kern/sys_generic.c1 Apr 2020 10:27:51 -
> @@ -703,7 +703,7 @@ selscan(struct proc *p, fd_set *ibits, f
> {
> caddr_t cibits = (caddr_t)ibits, cobits = (caddr_t)obits;
> struct filedesc *fdp = p->p_fd;
> - int msk, i, j, fd;
> + int msk, i, j, fd, foundfds = 0;
> fd_mask bits;
> struct file *fp;
> int n = 0;
> @@ -719,6 +719,7 @@ selscan(struct proc *p, fd_set *ibits, f
> bits &= ~(1 << j);
> if ((fp = fd_getfile(fdp, fd)) == NULL)
> return (EBADF);
> + foundfds++;
> if ((*fp->f_ops->fo_poll)(fp, flag[msk], p)) {
> FD_SET(fd, pobits);
> n++;
> @@ -727,6 +728,8 @@ selscan(struct proc *p, fd_set *ibits, f
> }
> }
> }
> + if (foundfds == 0)
> + return (EINVAL);
> *retval = n;
> return (0);
> }
> Index: lib/libc/sys/select.2
> ===
> RCS file: /cvs/src/lib/libc/sys/select.2,v
> retrieving revision 1.42
> diff -u -p -r1.42 select.2
> --- lib/libc/sys/select.2 17 Sep 2016 01:01:42 - 1.42
> +++ lib/libc/sys/select.2 1 Apr 2020 10:35:29 -
> @@ -188,7 +188,7 @@ The specified time limit is invalid.
> One of its components is negative or too large.
> .It Bq Er EINVAL
> .Fa nfds
> -was less than 0.
> +was smaller than the smallest descriptor specified in the sets.
> .El
> .Sh SEE ALSO
> .Xr accept 2 ,
>
--
:wq Claudio
pselect(2) bug or feature?
While working on moving pselect(2) outside of the KERNEL_LOCK() I found
this surprising {p,}select(2) behavior exposed by the libc/sys/t_select.c
regression test. Should it be considered as a bug or feature?
The above mentioned test does the following (simplified):
fd = open("/dev/null", O_RDONLY);
FD_ZERO();
FD_SET(fd, );
pselect(1, , NULL, NULL, NULL, );
Note that in most environments, open(2) will return a value higher than
1 which means the value written by FD_SET() will never be checked by the
current implementation of {p,}select(2). In this case, even if it isn't
waiting for any condition, the current implementation will block.
To put it differently, the code above is an obfuscated way to write:
sigsuspend();
If we agree that this is not what the user wants to do, I'm suggesting
the diff below which makes {p,}select(2) return EINVAL if no bit has
been found in the given set up to `nfds'.
Independently from our decision, I'd suggest to rename the variable
name `nfds' in the syscall documentation because it can lead to this
kind of confusion.
Finally if we accept this change the regression test will have to be
adapted because polling "/dev/null" for reading always return true so
pselect(2) wont block waiting for a signal. Any suggestion?
Index: sys/kern/sys_generic.c
===
RCS file: /cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.131
diff -u -p -r1.131 sys_generic.c
--- sys/kern/sys_generic.c 20 Mar 2020 04:11:05 - 1.131
+++ sys/kern/sys_generic.c 1 Apr 2020 10:27:51 -
@@ -703,7 +703,7 @@ selscan(struct proc *p, fd_set *ibits, f
{
caddr_t cibits = (caddr_t)ibits, cobits = (caddr_t)obits;
struct filedesc *fdp = p->p_fd;
- int msk, i, j, fd;
+ int msk, i, j, fd, foundfds = 0;
fd_mask bits;
struct file *fp;
int n = 0;
@@ -719,6 +719,7 @@ selscan(struct proc *p, fd_set *ibits, f
bits &= ~(1 << j);
if ((fp = fd_getfile(fdp, fd)) == NULL)
return (EBADF);
+ foundfds++;
if ((*fp->f_ops->fo_poll)(fp, flag[msk], p)) {
FD_SET(fd, pobits);
n++;
@@ -727,6 +728,8 @@ selscan(struct proc *p, fd_set *ibits, f
}
}
}
+ if (foundfds == 0)
+ return (EINVAL);
*retval = n;
return (0);
}
Index: lib/libc/sys/select.2
===
RCS file: /cvs/src/lib/libc/sys/select.2,v
retrieving revision 1.42
diff -u -p -r1.42 select.2
--- lib/libc/sys/select.2 17 Sep 2016 01:01:42 - 1.42
+++ lib/libc/sys/select.2 1 Apr 2020 10:35:29 -
@@ -188,7 +188,7 @@ The specified time limit is invalid.
One of its components is negative or too large.
.It Bq Er EINVAL
.Fa nfds
-was less than 0.
+was smaller than the smallest descriptor specified in the sets.
.El
.Sh SEE ALSO
.Xr accept 2 ,
rpki-client and non-existing files
Currently rpki-client logs missing files like this:
rpki-client: ...trace: error:02FFF002:system library:func(4095):No such file
or directory
rpki-client: ...trace: error:20FFF080:BIO routines:CRYPTO_internal:no such file
rpki-client:
rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft:
BIO_new_file
Yes, you need to read the errors in reverse and even then the errors are
just hard to read.
This ugly format is mostly to blame on the error stack of OpenSSL.
As a workaround I switched to using fopen() and then BIO_new_fd()
which does the same thing but allows me to get a nice error from fopen():
rpki-client:
rpki.cnnic.cn/rpki/A9162E3D/515/FE-4PMY9qqTI2aJ0xLDm7cD-fvw.mft: fopen: No
such file or directory
Any opinions?
--
:wq Claudio
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.14
diff -u -p -r1.14 cert.c
--- cert.c 26 Feb 2020 02:35:08 - 1.14
+++ cert.c 30 Mar 2020 11:40:28 -
@@ -930,12 +930,18 @@ cert_parse_inner(X509 **xp, const char *
ASN1_OBJECT *obj;
struct parse p;
BIO *bio = NULL, *shamd;
+ FILE*f;
EVP_MD *md;
char mdbuf[EVP_MAX_MD_SIZE];
*xp = NULL;
- if ((bio = BIO_new_file(fn, "rb")) == NULL) {
+ if ((f = fopen(fn, "rb")) == NULL) {
+ warn("%s: fopen", fn);
+ return NULL;
+ }
+
+ if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
if (verbose > 0)
cryptowarnx("%s: BIO_new_file", fn);
return NULL;
Index: cms.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
retrieving revision 1.6
diff -u -p -r1.6 cms.c
--- cms.c 29 Nov 2019 05:14:11 - 1.6
+++ cms.c 30 Mar 2020 11:40:23 -
@@ -42,6 +42,7 @@ cms_parse_validate(X509 **xp, const char
ASN1_OCTET_STRING **os = NULL;
BIO *bio = NULL, *shamd;
CMS_ContentInfo *cms;
+ FILE*f;
char buf[128], mdbuf[EVP_MAX_MD_SIZE];
int rc = 0, sz;
STACK_OF(X509) *certs = NULL;
@@ -55,10 +56,13 @@ cms_parse_validate(X509 **xp, const char
* This is usually fopen() failure, so let it pass through to
* the handler, which will in turn ignore the entity.
*/
+ if ((f = fopen(fn, "rb")) == NULL) {
+ warn("%s: fopen", fn);
+ return NULL;
+ }
- if ((bio = BIO_new_file(fn, "rb")) == NULL) {
- if (verbose > 0)
- cryptowarnx("%s: BIO_new_file", fn);
+ if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
+ cryptowarnx("%s: BIO_new_fp", fn);
return NULL;
}
Index: crl.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
retrieving revision 1.7
diff -u -p -r1.7 crl.c
--- crl.c 29 Nov 2019 04:40:04 - 1.7
+++ crl.c 30 Mar 2020 11:40:32 -
@@ -36,10 +36,16 @@ crl_parse(const char *fn, const unsigned
int rc = 0, sz;
X509_CRL*x = NULL;
BIO *bio = NULL, *shamd;
+ FILE*f;
EVP_MD *md;
char mdbuf[EVP_MAX_MD_SIZE];
- if ((bio = BIO_new_file(fn, "rb")) == NULL) {
+ if ((f = fopen(fn, "rb")) == NULL) {
+ warn("%s: fopen", fn);
+ return NULL;
+ }
+
+ if ((bio = BIO_new_fp(f, BIO_CLOSE)) == NULL) {
if (verbose > 0)
cryptowarnx("%s: BIO_new_file", fn);
return NULL;
Re: usb(4): use cacheable buffers for data transfers (massive speedup)
On Wed, Apr 01, 2020 at 12:04:25PM +0200, Patrick Wildt wrote:
> On Wed, Apr 01, 2020 at 09:40:06AM +0200, Patrick Wildt wrote:
> > On Wed, Apr 01, 2020 at 09:22:07AM +0200, Patrick Wildt wrote:
> > > On Wed, Apr 01, 2020 at 04:47:10PM +1100, Jonathan Gray wrote:
> > > > On Wed, Apr 01, 2020 at 12:58:23PM +1100, Jonathan Gray wrote:
> > > > > On Wed, Mar 18, 2020 at 01:41:06PM +0100, Patrick Wildt wrote:
> > > > > > On Wed, Mar 18, 2020 at 11:22:40AM +0100, Patrick Wildt wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > I've spent a few days investigating why USB ethernet adapters are
> > > > > > > so
> > > > > > > horribly slow on my ARMs. Using dt(4) I realized that it was
> > > > > > > spending
> > > > > > > most of its time in memcpy. But, why? As it turns out, all USB
> > > > > > > data
> > > > > > > buffers are mapped COHERENT, which on some/most ARMs means
> > > > > > > uncached.
> > > > > > > Using cached data buffers makes the performance rise from 20
> > > > > > > mbit/s to
> > > > > > > 200 mbit/s. Quite a difference.
> > > > > > >
> > > > > > > sys/dev/usb/usb_mem.c:
> > > > > > > error = bus_dmamem_map(tag, p->segs, p->nsegs, p->size,
> > > > > > > >kaddr,
> > > > > > > BUS_DMA_NOWAIT|BUS_DMA_COHERENT);
> > > > > > >
> > > > > > > On x86, COHERENT is essentially a no-op. On ARM, it depends on
> > > > > > > the SoC.
> > > > > > > Some SoCs have cache-coherent USB controllers, some don't. Mine
> > > > > > > does
> > > > > > > not, so mapping it COHERENT means uncached and thus slow.
> > > > > > >
> > > > > > > Why do we do that? Well, when the code was imported in 99, it was
> > > > > > > already there. Since then we have gained infrastructure for DMA
> > > > > > > syncs in the USB stack, which I think are proper.
> > > > > > >
> > > > > > > sys/dev/usb/usbdi.c - usbd_transfer() (before transfer)
> > > > > > >
> > > > > > > if (!usbd_xfer_isread(xfer)) {
> > > > > > > if ((xfer->flags & USBD_NO_COPY) == 0)
> > > > > > > memcpy(KERNADDR(>dmabuf, 0), xfer->buffer,
> > > > > > > xfer->length);
> > > > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > > > BUS_DMASYNC_PREWRITE);
> > > > > > > } else
> > > > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > > > BUS_DMASYNC_PREREAD);
> > > > > > > err = pipe->methods->transfer(xfer);
> > > > > > >
> > > > > > > sys/dev/usb/usbdi.c - usb_transfer_complete() (after transfer)
> > > > > > >
> > > > > > > if (xfer->actlen != 0) {
> > > > > > > if (usbd_xfer_isread(xfer)) {
> > > > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > > > BUS_DMASYNC_POSTREAD);
> > > > > > > if (!(xfer->flags & USBD_NO_COPY))
> > > > > > > memcpy(xfer->buffer,
> > > > > > > KERNADDR(>dmabuf, 0),
> > > > > > > xfer->actlen);
> > > > > > > } else
> > > > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > > > BUS_DMASYNC_POSTWRITE);
> > > > > > > }
> > > > > > >
> > > > > > > We cannot just remove COHERENT, since some drivers, like ehci(4),
> > > > > > > use
> > > > > > > the same backend to allocate their rings. And I can't vouch for
> > > > > > > those
> > > > > > > drivers' sanity.
> > > > > > >
> > > > > > > As a first step, I would like to go ahead with another solution,
> > > > > > > which
> > > > > > > is based on a diff from Marius Strobl, who added those syncs in
> > > > > > > the
> > > > > > > first place. Essentially it splits the memory handling into
> > > > > > > cacheable
> > > > > > > and non-cacheable blocks. The USB data transfers and everyone
> > > > > > > who uses
> > > > > > > usbd_alloc_buffer() then use cacheable buffers, while code like
> > > > > > > ehci(4)
> > > > > > > still don't. This is a bit of a safer approach imho, since we
> > > > > > > don't
> > > > > > > hurt the controller drivers, but speed up the data buffers.
> > > > > > >
> > > > > > > Once we have verified that there are no regressions, we can adjust
> > > > > > > ehci(4) and the like, add proper syncs, make sure they still work
> > > > > > > as
> > > > > > > well as before, and maybe then back this out again.
> > > > > > >
> > > > > > > Keep note that this is all a no-op on X86, but all the other
> > > > > > > archs will
> > > > > > > profit from this.
> > > > > > >
> > > > > > > ok?
> > > > > > >
> > > > > > > Patrick
> > > > > >
> > > > > > Update diff with inverted logic. kettenis@ argues that we should
> > > > > > invert the logic, and those who need COHERENT memory should ask
> > > > > > for that explicitly, since for bus_dmamem_map() it also needs to
> > > > > > be passed explicitly. This also points out all those users that
> > > > > > use usb_allocmem() internally, where we might want to have a look
> > > > > > if
Re: Dedulpicate pipex(4) and pppx(4) code
Updated diff. The idea is to use already existing pipex API for
pipex_session creation and destruction. pppx_if now holds a reference
to pipex_session.
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.78
diff -u -p -r1.78 if_pppx.c
--- sys/net/if_pppx.c 1 Apr 2020 07:15:59 - 1.78
+++ sys/net/if_pppx.c 1 Apr 2020 09:50:19 -
@@ -155,7 +155,7 @@ struct pppx_if {
int pxi_unit;
struct ifnetpxi_if;
struct pppx_dev *pxi_dev;
- struct pipex_sessionpxi_session;
+ struct pipex_session*pxi_session;
struct pipex_iface_context pxi_ifcontext;
};
@@ -655,15 +655,10 @@ int
pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
{
struct pppx_if *pxi;
- struct pipex_session *session;
- struct pipex_hash_head *chain;
struct ifnet *ifp;
int unit, error = 0;
struct in_ifaddr *ia;
struct sockaddr_in ifaddr;
-#ifdef PIPEX_PPPOE
- struct ifnet *over_ifp = NULL;
-#endif
/*
* XXX: As long as `session' is allocated as part of a `pxi'
@@ -673,157 +668,16 @@ pppx_add_session(struct pppx_dev *pxd, s
if (req->pr_timeout_sec != 0)
return (EINVAL);
- switch (req->pr_protocol) {
-#ifdef PIPEX_PPPOE
- case PIPEX_PROTO_PPPOE:
- over_ifp = ifunit(req->pr_proto.pppoe.over_ifname);
- if (over_ifp == NULL)
- return (EINVAL);
- if (req->pr_peer_address.ss_family != AF_UNSPEC)
- return (EINVAL);
- break;
-#endif
-#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
- case PIPEX_PROTO_PPTP:
- case PIPEX_PROTO_L2TP:
- switch (req->pr_peer_address.ss_family) {
- case AF_INET:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in))
- return (EINVAL);
- break;
-#ifdef INET6
- case AF_INET6:
- if (req->pr_peer_address.ss_len != sizeof(struct
sockaddr_in6))
- return (EINVAL);
- break;
-#endif
- default:
- return (EPROTONOSUPPORT);
- }
- if (req->pr_peer_address.ss_family !=
- req->pr_local_address.ss_family ||
- req->pr_peer_address.ss_len !=
- req->pr_local_address.ss_len)
- return (EINVAL);
- break;
-#endif /* defined(PIPEX_PPTP) || defined(PIPEX_L2TP) */
- default:
- return (EPROTONOSUPPORT);
- }
-
pxi = pool_get(pppx_if_pl, PR_WAITOK | PR_ZERO);
- if (pxi == NULL)
- return (ENOMEM);
-
- session = >pxi_session;
ifp = >pxi_if;
- /* fake a pipex interface context */
- session->pipex_iface = >pxi_ifcontext;
- session->pipex_iface->ifnet_this = ifp;
- session->pipex_iface->pipexmode = PIPEX_ENABLED;
-
- /* setup session */
- session->state = PIPEX_STATE_OPENED;
- session->protocol = req->pr_protocol;
- session->session_id = req->pr_session_id;
- session->peer_session_id = req->pr_peer_session_id;
- session->peer_mru = req->pr_peer_mru;
- session->timeout_sec = req->pr_timeout_sec;
- session->ppp_flags = req->pr_ppp_flags;
- session->ppp_id = req->pr_ppp_id;
-
- session->ip_forward = 1;
-
- session->ip_address.sin_family = AF_INET;
- session->ip_address.sin_len = sizeof(struct sockaddr_in);
- session->ip_address.sin_addr = req->pr_ip_address;
-
- session->ip_netmask.sin_family = AF_INET;
- session->ip_netmask.sin_len = sizeof(struct sockaddr_in);
- session->ip_netmask.sin_addr = req->pr_ip_netmask;
-
- if (session->ip_netmask.sin_addr.s_addr == 0L)
- session->ip_netmask.sin_addr.s_addr = 0xL;
- session->ip_address.sin_addr.s_addr &=
- session->ip_netmask.sin_addr.s_addr;
-
- if (req->pr_peer_address.ss_len > 0)
- memcpy(>peer, >pr_peer_address,
- MIN(req->pr_peer_address.ss_len, sizeof(session->peer)));
- if (req->pr_local_address.ss_len > 0)
- memcpy(>local, >pr_local_address,
- MIN(req->pr_local_address.ss_len, sizeof(session->local)));
-#ifdef PIPEX_PPPOE
- if (req->pr_protocol == PIPEX_PROTO_PPPOE)
- session->proto.pppoe.over_ifidx = over_ifp->if_index;
-#endif
-#ifdef PIPEX_PPTP
- if (req->pr_protocol == PIPEX_PROTO_PPTP) {
- struct pipex_pptp_session *sess_pptp = >proto.pptp;
-
- sess_pptp->snd_gap = 0;
- sess_pptp->rcv_gap = 0;
- sess_pptp->snd_una =
Re: usb(4): use cacheable buffers for data transfers (massive speedup)
On Wed, Apr 01, 2020 at 09:40:06AM +0200, Patrick Wildt wrote:
> On Wed, Apr 01, 2020 at 09:22:07AM +0200, Patrick Wildt wrote:
> > On Wed, Apr 01, 2020 at 04:47:10PM +1100, Jonathan Gray wrote:
> > > On Wed, Apr 01, 2020 at 12:58:23PM +1100, Jonathan Gray wrote:
> > > > On Wed, Mar 18, 2020 at 01:41:06PM +0100, Patrick Wildt wrote:
> > > > > On Wed, Mar 18, 2020 at 11:22:40AM +0100, Patrick Wildt wrote:
> > > > > > Hi,
> > > > > >
> > > > > > I've spent a few days investigating why USB ethernet adapters are so
> > > > > > horribly slow on my ARMs. Using dt(4) I realized that it was
> > > > > > spending
> > > > > > most of its time in memcpy. But, why? As it turns out, all USB
> > > > > > data
> > > > > > buffers are mapped COHERENT, which on some/most ARMs means uncached.
> > > > > > Using cached data buffers makes the performance rise from 20 mbit/s
> > > > > > to
> > > > > > 200 mbit/s. Quite a difference.
> > > > > >
> > > > > > sys/dev/usb/usb_mem.c:
> > > > > > error = bus_dmamem_map(tag, p->segs, p->nsegs, p->size,
> > > > > >>kaddr,
> > > > > > BUS_DMA_NOWAIT|BUS_DMA_COHERENT);
> > > > > >
> > > > > > On x86, COHERENT is essentially a no-op. On ARM, it depends on the
> > > > > > SoC.
> > > > > > Some SoCs have cache-coherent USB controllers, some don't. Mine
> > > > > > does
> > > > > > not, so mapping it COHERENT means uncached and thus slow.
> > > > > >
> > > > > > Why do we do that? Well, when the code was imported in 99, it was
> > > > > > already there. Since then we have gained infrastructure for DMA
> > > > > > syncs in the USB stack, which I think are proper.
> > > > > >
> > > > > > sys/dev/usb/usbdi.c - usbd_transfer() (before transfer)
> > > > > >
> > > > > > if (!usbd_xfer_isread(xfer)) {
> > > > > > if ((xfer->flags & USBD_NO_COPY) == 0)
> > > > > > memcpy(KERNADDR(>dmabuf, 0), xfer->buffer,
> > > > > > xfer->length);
> > > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > > BUS_DMASYNC_PREWRITE);
> > > > > > } else
> > > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > > BUS_DMASYNC_PREREAD);
> > > > > > err = pipe->methods->transfer(xfer);
> > > > > >
> > > > > > sys/dev/usb/usbdi.c - usb_transfer_complete() (after transfer)
> > > > > >
> > > > > > if (xfer->actlen != 0) {
> > > > > > if (usbd_xfer_isread(xfer)) {
> > > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > > BUS_DMASYNC_POSTREAD);
> > > > > > if (!(xfer->flags & USBD_NO_COPY))
> > > > > > memcpy(xfer->buffer,
> > > > > > KERNADDR(>dmabuf, 0),
> > > > > > xfer->actlen);
> > > > > > } else
> > > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > > BUS_DMASYNC_POSTWRITE);
> > > > > > }
> > > > > >
> > > > > > We cannot just remove COHERENT, since some drivers, like ehci(4),
> > > > > > use
> > > > > > the same backend to allocate their rings. And I can't vouch for
> > > > > > those
> > > > > > drivers' sanity.
> > > > > >
> > > > > > As a first step, I would like to go ahead with another solution,
> > > > > > which
> > > > > > is based on a diff from Marius Strobl, who added those syncs in the
> > > > > > first place. Essentially it splits the memory handling into
> > > > > > cacheable
> > > > > > and non-cacheable blocks. The USB data transfers and everyone who
> > > > > > uses
> > > > > > usbd_alloc_buffer() then use cacheable buffers, while code like
> > > > > > ehci(4)
> > > > > > still don't. This is a bit of a safer approach imho, since we don't
> > > > > > hurt the controller drivers, but speed up the data buffers.
> > > > > >
> > > > > > Once we have verified that there are no regressions, we can adjust
> > > > > > ehci(4) and the like, add proper syncs, make sure they still work as
> > > > > > well as before, and maybe then back this out again.
> > > > > >
> > > > > > Keep note that this is all a no-op on X86, but all the other archs
> > > > > > will
> > > > > > profit from this.
> > > > > >
> > > > > > ok?
> > > > > >
> > > > > > Patrick
> > > > >
> > > > > Update diff with inverted logic. kettenis@ argues that we should
> > > > > invert the logic, and those who need COHERENT memory should ask
> > > > > for that explicitly, since for bus_dmamem_map() it also needs to
> > > > > be passed explicitly. This also points out all those users that
> > > > > use usb_allocmem() internally, where we might want to have a look
> > > > > if COHERENT is actually needed or not, or if it can be refactored
> > > > > in another way.
> > > >
> > > > These commits broke usb on imx.6 with cubox:
> > > >
> > > > imxehci0 at simplebus3
> > > > usb0 at imxehci0: USB revision 2.0
> > > > usb0: root hub problem
> > > > imxehci1 at
Unify file descriptor kqfilter errors
kevent(2) notifies userland if an event happened based on given filter.
If an error is encountered during execution, the given descriptor will
have the EV_ERROR flag set and `data' will contain an errno(2) value.
This value might come from the kqueue subsystem itself or from the
f_attach() function responsible for attaching filters. Every kind of
event watchable with kevent(2) has a specific f_attach() function. When
it comes to file descriptors it translate into specific fo_kqfilter()
function.
The diff below fix some incoherences of errors returned by those
functions. Note that these values aren't unified across BSDs. So this
only makes all OpenBSD kqfilter coherent between themself. It follows
the most-used approach, the following errno(2) values are returned:
- EOPNOTSUPP: when there's no "filterops" for a given fd
- EINVAL: when the requested filter isn't supported by the current
"filterops"
- ENXIO:when the underlying device is no longer valid/present
On top of those 3 values, which are returned by most of the kqfilters
the following exception exist and aren't touched by the diff below:
- EPIPE:returned only by pipe_kqfilter() when using EVFILT_WRITE
if the other end of the pipe has been closed
- ENOMEM, and
EAGAIN: returned by nfs_kqfilter() when it fails to create a
thread.
I'm questioning if it makes sense to return both EOPNOTSUPP and EINVAL,
I can see if there's any value for the user of such API. But that might
be seen as a different discussion, at least this diff brings coherency.
Comments? Oks?
Index: arch/arm64/arm64/acpiapm.c
===
RCS file: /cvs/src/sys/arch/arm64/arm64/acpiapm.c,v
retrieving revision 1.1
diff -u -p -r1.1 acpiapm.c
--- arch/arm64/arm64/acpiapm.c 23 Jan 2019 09:57:36 - 1.1
+++ arch/arm64/arm64/acpiapm.c 1 Apr 2020 08:39:14 -
@@ -53,6 +53,6 @@ int
acpiapmkqfilter(dev_t dev, struct knote *kn)
{
if (!acpiapm_kqfilter)
- return ENODEV;
+ return EOPNOTSUPP;
return acpiapm_kqfilter(dev, kn);
}
Index: arch/i386/i386/acpiapm.c
===
RCS file: /cvs/src/sys/arch/i386/i386/acpiapm.c,v
retrieving revision 1.1
diff -u -p -r1.1 acpiapm.c
--- arch/i386/i386/acpiapm.c25 Nov 2007 15:45:17 - 1.1
+++ arch/i386/i386/acpiapm.c1 Apr 2020 08:39:14 -
@@ -53,6 +53,6 @@ int
acpiapmkqfilter(dev_t dev, struct knote *kn)
{
if (!acpiapm_kqfilter)
- return ENODEV;
+ return EOPNOTSUPP;
return acpiapm_kqfilter(dev, kn);
}
Index: dev/acpi/acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.378
diff -u -p -r1.378 acpi.c
--- dev/acpi/acpi.c 20 Feb 2020 16:56:52 - 1.378
+++ dev/acpi/acpi.c 1 Apr 2020 08:39:14 -
@@ -3539,7 +3539,7 @@ acpiioctl(dev_t dev, u_long cmd, caddr_t
int
acpikqfilter(dev_t dev, struct knote *kn)
{
- return (ENXIO);
+ return (EINVAL);
}
#endif /* SMALL_KERNEL */
Index: dev/cons.c
===
RCS file: /cvs/src/sys/dev/cons.c,v
retrieving revision 1.28
diff -u -p -r1.28 cons.c
--- dev/cons.c 19 Feb 2018 08:59:52 - 1.28
+++ dev/cons.c 1 Apr 2020 08:39:14 -
@@ -215,7 +215,7 @@ cnkqfilter(dev_t dev, struct knote *kn)
dev = cn_tab->cn_dev;
if (cdevsw[major(dev)].d_kqfilter)
return ((*cdevsw[major(dev)].d_kqfilter)(dev, kn));
- return (ENXIO);
+ return (EOPNOTSUPP);
}
int
Index: dev/usb/ugen.c
===
RCS file: /cvs/src/sys/dev/usb/ugen.c,v
retrieving revision 1.103
diff -u -p -r1.103 ugen.c
--- dev/usb/ugen.c 22 Feb 2020 14:01:34 - 1.103
+++ dev/usb/ugen.c 1 Apr 2020 08:39:14 -
@@ -1365,7 +1365,7 @@ ugenkqfilter(dev_t dev, struct knote *kn
/* XXX always IN */
sce = >sc_endpoints[UGENENDPOINT(dev)][IN];
if (sce == NULL)
- return (EINVAL);
+ return (ENXIO);
switch (kn->kn_filter) {
case EVFILT_READ:
Index: dev/usb/uhid.c
===
RCS file: /cvs/src/sys/dev/usb/uhid.c,v
retrieving revision 1.77
diff -u -p -r1.77 uhid.c
--- dev/usb/uhid.c 20 Feb 2020 16:56:52 - 1.77
+++ dev/usb/uhid.c 1 Apr 2020 08:39:14 -
@@ -485,7 +485,7 @@ uhidkqfilter(dev_t dev, struct knote *kn
return (ENXIO);
if (usbd_is_dying(sc->sc_hdev.sc_udev))
- return (EIO);
+ return (ENXIO);
switch (kn->kn_filter) {
case EVFILT_READ:
Re: Introduce kqsleep()
> Date: Wed, 1 Apr 2020 10:28:33 +0200
> From: Martin Pieuchot
>
> I've started to refactor some of the kqueue() subsystem to make progress
> on moving selwakeup() out of the KERNEL_LOCK(). Diff below is a small
> part of this work. It extracts the sleeping logic outside of the main
> loop.
>
> I find this easier to read and it allows me to make my huge diff more
> easily reviewable. Do we see value in this alone or do we prefer to
> wait for a big diff?
>
> Any comment or ok?
Doing the validity test earlier makes sense. The behaviour changes
slightly if nevents == 0 since now the validity does get checked where
it would previously succeed. But that is probably ok.
ok kettenis@
> Index: kern/kern_event.c
> ===
> RCS file: /cvs/src/sys/kern/kern_event.c,v
> retrieving revision 1.128
> diff -u -p -r1.128 kern_event.c
> --- kern/kern_event.c 20 Mar 2020 04:14:50 - 1.128
> +++ kern/kern_event.c 1 Apr 2020 08:16:52 -
> @@ -61,6 +61,7 @@ voidkqueue_init(void);
> void KQREF(struct kqueue *);
> void KQRELE(struct kqueue *);
>
> +int kqueue_sleep(struct kqueue *, struct timespec *);
> int kqueue_scan(struct kqueue *kq, int maxevents,
> struct kevent *ulistp, struct timespec *timeout,
> struct proc *p, int *retval);
> @@ -556,6 +557,10 @@ sys_kevent(struct proc *p, void *v, regi
> error = copyin(SCARG(uap, timeout), , sizeof(ts));
> if (error)
> goto done;
> + if (ts.tv_sec < 0 || !timespecisvalid()) {
> + error = EINVAL;
> + goto done;
> + }
> #ifdef KTRACE
> if (KTRPOINT(p, KTR_STRUCT))
> ktrreltimespec(p, );
> @@ -838,13 +843,37 @@ done:
> }
>
> int
> +kqueue_sleep(struct kqueue *kq, struct timespec *tsp)
> +{
> + struct timespec elapsed, start, stop;
> + uint64_t nsecs;
> + int error;
> +
> + splassert(IPL_HIGH);
> +
> + if (tsp != NULL) {
> + getnanouptime();
> + nsecs = MIN(TIMESPEC_TO_NSEC(tsp), MAXTSLP);
> + } else
> + nsecs = INFSLP;
> + error = tsleep_nsec(kq, PSOCK | PCATCH, "kqread", nsecs);
> + if (tsp != NULL) {
> + getnanouptime();
> + timespecsub(, , );
> + timespecsub(tsp, , tsp);
> + if (tsp->tv_sec < 0)
> + timespecclear(tsp);
> + }
> +
> + return (error);
> +}
> +
> +int
> kqueue_scan(struct kqueue *kq, int maxevents, struct kevent *ulistp,
> struct timespec *tsp, struct proc *p, int *retval)
> {
> struct kevent *kevp;
> - struct timespec elapsed, start, stop;
> struct knote mend, mstart, *kn;
> - uint64_t nsecs;
> int s, count, nkev = 0, error = 0;
> struct kevent kev[KQ_NEVENTS];
>
> @@ -852,11 +881,6 @@ kqueue_scan(struct kqueue *kq, int maxev
> if (count == 0)
> goto done;
>
> - if (tsp != NULL && (tsp->tv_sec < 0 || !timespecisvalid(tsp))) {
> - error = EINVAL;
> - goto done;
> - }
> -
> memset(, 0, sizeof(mstart));
> memset(, 0, sizeof(mend));
>
> @@ -875,19 +899,7 @@ retry:
> goto done;
> }
> kq->kq_state |= KQ_SLEEP;
> - if (tsp != NULL) {
> - getnanouptime();
> - nsecs = MIN(TIMESPEC_TO_NSEC(tsp), MAXTSLP);
> - } else
> - nsecs = INFSLP;
> - error = tsleep_nsec(kq, PSOCK | PCATCH, "kqread", nsecs);
> - if (tsp != NULL) {
> - getnanouptime();
> - timespecsub(, , );
> - timespecsub(tsp, , tsp);
> - if (tsp->tv_sec < 0)
> - timespecclear(tsp);
> - }
> + error = kqueue_sleep(kq, tsp);
> splx(s);
> if (error == 0 || error == EWOULDBLOCK)
> goto retry;
>
>
Re: usb(4): use cacheable buffers for data transfers (massive speedup)
On 01/04/20(Wed) 10:06, Mark Kettenis wrote:
> > Date: Wed, 1 Apr 2020 09:40:05 +0200
> > From: Patrick Wildt
> >
> > On Wed, Apr 01, 2020 at 09:22:07AM +0200, Patrick Wildt wrote:
> > > On Wed, Apr 01, 2020 at 04:47:10PM +1100, Jonathan Gray wrote:
> > > > On Wed, Apr 01, 2020 at 12:58:23PM +1100, Jonathan Gray wrote:
> > > > > On Wed, Mar 18, 2020 at 01:41:06PM +0100, Patrick Wildt wrote:
> > > > > > On Wed, Mar 18, 2020 at 11:22:40AM +0100, Patrick Wildt wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > I've spent a few days investigating why USB ethernet adapters are
> > > > > > > so
> > > > > > > horribly slow on my ARMs. Using dt(4) I realized that it was
> > > > > > > spending
> > > > > > > most of its time in memcpy. But, why? As it turns out, all USB
> > > > > > > data
> > > > > > > buffers are mapped COHERENT, which on some/most ARMs means
> > > > > > > uncached.
> > > > > > > Using cached data buffers makes the performance rise from 20
> > > > > > > mbit/s to
> > > > > > > 200 mbit/s. Quite a difference.
> > > > > > >
> > > > > > > sys/dev/usb/usb_mem.c:
> > > > > > > error = bus_dmamem_map(tag, p->segs, p->nsegs, p->size,
> > > > > > > >kaddr,
> > > > > > > BUS_DMA_NOWAIT|BUS_DMA_COHERENT);
> > > > > > >
> > > > > > > On x86, COHERENT is essentially a no-op. On ARM, it depends on
> > > > > > > the SoC.
> > > > > > > Some SoCs have cache-coherent USB controllers, some don't. Mine
> > > > > > > does
> > > > > > > not, so mapping it COHERENT means uncached and thus slow.
> > > > > > >
> > > > > > > Why do we do that? Well, when the code was imported in 99, it was
> > > > > > > already there. Since then we have gained infrastructure for DMA
> > > > > > > syncs in the USB stack, which I think are proper.
> > > > > > >
> > > > > > > sys/dev/usb/usbdi.c - usbd_transfer() (before transfer)
> > > > > > >
> > > > > > > if (!usbd_xfer_isread(xfer)) {
> > > > > > > if ((xfer->flags & USBD_NO_COPY) == 0)
> > > > > > > memcpy(KERNADDR(>dmabuf, 0), xfer->buffer,
> > > > > > > xfer->length);
> > > > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > > > BUS_DMASYNC_PREWRITE);
> > > > > > > } else
> > > > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > > > BUS_DMASYNC_PREREAD);
> > > > > > > err = pipe->methods->transfer(xfer);
> > > > > > >
> > > > > > > sys/dev/usb/usbdi.c - usb_transfer_complete() (after transfer)
> > > > > > >
> > > > > > > if (xfer->actlen != 0) {
> > > > > > > if (usbd_xfer_isread(xfer)) {
> > > > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > > > BUS_DMASYNC_POSTREAD);
> > > > > > > if (!(xfer->flags & USBD_NO_COPY))
> > > > > > > memcpy(xfer->buffer,
> > > > > > > KERNADDR(>dmabuf, 0),
> > > > > > > xfer->actlen);
> > > > > > > } else
> > > > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > > > BUS_DMASYNC_POSTWRITE);
> > > > > > > }
> > > > > > >
> > > > > > > We cannot just remove COHERENT, since some drivers, like ehci(4),
> > > > > > > use
> > > > > > > the same backend to allocate their rings. And I can't vouch for
> > > > > > > those
> > > > > > > drivers' sanity.
> > > > > > >
> > > > > > > As a first step, I would like to go ahead with another solution,
> > > > > > > which
> > > > > > > is based on a diff from Marius Strobl, who added those syncs in
> > > > > > > the
> > > > > > > first place. Essentially it splits the memory handling into
> > > > > > > cacheable
> > > > > > > and non-cacheable blocks. The USB data transfers and everyone
> > > > > > > who uses
> > > > > > > usbd_alloc_buffer() then use cacheable buffers, while code like
> > > > > > > ehci(4)
> > > > > > > still don't. This is a bit of a safer approach imho, since we
> > > > > > > don't
> > > > > > > hurt the controller drivers, but speed up the data buffers.
> > > > > > >
> > > > > > > Once we have verified that there are no regressions, we can adjust
> > > > > > > ehci(4) and the like, add proper syncs, make sure they still work
> > > > > > > as
> > > > > > > well as before, and maybe then back this out again.
> > > > > > >
> > > > > > > Keep note that this is all a no-op on X86, but all the other
> > > > > > > archs will
> > > > > > > profit from this.
> > > > > > >
> > > > > > > ok?
> > > > > > >
> > > > > > > Patrick
> > > > > >
> > > > > > Update diff with inverted logic. kettenis@ argues that we should
> > > > > > invert the logic, and those who need COHERENT memory should ask
> > > > > > for that explicitly, since for bus_dmamem_map() it also needs to
> > > > > > be passed explicitly. This also points out all those users that
> > > > > > use usb_allocmem() internally, where we might want to have a look
> > > > > > if COHERENT is
worm(6): Remove stdin garbage after end
When playing worm(6) and you crash there's a sleep of 2 seconds that
still allows input to be put into the terminal buffer that doesn't get
read until the application stops.
A similar case is possible in a more limited timespan if you win the
game.
The diff below flushes the input buffer just before exiting curses and
the application, leaving me with no clutter in my $SHELL.
I choose to also add '\n' as an escape character, since it's not part
of the game and allows me to exit after a crash faster (which always
annoys me) and allows me to add a new command straight away.
The crash() case might be a bit elaborate, but it's the closest to the
behaviour we have right now, minus the garbage.
Thoughts? OK?
martijn
Index: worm.c
===
RCS file: /cvs/src/games/worm/worm.c,v
retrieving revision 1.39
diff -u -p -r1.39 worm.c
--- worm.c 24 Aug 2018 11:14:49 - 1.39
+++ worm.c 1 Apr 2020 08:27:03 -
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#define HEAD '@'
@@ -239,7 +240,12 @@ rnd(int range)
void
newpos(struct body *bp)
{
+ int ch;
+
if (visible_len == (LINES-3) * (COLS-3) - 1) {
+ timeout(0);
+ while ((ch = getch()) != '\n' && ch != ERR)
+ continue;
endwin();
printf("\nYou won!\nYour final score was %d\n\n", score);
exit(0);
@@ -387,7 +393,17 @@ newlink(void)
void
crash(void)
{
- sleep(2);
+ int ch, delay;
+ struct timespec tstart, tnow, texp;
+
+ timeout(2000);
+ (void) clock_gettime(CLOCK_MONOTONIC, );
+ while ((ch = getch()) != ERR && ch != '\n') {
+ (void) clock_gettime(CLOCK_MONOTONIC, );
+ timespecsub(, , );
+ delay = 2000 - (texp.tv_sec * 1000) - (texp.tv_nsec / 100);
+ timeout(delay >= 0 ? delay : 0);
+ }
clear();
endwin();
printf("Well, you ran into something and the game is over.\n");
Introduce kqsleep()
I've started to refactor some of the kqueue() subsystem to make progress
on moving selwakeup() out of the KERNEL_LOCK(). Diff below is a small
part of this work. It extracts the sleeping logic outside of the main
loop.
I find this easier to read and it allows me to make my huge diff more
easily reviewable. Do we see value in this alone or do we prefer to
wait for a big diff?
Any comment or ok?
Index: kern/kern_event.c
===
RCS file: /cvs/src/sys/kern/kern_event.c,v
retrieving revision 1.128
diff -u -p -r1.128 kern_event.c
--- kern/kern_event.c 20 Mar 2020 04:14:50 - 1.128
+++ kern/kern_event.c 1 Apr 2020 08:16:52 -
@@ -61,6 +61,7 @@ void kqueue_init(void);
void KQREF(struct kqueue *);
void KQRELE(struct kqueue *);
+intkqueue_sleep(struct kqueue *, struct timespec *);
intkqueue_scan(struct kqueue *kq, int maxevents,
struct kevent *ulistp, struct timespec *timeout,
struct proc *p, int *retval);
@@ -556,6 +557,10 @@ sys_kevent(struct proc *p, void *v, regi
error = copyin(SCARG(uap, timeout), , sizeof(ts));
if (error)
goto done;
+ if (ts.tv_sec < 0 || !timespecisvalid()) {
+ error = EINVAL;
+ goto done;
+ }
#ifdef KTRACE
if (KTRPOINT(p, KTR_STRUCT))
ktrreltimespec(p, );
@@ -838,13 +843,37 @@ done:
}
int
+kqueue_sleep(struct kqueue *kq, struct timespec *tsp)
+{
+ struct timespec elapsed, start, stop;
+ uint64_t nsecs;
+ int error;
+
+ splassert(IPL_HIGH);
+
+ if (tsp != NULL) {
+ getnanouptime();
+ nsecs = MIN(TIMESPEC_TO_NSEC(tsp), MAXTSLP);
+ } else
+ nsecs = INFSLP;
+ error = tsleep_nsec(kq, PSOCK | PCATCH, "kqread", nsecs);
+ if (tsp != NULL) {
+ getnanouptime();
+ timespecsub(, , );
+ timespecsub(tsp, , tsp);
+ if (tsp->tv_sec < 0)
+ timespecclear(tsp);
+ }
+
+ return (error);
+}
+
+int
kqueue_scan(struct kqueue *kq, int maxevents, struct kevent *ulistp,
struct timespec *tsp, struct proc *p, int *retval)
{
struct kevent *kevp;
- struct timespec elapsed, start, stop;
struct knote mend, mstart, *kn;
- uint64_t nsecs;
int s, count, nkev = 0, error = 0;
struct kevent kev[KQ_NEVENTS];
@@ -852,11 +881,6 @@ kqueue_scan(struct kqueue *kq, int maxev
if (count == 0)
goto done;
- if (tsp != NULL && (tsp->tv_sec < 0 || !timespecisvalid(tsp))) {
- error = EINVAL;
- goto done;
- }
-
memset(, 0, sizeof(mstart));
memset(, 0, sizeof(mend));
@@ -875,19 +899,7 @@ retry:
goto done;
}
kq->kq_state |= KQ_SLEEP;
- if (tsp != NULL) {
- getnanouptime();
- nsecs = MIN(TIMESPEC_TO_NSEC(tsp), MAXTSLP);
- } else
- nsecs = INFSLP;
- error = tsleep_nsec(kq, PSOCK | PCATCH, "kqread", nsecs);
- if (tsp != NULL) {
- getnanouptime();
- timespecsub(, , );
- timespecsub(tsp, , tsp);
- if (tsp->tv_sec < 0)
- timespecclear(tsp);
- }
+ error = kqueue_sleep(kq, tsp);
splx(s);
if (error == 0 || error == EWOULDBLOCK)
goto retry;
EV_SET(2) shadows variable
The current form of EV_SET(2) declares a `kevp' variable. This can
cause to subtle bugs hard to discover if one uses a variable of the
same to retrieve events.
Diff below prefixes the locally declared variable by an underscore,
like it it done in FD_ZERO(), which should be good enough to prevent
surprises.
Is it the right way to correct such issue? How many underscores are
enough? Can the standards gurus tell me?
Index: sys/event.h
===
RCS file: /cvs/src/sys/sys/event.h,v
retrieving revision 1.33
diff -u -p -r1.33 event.h
--- sys/event.h 20 Feb 2020 16:56:52 - 1.33
+++ sys/event.h 1 Apr 2020 08:16:05 -
@@ -42,14 +42,14 @@
#define EVFILT_SYSCOUNT8
-#define EV_SET(kevp_, a, b, c, d, e, f) do { \
- struct kevent *kevp = (kevp_); \
- (kevp)->ident = (a);\
- (kevp)->filter = (b); \
- (kevp)->flags = (c);\
- (kevp)->fflags = (d); \
- (kevp)->data = (e); \
- (kevp)->udata = (f);\
+#define EV_SET(kevp, a, b, c, d, e, f) do {\
+ struct kevent *_kevp = (kevp); \
+ (_kevp)->ident = (a); \
+ (_kevp)->filter = (b); \
+ (_kevp)->flags = (c); \
+ (_kevp)->fflags = (d); \
+ (_kevp)->data = (e);\
+ (_kevp)->udata = (f); \
} while(0)
struct kevent {
Re: usb(4): use cacheable buffers for data transfers (massive speedup)
> Date: Wed, 1 Apr 2020 09:40:05 +0200
> From: Patrick Wildt
>
> On Wed, Apr 01, 2020 at 09:22:07AM +0200, Patrick Wildt wrote:
> > On Wed, Apr 01, 2020 at 04:47:10PM +1100, Jonathan Gray wrote:
> > > On Wed, Apr 01, 2020 at 12:58:23PM +1100, Jonathan Gray wrote:
> > > > On Wed, Mar 18, 2020 at 01:41:06PM +0100, Patrick Wildt wrote:
> > > > > On Wed, Mar 18, 2020 at 11:22:40AM +0100, Patrick Wildt wrote:
> > > > > > Hi,
> > > > > >
> > > > > > I've spent a few days investigating why USB ethernet adapters are so
> > > > > > horribly slow on my ARMs. Using dt(4) I realized that it was
> > > > > > spending
> > > > > > most of its time in memcpy. But, why? As it turns out, all USB
> > > > > > data
> > > > > > buffers are mapped COHERENT, which on some/most ARMs means uncached.
> > > > > > Using cached data buffers makes the performance rise from 20 mbit/s
> > > > > > to
> > > > > > 200 mbit/s. Quite a difference.
> > > > > >
> > > > > > sys/dev/usb/usb_mem.c:
> > > > > > error = bus_dmamem_map(tag, p->segs, p->nsegs, p->size,
> > > > > >>kaddr,
> > > > > > BUS_DMA_NOWAIT|BUS_DMA_COHERENT);
> > > > > >
> > > > > > On x86, COHERENT is essentially a no-op. On ARM, it depends on the
> > > > > > SoC.
> > > > > > Some SoCs have cache-coherent USB controllers, some don't. Mine
> > > > > > does
> > > > > > not, so mapping it COHERENT means uncached and thus slow.
> > > > > >
> > > > > > Why do we do that? Well, when the code was imported in 99, it was
> > > > > > already there. Since then we have gained infrastructure for DMA
> > > > > > syncs in the USB stack, which I think are proper.
> > > > > >
> > > > > > sys/dev/usb/usbdi.c - usbd_transfer() (before transfer)
> > > > > >
> > > > > > if (!usbd_xfer_isread(xfer)) {
> > > > > > if ((xfer->flags & USBD_NO_COPY) == 0)
> > > > > > memcpy(KERNADDR(>dmabuf, 0), xfer->buffer,
> > > > > > xfer->length);
> > > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > > BUS_DMASYNC_PREWRITE);
> > > > > > } else
> > > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > > BUS_DMASYNC_PREREAD);
> > > > > > err = pipe->methods->transfer(xfer);
> > > > > >
> > > > > > sys/dev/usb/usbdi.c - usb_transfer_complete() (after transfer)
> > > > > >
> > > > > > if (xfer->actlen != 0) {
> > > > > > if (usbd_xfer_isread(xfer)) {
> > > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > > BUS_DMASYNC_POSTREAD);
> > > > > > if (!(xfer->flags & USBD_NO_COPY))
> > > > > > memcpy(xfer->buffer,
> > > > > > KERNADDR(>dmabuf, 0),
> > > > > > xfer->actlen);
> > > > > > } else
> > > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > > BUS_DMASYNC_POSTWRITE);
> > > > > > }
> > > > > >
> > > > > > We cannot just remove COHERENT, since some drivers, like ehci(4),
> > > > > > use
> > > > > > the same backend to allocate their rings. And I can't vouch for
> > > > > > those
> > > > > > drivers' sanity.
> > > > > >
> > > > > > As a first step, I would like to go ahead with another solution,
> > > > > > which
> > > > > > is based on a diff from Marius Strobl, who added those syncs in the
> > > > > > first place. Essentially it splits the memory handling into
> > > > > > cacheable
> > > > > > and non-cacheable blocks. The USB data transfers and everyone who
> > > > > > uses
> > > > > > usbd_alloc_buffer() then use cacheable buffers, while code like
> > > > > > ehci(4)
> > > > > > still don't. This is a bit of a safer approach imho, since we don't
> > > > > > hurt the controller drivers, but speed up the data buffers.
> > > > > >
> > > > > > Once we have verified that there are no regressions, we can adjust
> > > > > > ehci(4) and the like, add proper syncs, make sure they still work as
> > > > > > well as before, and maybe then back this out again.
> > > > > >
> > > > > > Keep note that this is all a no-op on X86, but all the other archs
> > > > > > will
> > > > > > profit from this.
> > > > > >
> > > > > > ok?
> > > > > >
> > > > > > Patrick
> > > > >
> > > > > Update diff with inverted logic. kettenis@ argues that we should
> > > > > invert the logic, and those who need COHERENT memory should ask
> > > > > for that explicitly, since for bus_dmamem_map() it also needs to
> > > > > be passed explicitly. This also points out all those users that
> > > > > use usb_allocmem() internally, where we might want to have a look
> > > > > if COHERENT is actually needed or not, or if it can be refactored
> > > > > in another way.
> > > >
> > > > These commits broke usb on imx.6 with cubox:
> > > >
> > > > imxehci0 at simplebus3
> > > > usb0 at imxehci0: USB revision 2.0
> > > > usb0: root hub problem
> > > > imxehci1
Re: Prevent memory corruption by pipex_timer()
Hi, Sorry for my silence. ok yasuoka for the daemon part. On Wed, 1 Apr 2020 09:27:10 +0200 Martin Pieuchot wrote: > On 31/03/20(Tue) 23:16, Vitaliy Makkoveev wrote: >> On Tue, Mar 31, 2020 at 06:15:46PM +0200, Martin Pieuchot wrote: >> > [...] >> > Well better fix npppd(8), no? Not crashing the kernel is priority 1. >> I made patch for npppd(8) too. I include it to this email below, without >> starting new thread, ok? If ioctl(PIPEXASESSION) failed and error != >> ENXIO it means that pipex is enabled and session creation failed so down >> this connection. > > Thanks, I committed the kernel part. I'm waiting to see if other devs > want to comment on the daemon part. > >> > Then if the daemon has a bug, should the kernel work around it? >> In most common cases should :( > > That's an opinion. There's no true or false answer. Working around it > has obvious advantages but it doesn't make us fix existing bug and instead > force us to maintain the work around. > > It is argued that the "failing hard" model, as it is practised in OpenBSD > software development, has the advantage of resulting in simpler code because > every layer is responsible for handling errors and doesn't pile workaround. > > This bug is a nice example. Thanks for the report! If you could submit > your refactoring in a new thread, I'd love to look at it. >
Re: usb(4): use cacheable buffers for data transfers (massive speedup)
On Wed, Apr 01, 2020 at 09:22:07AM +0200, Patrick Wildt wrote:
> On Wed, Apr 01, 2020 at 04:47:10PM +1100, Jonathan Gray wrote:
> > On Wed, Apr 01, 2020 at 12:58:23PM +1100, Jonathan Gray wrote:
> > > On Wed, Mar 18, 2020 at 01:41:06PM +0100, Patrick Wildt wrote:
> > > > On Wed, Mar 18, 2020 at 11:22:40AM +0100, Patrick Wildt wrote:
> > > > > Hi,
> > > > >
> > > > > I've spent a few days investigating why USB ethernet adapters are so
> > > > > horribly slow on my ARMs. Using dt(4) I realized that it was spending
> > > > > most of its time in memcpy. But, why? As it turns out, all USB data
> > > > > buffers are mapped COHERENT, which on some/most ARMs means uncached.
> > > > > Using cached data buffers makes the performance rise from 20 mbit/s to
> > > > > 200 mbit/s. Quite a difference.
> > > > >
> > > > > sys/dev/usb/usb_mem.c:
> > > > > error = bus_dmamem_map(tag, p->segs, p->nsegs, p->size,
> > > > > >kaddr,
> > > > > BUS_DMA_NOWAIT|BUS_DMA_COHERENT);
> > > > >
> > > > > On x86, COHERENT is essentially a no-op. On ARM, it depends on the
> > > > > SoC.
> > > > > Some SoCs have cache-coherent USB controllers, some don't. Mine does
> > > > > not, so mapping it COHERENT means uncached and thus slow.
> > > > >
> > > > > Why do we do that? Well, when the code was imported in 99, it was
> > > > > already there. Since then we have gained infrastructure for DMA
> > > > > syncs in the USB stack, which I think are proper.
> > > > >
> > > > > sys/dev/usb/usbdi.c - usbd_transfer() (before transfer)
> > > > >
> > > > > if (!usbd_xfer_isread(xfer)) {
> > > > > if ((xfer->flags & USBD_NO_COPY) == 0)
> > > > > memcpy(KERNADDR(>dmabuf, 0), xfer->buffer,
> > > > > xfer->length);
> > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > BUS_DMASYNC_PREWRITE);
> > > > > } else
> > > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > > BUS_DMASYNC_PREREAD);
> > > > > err = pipe->methods->transfer(xfer);
> > > > >
> > > > > sys/dev/usb/usbdi.c - usb_transfer_complete() (after transfer)
> > > > >
> > > > > if (xfer->actlen != 0) {
> > > > > if (usbd_xfer_isread(xfer)) {
> > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > BUS_DMASYNC_POSTREAD);
> > > > > if (!(xfer->flags & USBD_NO_COPY))
> > > > > memcpy(xfer->buffer,
> > > > > KERNADDR(>dmabuf, 0),
> > > > > xfer->actlen);
> > > > > } else
> > > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > > BUS_DMASYNC_POSTWRITE);
> > > > > }
> > > > >
> > > > > We cannot just remove COHERENT, since some drivers, like ehci(4), use
> > > > > the same backend to allocate their rings. And I can't vouch for those
> > > > > drivers' sanity.
> > > > >
> > > > > As a first step, I would like to go ahead with another solution, which
> > > > > is based on a diff from Marius Strobl, who added those syncs in the
> > > > > first place. Essentially it splits the memory handling into cacheable
> > > > > and non-cacheable blocks. The USB data transfers and everyone who
> > > > > uses
> > > > > usbd_alloc_buffer() then use cacheable buffers, while code like
> > > > > ehci(4)
> > > > > still don't. This is a bit of a safer approach imho, since we don't
> > > > > hurt the controller drivers, but speed up the data buffers.
> > > > >
> > > > > Once we have verified that there are no regressions, we can adjust
> > > > > ehci(4) and the like, add proper syncs, make sure they still work as
> > > > > well as before, and maybe then back this out again.
> > > > >
> > > > > Keep note that this is all a no-op on X86, but all the other archs
> > > > > will
> > > > > profit from this.
> > > > >
> > > > > ok?
> > > > >
> > > > > Patrick
> > > >
> > > > Update diff with inverted logic. kettenis@ argues that we should
> > > > invert the logic, and those who need COHERENT memory should ask
> > > > for that explicitly, since for bus_dmamem_map() it also needs to
> > > > be passed explicitly. This also points out all those users that
> > > > use usb_allocmem() internally, where we might want to have a look
> > > > if COHERENT is actually needed or not, or if it can be refactored
> > > > in another way.
> > >
> > > These commits broke usb on imx.6 with cubox:
> > >
> > > imxehci0 at simplebus3
> > > usb0 at imxehci0: USB revision 2.0
> > > usb0: root hub problem
> > > imxehci1 at simplebus3
> > > usb1 at imxehci1: USB revision 2.0
> > > usb1: root hub problem
> > > "usbmisc" at simplebus3 not configured
> >
> > pandaboard with omap4 (another cortex a9) also has broken usb with
> > the latest snapshot:
> >
> > omehci0 at simplebus0
> > usb0 at omehci0: USB revision 2.0
> > usb0: root
Re: Prevent memory corruption by pipex_timer()
On 31/03/20(Tue) 23:16, Vitaliy Makkoveev wrote: > On Tue, Mar 31, 2020 at 06:15:46PM +0200, Martin Pieuchot wrote: > > [...] > > Well better fix npppd(8), no? Not crashing the kernel is priority 1. > I made patch for npppd(8) too. I include it to this email below, without > starting new thread, ok? If ioctl(PIPEXASESSION) failed and error != > ENXIO it means that pipex is enabled and session creation failed so down > this connection. Thanks, I committed the kernel part. I'm waiting to see if other devs want to comment on the daemon part. > > Then if the daemon has a bug, should the kernel work around it? > In most common cases should :( That's an opinion. There's no true or false answer. Working around it has obvious advantages but it doesn't make us fix existing bug and instead force us to maintain the work around. It is argued that the "failing hard" model, as it is practised in OpenBSD software development, has the advantage of resulting in simpler code because every layer is responsible for handling errors and doesn't pile workaround. This bug is a nice example. Thanks for the report! If you could submit your refactoring in a new thread, I'd love to look at it.
Re: usb(4): use cacheable buffers for data transfers (massive speedup)
On Wed, Apr 01, 2020 at 04:47:10PM +1100, Jonathan Gray wrote:
> On Wed, Apr 01, 2020 at 12:58:23PM +1100, Jonathan Gray wrote:
> > On Wed, Mar 18, 2020 at 01:41:06PM +0100, Patrick Wildt wrote:
> > > On Wed, Mar 18, 2020 at 11:22:40AM +0100, Patrick Wildt wrote:
> > > > Hi,
> > > >
> > > > I've spent a few days investigating why USB ethernet adapters are so
> > > > horribly slow on my ARMs. Using dt(4) I realized that it was spending
> > > > most of its time in memcpy. But, why? As it turns out, all USB data
> > > > buffers are mapped COHERENT, which on some/most ARMs means uncached.
> > > > Using cached data buffers makes the performance rise from 20 mbit/s to
> > > > 200 mbit/s. Quite a difference.
> > > >
> > > > sys/dev/usb/usb_mem.c:
> > > > error = bus_dmamem_map(tag, p->segs, p->nsegs, p->size,
> > > >>kaddr,
> > > > BUS_DMA_NOWAIT|BUS_DMA_COHERENT);
> > > >
> > > > On x86, COHERENT is essentially a no-op. On ARM, it depends on the SoC.
> > > > Some SoCs have cache-coherent USB controllers, some don't. Mine does
> > > > not, so mapping it COHERENT means uncached and thus slow.
> > > >
> > > > Why do we do that? Well, when the code was imported in 99, it was
> > > > already there. Since then we have gained infrastructure for DMA
> > > > syncs in the USB stack, which I think are proper.
> > > >
> > > > sys/dev/usb/usbdi.c - usbd_transfer() (before transfer)
> > > >
> > > > if (!usbd_xfer_isread(xfer)) {
> > > > if ((xfer->flags & USBD_NO_COPY) == 0)
> > > > memcpy(KERNADDR(>dmabuf, 0), xfer->buffer,
> > > > xfer->length);
> > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > BUS_DMASYNC_PREWRITE);
> > > > } else
> > > > usb_syncmem(>dmabuf, 0, xfer->length,
> > > > BUS_DMASYNC_PREREAD);
> > > > err = pipe->methods->transfer(xfer);
> > > >
> > > > sys/dev/usb/usbdi.c - usb_transfer_complete() (after transfer)
> > > >
> > > > if (xfer->actlen != 0) {
> > > > if (usbd_xfer_isread(xfer)) {
> > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > BUS_DMASYNC_POSTREAD);
> > > > if (!(xfer->flags & USBD_NO_COPY))
> > > > memcpy(xfer->buffer,
> > > > KERNADDR(>dmabuf, 0),
> > > > xfer->actlen);
> > > > } else
> > > > usb_syncmem(>dmabuf, 0, xfer->actlen,
> > > > BUS_DMASYNC_POSTWRITE);
> > > > }
> > > >
> > > > We cannot just remove COHERENT, since some drivers, like ehci(4), use
> > > > the same backend to allocate their rings. And I can't vouch for those
> > > > drivers' sanity.
> > > >
> > > > As a first step, I would like to go ahead with another solution, which
> > > > is based on a diff from Marius Strobl, who added those syncs in the
> > > > first place. Essentially it splits the memory handling into cacheable
> > > > and non-cacheable blocks. The USB data transfers and everyone who uses
> > > > usbd_alloc_buffer() then use cacheable buffers, while code like ehci(4)
> > > > still don't. This is a bit of a safer approach imho, since we don't
> > > > hurt the controller drivers, but speed up the data buffers.
> > > >
> > > > Once we have verified that there are no regressions, we can adjust
> > > > ehci(4) and the like, add proper syncs, make sure they still work as
> > > > well as before, and maybe then back this out again.
> > > >
> > > > Keep note that this is all a no-op on X86, but all the other archs will
> > > > profit from this.
> > > >
> > > > ok?
> > > >
> > > > Patrick
> > >
> > > Update diff with inverted logic. kettenis@ argues that we should
> > > invert the logic, and those who need COHERENT memory should ask
> > > for that explicitly, since for bus_dmamem_map() it also needs to
> > > be passed explicitly. This also points out all those users that
> > > use usb_allocmem() internally, where we might want to have a look
> > > if COHERENT is actually needed or not, or if it can be refactored
> > > in another way.
> >
> > These commits broke usb on imx.6 with cubox:
> >
> > imxehci0 at simplebus3
> > usb0 at imxehci0: USB revision 2.0
> > usb0: root hub problem
> > imxehci1 at simplebus3
> > usb1 at imxehci1: USB revision 2.0
> > usb1: root hub problem
> > "usbmisc" at simplebus3 not configured
>
> pandaboard with omap4 (another cortex a9) also has broken usb with
> the latest snapshot:
>
> omehci0 at simplebus0
> usb0 at omehci0: USB revision 2.0
> usb0: root hub problem
I think I know what it is. When we enqueue a request for the root
hub, the buffer, for which the USB subsystem allocates a DMA buffer,
is filled not by an external device, but by our driver.
Now on completion of that request, since it's doing a READ
