Skip to contentSkip to conversation

IPv6: small fix for ip6_mc_msfilter(...)

PATCH In review Yan Zheng 6 participants 18 messages Oct 31, 2005 – Nov 3, 2005

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Yan Zheng

author
#0 lore raw

PATCH 1/1 IPv6: small fix for ip6_mc_msfilter(...)

Hi.
I think ip6_mc_add_src(...) should be called when number of sources is zero and filter mode is exclude.

Regards

Signed-off-by Yan Zheng <[email protected]>
net/ipv6/mcast.c +0 -0
Index: net/ipv6/mcast.c
================================================================================
@@ -545,8 +545,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;
+		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,
#1 lore raw

You can reproduce this bug by follow codes. This program will cause a change to include report even though the first socket's filter mode is exclude.

Please adjust IFINDEX first.
========================================
#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 i, sockfds[3];
	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);

	for (i = 0; i < 3; ++i) {
		sockfds[i] = socket(PF_INET6, SOCK_DGRAM, 0);
		setsockopt(sockfds[i], SOL_IPV6, IPV6_ADD_MEMBERSHIP, &req, sizeof(req));
	}
	
	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[1], SOL_IPV6, MCAST_MSFILTER, &filter, sizeof(filter));
	
	filter.gf_fmode = MCAST_EXCLUDE;
	filter.gf_numsrc = 0;
	setsockopt(sockfds[2], SOL_IPV6, MCAST_MSFILTER, &filter, sizeof(filter));
	setsockopt(sockfds[2], SOL_IPV6, MCAST_MSFILTER, &filter, sizeof(filter));

	pause();
}
#2 lore raw

Hi,

Quoted 3 lines · Yan Zheng

You can reproduce this bug by follow codes. This program will cause a change to include report even though the first socket's filter mode is exclude.

I've not clearly understood the nature of the bug, does it also affect 2.4 ? Marcelo will be releasing 2.4.32 in a few days, so it would be wise to merge the fix soon.

Regards,
Willy

#3 lore raw

Willy Tarreau <[email protected]> wrote on 11/01/2005 09:47:02 PM:

Quoted 7 lines

Hi,

Quoted 3 lines · Yan Zheng

You can reproduce this bug by follow codes. This program will cause a change to include report even though the first socket's filter mode is exclude.

Quoted 7 lines

I've not clearly understood the nature of the bug, does it also affect 2.4 ? Marcelo will be releasing 2.4.32 in a few days, so it would be wise to merge the fix soon.

Regards,
Willy

Yes.

Multicast source filters aren't widely used yet, and that's
really the only feature that's affected if an application actually exercises this bug, as far as I can tell. An ordinary filter-less multicast join should still work, and only forwarded multicast traffic making use of filters and doing empty-source filters with the MSFILTER ioctl would be at risk of not getting multicast traffic forwarded to them because the reports generated would not be based on the correct counts.
But having the fix in is better than having it broken, even
for that case. :-)

                                                        +-DLS
#4 lore raw
Quoted 19 lines · David Stevens

Willy Tarreau <[email protected]> wrote on 11/01/2005 09:47:02 PM:

Quoted 7 lines

Hi,

Quoted 3 lines · Yan Zheng

You can reproduce this bug by follow codes. This program will cause a change to include report even though the first socket's filter mode is exclude.

Quoted 7 lines

I've not clearly understood the nature of the bug, does it also affect 2.4 ? Marcelo will be releasing 2.4.32 in a few days, so it would be wise to merge the fix soon.

Regards,
Willy

Yes.

Quoted 10 lines

Multicast source filters aren't widely used yet, and that's
really the only feature that's affected if an application actually exercises this bug, as far as I can tell. An ordinary filter-less multicast join should still work, and only forwarded multicast traffic making use of filters and doing empty-source filters with the MSFILTER ioctl would be at risk of not getting multicast traffic forwarded to them because the reports generated would not be based on the correct counts.
But having the fix in is better than having it broken, even
for that case. :-)

OK, thanks for the clarification, I understand better now :-)

Marcelo, David, does this backport seem appropriate for 2.4.32 ? I verified that it compiles, nothing more. If it's OK, I've noticed another patch that Yan posted today and which might be of interest before a very solid release.

Regards,
Willy

