Re: [Dnsmasq-discuss] [PATCH] Re: RA-acquired address not marked as 'dynamic' with 2.82
Hey Iain,
On 07.09.20 13:22, Iain Lane wrote:
> The lifetimes are *forever* now, but the intention of that commit is
> that they were supposed to be one day (86400 seconds). I think maybe the
> intention of the commit was this (attached)?
Yes, that's the problem here. The variable time is initialized to
0x A.K.A. Infinity.
Before this patch, the if kicked in because (time > context->lease_time)
was clearly true with time being Infinity. As result the lease time was
reduced. The mentioned patch added the new condition (context->flags &
CONTEXT_SETLEASE) on top. As this evaluates to false when no explicit
lease time was set by the user, dnsmasq keeps Infinity and doesn't limit
anything while it should.
I attach a simple patch for this bug. It would be awesome if you could
try if this solves the problem. The idea is to fix the logic so that the
lease time is always set when the user explicitly wants to do this and
otherwise only limited when it is too large.
Best,
Dominik
>From ad2aeb6af449a3cbe8a222545c811fc593544f7c Mon Sep 17 00:00:00 2001
From: Dominik
Date: Mon, 7 Sep 2020 21:45:02 +0200
Subject: [PATCH] DHCPv6: Use the desired default when the user did not explicitly
set a lease time. This fixes 4d85e409cd2f4b0935d6ac5e8c72f6a151735d52 which
accidentally changed the default lease time to Inifinity instead of the desired
default of one day.
---
src/radv.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/radv.c b/src/radv.c
index 41df852..269d3c4 100644
--- a/src/radv.c
+++ b/src/radv.c
@@ -628,8 +628,9 @@ static int add_prefixes(struct in6_addr *local, int prefix,
/* find floor time, don't reduce below 3 * RA interval.
If the lease time has been left as default, don't
- use that as a floor. */
- if ((context->flags & CONTEXT_SETLEASE) &&
+ use that as a floor.
+ Always set lease time if requested to do so. */
+ if ((context->flags & CONTEXT_SETLEASE) ||
time > context->lease_time)
{
time = context->lease_time;
--
2.17.1
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Re: RA-acquired address not marked as 'dynamic' with 2.82
On Mon, Sep 07, 2020 at 07:37:10PM +0200, Geert Stappers wrote:
> I might understand the re-location of CONTEXT_SETLEASE
>
> I don't understand the change from '3 * param->adv_interval'
> to '2 * param->adv_interval'.
>
>
> And my actual message: the patch has been seen ...
How curious. Is there a vim keybinding to decrement a number that I
might have hit by mistake? (searched, yes, it's ctrl-x: TIL)
New one attached, thanks for the review!
Cheers,
--
Iain Lane [ i...@orangesquash.org.uk ]
Debian Developer [ la...@debian.org ]
Ubuntu Developer [ la...@ubuntu.com ]
From 91657a466cdf1d90d3afb64e47803e72cce5678b Mon Sep 17 00:00:00 2001
From: Iain Lane
Date: Mon, 7 Sep 2020 10:20:02 +0100
Subject: [PATCH] Make sure valid and preferred lifetimes always get set
In 4d85e409cd2f4b0935d6ac5e8c72f6a151735d52 we skipped setting the floor
time if we were using the default RA interval. The commit was a bit too
broad; it also caused the valid and preferred lifetimes to be skipped
too, meaning that they were set to infinite.
Adjust the check, so that we only apply the "are we using the default?"
check when calculating the floor; but still set up the `time` variable
because that is used later on as a ceiling for valid_lft and
preferred_lft.
---
src/radv.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/radv.c b/src/radv.c
index 41df852..19d7954 100644
--- a/src/radv.c
+++ b/src/radv.c
@@ -629,11 +629,11 @@ static int add_prefixes(struct in6_addr *local, int prefix,
/* find floor time, don't reduce below 3 * RA interval.
If the lease time has been left as default, don't
use that as a floor. */
- if ((context->flags & CONTEXT_SETLEASE) &&
- time > context->lease_time)
+ if (time > context->lease_time)
{
time = context->lease_time;
- if (time < ((unsigned int)(3 * param->adv_interval)))
+ if ((context->flags & CONTEXT_SETLEASE) &&
+ time < ((unsigned int)(3 * param->adv_interval)))
time = 3 * param->adv_interval;
}
--
2.27.0
signature.asc
Description: PGP signature
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] Re: RA-acquired address not marked as 'dynamic' with 2.82
On Mon, Sep 07, 2020 at 12:22:24PM +0100, Iain Lane wrote:
> >
> > The only related difference I can see between v2.81 and v2.82 seem to be
> > this one:
> > http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=4d85e409cd2f4b0935d6ac5e8c72f6a151735d52
> >
> > It's not clear to me when the kernel marks an address as "dynamic".
> > Changing the flooring of the lease time may or not have an effect here.
> > Would you be able to compile dnsmasq from source and check if this
> > behavior you observed can be triggered by going to 4d85e40 and then back
> > to its parent (2bd02d2)?
>
> Yeah, thanks, I bisected just now and it is this change:
> laney@groovy-vm:~/temp/dnsmasq$ git bisect log
> git bisect start
..
> # first bad commit: [4d85e409cd2f4b0935d6ac5e8c72f6a151735d52] Change default
> lease time for DHCPv6 to one day.
>
> Good to know. Actually, I suppose that means in my pasted output I left
> out the real bug, which is:
>
> inet6 fd42:d287:488a:d7e8:216:3eff:fecb:d41b/64 scope global mngtmpaddr
> noprefixroute
>valid_lft forever preferred_lft forever
>
> The lifetimes are *forever* now, but the intention of that commit is
> that they were supposed to be one day (86400 seconds). I think maybe the
> intention of the commit was this (attached)?
>
> Cheers,
> Iain Lane [ i...@orangesquash.org.uk ]
> From c1183528816f5d9d61a12c05ceeda5975f422b32 Mon Sep 17 00:00:00 2001
> From: Iain Lane
> Date: Mon, 7 Sep 2020 10:20:02 +0100
> Subject: [PATCH] Make sure valid and preferred lifetimes always get set
>
> In 4d85e409cd2f4b0935d6ac5e8c72f6a151735d52 we skipped setting the floor
> time if we were using the default RA interval. The commit was a bit too
> broad; it also caused the valid and preferred lifetimes to be skipped
> too, meaning that they were set to infinite.
>
> Adjust the check, so that we only apply the "are we using the default?"
> check when calculating the floor; but still set up the `time` variable
> because that is used later on as a ceiling for valid_lft and
> preferred_lft.
> ---
> src/radv.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/radv.c b/src/radv.c
> index 41df852..78edaab 100644
> --- a/src/radv.c
> +++ b/src/radv.c
> @@ -629,11 +629,11 @@ static int add_prefixes(struct in6_addr *local, int
> prefix,
> /* find floor time, don't reduce below 3 * RA interval.
> If the lease time has been left as default, don't
> use that as a floor. */
> - if ((context->flags & CONTEXT_SETLEASE) &&
> - time > context->lease_time)
> + if (time > context->lease_time)
> {
> time = context->lease_time;
> - if (time < ((unsigned int)(3 * param->adv_interval)))
^
three
> + if ((context->flags & CONTEXT_SETLEASE) &&
> + time < ((unsigned int)(2 * param->adv_interval)))
^
two
> time = 3 * param->adv_interval;
^
three
> }
>
I might understand the re-location of CONTEXT_SETLEASE
I don't understand the change from '3 * param->adv_interval'
to '2 * param->adv_interval'.
And my actual message: the patch has been seen ...
Groeten
Geert Stappers
--
Silence is hard to parse
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] RA-acquired address not marked as 'dynamic' with 2.82
On 04.09.20 11:46, Iain Lane wrote: > Hi, > > In Ubuntu we started seeing failures of NetworkManager's tests when we > updated to dnsmasq 2.82 (from the Debian package; thanks Simon for > maintaining this). Search for 'Regex didn't match' in: > > > https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-groovy/groovy/amd64/n/network-manager/20200903_200241_5f5fd@/log.gz > > and you can work out after a bit of squinting that it's because the > kernel is saying the address isn't 'dynamic' any more. > > I did a bit of manual testing in a VM (spawning LXD containers which get > IPs from dnsmasq running on the host) and managed to reproduce this > change: > > dnsmasq 2.81: > inet6 fd42:d287:488a:d7e8:216:3eff:fecb:d41b/64 scope global dynamic > mngtmpaddr noprefixroute > > dnsmasq 2.82: > inet6 fd42:d287:488a:d7e8:216:3eff:fecb:d41b/64 scope global mngtmpaddr > noprefixroute > > Was this intentional and is it actually a problem? i.e. I'm wondering if > we should update the tests to not check for 'dynamic', or if a fix in > dnsmasq is needed instead. Hey Iain, The only related difference I can see between v2.81 and v2.82 seem to be this one: http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=4d85e409cd2f4b0935d6ac5e8c72f6a151735d52 It's not clear to me when the kernel marks an address as "dynamic". Changing the flooring of the lease time may or not have an effect here. Would you be able to compile dnsmasq from source and check if this behavior you observed can be triggered by going to 4d85e40 and then back to its parent (2bd02d2)? Best, Dominik > > Cheers, > > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
[Dnsmasq-discuss] [PATCH] Re: RA-acquired address not marked as 'dynamic' with 2.82
Hi Dominik,
On Sun, Sep 06, 2020 at 11:30:46PM +0200, Dominik wrote:
> > dnsmasq 2.81:
> > inet6 fd42:d287:488a:d7e8:216:3eff:fecb:d41b/64 scope global dynamic
> > mngtmpaddr noprefixroute
> >
> > dnsmasq 2.82:
> > inet6 fd42:d287:488a:d7e8:216:3eff:fecb:d41b/64 scope global mngtmpaddr
> > noprefixroute
> >
> > Was this intentional and is it actually a problem? i.e. I'm wondering if
> > we should update the tests to not check for 'dynamic', or if a fix in
> > dnsmasq is needed instead.
>
> Hey Iain,
>
> The only related difference I can see between v2.81 and v2.82 seem to be
> this one:
> http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=4d85e409cd2f4b0935d6ac5e8c72f6a151735d52
>
> It's not clear to me when the kernel marks an address as "dynamic".
> Changing the flooring of the lease time may or not have an effect here.
> Would you be able to compile dnsmasq from source and check if this
> behavior you observed can be triggered by going to 4d85e40 and then back
> to its parent (2bd02d2)?
Yeah, thanks, I bisected just now and it is this change:
laney@groovy-vm:~/temp/dnsmasq$ git bisect log
git bisect start
# good: [7ddb99d251c3f5870c8c308a98bb8f283c831872] Debian changelog entry for
CVE-2019-14834
git bisect good 7ddb99d251c3f5870c8c308a98bb8f283c831872
# bad: [f60fea1fb0a288011f57a25dfb653b8f6f8b46b9] CHANGELOG: Fix three typoes.
git bisect bad f60fea1fb0a288011f57a25dfb653b8f6f8b46b9
# good: [49bdf1ead9046c5c554c18ff62fe6e6a9e8a880c] Handle listening on
duplicate addresses
git bisect good 49bdf1ead9046c5c554c18ff62fe6e6a9e8a880c
# good: [837e8f4eb550c688e8a83415c42a99c7bf9a4311] Remove runit support when
building debs for Ubuntu.
git bisect good 837e8f4eb550c688e8a83415c42a99c7bf9a4311
# good: [7e194a0a7d483932eb3f416b8f26131ade588acc] Apply floor of 60s to TTL of
DNSKEY and DS records in cache.
git bisect good 7e194a0a7d483932eb3f416b8f26131ade588acc
# bad: [4d85e409cd2f4b0935d6ac5e8c72f6a151735d52] Change default lease time for
DHCPv6 to one day.
git bisect bad 4d85e409cd2f4b0935d6ac5e8c72f6a151735d52
# good: [2bd02d2f595f1d45a8598a5fce85cfc3d414] Backdated CHANGELOG update.
git bisect good 2bd02d2f595f1d45a8598a5fce85cfc3d414
# first bad commit: [4d85e409cd2f4b0935d6ac5e8c72f6a151735d52] Change default
lease time for DHCPv6 to one day.
Good to know. Actually, I suppose that means in my pasted output I left
out the real bug, which is:
inet6 fd42:d287:488a:d7e8:216:3eff:fecb:d41b/64 scope global mngtmpaddr
noprefixroute
valid_lft forever preferred_lft forever
The lifetimes are *forever* now, but the intention of that commit is
that they were supposed to be one day (86400 seconds). I think maybe the
intention of the commit was this (attached)?
Cheers,
--
Iain Lane [ i...@orangesquash.org.uk ]
Debian Developer [ la...@debian.org ]
Ubuntu Developer [ la...@ubuntu.com ]
From c1183528816f5d9d61a12c05ceeda5975f422b32 Mon Sep 17 00:00:00 2001
From: Iain Lane
Date: Mon, 7 Sep 2020 10:20:02 +0100
Subject: [PATCH] Make sure valid and preferred lifetimes always get set
In 4d85e409cd2f4b0935d6ac5e8c72f6a151735d52 we skipped setting the floor
time if we were using the default RA interval. The commit was a bit too
broad; it also caused the valid and preferred lifetimes to be skipped
too, meaning that they were set to infinite.
Adjust the check, so that we only apply the "are we using the default?"
check when calculating the floor; but still set up the `time` variable
because that is used later on as a ceiling for valid_lft and
preferred_lft.
---
src/radv.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/radv.c b/src/radv.c
index 41df852..78edaab 100644
--- a/src/radv.c
+++ b/src/radv.c
@@ -629,11 +629,11 @@ static int add_prefixes(struct in6_addr *local, int prefix,
/* find floor time, don't reduce below 3 * RA interval.
If the lease time has been left as default, don't
use that as a floor. */
- if ((context->flags & CONTEXT_SETLEASE) &&
- time > context->lease_time)
+ if (time > context->lease_time)
{
time = context->lease_time;
- if (time < ((unsigned int)(3 * param->adv_interval)))
+ if ((context->flags & CONTEXT_SETLEASE) &&
+ time < ((unsigned int)(2 * param->adv_interval)))
time = 3 * param->adv_interval;
}
--
2.27.0
signature.asc
Description: PGP signature
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
