[PATCH 2/2][MCAST] Fix for add_grec(...)

[PATCH 2/2][MCAST] Fix for add_grec(...)

[Yan Zheng]

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

Hi

When ifmcaddr6's mca_sources is not NULL, but none of the sources in the list can be included in report,
(For example: when filter mode is exclude and the only source in the list has include count greater than zero.)
add_grec(...) may returns without add any data to the sk_buff. So MLD2_CHANGE_TO_EXCLUDE or MLD2_MODE_IS_EXCLUDE
report may be eliminated.

You can check this bug by test1.c in attachments. You will notice that there is no MLD2_CHANGE_TO_EXCLUDE report.

Regards

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

Index:net/ipv6/mcast.c
==============================================================
--- linux-2.6.14/net/ipv6/mcast.c	2005-11-09 16:00:48.000000000 +0800
+++ linux/net/ipv6/mcast.c	2005-11-09 19:47:50.000000000 +0800
@@ -1445,18 +1445,21 @@ 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;
+		   type == MLD2_CHANGE_TO_EXCLUDE;
 
 	psf_list = sdeleted ? &pmc->mca_tomb : &pmc->mca_sources;
 
+#if 0
 	if (!*psf_list) {
 		if (type == MLD2_ALLOW_NEW_SOURCES ||
 		    type == MLD2_BLOCK_OLD_SOURCES)
@@ -1474,12 +1477,15 @@ static struct sk_buff *add_grec(struct s
 		}
 		return skb;
 	}
+#endif
 	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 && AVAILABLE(skb) < min_len) {
 			if (skb)
 				mld_sendpack(skb);
 			skb = mld_newpack(dev, dev->mtu);
@@ -1488,6 +1494,10 @@ static struct sk_buff *add_grec(struct s
 	first = 1;
 	scount = 0;
 	psf_prev = NULL;
+	if (ischange || type == MLD2_MODE_IS_EXCLUDE) {
+		skb = add_grhead(skb, pmc, type, &pgr);
+		first = 0;
+	}
 	for (psf=*psf_list; psf; psf=psf_next) {
 		struct in6_addr *psrc;
 




[-- Attachment #2: test1.c --]
[-- Type: text/x-csrc, Size: 1203 bytes --]

#include <stdio.h>
#include <unistd.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <sys/types.h>
#define IFINDEX 5  //Please adjust me first.

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::2000", &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::2000", &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(10); //wait state change reports
	printf("change mode to exclude\n");
	setsockopt(sockfds[1], SOL_IPV6, IPV6_ADD_MEMBERSHIP, &req, sizeof(req));

	pause();
	return 0;
}

Re: [PATCH 2/2][MCAST] Fix for add_grec(...)

[David Stevens]

Yan,
        I think your patch has some problems.

Yan Zheng <yanzheng@21cn.com> wrote on 11/09/2005 03:58:20 AM:

> +#if 0
>     if (!*psf_list) {
>        if (type == MLD2_ALLOW_NEW_SOURCES ||
>            type == MLD2_BLOCK_OLD_SOURCES)
> @@ -1474,12 +1477,15 @@ static struct sk_buff *add_grec(struct s
>        }
>        return skb;
>     }
> +#endif


        This code is the only place in the current code where you
can generate a group header with an empty source list (what it is
checking for). Your patch has added an add_grhead() for change
and EXCLUDE records, but it isn't checking mca_crcount or isquery.
I need to check, but I'm concerned this will create a group header
in a report for cases where it should not.


>     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 && AVAILABLE(skb) < min_len) {
>           if (skb)
>              mld_sendpack(skb);
>           skb = mld_newpack(dev, dev->mtu);

        This "truncate" code is to handle exclude records that may be
truncated. It gets a new packet when adding this record and the whole
thing won't fit in a single packet. This is not appropriate for anything
but IS_EX and TO_EX, but "ischange" in your patch will be true for
TO_IN. So, I think this will waste space in a report that could hold
some of these TO_IN sources.

        I haven't run your test code, or tested with your patch yet,
just observing the differences from the original code path and your
patch (and they appear to be more than you intended).

                                                        +-DLS

Re: [PATCH 2/2][MCAST] Fix for add_grec(...)

[Yan Zheng]

David Stevens wrote:
> Yan,
>         I think your patch has some problems.
> 
> Yan Zheng <yanzheng@21cn.com> wrote on 11/09/2005 03:58:20 AM:
> 
> 
>>+#if 0
>>    if (!*psf_list) {
>>       if (type == MLD2_ALLOW_NEW_SOURCES ||
>>           type == MLD2_BLOCK_OLD_SOURCES)
>>@@ -1474,12 +1477,15 @@ static struct sk_buff *add_grec(struct s
>>       }
>>       return skb;
>>    }
>>+#endif
> 
> 
> 
>         This code is the only place in the current code where you
> can generate a group header with an empty source list (what it is
> checking for). Your patch has added an add_grhead() for change
> and EXCLUDE records, but it isn't checking mca_crcount or isquery.
> I need to check, but I'm concerned this will create a group header
> in a report for cases where it should not.
> 
ischange implicits mca_crcount has already been ckecked in mld_send_cr(...)
Actually, check mca_crcount directly will get mode change report sent one times less 
than you intend, because mca_crcount has decreased by one before call add_grec(...).


Now We only have MLD2_MODE_IS_INCLUDE left. but "mode is include and source list is empty"
is an impossible event.
> 
> 
>>    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 && AVAILABLE(skb) < min_len) {
>>          if (skb)
>>             mld_sendpack(skb);
>>          skb = mld_newpack(dev, dev->mtu);
> 
> 
>         This "truncate" code is to handle exclude records that may be
> truncated. It gets a new packet when adding this record and the whole
> thing won't fit in a single packet. This is not appropriate for anything
> but IS_EX and TO_EX, but "ischange" in your patch will be true for
> TO_IN. So, I think this will waste space in a report that could hold
> some of these TO_IN sources.

When type is MLD2_MODE_IS_INCLUDE, min_len is equal to "sizeof(struct mld2_grec) + 
sizeof(struct in6_addr)". it satisfies the comment above. (make sure we have room 
for group header and at least one source.)