Re: Don't link multicast records to the first address
On 22 November 2013 09:26, Martin Pieuchot mpieuc...@nolizard.org wrote: On 18/11/13(Mon) 11:43, Martin Pieuchot wrote: Diff below changes the way protocol multicast addresses are linked to an interface. Right now they are added to a list attached to the first protocol address of an interface. That makes this address descriptor and its position in the global list special. Plus in the IPv6 case, a special kludge is used to move multicast records from one address to another. So this diff reuse the design of the protocol agnostic struct ifaddr and adds a new list of multicast addresses, struct ifmaddr, to the interface descriptor. It solves the problems described previously and as a bonus properly free the IPv4 multicast records of an interface when it is detached, thus plugging some more leaks. I tested it with a carp setup, I appreciate any multicast related tests. Here's an updated diff after recent header changes. looks better, indeed. OK mikeb Did anybody tested it? Does it break one of your use cases? Comments?
Re: Don't link multicast records to the first address
On 18/11/13(Mon) 11:43, Martin Pieuchot wrote:
Diff below changes the way protocol multicast addresses are linked to
an interface.
Right now they are added to a list attached to the first protocol
address of an interface. That makes this address descriptor and
its position in the global list special. Plus in the IPv6 case,
a special kludge is used to move multicast records from one
address to another.
So this diff reuse the design of the protocol agnostic struct ifaddr
and adds a new list of multicast addresses, struct ifmaddr, to the
interface descriptor. It solves the problems described previously and
as a bonus properly free the IPv4 multicast records of an interface
when it is detached, thus plugging some more leaks.
I tested it with a carp setup, I appreciate any multicast related
tests.
Here's an updated diff after recent header changes.
Did anybody tested it? Does it break one of your use cases? Comments?
Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.277
diff -u -p -r1.277 if.c
--- net/if.c19 Nov 2013 09:00:43 - 1.277
+++ net/if.c22 Nov 2013 08:17:11 -
@@ -437,8 +437,9 @@ if_attach(struct ifnet *ifp)
void
if_attach_common(struct ifnet *ifp)
{
-
TAILQ_INIT(ifp-if_addrlist);
+ TAILQ_INIT(ifp-if_maddrlist);
+
ifp-if_addrhooks = malloc(sizeof(*ifp-if_addrhooks),
M_TEMP, M_WAITOK);
TAILQ_INIT(ifp-if_addrhooks);
Index: net/if_var.h
===
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.1
diff -u -p -r1.1 if_var.h
--- net/if_var.h21 Nov 2013 17:32:12 - 1.1
+++ net/if_var.h22 Nov 2013 08:17:11 -
@@ -123,6 +123,7 @@ struct ifnet { /* and the
entries */
TAILQ_ENTRY(ifnet) if_list; /* all struct ifnets are chained */
TAILQ_ENTRY(ifnet) if_txlist; /* list of ifnets ready to tx */
TAILQ_HEAD(, ifaddr) if_addrlist; /* linked list of addresses per if */
+ TAILQ_HEAD(, ifmaddr) if_maddrlist; /* list of multicast records */
TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */
struct hook_desc_head *if_addrhooks; /* address change callbacks */
struct hook_desc_head *if_linkstatehooks; /* link change callbacks */
@@ -302,6 +303,16 @@ struct ifaddr_item {
struct ifaddr *ifai_ifa;
struct ifaddr_item *ifai_next;
u_intifai_rdomain;
+};
+
+/*
+ * Interface multicast address.
+ */
+struct ifmaddr {
+ struct sockaddr *ifma_addr; /* Protocol address */
+ struct ifnet*ifma_ifp; /* Back pointer to ifnet */
+ unsigned int ifma_refcnt; /* Count of references */
+ TAILQ_ENTRY(ifmaddr) ifma_list; /* Per-interface list */
};
/*
Index: netinet/igmp.c
===
RCS file: /cvs/src/sys/netinet/igmp.c,v
retrieving revision 1.35
diff -u -p -r1.35 igmp.c
--- netinet/igmp.c 18 Oct 2013 09:04:02 - 1.35
+++ netinet/igmp.c 22 Nov 2013 08:17:12 -
@@ -83,6 +83,7 @@
#include sys/sysctl.h
#include net/if.h
+#include net/if_var.h
#include net/route.h
#include netinet/in.h
@@ -193,6 +194,7 @@ igmp_input(struct mbuf *m, ...)
struct igmp *igmp;
int igmplen;
int minlen;
+ struct ifmaddr *ifma;
struct in_multi *inm;
struct router_info *rti;
struct in_ifaddr *ia;
@@ -266,7 +268,10 @@ igmp_input(struct mbuf *m, ...)
* except those that are already running and those
* that belong to a local group (224.0.0.X).
*/
- IN_FOREACH_MULTI(ia, ifp, inm) {
+ TAILQ_FOREACH(ifma, ifp-if_maddrlist, ifma_list) {
+ if (ifma-ifma_addr-sa_family != AF_INET)
+ continue;
+ inm = ifmatoinm(ifma);
if (inm-inm_timer == 0
!IN_LOCAL_GROUP(inm-inm_addr.s_addr)) {
inm-inm_state = IGMP_DELAYING_MEMBER;
@@ -294,7 +299,10 @@ igmp_input(struct mbuf *m, ...)
* timers already running, check if they need to be
* reset.
*/
- IN_FOREACH_MULTI(ia, ifp, inm) {
+ TAILQ_FOREACH(ifma, ifp-if_maddrlist, ifma_list) {
+ if (ifma-ifma_addr-sa_family != AF_INET)
+ continue;
+ inm = ifmatoinm(ifma);
if (!IN_LOCAL_GROUP(inm-inm_addr.s_addr)
Re: Don't link multicast records to the first address
I tried the old version (I'm using igmpproxy on my firewall and have native multicast over pppoe), no problems, but I haven't carefully read the diff yet.
Re: Don't link multicast records to the first address
On 18/11/13(Mon) 12:54, Alexey E. Suslikov wrote:
Martin Pieuchot mpieuchot at nolizard.org writes:
at at -1803,8 +1651,12 at at in6_delmulti(struct in6_multi
*in6m)
snip
+ s = splsoftnet();
+ TAILQ_REMOVE(ifp-if_maddrlist, in6m-in6m_ifma, ifma_list);
+ splx(s);
+
free(in6m, M_IPMADDR);
}
splx(s);
this splx seems wrong.
Good catch! Here's and updated version, that comes with more spl love.
Index: net/if.c
===
RCS file: /home/ncvs/src/sys/net/if.c,v
retrieving revision 1.277
diff -u -p -r1.277 if.c
--- net/if.c19 Nov 2013 09:00:43 - 1.277
+++ net/if.c19 Nov 2013 10:07:24 -
@@ -437,8 +437,9 @@ if_attach(struct ifnet *ifp)
void
if_attach_common(struct ifnet *ifp)
{
-
TAILQ_INIT(ifp-if_addrlist);
+ TAILQ_INIT(ifp-if_maddrlist);
+
ifp-if_addrhooks = malloc(sizeof(*ifp-if_addrhooks),
M_TEMP, M_WAITOK);
TAILQ_INIT(ifp-if_addrhooks);
Index: net/if.h
===
RCS file: /home/ncvs/src/sys/net/if.h,v
retrieving revision 1.152
diff -u -p -r1.152 if.h
--- net/if.h9 Nov 2013 06:38:42 - 1.152
+++ net/if.h19 Nov 2013 10:07:24 -
@@ -262,6 +262,7 @@ struct ifnet { /* and the
entries */
TAILQ_ENTRY(ifnet) if_list; /* all struct ifnets are chained */
TAILQ_ENTRY(ifnet) if_txlist; /* list of ifnets ready to tx */
TAILQ_HEAD(, ifaddr) if_addrlist; /* linked list of addresses per if */
+ TAILQ_HEAD(, ifmaddr) if_maddrlist; /* list of multicast records */
TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */
struct hook_desc_head *if_addrhooks; /* address change callbacks */
struct hook_desc_head *if_linkstatehooks; /* link change callbacks */
@@ -506,6 +507,16 @@ struct ifaddr_item {
struct ifaddr *ifai_ifa;
struct ifaddr_item *ifai_next;
u_intifai_rdomain;
+};
+
+/*
+ * Interface multicast address.
+ */
+struct ifmaddr {
+ struct sockaddr *ifma_addr; /* Protocol address */
+ struct ifnet*ifma_ifp; /* Back pointer to ifnet */
+ unsigned int ifma_refcnt; /* Count of references */
+ TAILQ_ENTRY(ifmaddr) ifma_list; /* Per-interface list */
};
/*
Index: netinet/igmp.c
===
RCS file: /home/ncvs/src/sys/netinet/igmp.c,v
retrieving revision 1.35
diff -u -p -r1.35 igmp.c
--- netinet/igmp.c 18 Oct 2013 09:04:02 - 1.35
+++ netinet/igmp.c 19 Nov 2013 10:07:24 -
@@ -193,6 +193,7 @@ igmp_input(struct mbuf *m, ...)
struct igmp *igmp;
int igmplen;
int minlen;
+ struct ifmaddr *ifma;
struct in_multi *inm;
struct router_info *rti;
struct in_ifaddr *ia;
@@ -266,7 +267,10 @@ igmp_input(struct mbuf *m, ...)
* except those that are already running and those
* that belong to a local group (224.0.0.X).
*/
- IN_FOREACH_MULTI(ia, ifp, inm) {
+ TAILQ_FOREACH(ifma, ifp-if_maddrlist, ifma_list) {
+ if (ifma-ifma_addr-sa_family != AF_INET)
+ continue;
+ inm = ifmatoinm(ifma);
if (inm-inm_timer == 0
!IN_LOCAL_GROUP(inm-inm_addr.s_addr)) {
inm-inm_state = IGMP_DELAYING_MEMBER;
@@ -294,7 +298,10 @@ igmp_input(struct mbuf *m, ...)
* timers already running, check if they need to be
* reset.
*/
- IN_FOREACH_MULTI(ia, ifp, inm) {
+ TAILQ_FOREACH(ifma, ifp-if_maddrlist, ifma_list) {
+ if (ifma-ifma_addr-sa_family != AF_INET)
+ continue;
+ inm = ifmatoinm(ifma);
if (!IN_LOCAL_GROUP(inm-inm_addr.s_addr)
(ip-ip_dst.s_addr == INADDR_ALLHOSTS_GROUP
||
ip-ip_dst.s_addr ==
inm-inm_addr.s_addr)) {
@@ -479,6 +486,8 @@ void
igmp_leavegroup(struct in_multi *inm)
{
+ int s = splsoftnet();
+
switch (inm-inm_state) {
case IGMP_DELAYING_MEMBER:
case IGMP_IDLE_MEMBER:
@@ -494,6 +503,7 @@ igmp_leavegroup(struct in_multi *inm)
case IGMP_SLEEPING_MEMBER:
break;
}
+ splx(s);
}
void
@@ -521,11 +531,14 @@ void
igmp_checktimer(struct ifnet *ifp)
{
struct in_multi *inm;
- struct in_ifaddr
Don't link multicast records to the first address
Diff below changes the way protocol multicast addresses are linked to
an interface.
Right now they are added to a list attached to the first protocol
address of an interface. That makes this address descriptor and
its position in the global list special. Plus in the IPv6 case,
a special kludge is used to move multicast records from one
address to another.
So this diff reuse the design of the protocol agnostic struct ifaddr
and adds a new list of multicast addresses, struct ifmaddr, to the
interface descriptor. It solves the problems described previously and
as a bonus properly free the IPv4 multicast records of an interface
when it is detached, thus plugging some more leaks.
I tested it with a carp setup, I appreciate any multicast related
tests.
Comments, ok?
Index: net/if.c
===
RCS file: /home/ncvs/src/sys/net/if.c,v
retrieving revision 1.275
diff -u -p -r1.275 if.c
--- net/if.c11 Nov 2013 09:15:34 - 1.275
+++ net/if.c18 Nov 2013 10:43:00 -
@@ -441,8 +441,9 @@ if_attach(struct ifnet *ifp)
void
if_attach_common(struct ifnet *ifp)
{
-
TAILQ_INIT(ifp-if_addrlist);
+ TAILQ_INIT(ifp-if_maddrlist);
+
ifp-if_addrhooks = malloc(sizeof(*ifp-if_addrhooks),
M_TEMP, M_WAITOK);
TAILQ_INIT(ifp-if_addrhooks);
Index: net/if.h
===
RCS file: /home/ncvs/src/sys/net/if.h,v
retrieving revision 1.152
diff -u -p -r1.152 if.h
--- net/if.h9 Nov 2013 06:38:42 - 1.152
+++ net/if.h18 Nov 2013 10:43:00 -
@@ -262,6 +262,7 @@ struct ifnet { /* and the
entries */
TAILQ_ENTRY(ifnet) if_list; /* all struct ifnets are chained */
TAILQ_ENTRY(ifnet) if_txlist; /* list of ifnets ready to tx */
TAILQ_HEAD(, ifaddr) if_addrlist; /* linked list of addresses per if */
+ TAILQ_HEAD(, ifmaddr) if_maddrlist; /* list of multicast records */
TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */
struct hook_desc_head *if_addrhooks; /* address change callbacks */
struct hook_desc_head *if_linkstatehooks; /* link change callbacks */
@@ -506,6 +507,16 @@ struct ifaddr_item {
struct ifaddr *ifai_ifa;
struct ifaddr_item *ifai_next;
u_intifai_rdomain;
+};
+
+/*
+ * Interface multicast address.
+ */
+struct ifmaddr {
+ struct sockaddr *ifma_addr; /* Protocol address */
+ struct ifnet*ifma_ifp; /* Back pointer to ifnet */
+ unsigned int ifma_refcnt; /* Count of references */
+ TAILQ_ENTRY(ifmaddr) ifma_list; /* Per-interface list */
};
/*
Index: netinet/igmp.c
===
RCS file: /home/ncvs/src/sys/netinet/igmp.c,v
retrieving revision 1.35
diff -u -p -r1.35 igmp.c
--- netinet/igmp.c 18 Oct 2013 09:04:02 - 1.35
+++ netinet/igmp.c 18 Nov 2013 10:43:00 -
@@ -193,6 +193,7 @@ igmp_input(struct mbuf *m, ...)
struct igmp *igmp;
int igmplen;
int minlen;
+ struct ifmaddr *ifma;
struct in_multi *inm;
struct router_info *rti;
struct in_ifaddr *ia;
@@ -266,7 +267,10 @@ igmp_input(struct mbuf *m, ...)
* except those that are already running and those
* that belong to a local group (224.0.0.X).
*/
- IN_FOREACH_MULTI(ia, ifp, inm) {
+ TAILQ_FOREACH(ifma, ifp-if_maddrlist, ifma_list) {
+ if (ifma-ifma_addr-sa_family != AF_INET)
+ continue;
+ inm = ifmatoinm(ifma);
if (inm-inm_timer == 0
!IN_LOCAL_GROUP(inm-inm_addr.s_addr)) {
inm-inm_state = IGMP_DELAYING_MEMBER;
@@ -294,7 +298,10 @@ igmp_input(struct mbuf *m, ...)
* timers already running, check if they need to be
* reset.
*/
- IN_FOREACH_MULTI(ia, ifp, inm) {
+ TAILQ_FOREACH(ifma, ifp-if_maddrlist, ifma_list) {
+ if (ifma-ifma_addr-sa_family != AF_INET)
+ continue;
+ inm = ifmatoinm(ifma);
if (!IN_LOCAL_GROUP(inm-inm_addr.s_addr)
(ip-ip_dst.s_addr == INADDR_ALLHOSTS_GROUP
||
ip-ip_dst.s_addr ==
inm-inm_addr.s_addr)) {
@@ -521,11 +528,14 @@ void
igmp_checktimer(struct ifnet *ifp)
{
struct in_multi *inm;
- struct in_ifaddr *ia;
+ struct ifmaddr *ifma;
Re: Don't link multicast records to the first address
Martin Pieuchot mpieuchot at nolizard.org writes: at at -1803,8 +1651,12 at at in6_delmulti(struct in6_multi *in6m) snip + s = splsoftnet(); + TAILQ_REMOVE(ifp-if_maddrlist, in6m-in6m_ifma, ifma_list); + splx(s); + free(in6m, M_IPMADDR); } splx(s); this splx seems wrong.
