Re: wg(4): encapsulated transport checksums
On Tue, 30 Jun 2020 20:40:10 -0600 "Theo de Raadt" wrote: > Matt Dunwoodie wrote: > > > Depends on your definition of significant, I've seen 1-3% throughput > > improvement without the patch. > > > Real networks require statistics, which you want to throw away. > > > Overall, it is still debatable whether to skip the IPv4 checksum as > > modern crypto certainly offers better integrity checks. However, the > > primary motivator for skipping the integrity checks is performance, > > and the performance isn't severely impacted. Additionally, I can > > sympathise with avoiding layer violations and bringing it inline > > with other tunnels in this case. > > If it is debatable, why don't you debate it? I don't see a debate. > > Let me debate it. > > The issue is not about integrity checks being needed, but about > integrity check counters -- such counters are used to short-cut > procedures during network diagostic failures in multi-configuration > systems. > > If a higher-level network overlay skips that counter updates for > lower-levels, the counters are incorrect, now how do you diagnose > quickly? Well, you don't. Before going any further, I should clarify that the outer packet checksums are already checked (with or without this patch) in ip_input_if and udp_input when being received on the lower-level interface. Therefore, any lower-level link layer corruption to the outer packet will be caught, dropped and counters incremented before being passed to wg_input. Does that change any of your following points? > It sounds like the overlay is being chosen and relevant as more > important than the underlay. Sorry to burst your bubble, but the > overlay will never be the whole internet. The underlay will persist > for a long time, and the underlay will see errors. But the counters > indicating those erors will be *incoherent*. > > To me, it seems your path leads to the inablity to diagnose underlying > issues correctly and quickly > > Are underlying issues suddenly absent, or rare enough, they don't need > quick diagnosis? > > Or do (all) overlay technologies now provide enough information > access to make evaluation of underlying failures easy? > > For those questions, in my experience, I don't think reality provides > easy paths yet. > > As I said, argue it from a non-wg diagnosis model. If the argument is > not convincing, we have to do the obvious right thing, even if it > costs a small amount. > > Honestly, i don't understand how you ended in the position you are. Now, the "debatability" is about whether we want to check the inner packet IPv4 checksum after successful decryption (not about counters). The story may be different if you have any cases to add to the three Jason sent through earlier. I said "debatable" because I still think both sides are vaild, however when I weigh up the arguments I see applying the patch as the right thing to do. - Matt
Re: wg(4): encapsulated transport checksums
Matt Dunwoodie wrote: > Depends on your definition of significant, I've seen 1-3% throughput > improvement without the patch. > Real networks require statistics, which you want to throw away. > Overall, it is still debatable whether to skip the IPv4 checksum as > modern crypto certainly offers better integrity checks. However, the > primary motivator for skipping the integrity checks is performance, and > the performance isn't severely impacted. Additionally, I can sympathise > with avoiding layer violations and bringing it inline with other > tunnels in this case. If it is debatable, why don't you debate it? I don't see a debate. Let me debate it. The issue is not about integrity checks being needed, but about integrity check counters -- such counters are used to short-cut procedures during network diagostic failures in multi-configuration systems. If a higher-level network overlay skips that counter updates for lower-levels, the counters are incorrect, now how do you diagnose quickly? Well, you don't. It sounds like the overlay is being chosen and relevant as more important than the underlay. Sorry to burst your bubble, but the overlay will never be the whole internet. The underlay will persist for a long time, and the underlay will see errors. But the counters indicating those erors will be *incoherent*. To me, it seems your path leads to the inablity to diagnose underlying issues correctly and quickly Are underlying issues suddenly absent, or rare enough, they don't need quick diagnosis? Or do (all) overlay technologies now provide enough information access to make evaluation of underlying failures easy? For those questions, in my experience, I don't think reality provides easy paths yet. As I said, argue it from a non-wg diagnosis model. If the argument is not convincing, we have to do the obvious right thing, even if it costs a small amount. Honestly, i don't understand how you ended in the position you are.
Re: wg(4): encapsulated transport checksums
On Tue, 30 Jun 2020 09:22:18 +1200 (NZST) richard.n.proc...@gmail.com wrote: > Hi Jason, > > On 27/06/2020, at 10:09 PM, Jason A. Donenfeld > wrote: > > [...] I thought I'd lay out my understanding of the matter, > > and you can let me know whether or not this corresponds with what > > you had in mind: > > [...] > > My main concern is the end-to-end TCP or UDP transport checksum of the > tunneled (or inner, or encapsulated) packet. Your argument seems to > concern instead the per-hop IPv4 header checksum (which is also > interesting to look at, but first things first). > > As I read it, wg_decap() tells the stack to ignore the transport > checksum of the tunneled packet, and I think this is incorrect for > the following reason: the packet may have been corrupted outside of > the tunnel, because the tunnel needn't cover the entire path the > packet took through the network. > > So a tunneled packet may be corrupt, and one addressed to the tunnel > endpoint will be delivered with its end-to-end transport checksum > unverified. Higher layers may receive corrupt data as as result.[*] > > (Routers, as intermediate network elements, are under no obligation to > check end-to-end transport checksums. It isn't their job.) > > One might argue (I do not) that this isn't a concern, because > encryption these days is ubiquitous and comes with far stronger > integrity checks. Nonetheless, even here transport checksums remain > relevant for two reasons. Firsly, because networks remain unreliable: > I see non-zero numbers of failed checksums on my systems. And, > secondly, because higher layers require a reliable transport: TLS for > instance will drop the connection when the MAC check fails[1]. > > Let's turn to the per-hop IPv4 checksum, which wg_decap() also tells > the stack to ignore. I suspect the cost of verifying that checksum -- > 20 bytes on a hot cache line -- is negligible both absolutely and > relative to the cost of decrypting the packet. And as the > optimisation appears nowhere else in the tree that I can find, > removing it would make wg(4) no worse than the other tunnels. Do you > have evidence that the cost is in fact significant? Depends on your definition of significant, I've seen 1-3% throughput improvement without the patch. Without patch: --- 10.0.0.2 tcpbench statistics --- 4455088992 bytes sent over 121.001 seconds bandwidth min/avg/max/std-dev = 122.940/294.737/324.080/30.782 Mbps With patch: --- 10.0.0.2 tcpbench statistics --- 4366145736 bytes sent over 120.999 seconds bandwidth min/avg/max/std-dev = 123.908/288.718/329.770/31.518 Mbps > Further, as Theo pointed out, the link layer has in any case no > business with the IPv4 checksum, being part of the network layer. > Layer violations are problematic for at least two reasons. Firstly, > they constrict the design space. For instance, I suppose the IPv4 > checksum needn't be per-hop. It might be updated incrementally, > increasing its scope. But this would be nullified by any link layer > that, acting on a false assumption about the layer above it, told the > stack to ignore that layer's checksum. Secondly, layer violations, an > optimisation, which almost by definition rely on additional premises, > burden the correctness obligation and so trade less work for the > computer in narrow circumstances for (often much) more work for the > fallible human to show that the process remains sound. > > On that balance I now think that skipping the IPv4 check a false > economy, too. Hopefully I have managed to make my reasons clearer > this time around, please let me know if not, or if you disagree. > > See updated patch below, looking for oks to commit that. Overall, it is still debatable whether to skip the IPv4 checksum as modern crypto certainly offers better integrity checks. However, the primary motivator for skipping the integrity checks is performance, and the performance isn't severely impacted. Additionally, I can sympathise with avoiding layer violations and bringing it inline with other tunnels in this case. I'm happy for the following patch to be committed. - Matt > cheers, > Richard. > > [*] By way of background, I understand the transport checksum was > added to the early ARPANET primarily to guard against corruption > inside its routers[0], after a router fault which "[brought] the > network to its knees". In other words, end-to-end checks are > necessary to catch faults in the routers; link-level checks aren't > enough. > > More generally, transport checksums set a lower bound on the > reliability of the end-to-end transport, which decouples it from the > reliability of the underlying network. > > [0] Kleinrock "Queuing Systems, Volume 2: Computer Applications", > John Wiley and Sons (1976), p441 > > [1] RFC5246 TLS v1.2 (2008), section 7.2.2. > > TLS's requirement is reasonable: it would otherwise be obliged to > reimplement for itself retransmission, which would drag with it other > transport layer
Re: fix races in if_clone_create()
On Tue, Jun 30, 2020 at 03:48:22PM +0300, Vitaliy Makkoveev wrote:
> On Tue, Jun 30, 2020 at 12:08:03PM +0200, Martin Pieuchot wrote:
> > On 29/06/20(Mon) 11:59, Vitaliy Makkoveev wrote:
> > > [...]
> > > I reworked tool for reproduce. Now I avoided fork()/exec() route and it
> > > takes couple of minutes to take panic on 4 cores. Also some screenshots
> > > attached.
> >
> > Setting kern.pool_debug=2 makes the race reproducible in seconds.
Unfortunately you will catch splassert() caused by kern/sched_bsd.c:304.
malloc() will call yield() while we are holding NET_LOCK(). I attached
screenshot with splassertion to this mail.
> >
> > Could you turn this test into something committable in regress/? We can
> > link it to the build once a fix is committed.
> >
>
> We have 3 races with cloned interfaces:
> 1. if_clone_create() vs if_clone_create()
> 2. if_clone_destroy() vs if_clone_destroy()
> 3. if_clone_destroy() vs the rest of stack
>
> It makes sences to commit unified test to regress/, so I suggest to wait
> a little.
The another solution.
Diff below introduces per-`ifc' serialization for if_clone_create() and
if_clone_destroy(). There is no index bitmap anymore.
Diff fixes the following races:
1. if_clone_create() vs if_clone_create()
2. if_clone_destroy() vs if_clone_destroy()
`ifc_create' will go the same lock path for each cloner, and
`ifc_destroy' will go this path but in reverse order. It seems
reasonable to allow simultaneous create/destroy for different cloners
but since different instances of one cloner will block each other it's
no reason have parallelism here.
Updated test tool
cut begin
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
static struct ifreq ifr;
static void *clone_create(void *arg)
{
int s;
if((s=socket(AF_INET, SOCK_DGRAM, 0))<0)
err(1, "socket()");
while(1){
if(ioctl(s, SIOCIFCREATE, )<0)
if(errno==EINVAL)
exit(1);
}
return NULL;
}
static void *clone_destroy(void *arg)
{
int s;
if((s=socket(AF_INET, SOCK_DGRAM, 0))<0)
err(1, "socket()");
while(1){
if(ioctl(s, SIOCIFDESTROY, )<0)
if(errno==EINVAL)
exit(1);
}
return NULL;
}
int main(int argc, char *argv[])
{
pthread_t thr;
int i;
if(argc!=2){
fprintf(stderr, "usage: %s ifname\n", getprogname());
return 1;
}
if(getuid()!=0){
fprintf(stderr, "should be root\n");
return 1;
}
memset(, 0, sizeof(ifr));
strlcpy(ifr.ifr_name, argv[1], sizeof(ifr.ifr_name));
for(i=0; i<8*4; ++i){
if(pthread_create(, NULL, clone_create, NULL)!=0)
errx(1, "pthread_create(clone_create)");
if(pthread_create(, NULL, clone_destroy, NULL)!=0)
errx(1, "pthread_create(clone_destroy)");
}
select(0, NULL, NULL, NULL, NULL);
return 0;
}
cut end
Index: sys/net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.611
diff -u -p -r1.611 if.c
--- sys/net/if.c30 Jun 2020 09:31:38 - 1.611
+++ sys/net/if.c30 Jun 2020 20:41:50 -
@@ -155,6 +155,8 @@ int if_getgrouplist(caddr_t);
void if_linkstate(struct ifnet *);
void if_linkstate_task(void *);
+intif_clone_lock(struct if_clone *);
+void if_clone_unlock(struct if_clone *);
intif_clone_list(struct if_clonereq *);
struct if_clone*if_clone_lookup(const char *, int *);
@@ -1244,27 +1246,35 @@ if_clone_create(const char *name, int rd
{
struct if_clone *ifc;
struct ifnet *ifp;
- int unit, ret;
+ int unit, error;
ifc = if_clone_lookup(name, );
if (ifc == NULL)
return (EINVAL);
- if (ifunit(name) != NULL)
- return (EEXIST);
+ error = if_clone_lock(ifc);
+ if (error != 0)
+ return (error);
+
+ if (ifunit(name) != NULL) {
+ error = (EEXIST);
+ goto unlock;
+ }
- ret = (*ifc->ifc_create)(ifc, unit);
+ error = (*ifc->ifc_create)(ifc, unit);
- if (ret != 0 || (ifp = ifunit(name)) == NULL)
- return (ret);
+ if (error != 0 || (ifp = ifunit(name)) == NULL)
+ goto unlock;
NET_LOCK();
if_addgroup(ifp, ifc->ifc_name);
if (rdomain != 0)
if_setrdomain(ifp, rdomain);
NET_UNLOCK();
+unlock:
+ if_clone_unlock(ifc);
- return (ret);
+ return (error);
}
/*
@@ -1275,18 +1285,26 @@ if_clone_destroy(const char *name)
{
struct if_clone *ifc;
sparc64: vdsk: add size to free(9) call
Without any later realloactions, size is taken from vdsk_dring_alloc(): struct vdisk_dring *vd; ... vd = malloc(sizeof(struct vdsk_dring), M_DEVBUF, M_NOWAIT); Tested on T4-2 guest domains, altough that of course just means that allocating rings keeps working as before and the free routines are not actually called: vdsk0 at cbus0 chan 0x2: ivec 0x4, 0x5 scsibus1 at vdsk0: 1 targets vdsk1 at cbus0 chan 0x3: ivec 0x6, 0x7 scsibus2 at vdsk1: 1 targets vdsk2 at cbus0 chan 0x4: ivec 0x8, 0x9 scsibus3 at vdsk2: 1 targets vdsk3 at cbus0 chan 0x5: ivec 0xa, 0xb scsibus4 at vdsk3: 1 targets Feedback? OK? diff --git a/sys/arch/sparc64/dev/vdsk.c b/sys/arch/sparc64/dev/vdsk.c index 169bd8399..358e6b40e 100644 --- a/sys/arch/sparc64/dev/vdsk.c +++ b/sys/arch/sparc64/dev/vdsk.c @@ -883,7 +883,7 @@ vdsk_dring_free(bus_dma_tag_t t, struct vdsk_dring *vd) bus_dmamem_unmap(t, (caddr_t)vd->vd_desc, size); bus_dmamem_free(t, >vd_seg, 1); bus_dmamap_destroy(t, vd->vd_map); - free(vd, M_DEVBUF, 0); + free(vd, M_DEVBUF, sizeof(*vd)); } void *
sparc64: vnet: add size to free(9) call
Without any later realloactions, size is taken from vnet_dring_alloc(): struct vnet_dring *vd; ... vd = malloc(sizeof(struct vnet_dring), M_DEVBUF, M_NOWAIT); Tested on T4-2 guest domains: vnet0 at cbus0 chan 0x6: ivec 0xc, 0xd, address 00:14:4f:f9:3f:34 vnet1 at cbus0 chan 0x7: ivec 0xe, 0xf, address 00:14:4f:fb:05:2d Feedback? OK? diff --git a/sys/arch/sparc64/dev/vnet.c b/sys/arch/sparc64/dev/vnet.c index 147caf1f2..97883df76 100644 --- a/sys/arch/sparc64/dev/vnet.c +++ b/sys/arch/sparc64/dev/vnet.c @@ -1535,5 +1535,5 @@ vnet_dring_free(bus_dma_tag_t t, struct vnet_dring *vd) bus_dmamem_unmap(t, (caddr_t)vd->vd_desc, size); bus_dmamem_free(t, >vd_seg, 1); bus_dmamap_destroy(t, vd->vd_map); - free(vd, M_DEVBUF, 0); + free(vd, M_DEVBUF, sizeof(*vd)); }
pipex(4): kill unused declaration
`udpcksum' declared but not used in net/pipex.c, so kill it Index: sys/net/pipex.c === RCS file: /cvs/src/sys/net/pipex.c,v retrieving revision 1.116 diff -u -p -r1.116 pipex.c --- sys/net/pipex.c 22 Jun 2020 09:38:15 - 1.116 +++ sys/net/pipex.c 30 Jun 2020 13:28:48 - @@ -104,9 +104,6 @@ struct mbuf_queue pipexoutq = MBUF_QUEUE /* borrow an mbuf pkthdr field */ #define ph_ppp_proto ether_vtag -/* from udp_usrreq.c */ -extern int udpcksum; - #ifdef PIPEX_DEBUG int pipex_debug = 0; /* systcl net.inet.ip.pipex_debug */ #endif
Re: fix races in if_clone_create()
On Tue, Jun 30, 2020 at 12:08:03PM +0200, Martin Pieuchot wrote:
> On 29/06/20(Mon) 11:59, Vitaliy Makkoveev wrote:
> > [...]
> > I reworked tool for reproduce. Now I avoided fork()/exec() route and it
> > takes couple of minutes to take panic on 4 cores. Also some screenshots
> > attached.
>
> Setting kern.pool_debug=2 makes the race reproducible in seconds.
>
> Could you turn this test into something committable in regress/? We can
> link it to the build once a fix is committed.
>
We have 3 races with cloned interfaces:
1. if_clone_create() vs if_clone_create()
2. if_clone_destroy() vs if_clone_destroy()
3. if_clone_destroy() vs the rest of stack
It makes sences to commit unified test to regress/, so I suggest to wait
a little.
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> >
> > static struct ifreq ifr;
> >
> > static void *clone_create(void *arg)
> > {
> > int s;
> >
> > if((s=socket(AF_INET, SOCK_DGRAM, 0))<0)
> > err(1, "socket()");
> > while(1){
> > if(ioctl(s, SIOCIFCREATE, )<0)
> > if(errno==EINVAL)
> > exit(1);
> > }
> >
> > return NULL;
> > }
> >
> > static void *clone_destroy(void *arg)
> > {
> > int s;
> >
> > if((s=socket(AF_INET, SOCK_DGRAM, 0))<0)
> > err(1, "socket()");
> > while(1){
> > if(ioctl(s, SIOCIFDESTROY, )<0)
> > if(errno==EINVAL)
> > exit(1);
> > }
> >
> > return NULL;
> > }
> >
> > int main(int argc, char *argv[])
> > {
> > pthread_t thr;
> > int i;
> >
> > if(argc!=2){
> > fprintf(stderr, "usage: %s ifname\n", getprogname());
> > return 1;
> > }
> >
> > if(getuid()!=0){
> > fprintf(stderr, "should be root\n");
> > return 1;
> > }
> >
> > memset(, 0, sizeof(ifr));
> > strlcpy(ifr.ifr_name, argv[1], sizeof(ifr.ifr_name));
> >
> > for(i=0; i<8*4; ++i){
> > if(pthread_create(, NULL, clone_create, NULL)!=0)
> > errx(1, "pthread_create(clone_create)");
> > }
> >
> > clone_destroy(NULL);
> >
> > return 0;
> > }
> >
> > cut end
>
>
>
>
>
Re: pipex(4): kill unused declaration
On Tue, Jun 30, 2020 at 04:30:24PM +0300, Vitaliy Makkoveev wrote: > `udpcksum' declared but not used in net/pipex.c, so kill it OK claudio@ > Index: sys/net/pipex.c > === > RCS file: /cvs/src/sys/net/pipex.c,v > retrieving revision 1.116 > diff -u -p -r1.116 pipex.c > --- sys/net/pipex.c 22 Jun 2020 09:38:15 - 1.116 > +++ sys/net/pipex.c 30 Jun 2020 13:28:48 - > @@ -104,9 +104,6 @@ struct mbuf_queue pipexoutq = MBUF_QUEUE > /* borrow an mbuf pkthdr field */ > #define ph_ppp_proto ether_vtag > > -/* from udp_usrreq.c */ > -extern int udpcksum; > - > #ifdef PIPEX_DEBUG > int pipex_debug = 0; /* systcl net.inet.ip.pipex_debug */ > #endif > -- :wq Claudio
Re: [PATCH] usr.sbin/rpki-client: remove -f (force) option
OK kn with the manual's synopsis (`man -h rpki-client') updated as well.
Re: [PATCH] usr.sbin/rpki-client: remove -f (force) option
On Tue, Jun 30, 2020 at 10:33:21AM +, Job Snijders wrote:
> Remove rpki-client's -f command line option
>
> I haven't come across a use case that requires tricking the software
> into accepting out-of-date manifests. Anyone using -f? I think this is a
> leftover from the initial debugging era.
>
> OK?
Agreed. I think the last time I used this was in Elk Lakes during
development. I would not encurage anyone to use -f in production.
OK claudio@
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.30
> diff -u -p -r1.30 extern.h
> --- extern.h 24 Jun 2020 14:39:21 - 1.30
> +++ extern.h 30 Jun 2020 10:21:04 -
> @@ -289,7 +289,7 @@ struct cert *cert_read(int);
>
> void mft_buffer(char **, size_t *, size_t *, const struct mft *);
> void mft_free(struct mft *);
> -struct mft *mft_parse(X509 **, const char *, int);
> +struct mft *mft_parse(X509 **, const char *);
> int mft_check(const char *, struct mft *);
> struct mft *mft_read(int);
>
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 main.c
> --- main.c24 Jun 2020 14:39:21 - 1.71
> +++ main.c30 Jun 2020 10:21:05 -
> @@ -148,7 +148,7 @@ struct filepath_tree fpt = RB_INITIALIZ
> /*
> * Mark that our subprocesses will never return.
> */
> -static void proc_parser(int, int) __attribute__((noreturn));
> +static void proc_parser(int) __attribute__((noreturn));
> static void proc_rsync(char *, char *, int, int)
> __attribute__((noreturn));
> static void build_chain(const struct auth *, STACK_OF(X509) **);
> @@ -892,8 +892,8 @@ proc_parser_roa(struct entity *entp,
> * Return the mft on success or NULL on failure.
> */
> static struct mft *
> -proc_parser_mft(struct entity *entp, int force, X509_STORE *store,
> -X509_STORE_CTX *ctx, struct auth_tree *auths, struct crl_tree *crlt)
> +proc_parser_mft(struct entity *entp, X509_STORE *store, X509_STORE_CTX *ctx,
> + struct auth_tree *auths, struct crl_tree *crlt)
> {
> struct mft *mft;
> X509*x509;
> @@ -902,7 +902,7 @@ proc_parser_mft(struct entity *entp, int
> STACK_OF(X509) *chain;
>
> assert(!entp->has_dgst);
> - if ((mft = mft_parse(, entp->uri, force)) == NULL)
> + if ((mft = mft_parse(, entp->uri)) == NULL)
> return NULL;
>
> a = valid_ski_aki(entp->uri, auths, mft->ski, mft->aki);
> @@ -1127,7 +1127,7 @@ build_crls(const struct auth *a, struct
> * The process will exit cleanly only when fd is closed.
> */
> static void
> -proc_parser(int fd, int force)
> +proc_parser(int fd)
> {
> struct tal *tal;
> struct cert *cert;
> @@ -1249,8 +1249,7 @@ proc_parser(int fd, int force)
>*/
> break;
> case RTYPE_MFT:
> - mft = proc_parser_mft(entp, force,
> - store, ctx, , );
> + mft = proc_parser_mft(entp, store, ctx, , );
> c = (mft != NULL);
> io_simple_buffer(, , , , sizeof(int));
> if (mft != NULL)
> @@ -1500,8 +1499,7 @@ int
> main(int argc, char *argv[])
> {
> int rc = 1, c, proc, st, rsync,
> - fl = SOCK_STREAM | SOCK_CLOEXEC, noop = 0,
> - force = 0;
> + fl = SOCK_STREAM | SOCK_CLOEXEC, noop = 0;
> size_t i, j, eid = 1, outsz = 0, talsz = 0;
> pid_tprocpid, rsyncpid;
> int fd[2];
> @@ -1539,7 +1537,7 @@ main(int argc, char *argv[])
> if (pledge("stdio rpath wpath cpath fattr proc exec unveil", NULL) ==
> -1)
> err(1, "pledge");
>
> - while ((c = getopt(argc, argv, "b:Bcd:e:fjnot:T:v")) != -1)
> + while ((c = getopt(argc, argv, "b:Bcd:e:jnot:T:v")) != -1)
> switch (c) {
> case 'b':
> bind_addr = optarg;
> @@ -1556,9 +1554,6 @@ main(int argc, char *argv[])
> case 'e':
> rsync_prog = optarg;
> break;
> - case 'f':
> - force = 1;
> - break;
> case 'j':
> outformats |= FORMAT_JSON;
> break;
> @@ -1634,7 +1629,7 @@ main(int argc, char *argv[])
> err(1, "%s: unveil", cachedir);
> if (pledge("stdio rpath", NULL) == -1)
> err(1, "pledge");
> - proc_parser(fd[0], force);
> + proc_parser(fd[0]);
> /* NOTREACHED */
> }
>
> @@ -1826,7 +1821,7 @@ main(int argc, char
[PATCH] usr.sbin/rpki-client: remove -f (force) option
Remove rpki-client's -f command line option
I haven't come across a use case that requires tricking the software
into accepting out-of-date manifests. Anyone using -f? I think this is a
leftover from the initial debugging era.
OK?
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.30
diff -u -p -r1.30 extern.h
--- extern.h24 Jun 2020 14:39:21 - 1.30
+++ extern.h30 Jun 2020 10:21:04 -
@@ -289,7 +289,7 @@ struct cert *cert_read(int);
voidmft_buffer(char **, size_t *, size_t *, const struct mft *);
voidmft_free(struct mft *);
-struct mft *mft_parse(X509 **, const char *, int);
+struct mft *mft_parse(X509 **, const char *);
int mft_check(const char *, struct mft *);
struct mft *mft_read(int);
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.71
diff -u -p -r1.71 main.c
--- main.c 24 Jun 2020 14:39:21 - 1.71
+++ main.c 30 Jun 2020 10:21:05 -
@@ -148,7 +148,7 @@ struct filepath_tree fpt = RB_INITIALIZ
/*
* Mark that our subprocesses will never return.
*/
-static voidproc_parser(int, int) __attribute__((noreturn));
+static voidproc_parser(int) __attribute__((noreturn));
static voidproc_rsync(char *, char *, int, int)
__attribute__((noreturn));
static voidbuild_chain(const struct auth *, STACK_OF(X509) **);
@@ -892,8 +892,8 @@ proc_parser_roa(struct entity *entp,
* Return the mft on success or NULL on failure.
*/
static struct mft *
-proc_parser_mft(struct entity *entp, int force, X509_STORE *store,
-X509_STORE_CTX *ctx, struct auth_tree *auths, struct crl_tree *crlt)
+proc_parser_mft(struct entity *entp, X509_STORE *store, X509_STORE_CTX *ctx,
+ struct auth_tree *auths, struct crl_tree *crlt)
{
struct mft *mft;
X509*x509;
@@ -902,7 +902,7 @@ proc_parser_mft(struct entity *entp, int
STACK_OF(X509) *chain;
assert(!entp->has_dgst);
- if ((mft = mft_parse(, entp->uri, force)) == NULL)
+ if ((mft = mft_parse(, entp->uri)) == NULL)
return NULL;
a = valid_ski_aki(entp->uri, auths, mft->ski, mft->aki);
@@ -1127,7 +1127,7 @@ build_crls(const struct auth *a, struct
* The process will exit cleanly only when fd is closed.
*/
static void
-proc_parser(int fd, int force)
+proc_parser(int fd)
{
struct tal *tal;
struct cert *cert;
@@ -1249,8 +1249,7 @@ proc_parser(int fd, int force)
*/
break;
case RTYPE_MFT:
- mft = proc_parser_mft(entp, force,
- store, ctx, , );
+ mft = proc_parser_mft(entp, store, ctx, , );
c = (mft != NULL);
io_simple_buffer(, , , , sizeof(int));
if (mft != NULL)
@@ -1500,8 +1499,7 @@ int
main(int argc, char *argv[])
{
int rc = 1, c, proc, st, rsync,
-fl = SOCK_STREAM | SOCK_CLOEXEC, noop = 0,
-force = 0;
+fl = SOCK_STREAM | SOCK_CLOEXEC, noop = 0;
size_t i, j, eid = 1, outsz = 0, talsz = 0;
pid_tprocpid, rsyncpid;
int fd[2];
@@ -1539,7 +1537,7 @@ main(int argc, char *argv[])
if (pledge("stdio rpath wpath cpath fattr proc exec unveil", NULL) ==
-1)
err(1, "pledge");
- while ((c = getopt(argc, argv, "b:Bcd:e:fjnot:T:v")) != -1)
+ while ((c = getopt(argc, argv, "b:Bcd:e:jnot:T:v")) != -1)
switch (c) {
case 'b':
bind_addr = optarg;
@@ -1556,9 +1554,6 @@ main(int argc, char *argv[])
case 'e':
rsync_prog = optarg;
break;
- case 'f':
- force = 1;
- break;
case 'j':
outformats |= FORMAT_JSON;
break;
@@ -1634,7 +1629,7 @@ main(int argc, char *argv[])
err(1, "%s: unveil", cachedir);
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
- proc_parser(fd[0], force);
+ proc_parser(fd[0]);
/* NOTREACHED */
}
@@ -1826,7 +1821,7 @@ main(int argc, char *argv[])
usage:
fprintf(stderr,
- "usage: rpki-client [-Bcfjnov] [-b sourceaddr] [-d cachedir]"
+ "usage: rpki-client [-Bcjnov] [-b sourceaddr] [-d cachedir]"
" [-e rsync_prog]\n"
" [-T table] [-t tal] [outputdir]\n");
return 1;
Index: mft.c
Re: if_delgroup(): Add size to free(9) call
On Tue, Jun 30, 2020 at 04:11:59AM +0200, Klemens Nanni wrote:
> Interface groups are allocated as follows:
>
> struct ifg_group *
> if_creategroup(const char *groupname)
> {
> struct ifg_group*ifg;
>
> if ((ifg = malloc(sizeof(*ifg), M_TEMP, M_NOWAIT)) == NULL)
> return (NULL);
>
> ...
> }
>
> Since this allocation per group does not change, we can use the same
> size when freeing it in if_delgroup() accordingly.
>
> Tested on sparc64.
>
> Feedback? OK?
>
OK mvs
Re: fix races in if_clone_create()
On 29/06/20(Mon) 11:59, Vitaliy Makkoveev wrote:
> [...]
> I reworked tool for reproduce. Now I avoided fork()/exec() route and it
> takes couple of minutes to take panic on 4 cores. Also some screenshots
> attached.
Setting kern.pool_debug=2 makes the race reproducible in seconds.
Could you turn this test into something committable in regress/? We can
link it to the build once a fix is committed.
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
> #include
>
> static struct ifreq ifr;
>
> static void *clone_create(void *arg)
> {
> int s;
>
> if((s=socket(AF_INET, SOCK_DGRAM, 0))<0)
> err(1, "socket()");
> while(1){
> if(ioctl(s, SIOCIFCREATE, )<0)
> if(errno==EINVAL)
> exit(1);
> }
>
> return NULL;
> }
>
> static void *clone_destroy(void *arg)
> {
> int s;
>
> if((s=socket(AF_INET, SOCK_DGRAM, 0))<0)
> err(1, "socket()");
> while(1){
> if(ioctl(s, SIOCIFDESTROY, )<0)
> if(errno==EINVAL)
> exit(1);
> }
>
> return NULL;
> }
>
> int main(int argc, char *argv[])
> {
> pthread_t thr;
> int i;
>
> if(argc!=2){
> fprintf(stderr, "usage: %s ifname\n", getprogname());
> return 1;
> }
>
> if(getuid()!=0){
> fprintf(stderr, "should be root\n");
> return 1;
> }
>
> memset(, 0, sizeof(ifr));
> strlcpy(ifr.ifr_name, argv[1], sizeof(ifr.ifr_name));
>
> for(i=0; i<8*4; ++i){
> if(pthread_create(, NULL, clone_create, NULL)!=0)
> errx(1, "pthread_create(clone_create)");
> }
>
> clone_destroy(NULL);
>
> return 0;
> }
>
> cut end
Re: wg(4): encapsulated transport checksums
On Tue, Jun 30, 2020 at 09:22:18AM +1200, richard.n.proc...@gmail.com wrote: > Hi Jason, > > On 27/06/2020, at 10:09 PM, Jason A. Donenfeld wrote: > > [...] I thought I'd lay out my understanding of the matter, > > and you can let me know whether or not this corresponds with what you > > had in mind: > > [...] > > My main concern is the end-to-end TCP or UDP transport checksum of the > tunneled (or inner, or encapsulated) packet. Your argument seems to > concern instead the per-hop IPv4 header checksum (which is also > interesting to look at, but first things first). > > As I read it, wg_decap() tells the stack to ignore the transport checksum > of the tunneled packet, and I think this is incorrect for the following > reason: the packet may have been corrupted outside of the tunnel, because > the tunnel needn't cover the entire path the packet took through the > network. > > So a tunneled packet may be corrupt, and one addressed to the tunnel > endpoint will be delivered with its end-to-end transport checksum > unverified. Higher layers may receive corrupt data as as result.[*] > > (Routers, as intermediate network elements, are under no obligation to > check end-to-end transport checksums. It isn't their job.) > > One might argue (I do not) that this isn't a concern, because encryption > these days is ubiquitous and comes with far stronger integrity checks. > Nonetheless, even here transport checksums remain relevant for two > reasons. Firsly, because networks remain unreliable: I see non-zero > numbers of failed checksums on my systems. And, secondly, because higher > layers require a reliable transport: TLS for instance will drop the > connection when the MAC check fails[1]. > > Let's turn to the per-hop IPv4 checksum, which wg_decap() also tells the > stack to ignore. I suspect the cost of verifying that checksum -- 20 bytes > on a hot cache line -- is negligible both absolutely and relative to the > cost of decrypting the packet. And as the optimisation appears nowhere > else in the tree that I can find, removing it would make wg(4) no worse > than the other tunnels. Do you have evidence that the cost is in fact > significant? > > Further, as Theo pointed out, the link layer has in any case no business > with the IPv4 checksum, being part of the network layer. Layer violations > are problematic for at least two reasons. Firstly, they constrict the > design space. For instance, I suppose the IPv4 checksum needn't be > per-hop. It might be updated incrementally, increasing its scope. But this > would be nullified by any link layer that, acting on a false assumption > about the layer above it, told the stack to ignore that layer's checksum. > Secondly, layer violations, an optimisation, which almost by definition > rely on additional premises, burden the correctness obligation and so > trade less work for the computer in narrow circumstances for (often much) > more work for the fallible human to show that the process remains sound. > > On that balance I now think that skipping the IPv4 check a false economy, > too. Hopefully I have managed to make my reasons clearer this time around, > please let me know if not, or if you disagree. > > See updated patch below, looking for oks to commit that. > > cheers, > Richard. > > [*] By way of background, I understand the transport checksum was added to > the early ARPANET primarily to guard against corruption inside its > routers[0], after a router fault which "[brought] the network to its > knees". In other words, end-to-end checks are necessary to catch faults in > the routers; link-level checks aren't enough. > > More generally, transport checksums set a lower bound on the reliability > of the end-to-end transport, which decouples it from the reliability of > the underlying network. > > [0] Kleinrock "Queuing Systems, Volume 2: Computer Applications", > John Wiley and Sons (1976), p441 > > [1] RFC5246 TLS v1.2 (2008), section 7.2.2. > > TLS's requirement is reasonable: it would otherwise be obliged to > reimplement for itself retransmission, which would drag with it other > transport layer mechanisms like acknowledgements and (for performance) > sequence numbers and sliding windows. I agree with all of this and think that this is the right fix. OK claudio@ > Index: net/if_wg.c > === > RCS file: /cvs/src/sys/net/if_wg.c,v > retrieving revision 1.7 > diff -u -p -u -p -r1.7 if_wg.c > --- net/if_wg.c 23 Jun 2020 10:03:49 - 1.7 > +++ net/if_wg.c 28 Jun 2020 20:17:07 - > @@ -1660,14 +1660,8 @@ wg_decap(struct wg_softc *sc, struct mbu > goto error; > } > > - /* > - * We can mark incoming packet csum OK. We mark all flags OK > - * irrespective to the packet type. > - */ > - m->m_pkthdr.csum_flags |= (M_IPV4_CSUM_IN_OK | M_TCP_CSUM_IN_OK | > - M_UDP_CSUM_IN_OK |
