[PATCHv3] bridge: disable snooping if there is no querier

If there is no querier on a link then we won't get periodic reports and
therefore won't be able to learn about multicast listeners behind ports,
potentially leading to lost multicast packets, especially for multicast
listeners that joined before the creation of the bridge.

These lost multicast packets can appear since c5c23260594
("bridge: Add multicast_querier toggle and disable queries by default")
in particular.

With this patch we are flooding multicast packets if our querier is
disabled and if we didn't detect any other querier.

A grace period of the Maximum Response Delay of the querier is added to
give multicast responses enough time to arrive and to be learned from
before disabling the flooding behaviour again.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
v3: changed type of multicast_querier_delay_time from
    atomic64_t to unsigned long
v2: added missing, empty br_multicast_querier_exists() to avoid
    build failures if CONFIG_BRIDGE_IGMP_SNOOPING is not set

 net/bridge/br_device.c    |    3 ++-
 net/bridge/br_input.c     |    3 ++-
 net/bridge/br_multicast.c |   39 ++++++++++++++++++++++++++++++---------
 net/bridge/br_private.h   |   12 ++++++++++++
 4 files changed, 46 insertions(+), 11 deletions(-)

From: Linus Lüssing <linus.luessing@web.de>
Date: Thu,  1 Aug 2013 01:06:20 +0200

> If there is no querier on a link then we won't get periodic reports and
> therefore won't be able to learn about multicast listeners behind ports,
> potentially leading to lost multicast packets, especially for multicast
> listeners that joined before the creation of the bridge.
> 
> These lost multicast packets can appear since c5c23260594
> ("bridge: Add multicast_querier toggle and disable queries by default")
> in particular.
> 
> With this patch we are flooding multicast packets if our querier is
> disabled and if we didn't detect any other querier.
> 
> A grace period of the Maximum Response Delay of the querier is added to
> give multicast responses enough time to arrive and to be learned from
> before disabling the flooding behaviour again.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>

Looks good, applied, thanks Linus.

--
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 Wed, 2013-07-31 at 17:40 -0700, David Miller wrote:
> > If there is no querier on a link then we won't get periodic reports and
> > therefore won't be able to learn about multicast listeners behind ports,
> > potentially leading to lost multicast packets, especially for multicast
> > listeners that joined before the creation of the bridge.
> > 
> > These lost multicast packets can appear since c5c23260594
> > ("bridge: Add multicast_querier toggle and disable queries by default")
> > in particular.
> > 
> > With this patch we are flooding multicast packets if our querier is
> > disabled and if we didn't detect any other querier.
> > 
> > A grace period of the Maximum Response Delay of the querier is added to
> > give multicast responses enough time to arrive and to be learned from
> > before disabling the flooding behaviour again.
> > 
> > Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> 
> Looks good, applied, thanks Linus.

0) This patch is part of v3.11-rc4 as commit b00589af3b0. It introduced
a GCC warning:
    net/bridge/br_multicast.c: In function ‘br_multicast_rcv’:
    net/bridge/br_multicast.c:1081:36: warning: ‘max_delay’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/bridge/br_multicast.c:1178:16: note: ‘max_delay’ was declared here

1) Summarized, the code reads:

        unsigned long max_delay;

        if (skb->len == sizeof(*mld))
                max_delay = msecs_to_jiffies(ntohs(mld->mld_maxdelay));
        else if (skb->len >= sizeof(*mld2q))
                max_delay = mld2q->mld2q_mrc ? MLDV2_MRC(ntohs(mld2q->mld2q_mrc)) : 1;

        br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr),
                                    max_delay);
 
So GCC notices that max_delay is still uninitialized if skb->len is
neither equal to sizeof(*mld) nor equal or bigger than sizeof(*mld2q).
To me it looks GCC is right here. At least, it is not obvious that
max_delay will actually not be used in br_multicast_query_received() if
it still is uninitialized.

2) I'm entirely unfamiliar to this code. So I can't say how this warning
might be silenced.


Paul Bolle

--
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

Thanks for the very quick reporting! Looks like my gcc was and still
isn't as supportive... You and the lists should have received a patch
to fix that.

Cheers, Linus


On Mon, Aug 05, 2013 at 10:41:06AM +0200, Paul Bolle wrote:
> On Wed, 2013-07-31 at 17:40 -0700, David Miller wrote:
> > > If there is no querier on a link then we won't get periodic reports and
> > > therefore won't be able to learn about multicast listeners behind ports,
> > > potentially leading to lost multicast packets, especially for multicast
> > > listeners that joined before the creation of the bridge.
> > > 
> > > These lost multicast packets can appear since c5c23260594
> > > ("bridge: Add multicast_querier toggle and disable queries by default")
> > > in particular.
> > > 
> > > With this patch we are flooding multicast packets if our querier is
> > > disabled and if we didn't detect any other querier.
> > > 
> > > A grace period of the Maximum Response Delay of the querier is added to
> > > give multicast responses enough time to arrive and to be learned from
> > > before disabling the flooding behaviour again.
> > > 
> > > Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> > 
> > Looks good, applied, thanks Linus.
> 
> 0) This patch is part of v3.11-rc4 as commit b00589af3b0. It introduced
> a GCC warning:
>     net/bridge/br_multicast.c: In function ‘br_multicast_rcv’:
>     net/bridge/br_multicast.c:1081:36: warning: ‘max_delay’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     net/bridge/br_multicast.c:1178:16: note: ‘max_delay’ was declared here
> 
> 1) Summarized, the code reads:
> 
>         unsigned long max_delay;
> 
>         if (skb->len == sizeof(*mld))
>                 max_delay = msecs_to_jiffies(ntohs(mld->mld_maxdelay));
>         else if (skb->len >= sizeof(*mld2q))
>                 max_delay = mld2q->mld2q_mrc ? MLDV2_MRC(ntohs(mld2q->mld2q_mrc)) : 1;
> 
>         br_multicast_query_received(br, port, !ipv6_addr_any(&ip6h->saddr),
>                                     max_delay);
>  
> So GCC notices that max_delay is still uninitialized if skb->len is
> neither equal to sizeof(*mld) nor equal or bigger than sizeof(*mld2q).
> To me it looks GCC is right here. At least, it is not obvious that
> max_delay will actually not be used in br_multicast_query_received() if
> it still is uninitialized.
> 
> 2) I'm entirely unfamiliar to this code. So I can't say how this warning
> might be silenced.
> 
> 
> Paul Bolle
> 
--
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