[PATCH net-next 0/3] mld updates, part 2

[PATCH net-next 0/3] mld updates, part 2

[Daniel Borkmann]

Daniel Borkmann (3):
  ipv6: mld: rename mc_maxdelay into mc_uri
  ipv6: mld: do not overwrite uri when receiving an mldv2 query
  ipv6: mld: remove duplicate code from mld_update_qri

 include/net/if_inet6.h |  2 +-
 net/ipv6/mcast.c       | 31 ++++++++++++-------------------
 2 files changed, 13 insertions(+), 20 deletions(-)

-- 
1.7.11.7

[PATCH net-next 1/3] ipv6: mld: rename mc_maxdelay into mc_uri

[Daniel Borkmann]

The name mc_maxdelay is quite confusing as it actually denotes the
unsolicited report interval. Since we have query response interval
named as mc_qri, name unsolicited report interval analogously as
mc_uri. Note that both are not the same!

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/if_inet6.h |  2 +-
 net/ipv6/mcast.c       | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
index d07b1a6..8daf683 100644
--- a/include/net/if_inet6.h
+++ b/include/net/if_inet6.h
@@ -180,7 +180,7 @@ struct inet6_dev {
 	unsigned long		mc_v1_seen;	/* Max time we stay in MLDv1 mode */
 	unsigned long		mc_qi;		/* Query Interval */
 	unsigned long		mc_qri;		/* Query Response Interval */
-	unsigned long		mc_maxdelay;
+	unsigned long		mc_uri;		/* Unsolicited Report Interval */
 
 	struct timer_list	mc_gq_timer;	/* general query timer */
 	struct timer_list	mc_ifc_timer;	/* interface change timer */
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 592eba6..3d0e8fc 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -996,7 +996,7 @@ bool ipv6_chk_mcast_addr(struct net_device *dev, const struct in6_addr *group,
 
 static void mld_gq_start_timer(struct inet6_dev *idev)
 {
-	unsigned long tv = prandom_u32() % idev->mc_maxdelay;
+	unsigned long tv = prandom_u32() % idev->mc_uri;
 
 	idev->mc_gq_running = 1;
 	if (!mod_timer(&idev->mc_gq_timer, jiffies+tv+2))
@@ -1274,7 +1274,7 @@ static int mld_process_v2(struct inet6_dev *idev, struct mld2_query *mld,
 	mld_update_qi(idev, mld);
 	mld_update_qri(idev, mld);
 
-	idev->mc_maxdelay = *max_delay;
+	idev->mc_uri = *max_delay;
 
 	return 0;
 }
@@ -2037,7 +2037,7 @@ void ipv6_mc_dad_complete(struct inet6_dev *idev)
 		mld_send_initial_cr(idev);
 		idev->mc_dad_count--;
 		if (idev->mc_dad_count)
-			mld_dad_start_timer(idev, idev->mc_maxdelay);
+			mld_dad_start_timer(idev, idev->mc_uri);
 	}
 }
 
@@ -2049,7 +2049,7 @@ static void mld_dad_timer_expire(unsigned long data)
 	if (idev->mc_dad_count) {
 		idev->mc_dad_count--;
 		if (idev->mc_dad_count)
-			mld_dad_start_timer(idev, idev->mc_maxdelay);
+			mld_dad_start_timer(idev, idev->mc_uri);
 	}
 	in6_dev_put(idev);
 }
@@ -2407,7 +2407,7 @@ static void mld_ifc_timer_expire(unsigned long data)
 	if (idev->mc_ifc_count) {
 		idev->mc_ifc_count--;
 		if (idev->mc_ifc_count)
-			mld_ifc_start_timer(idev, idev->mc_maxdelay);
+			mld_ifc_start_timer(idev, idev->mc_uri);
 	}
 	in6_dev_put(idev);
 }
@@ -2481,8 +2481,8 @@ static void ipv6_mc_reset(struct inet6_dev *idev)
 	idev->mc_qrv = sysctl_mld_qrv;
 	idev->mc_qi = MLD_QI_DEFAULT;
 	idev->mc_qri = MLD_QRI_DEFAULT;
