| Message ID | 1411455828-5196-3-git-send-email-dborkman@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(-)