From 5e2edfb8726ddb255a02352e2f68ea028f543e4b Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Fri, 29 Jan 2021 13:47:58 -0800 Subject: [PATCH] Refresh delayed report timers on query messages ...as per As per RFC 2236 section 3 page 3 (for IGMPv2) and RFC 2710 section 4 page 5 (for MLDv1). See comments in code for more details. Test: ip_test.TestHandleQuery PiperOrigin-RevId: 354603068 --- .../network/ip/generic_multicast_protocol.go | 32 ++++++-- .../ip/generic_multicast_protocol_test.go | 78 ++++++++++--------- 2 files changed, 68 insertions(+), 42 deletions(-) diff --git a/pkg/tcpip/network/ip/generic_multicast_protocol.go b/pkg/tcpip/network/ip/generic_multicast_protocol.go index a81f5c8c3..b9f129728 100644 --- a/pkg/tcpip/network/ip/generic_multicast_protocol.go +++ b/pkg/tcpip/network/ip/generic_multicast_protocol.go @@ -126,6 +126,16 @@ type multicastGroupState struct { // // Must not be nil. delayedReportJob *tcpip.Job + + // delyedReportJobFiresAt is the time when the delayed report job will fire. + // + // A zero value indicates that the job is not scheduled. + delayedReportJobFiresAt time.Time +} + +func (m *multicastGroupState) cancelDelayedReportJob() { + m.delayedReportJob.Cancel() + m.delayedReportJobFiresAt = time.Time{} } // GenericMulticastProtocolOptions holds options for the generic multicast @@ -428,7 +438,7 @@ func (g *GenericMulticastProtocolState) HandleReportLocked(groupAddress tcpip.Ad // on that interface, it stops its timer and does not send a Report for // that address, thus suppressing duplicate reports on the link. if info, ok := g.memberships[groupAddress]; ok && info.state.isDelayingMember() { - info.delayedReportJob.Cancel() + info.cancelDelayedReportJob() info.lastToSendReport = false info.state = idleMember g.memberships[groupAddress] = info @@ -603,7 +613,7 @@ func (g *GenericMulticastProtocolState) transitionToNonMemberLocked(groupAddress return } - info.delayedReportJob.Cancel() + info.cancelDelayedReportJob() g.maybeSendLeave(groupAddress, info.lastToSendReport) info.lastToSendReport = false info.state = nonMember @@ -645,14 +655,24 @@ func (g *GenericMulticastProtocolState) setDelayTimerForAddressRLocked(groupAddr // If a timer for any address is already running, it is reset to the new // random value only if the requested Maximum Response Delay is less than // the remaining value of the running timer. + now := time.Unix(0 /* seconds */, g.opts.Clock.NowNanoseconds()) if info.state == delayingMember { - // TODO: Reset the timer if time remaining is greater than maxResponseTime. - return + if info.delayedReportJobFiresAt.IsZero() { + panic(fmt.Sprintf("delayed report unscheduled while in the delaying member state; group = %s", groupAddress)) + } + + if info.delayedReportJobFiresAt.Sub(now) <= maxResponseTime { + // The timer is scheduled to fire before the maximum response time so we + // leave our timer as is. + return + } } info.state = delayingMember - info.delayedReportJob.Cancel() - info.delayedReportJob.Schedule(g.calculateDelayTimerDuration(maxResponseTime)) + info.cancelDelayedReportJob() + maxResponseTime = g.calculateDelayTimerDuration(maxResponseTime) + info.delayedReportJob.Schedule(maxResponseTime) + info.delayedReportJobFiresAt = now.Add(maxResponseTime) } // calculateDelayTimerDuration returns a random time between (0, maxRespTime]. diff --git a/pkg/tcpip/network/ip/generic_multicast_protocol_test.go b/pkg/tcpip/network/ip/generic_multicast_protocol_test.go index d5d5a449e..60eaea37e 100644 --- a/pkg/tcpip/network/ip/generic_multicast_protocol_test.go +++ b/pkg/tcpip/network/ip/generic_multicast_protocol_test.go @@ -408,40 +408,46 @@ func TestHandleReport(t *testing.T) { func TestHandleQuery(t *testing.T) { tests := []struct { - name string - queryAddr tcpip.Address - maxDelay time.Duration - expectReportsFor []tcpip.Address + name string + queryAddr tcpip.Address + maxDelay time.Duration + expectQueriedReportsFor []tcpip.Address + expectDelayedReportsFor []tcpip.Address }{ { - name: "Unpecified empty", - queryAddr: "", - maxDelay: 0, - expectReportsFor: []tcpip.Address{addr1, addr2}, + name: "Unpecified empty", + queryAddr: "", + maxDelay: 0, + expectQueriedReportsFor: []tcpip.Address{addr1, addr2}, + expectDelayedReportsFor: nil, }, { - name: "Unpecified any", - queryAddr: "\x00", - maxDelay: 1, - expectReportsFor: []tcpip.Address{addr1, addr2}, + name: "Unpecified any", + queryAddr: "\x00", + maxDelay: 1, + expectQueriedReportsFor: []tcpip.Address{addr1, addr2}, + expectDelayedReportsFor: nil, }, { - name: "Specified", - queryAddr: addr1, - maxDelay: 2, - expectReportsFor: []tcpip.Address{addr1}, + name: "Specified", + queryAddr: addr1, + maxDelay: 2, + expectQueriedReportsFor: []tcpip.Address{addr1}, + expectDelayedReportsFor: []tcpip.Address{addr2}, }, { - name: "Specified all-nodes", - queryAddr: addr3, - maxDelay: 3, - expectReportsFor: nil, + name: "Specified all-nodes", + queryAddr: addr3, + maxDelay: 3, + expectQueriedReportsFor: nil, + expectDelayedReportsFor: []tcpip.Address{addr1, addr2}, }, { - name: "Specified other", - queryAddr: addr4, - maxDelay: 4, - expectReportsFor: nil, + name: "Specified other", + queryAddr: addr4, + maxDelay: 4, + expectQueriedReportsFor: nil, + expectDelayedReportsFor: []tcpip.Address{addr1, addr2}, }, } @@ -469,20 +475,20 @@ func TestHandleQuery(t *testing.T) { if diff := mgp.check(nil /* sendReportGroupAddresses */, nil /* sendLeaveGroupAddresses */); diff != "" { t.Fatalf("mockMulticastGroupProtocol mismatch (-want +got):\n%s", diff) } - // Generic multicast protocol timers are expected to take the job mutex. - clock.Advance(maxUnsolicitedReportDelay) - if diff := mgp.check([]tcpip.Address{addr1, addr2} /* sendReportGroupAddresses */, nil /* sendLeaveGroupAddresses */); diff != "" { - t.Fatalf("mockMulticastGroupProtocol mismatch (-want +got):\n%s", diff) + + // Receiving a query should make us reschedule our delayed report timer + // to some time within the new max response delay. + mgp.handleQuery(test.queryAddr, test.maxDelay) + clock.Advance(test.maxDelay) + if diff := mgp.check(test.expectQueriedReportsFor /* sendReportGroupAddresses */, nil /* sendLeaveGroupAddresses */); diff != "" { + t.Errorf("mockMulticastGroupProtocol mismatch (-want +got):\n%s", diff) } - // Receiving a query should make us schedule a new delayed report if it - // is a query directed at us or a general query. - mgp.handleQuery(test.queryAddr, test.maxDelay) - if len(test.expectReportsFor) != 0 { - clock.Advance(test.maxDelay) - if diff := mgp.check(test.expectReportsFor /* sendReportGroupAddresses */, nil /* sendLeaveGroupAddresses */); diff != "" { - t.Errorf("mockMulticastGroupProtocol mismatch (-want +got):\n%s", diff) - } + // The groups that were not affected by the query should still send a + // report after the max unsolicited report delay. + clock.Advance(maxUnsolicitedReportDelay) + if diff := mgp.check(test.expectDelayedReportsFor /* sendReportGroupAddresses */, nil /* sendLeaveGroupAddresses */); diff != "" { + t.Errorf("mockMulticastGroupProtocol mismatch (-want +got):\n%s", diff) } // Should have no more messages to send.