From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=fweKKiZM; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 13FB75A061E for ; Wed, 13 Nov 2024 09:18:14 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1731485894; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CboybZb0kL7EfWTdBsp6j9UbuUfLTVEk1aAFvAP7hIs=; b=fweKKiZMm0iE4im9I+u2fF5FmN7LjIVC9aNjqbyGJKTFtxz8Vn11P5A55K4itMRA0vU/3Y RzAHCONKmrCGbG0zBiCI+Hj9DXqEBff8+rm0+IlgWIyiyPhbWZ57OWVQev0nyBKvlAt+IQ P08+jFDJ6iXlmZvl5nfXtcBaTuqaFyk= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-245-5mDSB9OEMNGqHeGVYJmGkg-1; Wed, 13 Nov 2024 03:18:12 -0500 X-MC-Unique: 5mDSB9OEMNGqHeGVYJmGkg-1 X-Mimecast-MFC-AGG-ID: 5mDSB9OEMNGqHeGVYJmGkg Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-4315a0f25afso51439695e9.3 for ; Wed, 13 Nov 2024 00:18:12 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731485891; x=1732090691; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=CboybZb0kL7EfWTdBsp6j9UbuUfLTVEk1aAFvAP7hIs=; b=vMBrQjfIRAW4Pa2ObQsrP90rQ2lm56v3GBHJOuzioWgiLinJyYGxGf5DoL+DS6rNrc yJdy53ekqO+iOfdxFjFMXetLGF47o91aYJ3Tb2NAB5VXzt9e4R22irGNtry57sDMFVFZ jBAZOLTh6Y15aZEerjK2vwzDbrv1fnf1qQA4oN1TO0pjgCeZuAYS+MrGxkqncUjjJMUW fbFUSOYYUWMC+LVZYZ5Hh49DudEOyJfKcL4XSA03wK4GbYiEV2agQeXwX5/S848VfFA6 T1PArX61bpQlsdYzFpIYAXBSc30caWQhZQeWuBwwudPYSVNXYdx/N0LC2MovnxrdBF5T bTcQ== X-Gm-Message-State: AOJu0YyYQ65j0K+Tg2sWAjqbU4MDaSwiB0vR7Hc+cl8WddxieAXnix+y UvAZ17ju3ehKB1MjxudKkc+qqsNbtBtvl7OcNHJNhnbJ8uDlQ7N9fLKJpOqJjaWD+muDxjJBIHL Uj5tOWCtup/tPUvpGSEsUwk/s7d+kzjZ7IhXoJ99Zrga8M2GdSijEeFHl2g== X-Received: by 2002:a5d:64cb:0:b0:37d:50f8:a7f4 with SMTP id ffacd0b85a97d-381f188c98bmr18578400f8f.52.1731485890619; Wed, 13 Nov 2024 00:18:10 -0800 (PST) X-Google-Smtp-Source: AGHT+IGIk8Udpy5W+LLcLHS7SD/tAb+We5epUmgRHOiwvCSghrXBAdQtASlnrn8yyYzcz5UKfOUeVA== X-Received: by 2002:a5d:64cb:0:b0:37d:50f8:a7f4 with SMTP id ffacd0b85a97d-381f188c98bmr18578372f8f.52.1731485890148; Wed, 13 Nov 2024 00:18:10 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-381ed9ec272sm17943732f8f.85.2024.11.13.00.18.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Nov 2024 00:18:09 -0800 (PST) Date: Wed, 13 Nov 2024 09:18:05 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 6/6] ndp: Send unsolicited Router Advertisements Message-ID: <20241113091805.6f6e2f6a@elisabeth> In-Reply-To: References: <20241112040618.1804081-1-david@gibson.dropbear.id.au> <20241112040618.1804081-7-david@gibson.dropbear.id.au> <20241113021416.418a338c@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: mrh5TmFa--hsSIvYfoY5QsVEI-RhRcL27r0INCUzrBQ_1731485891 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: DRJI3NIJQNMLKSM7M5N6UQ5I7NRK24ZK X-Message-ID-Hash: DRJI3NIJQNMLKSM7M5N6UQ5I7NRK24ZK X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, 13 Nov 2024 14:01:16 +1100 David Gibson wrote: > On Wed, Nov 13, 2024 at 02:14:16AM +0100, Stefano Brivio wrote: > > Kind of nits only, the whole series looks good to me otherwise: > > > > On Tue, 12 Nov 2024 15:06:18 +1100 > > David Gibson wrote: > > > > > Currently, our NDP implementation only sends Router Advertisements (RA) > > > when it receives a Router Solicitation (RS) from the guest. However, > > > RFC 4861 requires that we periodically send unsolicited RAs. > > > > > > Linux as a guest also requires this: it will send an RS when a link first > > > comes up, but the route it gets from this will have a finite lifetime (we > > > set this to 65535s, the maximum allowed, around 18 hours). When that > > > expires the guest will not send a new RS, but instead expects the route to > > > have been renewed (if still valid) by an unsolicited RA. > > > > > > Implement sending unsolicited RAs on a partially randomised timer, as > > > required by RFC 4861. The RFC also specifies that solicited RAs should > > > also be delayed, or even not omitted, if the next unsolicited RA is soon > > > > s/not// > > Fixed. > > > > enough. For now we don't do that, always sending an immediate RA in > > > response to an RS. We can get away with this because in our use cases > > > we expect to just have passt itself and the guest on the link, rather than > > > a large broadcast domain. > > > > > > Link: https://github.com/kubevirt/kubevirt/issues/13191 > > > Signed-off-by: David Gibson > > > --- > > > ip.h | 9 +++++++++ > > > ndp.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > > ndp.h | 3 +++ > > > passt.c | 3 +++ > > > 4 files changed, 56 insertions(+) > > > > > > diff --git a/ip.h b/ip.h > > > index b8d4a5b..0742612 100644 > > > --- a/ip.h > > > +++ b/ip.h > > > @@ -92,4 +92,13 @@ struct ipv6_opt_hdr { > > > > > > char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto, > > > size_t *dlen); > > > + > > > +/* IPv6 link-local all-nodes multicast adddress, ff02::1 */ > > > +static const struct in6_addr in6addr_ll_all_nodes = { > > > + .s6_addr = { > > > + 0xff, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, > > > + }, > > > +}; > > > + > > > #endif /* IP_H */ > > > diff --git a/ndp.c b/ndp.c > > > index c5fbccf..8c019df 100644 > > > --- a/ndp.c > > > +++ b/ndp.c > > > @@ -372,3 +372,44 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih, > > > > > > return 1; > > > } > > > + > > > +/* Default interval between unsolicited RAs (seconds) */ > > > +#define DEFAULT_MAX_RTR_ADV_INTERVAL 600 /* RFC 4861, 6.2.1 */ > > > + > > > +/* Minimum required interval between RAs (seconds) */ > > > +#define MIN_DELAY_BETWEEN_RAS 3 /* RFC 4861, 10 */ > > > > By the way, I think this is still all correct, as I quickly double > > checked it against RFC 8319. If you're not aware: see also sections 3 > > and 4 there. > > Ah, thanks for checking. I didn't think to look for a superseding RFC. > > > > + > > > +static time_t next_ra; > > > + > > > +/** > > > + * ndp_timer() - Send unsolicited NDP messages if necessary > > > + * @c: Execution context > > > + * @now: Current (monotonic) time > > > + */ > > > +void ndp_timer(const struct ctx *c, const struct timespec *now) > > > +{ > > > + time_t max_rtr_adv_interval = DEFAULT_MAX_RTR_ADV_INTERVAL; > > > + time_t min_rtr_adv_interval, interval; > > > + > > > + if (c->no_ra || now->tv_sec < next_ra) > > > + return; > > > + > > > + /* We must advertise before the route's lifetime expires */ > > > + max_rtr_adv_interval = MIN(max_rtr_adv_interval, RT_LIFETIME - 1); > > > + > > > + /* But we must not go smaller than the minimum delay */ > > > + max_rtr_adv_interval = MAX(max_rtr_adv_interval, MIN_DELAY_BETWEEN_RAS); > > > + > > > + /* RFC 4861, 6.2.1 */ > > > + min_rtr_adv_interval = MAX(max_rtr_adv_interval / 3, > > > + MIN_DELAY_BETWEEN_RAS); > > > + > > > + interval = min_rtr_adv_interval + > > > + random() % (max_rtr_adv_interval - min_rtr_adv_interval); > > > > So, I would have called srandom() before this, especially in the case > > we get, one day, two instances of passt advertising at the same > > time. > > Good point, I'll add that. > > > But Coverity is more annoying than I am and reports: > > > > /home/sbrivio/passt/ndp.c:408:3: > > Type: Calling risky function (DC.WEAK_CRYPTO) > > > > /home/sbrivio/passt/ndp.c:408:3: > > dont_call: "random" should not be used for security-related applications, because linear congruential algorithms are too easy to break. > > /home/sbrivio/passt/ndp.c:408:3: > > remediation: Use a compliant random number generator, such as "/dev/random" or "/dev/urandom" on Unix-like systems, and CNG (Cryptography API: Next Generation) on Windows. > > > > Of course it's all bogus but I would have a *slight* preference to get > > rid of this, by either picking a fixed interval deviation at the > > beginning with getrandom(), or using something like tcp_init_seq() > > modulo something << 600. > > I don't think the latter approach really works. Using the time as the > basic input means its still subject to two passt instances (say) > starting at the same time on the same link picking the same values and > keeping on sending their announcements in lockstep. Well, but the seeds would be different (I would still factor hash_secret in), so we would start out of sync right away, and we would use a fine-grained time resolution to pick a coarser one, which adds (enough) to the pseudorandomness. > Picking a random interval once is better. Still, I don't particularly > like deviating from the RFC's recommendations just to keep a fussy > tool happy. Oh, I thought it would still fit the recommendation, but now I read RFC 4861 6.2.4 again and it definitely doesn't. > > Alternatively, we could also keep /dev/random or /dev/urandom open but > > it looks totally overkill. At that point I'd rather keep random() > > here. > > Also a waste of /dev/random entropy, IMO. > > Surely there must be some way we can suppress the Coverity whinge. That adds more maintenance burden than having a warning at this point, so I'd rather not try to suppress anything. If the tcp_init_seq()-like thing works, I'd be happy, otherwise let's leave this like it is. -- Stefano