Self NAK this patch, there is some issue I need to fix.
On Wed, Jun 13, 2018 at 02:32:31PM +0800, Hangbin Liu wrote: > Based on RFC3376 5.1 and RFC3810 6.1: > If no interface > state existed for that multicast address before the change (i.e., the > change consisted of creating a new per-interface record), or if no > state exists after the change (i.e., the change consisted of deleting > a per-interface record), then the "non-existent" state is considered > to have a filter mode of INCLUDE and an empty source list. > > Which means a new multicast group should start with state IN(). That is > exactly what we did with ip_mc_join_group()/ipv6_sock_mc_join(), which > adds a group with state EX() and init crcount to mc_qrv. The kernel will > send a TO_EX() report message after adding group. This is what IGMPv3/MLDv2 > ASM(Any-Source Multicast) mode should look like. > > But for IGMPv3/MLDv2 SSM JOIN_SOURCE_GROUP mode, we split the group > joining into two steps. First step we join the group like ASM, i.e. via > ip_mc_join_group()/ipv6_sock_mc_join(). So the state changes from IN() to > EX(). > > Then we add the Source-specific address with INCLUDE mode. So the state > changes from EX() to IN(A). > > Before the first step sends a group change record, we finished the second > step. > So we will only send the second change record. i.e. TO_IN(A) > > Regarding the RFC stands, we should actually send an ALLOW(A) message for > SSM JOIN_SOURCE_GROUP as the state should mimic the 'IN() to IN(A)' > transition. > > The issue was exposed by commit a052517a8ff65 ("net/multicast: should not send > source list records when have filter mode change"). Before this commit we will > send both ALLOW(A) and TO_IN(A). After this commit we only send TO_IN(A). > > Fix it by adding a is_new key to clean the crcount when we add a new > INCLUDE SSM group. > > Fixes: a052517a8ff65 ("net/multicast: should not send source list records > when have filter mode change") > Reviewed-by: Paolo Abeni <pab...@redhat.com> > Reviewed-by: Stefano Brivio <sbri...@redhat.com> > Signed-off-by: Hangbin Liu <liuhang...@gmail.com> > --- > include/linux/igmp.h | 2 +- > include/net/ipv6.h | 2 +- > net/ipv4/igmp.c | 27 ++++++++++++++++++++++++++- > net/ipv4/ip_sockglue.c | 8 ++++++-- > net/ipv6/ipv6_sockglue.c | 4 +++- > net/ipv6/mcast.c | 25 ++++++++++++++++++++++++- > 6 files changed, 61 insertions(+), 7 deletions(-) > > diff --git a/include/linux/igmp.h b/include/linux/igmp.h > index f823185..32cb02b 100644 > --- a/include/linux/igmp.h > +++ b/include/linux/igmp.h > @@ -112,7 +112,7 @@ extern int ip_mc_join_group(struct sock *sk, struct > ip_mreqn *imr); > extern int ip_mc_leave_group(struct sock *sk, struct ip_mreqn *imr); > extern void ip_mc_drop_socket(struct sock *sk); > extern int ip_mc_source(int add, int omode, struct sock *sk, > - struct ip_mreq_source *mreqs, int ifindex); > + struct ip_mreq_source *mreqs, int ifindex, bool is_new); > extern int ip_mc_msfilter(struct sock *sk, struct ip_msfilter *msf,int > ifindex); > extern int ip_mc_msfget(struct sock *sk, struct ip_msfilter *msf, > struct ip_msfilter __user *optval, int __user *optlen); > diff --git a/include/net/ipv6.h b/include/net/ipv6.h > index 836f31a..754c5cb 100644 > --- a/include/net/ipv6.h > +++ b/include/net/ipv6.h > @@ -1065,7 +1065,7 @@ struct group_source_req; > struct group_filter; > > int ip6_mc_source(int add, int omode, struct sock *sk, > - struct group_source_req *pgsr); > + struct group_source_req *pgsr, bool is_new); > int ip6_mc_msfilter(struct sock *sk, struct group_filter *gsf); > int ip6_mc_msfget(struct sock *sk, struct group_filter *gsf, > struct group_filter __user *optval, int __user *optlen); > diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c > index b26a81a..8d6ecc3 100644 > --- a/net/ipv4/igmp.c > +++ b/net/ipv4/igmp.c > @@ -2249,8 +2249,27 @@ int ip_mc_leave_group(struct sock *sk, struct ip_mreqn > *imr) > } > EXPORT_SYMBOL(ip_mc_leave_group); > > +static void ip_mc_clear_cr(struct in_device *in_dev, __be32 pmca) > +{ > +#ifdef CONFIG_IP_MULTICAST > + struct ip_mc_list *pmc; > + > + rcu_read_lock(); > + for_each_pmc_rcu(in_dev, pmc) { > + if (pmca == pmc->multiaddr) > + break; > + } > + if (pmc) { > + spin_lock_bh(&pmc->lock); > + pmc->crcount = 0; > + spin_unlock_bh(&pmc->lock); > + } > + rcu_read_unlock(); > +#endif > +} > + > int ip_mc_source(int add, int omode, struct sock *sk, struct > - ip_mreq_source *mreqs, int ifindex) > + ip_mreq_source *mreqs, int ifindex, bool is_new) > { > int err; > struct ip_mreqn imr; > @@ -2301,6 +2320,12 @@ int ip_mc_source(int add, int omode, struct sock *sk, > struct > ip_mc_del_src(in_dev, &mreqs->imr_multiaddr, pmc->sfmode, 0, > NULL, 0); > pmc->sfmode = omode; > + /* Based on RFC3376 5.1, for newly added INCLUDE SSM, we should > + * not send filter-mode change record as the mode should be > + * from IN() to IN(A). > + */ > + if (is_new) > + ip_mc_clear_cr(in_dev, mreqs->imr_multiaddr); > } > > psl = rtnl_dereference(pmc->sflist); > diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c > index 57bbb06..8d8c0cd 100644 > --- a/net/ipv4/ip_sockglue.c > +++ b/net/ipv4/ip_sockglue.c > @@ -962,6 +962,7 @@ static int do_ip_setsockopt(struct sock *sk, int level, > case IP_DROP_SOURCE_MEMBERSHIP: > { > struct ip_mreq_source mreqs; > + bool is_new = false; > int omode, add; > > if (optlen != sizeof(struct ip_mreq_source)) > @@ -987,11 +988,12 @@ static int do_ip_setsockopt(struct sock *sk, int level, > break; > omode = MCAST_INCLUDE; > add = 1; > + is_new = true; > } else /* IP_DROP_SOURCE_MEMBERSHIP */ { > omode = MCAST_INCLUDE; > add = 0; > } > - err = ip_mc_source(add, omode, sk, &mreqs, 0); > + err = ip_mc_source(add, omode, sk, &mreqs, 0, is_new); > break; > } > case MCAST_JOIN_GROUP: > @@ -1027,6 +1029,7 @@ static int do_ip_setsockopt(struct sock *sk, int level, > struct group_source_req greqs; > struct ip_mreq_source mreqs; > struct sockaddr_in *psin; > + bool is_new = false; > int omode, add; > > if (optlen != sizeof(struct group_source_req)) > @@ -1065,12 +1068,13 @@ static int do_ip_setsockopt(struct sock *sk, int > level, > greqs.gsr_interface = mreq.imr_ifindex; > omode = MCAST_INCLUDE; > add = 1; > + is_new = true; > } else /* MCAST_LEAVE_SOURCE_GROUP */ { > omode = MCAST_INCLUDE; > add = 0; > } > err = ip_mc_source(add, omode, sk, &mreqs, > - greqs.gsr_interface); > + greqs.gsr_interface, is_new); > break; > } > case MCAST_MSFILTER: > diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c > index 4d780c7..36e7c40 100644 > --- a/net/ipv6/ipv6_sockglue.c > +++ b/net/ipv6/ipv6_sockglue.c > @@ -695,6 +695,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, > int optname, > case MCAST_UNBLOCK_SOURCE: > { > struct group_source_req greqs; > + bool is_new = false; > int omode, add; > > if (optlen < sizeof(struct group_source_req)) > @@ -725,11 +726,12 @@ static int do_ipv6_setsockopt(struct sock *sk, int > level, int optname, > break; > omode = MCAST_INCLUDE; > add = 1; > + is_new = true; > } else /* MCAST_LEAVE_SOURCE_GROUP */ { > omode = MCAST_INCLUDE; > add = 0; > } > - retv = ip6_mc_source(add, omode, sk, &greqs); > + retv = ip6_mc_source(add, omode, sk, &greqs, is_new); > break; > } > case MCAST_MSFILTER: > diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c > index 793159d..f508a1c 100644 > --- a/net/ipv6/mcast.c > +++ b/net/ipv6/mcast.c > @@ -315,8 +315,25 @@ void ipv6_sock_mc_close(struct sock *sk) > rtnl_unlock(); > } > > +static void ip6_mc_clear_cr(struct inet6_dev *idev, const struct in6_addr > *pmca) > +{ > + struct ifmcaddr6 *pmc; > + > + read_lock_bh(&idev->lock); > + for (pmc = idev->mc_list; pmc; pmc = pmc->next) { > + if (ipv6_addr_equal(pmca, &pmc->mca_addr)) > + break; > + } > + if (pmc) { > + spin_lock_bh(&pmc->mca_lock); > + pmc->mca_crcount = 0; > + spin_unlock_bh(&pmc->mca_lock); > + } > + read_unlock_bh(&idev->lock); > +} > + > int ip6_mc_source(int add, int omode, struct sock *sk, > - struct group_source_req *pgsr) > + struct group_source_req *pgsr, bool is_new) > { > struct in6_addr *source, *group; > struct ipv6_mc_socklist *pmc; > @@ -365,6 +382,12 @@ int ip6_mc_source(int add, int omode, struct sock *sk, > ip6_mc_add_src(idev, group, omode, 0, NULL, 0); > ip6_mc_del_src(idev, group, pmc->sfmode, 0, NULL, 0); > pmc->sfmode = omode; > + /* Based on RFC3810 6.1, for newly added INCLUDE SSM, we > + * should not send filter-mode change record as the mode > + * should be from IN() to IN(A). > + */ > + if (is_new) > + ip6_mc_clear_cr(idev, group); > } > > write_lock(&pmc->sflock); > -- > 2.5.5 >