net/ipv4/igmp.c +5 -2
diff -urN linux-2.4.32-rc2/net/ipv4/igmp.c linux-2.4.32-rc2-mcast/net/ipv4/igmp.c
@@ -1876,8 +1876,11 @@
 			sock_kfree_s(sk, newpsl, IP_SFLSIZE(newpsl->sl_max));
 			goto done;
 		}
-	} else
-		newpsl = 0;
+	} else {
+		newpsl = NULL;
+		(void) ip_mc_add_src(in_dev, &msf->imsf_multiaddr,
+		       msf->imsf_fmode, 0, NULL, 0);
+	}
 	psl = pmc->sflist;
 	if (psl) {
 		(void) ip_mc_del_src(in_dev, &msf->imsf_multiaddr, pmc->sfmode,
net/ipv6/mcast.c +5 -2
diff -urN linux-2.4.32-rc2/net/ipv6/mcast.c linux-2.4.32-rc2-mcast/net/ipv6/mcast.c
@@ -505,8 +505,11 @@
 			sock_kfree_s(sk, newpsl, IP6_SFLSIZE(newpsl->sl_max));
 			goto done;
 		}
-	} else
-		newpsl = 0;
+	} 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,
#5 lore raw
Quoted 32 lines · Willy Tarreau
Quoted 19 lines · David Stevens

Willy Tarreau <[email protected]> wrote on 11/01/2005 09:47:02 PM:

Quoted 7 lines

Hi,

Quoted 3 lines · Yan Zheng

You can reproduce this bug by follow codes. This program will cause a change to include report even though the first socket's filter mode is exclude.

Quoted 7 lines

I've not clearly understood the nature of the bug, does it also affect 2.4 ? Marcelo will be releasing 2.4.32 in a few days, so it would be wise to merge the fix soon.

Regards,
Willy

Yes.

Quoted 10 lines

Multicast source filters aren't widely used yet, and that's
really the only feature that's affected if an application actually exercises this bug, as far as I can tell. An ordinary filter-less multicast join should still work, and only forwarded multicast traffic making use of filters and doing empty-source filters with the MSFILTER ioctl would be at risk of not getting multicast traffic forwarded to them because the reports generated would not be based on the correct counts.
But having the fix in is better than having it broken, even
for that case. :-)

Quoted 5 lines

OK, thanks for the clarification, I understand better now :-)

Marcelo, David, does this backport seem appropriate for 2.4.32 ? I verified that it compiles, nothing more. If it's OK, I've noticed another patch that Yan posted today and which might be of interest before a very solid release.

Hi Willy,

Given the fact that it is a bug correction, sure.
Could you please prepare a nice e-mail with the full description?

Thanks

#6 lore raw

Hi Marcelo,

Quoted 2 lines · Marcelo Tosatti

Given the fact that it is a bug correction, sure.
Could you please prepare a nice e-mail with the full description?

Yes, I see... email was not nice enough to the scripts :-) Here it is complete and clean below.

Regards,
Willy

[PATCH-2.4][MCAST]IPv6: small fix for ip6_mc_msfilter(...)

Multicast source filters aren't widely used yet, and that's really the only feature that's affected if an application actually exercises this bug, as far as I can tell. An ordinary filter-less multicast join should still work, and only forwarded multicast traffic making use of filters and doing empty-source filters with the MSFILTER ioctl would be at risk of not getting multicast traffic forwarded to them because the reports generated would not be based on the correct counts. Initial 2.6 patch by Yan Zheng, bug explanation by David Stevens, patch ACKed by David.

Signed-off-by Willy Tarreau <[email protected]>
net/ipv4/igmp.c +5 -2
diff -urN linux-2.4.32-rc2/net/ipv4/igmp.c linux-2.4.32-rc2-mcast/net/ipv4/igmp.c
@@ -1876,8 +1876,11 @@
 			sock_kfree_s(sk, newpsl, IP_SFLSIZE(newpsl->sl_max));
 			goto done;
 		}
-	} else
-		newpsl = 0;
+	} else {
+		newpsl = NULL;
+		(void) ip_mc_add_src(in_dev, &msf->imsf_multiaddr,
+		       msf->imsf_fmode, 0, NULL, 0);
+	}
 	psl = pmc->sflist;
 	if (psl) {
 		(void) ip_mc_del_src(in_dev, &msf->imsf_multiaddr, pmc->sfmode,
net/ipv6/mcast.c +5 -2
diff -urN linux-2.4.32-rc2/net/ipv6/mcast.c linux-2.4.32-rc2-mcast/net/ipv6/mcast.c
@@ -505,8 +505,11 @@
 			sock_kfree_s(sk, newpsl, IP6_SFLSIZE(newpsl->sl_max));
 			goto done;
 		}
-	} else
-		newpsl = 0;
+	} 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,
#7 lore raw
In reply to Willy Tarreau

