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
> 

Reply via email to