[PATCH][MCAST]Two fix for implementation of MLDv2 .

[PATCH][MCAST]Two fix for implementation of MLDv2 .

[Yan Zheng]

Hi.
I find two bug yesterday when I was thinking whether ip6_mc_del_src(...) should be called in __ipv6_dev_mc_dec(...).


The first change in function is_in(...) is to make process Multicast Address and Source Specific Query same as Multicast Address Specific Query.
Although it still doesn't satisfy the requirement in rfc3810, but not at risk of losing multicast traffic.
For example:
A Node's filter for Multicast address M is EXCLUDE (X,Y) and receice a Multicast Address and Source Specific Query Q(M,Z).
Without the change No report will send to router, so the node may get multicast traffic from address Z lost.


The second change in function add_grec(...) is to make sure Mode Change Report send properly.
When A filter's source address list is not empty, but none of them can be included in report.


Regards


Signed-off-by: Yan Zheng <yanzheng@21cn.com>

Index:net/ipv6/mcast.c
============================================================
--- linux-2.6.14/net/ipv6/mcast.c	2005-10-30 23:09:33.000000000 +0800
+++ linux/net/ipv6/mcast.c	2005-11-02 15:01:28.000000000 +0800
@@ -1256,10 +1256,13 @@ static int is_in(struct ifmcaddr6 *pmc, 
 {
 	switch (type) {
 	case MLD2_MODE_IS_INCLUDE:
-	case MLD2_MODE_IS_EXCLUDE:
 		if (gdeleted || sdeleted)
 			return 0;
 		return !((pmc->mca_flags & MAF_GSQUERY) && !psf->sf_gsresp);
+	case MLD2_MODE_IS_EXCLUDE:
+		if (gdeleted || sdeleted)
+			return 0;
+		return 1;
 	case MLD2_CHANGE_TO_INCLUDE:
 		if (gdeleted || sdeleted)
 			return 0;
@@ -1428,13 +1431,15 @@ static struct sk_buff *add_grec(struct s
 	struct mld2_report *pmr;
 	struct mld2_grec *pgr = NULL;
 	struct ip6_sf_list *psf, *psf_next, *psf_prev, **psf_list;
-	int scount, first, isquery, truncate;
+	int scount, first, isquery, ischange, truncate;
 
 	if (pmc->mca_flags & MAF_NOREPORT)
 		return skb;
 
 	isquery = type == MLD2_MODE_IS_INCLUDE ||
 		  type == MLD2_MODE_IS_EXCLUDE;
+	ischange = type == MLD2_CHANGE_TO_INCLUDE ||
+		   type == MLD2_CHANGE_TO_EXCLUDE; 
 	truncate = type == MLD2_MODE_IS_EXCLUDE ||
 		    type == MLD2_CHANGE_TO_EXCLUDE;
 
@@ -1444,7 +1449,7 @@ static struct sk_buff *add_grec(struct s
 		if (type == MLD2_ALLOW_NEW_SOURCES ||
 		    type == MLD2_BLOCK_OLD_SOURCES)
 			return skb;
-		if (pmc->mca_crcount || isquery) {
+		if (ischange || isquery) {
 			/* make sure we have room for group header and at
 			 * least one source.
 			 */
@@ -1460,9 +1465,12 @@ static struct sk_buff *add_grec(struct s
 	pmr = skb ? (struct mld2_report *)skb->h.raw : NULL;
 
 	/* EX and TO_EX get a fresh packet, if needed */
-	if (truncate) {
-		if (pmr && pmr->ngrec &&
-		    AVAILABLE(skb) < grec_size(pmc, type, gdeleted, sdeleted)) {
+	if (truncate || ischange) {
+		int min_len;
+		min_len	= truncate ? grec_size(pmc, type, gdeleted, sdeleted) : 
+			  (sizeof(struct mld2_grec) + sizeof(struct in6_addr));
+		if (((pmr && pmr->ngrec) || ischange) &&
+		    AVAILABLE(skb) < min_len) {
 			if (skb)
 				mld_sendpack(skb);
 			skb = mld_newpack(dev, dev->mtu);
@@ -1471,6 +1479,10 @@ static struct sk_buff *add_grec(struct s
 	first = 1;
 	scount = 0;
 	psf_prev = NULL;
+	if (ischange) {
+		skb = add_grhead(skb, pmc, type, &pgr);
+		first = 0;
+	}
 	for (psf=*psf_list; psf; psf=psf_next) {
 		struct in6_addr *psrc;
 

Re: [PATCH][MCAST]Two fix for implementation of MLDv2 .

[Yan Zheng]

[-- Attachment #1: Type: text/plain, Size: 2382 bytes --]

Hi.

Here is my another patch. It makes a node send state change report properly when a multicast address's soure filter mode change is cause by forwarding state change and etc.

Regards.

Signed-off-by: Yan Zheng <yanzheng@21cn.com>

Index:net/ipv6/mcast.c
===========================================================
--- linux-2.6.14/net/ipv6/mcast.c	2005-10-30 23:09:33.000000000 +0800
+++ /tmp/ipv6/mcast.c	2005-11-02 22:04:32.000000000 +0800
@@ -128,7 +128,8 @@ static DEFINE_RWLOCK(ipv6_sk_mc_lock);
 
 static struct socket *igmp6_socket;
 
-int __ipv6_dev_mc_dec(struct inet6_dev *idev, struct in6_addr *addr);
+static int ipv6_dev_mc_dec_intern(struct inet6_dev *idev, 
+			struct in6_addr *addr, int delta);
 
 static void igmp6_join_group(struct ifmcaddr6 *ma);
 static void igmp6_leave_group(struct ifmcaddr6 *ma);
@@ -270,7 +271,7 @@ int ipv6_sock_mc_drop(struct sock *sk, i
 
 				if (idev) {
 					(void) ip6_mc_leave_src(sk,mc_lst,idev);
-					__ipv6_dev_mc_dec(idev, &mc_lst->addr);
+					ipv6_dev_mc_dec_intern(idev,&mc_lst->addr,1);
 					in6_dev_put(idev);
 				}
 				dev_put(dev);
@@ -336,7 +337,7 @@ void ipv6_sock_mc_close(struct sock *sk)
 
 			if (idev) {
 				(void) ip6_mc_leave_src(sk, mc_lst, idev);
-				__ipv6_dev_mc_dec(idev, &mc_lst->addr);
+				ipv6_dev_mc_dec_intern(idev, &mc_lst->addr, 1);
 				in6_dev_put(idev);
 			}
 			dev_put(dev);
@@ -907,10 +908,9 @@ int ipv6_dev_mc_inc(struct net_device *d
 	return 0;
 }
 
-/*
- *	device multicast group del
- */
-int __ipv6_dev_mc_dec(struct inet6_dev *idev, struct in6_addr *addr)
+
+static int ipv6_dev_mc_dec_intern(struct inet6_dev *idev, 
+			struct in6_addr *addr, int delta)
 {
 	struct ifmcaddr6 *ma, **map;
 
@@ -920,20 +920,27 @@ int __ipv6_dev_mc_dec(struct inet6_dev *
 			if (--ma->mca_users == 0) {
 				*map = ma->next;
 				write_unlock_bh(&idev->lock);
-
 				igmp6_group_dropped(ma);
-
 				ma_put(ma);
 				return 0;
 			}
 			write_unlock_bh(&idev->lock);
+			if (!delta && ip6_mc_del_src(idev, addr, 
+					MCAST_EXCLUDE, 0, NULL, 0))
+				return -EINVAL; // bug 	
 			return 0;
 		}
 	}
 	write_unlock_bh(&idev->lock);
-
 	return -ENOENT;
 }
+/*
+ *	device multicast group del
+ */
+int __ipv6_dev_mc_dec(struct inet6_dev *idev, struct in6_addr *addr)
+{
+	return ipv6_dev_mc_dec_intern(idev, addr, 0);
+}
 
 int ipv6_dev_mc_dec(struct net_device *dev, struct in6_addr *addr)
 {

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 5996 bytes --]

--- linux-2.6.14/net/ipv6/mcast.c	2005-10-30 23:09:33.000000000 +0800
+++ linux/net/ipv6/mcast.c	2005-11-02 21:25:27.000000000 +0800
@@ -128,7 +128,8 @@ static DEFINE_RWLOCK(ipv6_sk_mc_lock);
 
 static struct socket *igmp6_socket;
 
-int __ipv6_dev_mc_dec(struct inet6_dev *idev, struct in6_addr *addr);
+static int ipv6_dev_mc_dec_intern(struct inet6_dev *idev, 
+			struct in6_addr *addr, int delta);
 
 static void igmp6_join_group(struct ifmcaddr6 *ma);
 static void igmp6_leave_group(struct ifmcaddr6 *ma);
@@ -164,7 +165,7 @@ static int ip6_mc_leave_src(struct sock 
 #define MLDV2_MASK(value, nb) ((nb)>=32 ? (value) : ((1<<(nb))-1) & (value))
 #define MLDV2_EXP(thresh, nbmant, nbexp, value) \
 	((value) < (thresh) ? (value) : \
-	((MLDV2_MASK(value, nbmant) | (1<<(nbmant+nbexp))) << \
+	((MLDV2_MASK(value, nbmant) | (1<<(nbmant))) << \
 	(MLDV2_MASK((value) >> (nbmant), nbexp) + (nbexp))))
 
 #define MLDV2_QQIC(value) MLDV2_EXP(0x80, 4, 3, value)
@@ -270,7 +271,7 @@ int ipv6_sock_mc_drop(struct sock *sk, i
 
 				if (idev) {
 					(void) ip6_mc_leave_src(sk,mc_lst,idev);
-					__ipv6_dev_mc_dec(idev, &mc_lst->addr);
+					ipv6_dev_mc_dec_intern(idev,&mc_lst->addr,1);
 					in6_dev_put(idev);
 				}
 				dev_put(dev);
@@ -336,7 +337,7 @@ void ipv6_sock_mc_close(struct sock *sk)
 
 			if (idev) {
 				(void) ip6_mc_leave_src(sk, mc_lst, idev);
-				__ipv6_dev_mc_dec(idev, &mc_lst->addr);
+				ipv6_dev_mc_dec_intern(idev, &mc_lst->addr, 1);
 				in6_dev_put(idev);
 			}
 			dev_put(dev);
@@ -545,8 +546,10 @@ int ip6_mc_msfilter(struct sock *sk, str
 			sock_kfree_s(sk, newpsl, IP6_SFLSIZE(newpsl->sl_max));
 			goto done;
 		}
-	} else
+	} else {
 		newpsl = NULL;
+		(void) ip6_mc_add_src(idev, group, gsf->gf_fmode, 0, NULL, 0);
+	}
 	psl = pmc->sflist;
 	if (psl) {
 		(void) ip6_mc_del_src(idev, group, pmc->sfmode,
@@ -907,10 +910,8 @@ int ipv6_dev_mc_inc(struct net_device *d
 	return 0;
 }
 
-/*
- *	device multicast group del
- */
-int __ipv6_dev_mc_dec(struct inet6_dev *idev, struct in6_addr *addr)
+static int ipv6_dev_mc_dec_intern(struct inet6_dev *idev, 
+			struct in6_addr *addr, int delta)
 {
 	struct ifmcaddr6 *ma, **map;
 
@@ -920,20 +921,27 @@ int __ipv6_dev_mc_dec(struct inet6_dev *
 			if (--ma->mca_users == 0) {
 				*map = ma->next;
 				write_unlock_bh(&idev->lock);
-
 				igmp6_group_dropped(ma);
-
 				ma_put(ma);
 				return 0;
 			}
 			write_unlock_bh(&idev->lock);
+			if (!delta && ip6_mc_del_src(idev, addr, 
+					MCAST_EXCLUDE, 0, NULL, 0))
+			       return -EINVAL; // bug 	
 			return 0;
 		}
 	}
 	write_unlock_bh(&idev->lock);
-
 	return -ENOENT;
 }
+/*
+ *	device multicast group del
+ */
+int __ipv6_dev_mc_dec(struct inet6_dev *idev, struct in6_addr *addr)
+{
+	return ipv6_dev_mc_dec_intern(idev, addr, 0);
+}
 
 int ipv6_dev_mc_dec(struct net_device *dev, struct in6_addr *addr)
 {
@@ -1087,7 +1095,7 @@ static void mld_marksources(struct ifmca
 
 int igmp6_event_query(struct sk_buff *skb)
 {
-	struct mld2_query *mlh2 = (struct mld2_query *) skb->h.raw;
+	struct mld2_query *mlh2 = NULL;
 	struct ifmcaddr6 *ma;
 	struct in6_addr *group;
 	unsigned long max_delay;
@@ -1140,6 +1148,21 @@ int igmp6_event_query(struct sk_buff *sk
 		/* clear deleted report items */
 		mld_clear_delrec(idev);
 	} else if (len >= 28) {
+		int srcs_offset = sizeof(struct mld2_query) -
+			sizeof(struct icmp6hdr);
+		if (!pskb_may_pull(skb, srcs_offset)) {
+			in6_dev_put(idev);
+			return -EINVAL;
+		}
+		mlh2 = (struct mld2_query *) skb->h.raw;
+		if (mlh2->nsrcs != 0) {
+			if (!pskb_may_pull(skb, srcs_offset +
+				mlh2->nsrcs * sizeof(struct in6_addr))) {
+				in6_dev_put(idev);
+				return -EINVAL;
+			}
+			mlh2 = (struct mld2_query *) skb->h.raw;
+		}
 		max_delay = (MLDV2_MRC(ntohs(mlh2->mrc))*HZ)/1000;
 		if (!max_delay)
 			max_delay = 1;
@@ -1256,10 +1279,13 @@ static int is_in(struct ifmcaddr6 *pmc, 
 {
 	switch (type) {
 	case MLD2_MODE_IS_INCLUDE:
-	case MLD2_MODE_IS_EXCLUDE:
 		if (gdeleted || sdeleted)
 			return 0;
 		return !((pmc->mca_flags & MAF_GSQUERY) && !psf->sf_gsresp);
+	case MLD2_MODE_IS_EXCLUDE:
+		if (gdeleted || sdeleted)
+			return 0;
+		return 1;
 	case MLD2_CHANGE_TO_INCLUDE:
 		if (gdeleted || sdeleted)
 			return 0;
@@ -1428,13 +1454,15 @@ static struct sk_buff *add_grec(struct s
 	struct mld2_report *pmr;
 	struct mld2_grec *pgr = NULL;
 	struct ip6_sf_list *psf, *psf_next, *psf_prev, **psf_list;
-	int scount, first, isquery, truncate;
+	int scount, first, isquery, ischange, truncate;
 
 	if (pmc->mca_flags & MAF_NOREPORT)
 		return skb;
 
 	isquery = type == MLD2_MODE_IS_INCLUDE ||
 		  type == MLD2_MODE_IS_EXCLUDE;
+	ischange = type == MLD2_CHANGE_TO_INCLUDE ||
+		   type == MLD2_CHANGE_TO_EXCLUDE; 
 	truncate = type == MLD2_MODE_IS_EXCLUDE ||
 		    type == MLD2_CHANGE_TO_EXCLUDE;
 
@@ -1444,7 +1472,7 @@ static struct sk_buff *add_grec(struct s
 		if (type == MLD2_ALLOW_NEW_SOURCES ||
 		    type == MLD2_BLOCK_OLD_SOURCES)
 			return skb;
-		if (pmc->mca_crcount || isquery) {
+		if (ischange || isquery) {
 			/* make sure we have room for group header and at
 			 * least one source.
 			 */
@@ -1460,9 +1488,12 @@ static struct sk_buff *add_grec(struct s
 	pmr = skb ? (struct mld2_report *)skb->h.raw : NULL;
 
 	/* EX and TO_EX get a fresh packet, if needed */
-	if (truncate) {
-		if (pmr && pmr->ngrec &&
-		    AVAILABLE(skb) < grec_size(pmc, type, gdeleted, sdeleted)) {
+	if (truncate || ischange) {
+		int min_len;
+		min_len	= truncate ? grec_size(pmc, type, gdeleted, sdeleted) : 
+			  (sizeof(struct mld2_grec) + sizeof(struct in6_addr));
+		if (((pmr && pmr->ngrec) || ischange) &&
+		    AVAILABLE(skb) < min_len) {
 			if (skb)
 				mld_sendpack(skb);
 			skb = mld_newpack(dev, dev->mtu);
@@ -1471,6 +1502,10 @@ static struct sk_buff *add_grec(struct s
 	first = 1;
 	scount = 0;
 	psf_prev = NULL;
+	if (ischange) {
+		skb = add_grhead(skb, pmc, type, &pgr);
+		first = 0;
+	}
 	for (psf=*psf_list; psf; psf=psf_next) {
 		struct in6_addr *psrc;
 

Re: [PATCH][MCAST]Two fix for implementation of MLDv2 .

[Yan Zheng]

I find something interesting .  Maybe more changes for is_in(...) are needed.


static int is_in(struct ifmcaddr6 *pmc, struct ip6_sf_list *psf, int type,
	int gdeleted, int sdeleted)
{
	switch (type) {
	case MLD2_MODE_IS_INCLUDE:
	case MLD2_CHANGE_TO_INCLUDE:
		if (gdeleted || sdeleted)
			return 0;
		if (psf->sf_count[MCAST_INCLUDE] == 0)
			return 0;    // maybe never happen
		if (type == MLD2_CHANGE_TO_INCLUDE)
			return 1;
		return !((pmc->mca_flags & MAF_GSQUERY) && !psf->sf_gsresp);
	case MLD2_MODE_IS_EXCLUDE:
	case MLD2_CHANGE_TO_EXCLUDE:
		if (gdeleted || sdeleted)
			return 0;
		if (pmc->mca_sfcount[MCAST_EXCLUDE] == 0 ||
		    psf->sf_count[MCAST_INCLUDE])
			return 0;
		if (type == MLD2_MODE_IS_EXCLUDE)
			return 1;
		return pmc->mca_sfcount[MCAST_EXCLUDE] ==
			psf->sf_count[MCAST_EXCLUDE];
	case MLD2_ALLOW_NEW_SOURCES:
		if (gdeleted || !psf->sf_crcount)
			return 0;
		return (pmc->mca_sfmode == MCAST_INCLUDE) ^ sdeleted;
	case MLD2_BLOCK_OLD_SOURCES:
		if (pmc->mca_sfmode == MCAST_INCLUDE)
			return gdeleted || (psf->sf_crcount && sdeleted);
		return psf->sf_crcount && !gdeleted && !sdeleted;
	}
	return 0;
}

Re: [PATCH][MCAST]Two fix for implementation of MLDv2 .

[Yan Zheng]

You can test change in add_grec(...) by follow codes. without the
change no change to exclude report will be sent.

please adjust IFINDEX and make sure initial state for FF02::2 is idle
===================================================
#include <stdio.h>
#include <unistd.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <sys/types.h>
#define IFINDEX 6

int main(int argc, char argv[])
{
	int sockfds[2];
	struct ipv6_mreq req;
	struct group_filter filter;
	struct sockaddr_in6 *psin6;
	req.ipv6mr_interface = IFINDEX;
	inet_pton(PF_INET6, "FF02::2", &req.ipv6mr_multiaddr);

	sockfds[0] = socket(PF_INET6, SOCK_DGRAM, 0);
	sockfds[1] = socket(PF_INET6, SOCK_DGRAM, 0);

	filter.gf_interface = IFINDEX;
	filter.gf_fmode = MCAST_INCLUDE;
	filter.gf_numsrc = 1;
	psin6 = (struct sockaddr_in6 *)&filter.gf_group;
	psin6->sin6_family = AF_INET6;
	inet_pton(PF_INET6, "FF02::2", &psin6->sin6_addr);
	psin6 = (struct sockaddr_in6 *)&filter.gf_slist[0];
	psin6->sin6_family = AF_INET6;
	inet_pton(PF_INET6, "2002:de12:1780::1", &psin6->sin6_addr);

	setsockopt(sockfds[0], SOL_IPV6, IPV6_ADD_MEMBERSHIP, &req, sizeof(req));
	setsockopt(sockfds[0], SOL_IPV6, MCAST_MSFILTER, &filter, sizeof(filter));

	sleep(5);
	setsockopt(sockfds[1], SOL_IPV6, IPV6_ADD_MEMBERSHIP, &req, sizeof(req));

	pause();
	return 0;
}

Re: [PATCH][MCAST]Two fix for implementation of MLDv2 .

[David Stevens]

Yan,
        I'm looking at this one. My original interpretation was that a
group and source specific query should only be answered if the source
were explicitly listed (which is what the code does), but I see your 
point.
A general or group-specific query should still be answered with the
existing code, and the unsolicited reports will also include the EXCLUDE
records for that group, so I'm not aware of any actual circumstances
where a multicast router wouldn't forward. But maybe you have such
a case. An administrator-initiated query, however, would certainly be
misleading to the administrator in the example you provided, and so
far I tend to agree that there should be a record in the report.
        However, I'm not sure your solution is appropriate since it
appears to include EXCLUDE records in cases where they aren't
needed.
        So, I'll look at this more carefully and see if I still agree
it needs a fix and whether or not your patch, or some alternative
method might be more appropriate. But it'll probably be sometime
next week before I'll be done reviewing/considering alternatives on
this one.

                                        +-DLS

Re: [PATCH][MCAST]Two fix for implementation of MLDv2 .

[Yan Zheng]

>         So, I'll look at this more carefully and see if I still agree
> it needs a fix and whether or not your patch, or some alternative
> method might be more appropriate. But it'll probably be sometime
> next week before I'll be done reviewing/considering alternatives on
> this one.
>
>                                         +-DLS
>
>


I am sorry, I can't understand your opinion completely.
Please forgive my poor english :-)

Here is my  opinion:

I think Multicast Address and Source Specific Query is sent only when
router want to block traffic from some source.  So when the filter
mode is exclude, node should send a report which includes a
MODE_IS_INCLUDE Record with sources in the query but NOT in the
filter's source list. This is the required behaviour in rfc3810, but
it need addiction item in struct ifmcaddr6 to record sources in the
query.  So I think make process Multicast Address Specific Query and
Multicast Address and Source Specific Query no difference is a
temporary fix.

the secoend change in is_in(...) is because I think include/exclude
counts also should be checked when type is MLD2_MODE_IS_INCLUDE or
MLD2_MODE_IS_EXCLUDE.



Regards

Here is my modify version is_in(...)
-----------------------------------------------------------------------------------------------
static int is_in(struct ifmcaddr6 *pmc, struct ip6_sf_list *psf, int type,
	int gdeleted, int sdeleted)
{
	switch (type) {
	case MLD2_MODE_IS_INCLUDE:
	case MLD2_CHANGE_TO_INCLUDE:
		if (gdeleted || sdeleted)
			return 0;
		if (psf->sf_count[MCAST_INCLUDE] == 0)
			return 0;    // maybe never happen
		if (type == MLD2_CHANGE_TO_INCLUDE)
			return 1;
		return !((pmc->mca_flags & MAF_GSQUERY) && !psf->sf_gsresp);
	case MLD2_MODE_IS_EXCLUDE:
	case MLD2_CHANGE_TO_EXCLUDE:
		if (gdeleted || sdeleted)
			return 0;
		if (pmc->mca_sfcount[MCAST_EXCLUDE] == 0 ||
		    psf->sf_count[MCAST_INCLUDE])
			return 0;
		return pmc->mca_sfcount[MCAST_EXCLUDE] ==
			psf->sf_count[MCAST_EXCLUDE];
	case MLD2_ALLOW_NEW_SOURCES:
		if (gdeleted || !psf->sf_crcount)
			return 0;
		return (pmc->mca_sfmode == MCAST_INCLUDE) ^ sdeleted;
	case MLD2_BLOCK_OLD_SOURCES:
		if (pmc->mca_sfmode == MCAST_INCLUDE)
			return gdeleted || (psf->sf_crcount && sdeleted);
		return psf->sf_crcount && !gdeleted && !sdeleted;
	}
	return 0;
}