Willy Tarreau <[email protected]> wrote on 11/02/2005 01:29:59 AM:

Quoted 1 line

Marcelo, David, does this backport seem appropriate for 2.4.32 ? I

verified

Quoted 1 line

that it compiles, nothing more.

Yes.

Quoted 2 lines

If it's OK, I've noticed another patch that
Yan posted today and which might be of interest before a very solid

release.

I think they should be reviewed first. :-)

+-DLS

#8 lore raw
Quoted 6 lines · David Stevens

Willy Tarreau <[email protected]> wrote on 11/02/2005 01:29:59 AM:

Quoted 1 line

Marcelo, David, does this backport seem appropriate for 2.4.32 ? I

verified

Quoted 1 line

that it compiles, nothing more.

Quoted 7 lines

Yes.

Quoted 2 lines

If it's OK, I've noticed another patch that
Yan posted today and which might be of interest before a very solid

release.

I think they should be reviewed first. :-)

Alright, then let it be tested in v2.6 first. Willy, can you queue for 2.4.33-pre ?

#9 lore raw
Quoted 9 lines · Marcelo Tosatti
Quoted 8 lines · David Stevens

Willy Tarreau <[email protected]> wrote on 11/02/2005 01:29:59 AM:

Quoted 1 line

Marcelo, David, does this backport seem appropriate for 2.4.32 ? I

verified

Quoted 1 line

that it compiles, nothing more.

Yes.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

If it's OK, I've noticed another patch that
Yan posted today and which might be of interest before a very solid

release.

I think they should be reviewed first. :-)

Alright, then let it be tested in v2.6 first. Willy, can you queue for 2.4.33-pre ?

OK.

Willy

#10 lore raw
In reply to David Stevens
Quoted 6 lines · David Stevens

Willy Tarreau <[email protected]> wrote on 11/02/2005 01:29:59 AM:

Quoted 1 line

Marcelo, David, does this backport seem appropriate for 2.4.32 ? I

verified

Quoted 1 line

that it compiles, nothing more.

Quoted 7 lines

Yes.

Quoted 2 lines

If it's OK, I've noticed another patch that
Yan posted today and which might be of interest before a very solid

release.

I think they should be reviewed first. :-)

Sure thing David, and thanks for your much appreciated review of patches for the area.

- Arnaldo

Acked-by on v1 1/1
#11 lore raw
In reply to Yan Zheng

Yan,
Please also make this equivalent change in IPv4 with
ip_mc_msfilter() and ip_mc_add_src().

                                                +-DLS
Acked-by David L Stevens <[email protected]>
Quoted 5 lines
Signed-off-by Yan Zheng <[email protected]>

Index: net/ipv6/mcast.c

================================================================================

Quoted 18 lines
net/ipv6/mcast.c 2005-10-31 10:37:36.000000000 +0800 renamed from net/ipv6/mcast.c 2005-10-30 23:09:33.000000000 +0800 +3 -2
@@ -545,8 +545,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;
+      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,
-

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#12 lore raw
Quoted 7 lines · David Stevens

Yan,
Please also make this equivalent change in IPv4 with
ip_mc_msfilter() and ip_mc_add_src().

                                                +-DLS
Acked-by David L Stevens <[email protected]>

To keep code style, I also create a new patch for IPv6. :-)

Signed-off-by Yan Zheng <[email protected]>

Patch for IPv4
Index:net/ipv4/igmp.c
============================================================

net/ipv4/igmp.c +0 -0
@@ -1908,8 +1908,11 @@ int ip_mc_msfilter(struct sock *sk, stru

			sock_kfree_s(sk, newpsl, IP_SFLSIZE(newpsl->sl_max));
			goto done;
		}