+	idev->mc_uri = unsolicited_report_interval(idev);
 	idev->mc_v1_seen = 0;
-	idev->mc_maxdelay = unsolicited_report_interval(idev);
 }
 
 /* Device going up */
-- 
1.7.11.7

[PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[Daniel Borkmann]

While reviewing the code, I found it confusing why we update the URI when
receiving an MLDv2 query. The RFC does not mention any of this, and I also
double-checked with other implementations.

It is true that we start the general query timer with the received max_delay,
as mentioned in the older RFC2710, section 5.:

  [...] "start timer" for the address on the interface, using a delay
  value chosen uniformly from the interval [0, Maximum Response Delay],
  where Maximum Response Delay is specified in the Query. If this is
  an unsolicited Report, the timer is set to a delay value chosen
  uniformly from the interval [0, [Unsolicited Report Interval] ].

It however does not say anywhere that we are supposed to overwrite that
value. The purpose of the report is quite different and described as:

  When a node starts listening to a multicast address on an interface,
  it should immediately transmit an unsolicited Report for that address
  on that interface, in case it is the first listener on the link.
  To cover the possibility of the initial Report being lost or damaged,
  it is recommended that it be repeated once or twice after short delays
  [Unsolicited Report Interval]. (A simple way to accomplish this is to
  send the initial Report and then act as if a Multicast-Address-Specific
  Query was received for that address, and set a timer appropriately).

RFC3810, section 9.11. only changed that default interval into 1 second (in
contrast to the older RFC2710). Therefore, do not update the URI sysctl
provided interval value when receiving an MLDv2 query, only pass that max
delay as mentioned in section 5. along to mld_gq_start_timer(). This
behaviour seems to be the case since the initial implementation of MLDv2.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 [ Sending to net-next to let this linger a bit here first, seems to be
   the case like this since initial MLDv2. ]

 net/ipv6/mcast.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 3d0e8fc..2a4d2b1 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -994,9 +994,9 @@ bool ipv6_chk_mcast_addr(struct net_device *dev, const struct in6_addr *group,
 	return rv;
 }
 
-static void mld_gq_start_timer(struct inet6_dev *idev)
+static void mld_gq_start_timer(struct inet6_dev *idev, unsigned long delay)
 {
-	unsigned long tv = prandom_u32() % idev->mc_uri;
+	unsigned long tv = prandom_u32() % delay;
 
 	idev->mc_gq_running = 1;
 	if (!mod_timer(&idev->mc_gq_timer, jiffies+tv+2))
@@ -1274,8 +1274,6 @@ static int mld_process_v2(struct inet6_dev *idev, struct mld2_query *mld,
 	mld_update_qi(idev, mld);
 	mld_update_qri(idev, mld);
 
-	idev->mc_uri = *max_delay;
-
 	return 0;
 }
 
@@ -1345,7 +1343,7 @@ int igmp6_event_query(struct sk_buff *skb)
 			if (mlh2->mld2q_nsrcs)
 				return -EINVAL; /* no sources allowed */
 
-			mld_gq_start_timer(idev);
+			mld_gq_start_timer(idev, max_delay);
 			return 0;
 		}
 		/* mark sources to include, if group & source-specific */
-- 
1.7.11.7

[PATCH net-next 3/3] ipv6: mld: remove duplicate code from mld_update_qri

[Daniel Borkmann]

The QRI (Query Response Interval) from RFC3810, section 9.3. is the same
as we calculate anyway earlier. Therefore, we can just remove that code
and simply reuse the value of max_delay.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/ipv6/mcast.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 2a4d2b1..e4139e5 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1226,16 +1226,6 @@ static void mld_update_qi(struct inet6_dev *idev,
 	idev->mc_qi = mc_qqi * HZ;
 }
 
-static void mld_update_qri(struct inet6_dev *idev,
-			   const struct mld2_query *mlh2)
-{
-	/* RFC3810, relevant sections:
-	 *  - 5.1.3. Maximum Response Code
-	 *  - 9.3. Query Response Interval
-	 */
-	idev->mc_qri = msecs_to_jiffies(mldv2_mrc(mlh2));
-}
-
 static int mld_process_v1(struct inet6_dev *idev, struct mld_msg *mld,
 			  unsigned long *max_delay)
 {
@@ -1272,7 +1262,12 @@ static int mld_process_v2(struct inet6_dev *idev, struct mld2_query *mld,
 
 	mld_update_qrv(idev, mld);
 	mld_update_qi(idev, mld);
-	mld_update_qri(idev, mld);
+
+	/* RFC3810, relevant sections:
+	 *  - 5.1.3. Maximum Response Code
+	 *  - 9.3. Query Response Interval
+	 */
+	idev->mc_qri = *max_delay;
 
 	return 0;
 }
-- 
1.7.11.7

Re: [PATCH net-next 1/3] ipv6: mld: rename mc_maxdelay into mc_uri

[Hannes Frederic Sowa]

On Tue, Sep 23, 2014, at 09:03, Daniel Borkmann wrote:
> The name mc_maxdelay is quite confusing as it actually denotes the
> unsolicited report interval. Since we have query response interval
> named as mc_qri, name unsolicited report interval analogously as
> mc_uri. Note that both are not the same!
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Re: [PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[Hannes Frederic Sowa]

On Tue, Sep 23, 2014, at 09:03, Daniel Borkmann wrote:
> While reviewing the code, I found it confusing why we update the URI when
> receiving an MLDv2 query. The RFC does not mention any of this, and I
> also
> double-checked with other implementations.
>
> [...]
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Re: [PATCH net-next 3/3] ipv6: mld: remove duplicate code from mld_update_qri

[Hannes Frederic Sowa]

On Tue, Sep 23, 2014, at 09:03, Daniel Borkmann wrote:
> The QRI (Query Response Interval) from RFC3810, section 9.3. is the same
> as we calculate anyway earlier. Therefore, we can just remove that code
> and simply reuse the value of max_delay.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Thanks,
Hannes

Re: [PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[David L Stevens]

While I can see the case you're making, I think the intent of MRC is
violated by arbitrary URI.

> 5.1.3.  Maximum Response Code
> 
>    The Maximum Response Code field specifies the maximum time allowed
>    before sending a responding Report. 
>...
>    Small values of Maximum Response Delay allow MLDv2 routers to tune
>    the "leave latency" (the time between the moment the last node on a
>    link ceases to listen to a specific multicast address and the moment
>    the routing protocol is notified that there are no more listeners for
>    that address).  Larger values, especially in the exponential range,
>    allow the tuning of the burstiness of MLD traffic on a link.

If URI is larger than MRD, then a lost unsolicited report, or series,
specifically will *not* propagate changes throughout the network in less
than MRD*QRV, as intended.

It was an intentional design choice, not required or prohibited by RFC.

I'm not sure what problem you think it's causing, but if they are not
equal, I think at least the URI should be enforced to <= MRD. The querier,
IMO, should set these network-wide relevant parameters, not the individual
hosts.

Is there actually some bad effect from this?

						+-DLS

Re: [PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[Daniel Borkmann]

On 09/25/2014 06:02 PM, David L Stevens wrote:
> While I can see the case you're making, I think the intent of MRC is
> violated by arbitrary URI.
>
>> 5.1.3.  Maximum Response Code
>>
>>     The Maximum Response Code field specifies the maximum time allowed
>>     before sending a responding Report.
>> ...
>>     Small values of Maximum Response Delay allow MLDv2 routers to tune
>>     the "leave latency" (the time between the moment the last node on a
>>     link ceases to listen to a specific multicast address and the moment
>>     the routing protocol is notified that there are no more listeners for
>>     that address).  Larger values, especially in the exponential range,
>>     allow the tuning of the burstiness of MLD traffic on a link.
>
> If URI is larger than MRD, then a lost unsolicited report, or series,
> specifically will *not* propagate changes throughout the network in less
> than MRD*QRV, as intended.
>
> It was an intentional design choice, not required or prohibited by RFC.
>
> I'm not sure what problem you think it's causing, but if they are not
> equal, I think at least the URI should be enforced to <= MRD. The querier,
> IMO, should set these network-wide relevant parameters, not the individual
> hosts.

One of the problems I see (also with this argumentation -- next to the fact
that it's not specified by the RFC) is that we're blindly overwriting with
any given value from the MLDv2 query, while when temporarily transitioning
back to MLDv1 compatibility mode, we're simply ignoring any MLD value provided
from that query; both specifications also specify different default values for
URI, where we would have already overwritten a pre-configured URI default value
for v1 when we previously received a v2 query. While we have tunable for IPv4
case via commit 2690048c01f32 ("net: igmp: Allow user-space configuration of
igmp unsolicited report interval") and for IPv6 case via commit fc4eba58b4c1
("ipv6: make unsolicited report intervals configurable for mld"), this renders
any admin provided IPv6 specific configuration useless.

Thanks,
Daniel

Re: [PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[David L Stevens]

On 09/25/2014 04:06 PM, Daniel Borkmann wrote:

> One of the problems I see (also with this argumentation -- next to the fact
> that it's not specified by the RFC) is that we're blindly overwriting with

You say "not specfied by the RFC" as if it's contrary to the RFC, when the RFC
also doesn't specify that it be set per-host via sysctl. It isn't specified means
that it is up to the implementation how to set it, and the implementation sets it
based on MRD. It does not say "SHOULD" or "MUST" for how this value is set, so
to be clear: the current mechanism is RFC-compliant.

Now you want to change this mechanism that is not covered by RFC to a different
mechanism. Is your change better?

I'm not sure what problem you're trying to fix (which is what I was asking),
but I think a fixed-value specified at each host, rather than one done via the
querier, is in fact worse, especially if that value is much greater or much smaller
than the MRD value, since it is effectively for the same purpose -- just for
unsolicited instead of queried reports.

Now, probably that discussion should've happened when the tunables were put in, but
having the sysctl's is still useful for setting the values when there is no querier
present.

When there is a querier, however, the original code IMO makes more sense, especially
in the absence of any input from an administrator.

I'm generally for allowing administrators complete flexibility, even if they use it
for evil, so I think I'd prefer something along the lines of:

1) have an initial default of 1sec (v2) or 10sec (v1)
2) if an administrator sets the sysctl, override any
	other choice with that setting
3) if an administator has not set it, use the querier value

That combination allows the querier to effectively set an appropriate interval for
the entire network, allows an admin to change it per-host if desired, and uses the
suggested defaults when there is no querier or admin intervention.

Or maybe split the sysctls into one that forces the value and one that just sets
a default which can be overridden by queriers.

I don't think your patches are incorrect, but I don't think the original behavior
is either. With your interpretation, the URI (but not the MRD or QRV), must be
changed on every individual host to tune a network away from the default values.
The current code doesn't have that problem.

							+-DLS

Re: [PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[David L Stevens]

On 09/25/2014 04:06 PM, Daniel Borkmann wrote:

> from that query; both specifications also specify different default values for
> URI, where we would have already overwritten a pre-configured URI default value
> for v1 when we previously received a v2 query. While we have tunable for IPv4
> case via commit 2690048c01f32 ("net: igmp: Allow user-space configuration of
> igmp unsolicited report interval") and for IPv6 case via commit fc4eba58b4c1
> ("ipv6: make unsolicited report intervals configurable for mld"), this renders
> any admin provided IPv6 specific configuration useless.


PS -

I didn't address this part -- I don't object at all to resetting the value
when doing a version switch, or even if we lose a querier for a while. That
isn't what your patch does.

						+-DLS

Re: [PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[Daniel Borkmann]

On 09/26/2014 01:23 AM, David L Stevens wrote:
...
> Now, probably that discussion should've happened when the tunables were put in, but
> having the sysctl's is still useful for setting the values when there is no querier
> present.
>
> When there is a querier, however, the original code IMO makes more sense, especially
> in the absence of any input from an administrator.
>
> I'm generally for allowing administrators complete flexibility, even if they use it
> for evil, so I think I'd prefer something along the lines of:
>
> 1) have an initial default of 1sec (v2) or 10sec (v1)
> 2) if an administrator sets the sysctl, override any
> 	other choice with that setting
> 3) if an administator has not set it, use the querier value
>
> That combination allows the querier to effectively set an appropriate interval for
> the entire network, allows an admin to change it per-host if desired, and uses the
> suggested defaults when there is no querier or admin intervention.
>
> Or maybe split the sysctls into one that forces the value and one that just sets
> a default which can be overridden by queriers.
>
> I don't think your patches are incorrect, but I don't think the original behavior
> is either. With your interpretation, the URI (but not the MRD or QRV), must be
> changed on every individual host to tune a network away from the default values.
> The current code doesn't have that problem.

I'm fine with either suggestion. Actually the _current_ situation we're in is
that in IPv4 we _always_ use the current, uncached _sysctl_ tuned setting of URI
(independent of any protocol version); while in IPv6 we use the _cached_ sysctl
URI in case of MLDv1 and _always_ overwrite the URI in case of MLDv2 (even for
MLDv1). Are you suggesting that, we then better adapt using the maxdelay value
everywhere and adapt URI to it, plus having a boolean knob defaulting to off for
an admin to enforce always using the provided sysctl default setting and not
the snooped MLD?

Re: [PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[David L Stevens]

On 09/26/2014 05:29 AM, Daniel Borkmann wrote:
> On 09/26/2014 01:23 AM, David L Stevens wrote:

> I'm fine with either suggestion. Actually the _current_ situation we're in is
> that in IPv4 we _always_ use the current, uncached _sysctl_ tuned setting of URI
> (independent of any protocol version); while in IPv6 we use the _cached_ sysctl
> URI in case of MLDv1 and _always_ overwrite the URI in case of MLDv2 (even for
> MLDv1). Are you suggesting that, we then better adapt using the maxdelay value
> everywhere and adapt URI to it, plus having a boolean knob defaulting to off for
> an admin to enforce always using the provided sysctl default setting and not
> the snooped MLD?

Yes.

Definitely, IGMP and MLD, all versions, should do the same thing and I think that
ought to use the querier MRC, if present and not overridden by an admin.

Further, I think a version switch or failure to hear from a querier for qrv*qi
ought to reset everything.
								+-DLS

Re: [PATCH net-next 2/3] ipv6: mld: do not overwrite uri when receiving an mldv2 query

[Daniel Borkmann]

On 09/26/2014 02:13 PM, David L Stevens wrote:
> On 09/26/2014 05:29 AM, Daniel Borkmann wrote:
>> On 09/26/2014 01:23 AM, David L Stevens wrote:
>
>> I'm fine with either suggestion. Actually the _current_ situation we're in is
>> that in IPv4 we _always_ use the current, uncached _sysctl_ tuned setting of URI
>> (independent of any protocol version); while in IPv6 we use the _cached_ sysctl
>> URI in case of MLDv1 and _always_ overwrite the URI in case of MLDv2 (even for
>> MLDv1). Are you suggesting that, we then better adapt using the maxdelay value
>> everywhere and adapt URI to it, plus having a boolean knob defaulting to off for
>> an admin to enforce always using the provided sysctl default setting and not
>> the snooped MLD?
>
> Yes.
>
> Definitely, IGMP and MLD, all versions, should do the same thing and I think that
> ought to use the querier MRC, if present and not overridden by an admin.
>
> Further, I think a version switch or failure to hear from a querier for qrv*qi
> ought to reset everything.

I'll recook the patch set and keep you in the loop. Thanks David!

Best,
Daniel