-	} else
+	} else {
		newpsl = NULL;
+		(void) ip_mc_add_src(in_dev, &msf->imsf_multiaddr,
+			msf->imsf_fmode, 0, NULL, 0)
+	}
	psl = pmc->sflist;
	if (psl) {
		(void) ip_mc_del_src(in_dev, &msf->imsf_multiaddr, pmc->sfmode,

Patch for IPv6
Index:net/ipv6/mcast.c
============================================================

net/ipv6/mcast.c +0 -0
@@ -545,8 +545,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,
Acked-by
#13 lore raw

Looks good. Thanks, Yan.

                        +-DLS
Acked-by David L Stevens <[email protected]>
Quoted 44 lines
Signed-off-by Yan Zheng <[email protected]>

Patch for IPv4
Index:net/ipv4/igmp.c
============================================================

net/ipv4/igmp.c 2005-11-02 07:31:01.000000000 +0800 renamed from net/ipv4/igmp.c 2005-10-28 08:02:08.000000000 +0800 +4 -1
@@ -1908,8 +1908,11 @@ int ip_mc_msfilter(struct sock *sk, stru
         sock_kfree_s(sk, newpsl, IP_SFLSIZE(newpsl->sl_max));
         goto done;
      }
-   } else
+   } else {
      newpsl = NULL;
+      (void) ip_mc_add_src(in_dev, &msf->imsf_multiaddr,
+         msf->imsf_fmode, 0, NULL, 0)
+   }
   psl = pmc->sflist;
   if (psl) {
      (void) ip_mc_del_src(in_dev, &msf->imsf_multiaddr, pmc->sfmode,

Patch for IPv6
Index:net/ipv6/mcast.c
============================================================

net/ipv6/mcast.c 2005-11-02 07:19:12.000000000 +0800 renamed from net/ipv6/mcast.c 2005-10-30 23:09:33.000000000 +0800 +3 -1
@@ -545,8 +545,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,
#15 lore raw
In reply to Yan Zheng
Quoted 19 lines · Yan Zheng
Quoted 7 lines · David Stevens

Yan,
Please also make this equivalent change in IPv4 with
ip_mc_msfilter() and ip_mc_add_src().

                                                +-DLS
Acked-by David L Stevens <[email protected]>

To keep code style, I also create a new patch for IPv6. :-)

Signed-off-by Yan Zheng <[email protected]>

Patch for IPv4
Index:net/ipv4/igmp.c
============================================================

net/ipv4/igmp.c 2005-11-02 07:31:01.000000000 +0800 int ip_mc_msfilter(struct sock *sk, stru · line 1914

Quoted 8 lines
net/ipv4/igmp.c 2005-11-02 07:31:01.000000000 +0800 renamed from net/ipv4/igmp.c 2005-10-28 08:02:08.000000000 +0800 +3 -1
@@ -1908,8 +1908,11 @@ int ip_mc_msfilter(struct sock *sk, stru
                        sock_kfree_s(sk, newpsl, IP_SFLSIZE(newpsl->sl_max));
                        goto done;
                }
-       } else
+       } else {
                newpsl = NULL;
+               (void) ip_mc_add_src(in_dev, &msf->imsf_multiaddr,
+                       msf->imsf_fmode, 0, NULL, 0)

Could you please compile test it next time :-) hint, missing ';'. Anyway, fixed up by hand.

- Arnaldo

author
#16 lore raw
Quoted 7 lines

Could you please compile test it next time :-) hint, missing ';'. Anyway, fixed up by hand.

- Arnaldo

I'm so sorry.

============================================================

net/ipv4/igmp.c +0 -0
@@ -1908,8 +1908,11 @@ int ip_mc_msfilter(struct sock *sk, stru

			sock_kfree_s(sk, newpsl, IP_SFLSIZE(newpsl->sl_max));
			goto done;
		}
-	} else
+	} else {
		newpsl = NULL;
+		(void) ip_mc_add_src(in_dev, &msf->imsf_multiaddr,
+			msf->imsf_fmode, 0, NULL, 0);
+	}
	psl = pmc->sflist;
	if (psl) {
		(void) ip_mc_del_src(in_dev, &msf->imsf_multiaddr, pmc->sfmode,
#17 lore raw
Quoted 9 lines · Yan Zheng
Quoted 7 lines

Could you please compile test it next time :-) hint, missing ';'. Anyway, fixed up by hand.

- Arnaldo

I'm so sorry.

Don't worry, I already fixed it and published in my net-2.6.git tree at www.kernel.org, its just that the same confidence that makes one commit something simple as in your case could introduce something nasty :-)

- Arnaldo