[PATCH] Set the correct RTNL family for multicast netconf messages

[PATCH] Set the correct RTNL family for multicast netconf messages

[Sven-Thorsten Dietrich]

There may be other cases that require a special case, so I chose the switch approach.

Please note, that all other messages from ipmr[6].c correctly set the family, while these do not.

Thanks

Sven



Subject: Set correct RTNL family for multicast netconf messages
From: Sven-Thorsten Dietrich sven@vyatta.com Thu Jun 27 16:40:17 2013 -0700
Date: Thu Jun 27 16:40:17 2013 -0700:


diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index dfc39d4..695858b 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
 		return -EMSGSIZE;
 
 	ncm = nlmsg_data(nlh);
-	ncm->ncm_family = AF_INET;
+
+	switch (type) {
+		case NETCONFA_MC_FORWARDING:
+			ncm->ncm_family = RTNL_FAMILY_IPMR;
+			break;
+
+		default:
+			ncm->ncm_family = AF_INET;
+			break;
+	}
 
 	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
 		goto nla_put_failure;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 4ab4c38..a177da4 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
 		return -EMSGSIZE;
 
 	ncm = nlmsg_data(nlh);
-	ncm->ncm_family = AF_INET6;
+
+	switch (type) {
+		case NETCONFA_MC_FORWARDING:
+			ncm->ncm_family = RTNL_FAMILY_IP6MR;
+			break;
+
+		default:
+			ncm->ncm_family = AF_INET6;
+			break;
+	}
 
 	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
 		goto nla_put_failure;

Re: [PATCH] Set the correct RTNL family for multicast netconf messages

[Hannes Frederic Sowa]

On Thu, Jun 27, 2013 at 04:44:47PM -0700, Sven-Thorsten Dietrich wrote:
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index dfc39d4..695858b 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>  		return -EMSGSIZE;
>  
>  	ncm = nlmsg_data(nlh);
> -	ncm->ncm_family = AF_INET;
> +
> +	switch (type) {
> +		case NETCONFA_MC_FORWARDING:
> +			ncm->ncm_family = RTNL_FAMILY_IPMR;
> +			break;
> +
> +		default:
> +			ncm->ncm_family = AF_INET;
> +			break;
> +	}
>  
>  	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
>  		goto nla_put_failure;
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 4ab4c38..a177da4 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>  		return -EMSGSIZE;
>  
>  	ncm = nlmsg_data(nlh);
> -	ncm->ncm_family = AF_INET6;
> +
> +	switch (type) {
> +		case NETCONFA_MC_FORWARDING:
> +			ncm->ncm_family = RTNL_FAMILY_IP6MR;
> +			break;
> +
> +		default:
> +			ncm->ncm_family = AF_INET6;
> +			break;
> +	}
>  

Hm, are you sure? NETCONFA_MC_FORWARDING is of type RTM_NEWNETCONF
and expects ncm_family to be either AF_INET or AF_INET6 (at least in
iproute2/ipmonitor.c).

Greetings,

  Hannes

Re: [PATCH] Set the correct RTNL family for multicast netconf messages

[Stephen Hemminger]

On Fri, 28 Jun 2013 03:23:07 +0200
Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:

> On Thu, Jun 27, 2013 at 04:44:47PM -0700, Sven-Thorsten Dietrich wrote:
> > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> > index dfc39d4..695858b 100644
> > --- a/net/ipv4/devinet.c
> > +++ b/net/ipv4/devinet.c
> > @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
> >  		return -EMSGSIZE;
> >  
> >  	ncm = nlmsg_data(nlh);
> > -	ncm->ncm_family = AF_INET;
> > +
> > +	switch (type) {
> > +		case NETCONFA_MC_FORWARDING:
> > +			ncm->ncm_family = RTNL_FAMILY_IPMR;
> > +			break;
> > +
> > +		default:
> > +			ncm->ncm_family = AF_INET;
> > +			break;
> > +	}
> >  
> >  	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
> >  		goto nla_put_failure;
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 4ab4c38..a177da4 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
> >  		return -EMSGSIZE;
> >  
> >  	ncm = nlmsg_data(nlh);
> > -	ncm->ncm_family = AF_INET6;
> > +
> > +	switch (type) {
> > +		case NETCONFA_MC_FORWARDING:
> > +			ncm->ncm_family = RTNL_FAMILY_IP6MR;
> > +			break;
> > +
> > +		default:
> > +			ncm->ncm_family = AF_INET6;
> > +			break;
> > +	}
> >  
> 
> Hm, are you sure? NETCONFA_MC_FORWARDING is of type RTM_NEWNETCONF
> and expects ncm_family to be either AF_INET or AF_INET6 (at least in
> iproute2/ipmonitor.c).
> 

I agree with Sven on this, looks like the recent addition of netconf
configuration to netlink didn't embrace how multicast is handled in kernel.

Multicast forwarding is a routing related configuration value.
All the multicast routing events come in as special family RTNL_FAMILY_IPMR
(see net/ipv4/ipmr.c function ipmr_fill_route). I would expect that multicast
routing daemons would like to be able to use special family to listen for
all multicast related changes (and not see non-multicast events).


Minor nit: the patch is formatted incorrectly (case should line up with switch).

Re: [PATCH] Set the correct RTNL family for multicast netconf messages

[Hannes Frederic Sowa]

On Thu, Jun 27, 2013 at 06:33:42PM -0700, Stephen Hemminger wrote:
> On Fri, 28 Jun 2013 03:23:07 +0200
> Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> > Hm, are you sure? NETCONFA_MC_FORWARDING is of type RTM_NEWNETCONF
> > and expects ncm_family to be either AF_INET or AF_INET6 (at least in
> > iproute2/ipmonitor.c).
> > 
> 
> I agree with Sven on this, looks like the recent addition of netconf
> configuration to netlink didn't embrace how multicast is handled in kernel.
> 
> Multicast forwarding is a routing related configuration value.
> All the multicast routing events come in as special family RTNL_FAMILY_IPMR
> (see net/ipv4/ipmr.c function ipmr_fill_route). I would expect that multicast
> routing daemons would like to be able to use special family to listen for
> all multicast related changes (and not see non-multicast events).
> 
> 
> Minor nit: the patch is formatted incorrectly (case should line up with switch).

Yes, this seems reasonable but would need a small update to ipnetconf.c, too.

Thanks,

  Hannes

Re: [PATCH] Set the correct RTNL family for multicast netconf messages

[Nicolas Dichtel]

Le 28/06/2013 03:51, Hannes Frederic Sowa a écrit :
> On Thu, Jun 27, 2013 at 06:33:42PM -0700, Stephen Hemminger wrote:
>> On Fri, 28 Jun 2013 03:23:07 +0200
>> Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
>>> Hm, are you sure? NETCONFA_MC_FORWARDING is of type RTM_NEWNETCONF
>>> and expects ncm_family to be either AF_INET or AF_INET6 (at least in
>>> iproute2/ipmonitor.c).
>>>
>>
>> I agree with Sven on this, looks like the recent addition of netconf
>> configuration to netlink didn't embrace how multicast is handled in kernel.
>>
>> Multicast forwarding is a routing related configuration value.
>> All the multicast routing events come in as special family RTNL_FAMILY_IPMR
>> (see net/ipv4/ipmr.c function ipmr_fill_route). I would expect that multicast
>> routing daemons would like to be able to use special family to listen for
>> all multicast related changes (and not see non-multicast events).
>>
>>
>> Minor nit: the patch is formatted incorrectly (case should line up with switch).
>
> Yes, this seems reasonable but would need a small update to ipnetconf.c, too.
I also agree with Sven and Stephen.

Note also that the Signed-off-by line is missing in the commit log.

Sven, can you take care of the iproute2 patch? If not, let me know so I can do it.


Thank you,
Nicolas

Re: [PATCH] Set the correct RTNL family for multicast netconf messages

[Stephen Hemminger]

On Thu, 27 Jun 2013 16:44:47 -0700
Sven-Thorsten Dietrich <sven@vyatta.com> wrote:

> There may be other cases that require a special case, so I chose the switch approach.
> 
> Please note, that all other messages from ipmr[6].c correctly set the family, while these do not.
> 
> Thanks
> 
> Sven
> 
> 
> 
> Subject: Set correct RTNL family for multicast netconf messages
> From: Sven-Thorsten Dietrich sven@vyatta.com Thu Jun 27 16:40:17 2013 -0700
> Date: Thu Jun 27 16:40:17 2013 -0700:
> 
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index dfc39d4..695858b 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>  		return -EMSGSIZE;
>  
>  	ncm = nlmsg_data(nlh);
> -	ncm->ncm_family = AF_INET;
> +
> +	switch (type) {
> +		case NETCONFA_MC_FORWARDING:
> +			ncm->ncm_family = RTNL_FAMILY_IPMR;
> +			break;
> +
> +		default:
> +			ncm->ncm_family = AF_INET;
> +			break;
> +	}
>  
>  	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
>  		goto nla_put_failure;
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 4ab4c38..a177da4 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>  		return -EMSGSIZE;
>  
>  	ncm = nlmsg_data(nlh);
> -	ncm->ncm_family = AF_INET6;
> +
> +	switch (type) {
> +		case NETCONFA_MC_FORWARDING:
> +			ncm->ncm_family = RTNL_FAMILY_IP6MR;
> +			break;
> +
> +		default:
> +			ncm->ncm_family = AF_INET6;
> +			break;
> +	}
>  
>  	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
>  		goto nla_put_failure;
> 

Found another issue that needs some thought.
If type == ALL, in that case the family comes up as AF_INET.
That means that if application is doing a request to get netconf
it will receive a different answer than if it is montoring for netconf
changes.

One way to solve would be to split fill_devconf into two parts, one
for unicast, and one for multicast.

Re: [PATCH] Set the correct RTNL family for multicast netconf messages

[Nicolas Dichtel]

Le 28/06/2013 17:26, Stephen Hemminger a écrit :
> On Thu, 27 Jun 2013 16:44:47 -0700
> Sven-Thorsten Dietrich <sven@vyatta.com> wrote:
>
>> There may be other cases that require a special case, so I chose the switch approach.
>>
>> Please note, that all other messages from ipmr[6].c correctly set the family, while these do not.
>>
>> Thanks
>>
>> Sven
>>
>>
>>
>> Subject: Set correct RTNL family for multicast netconf messages
>> From: Sven-Thorsten Dietrich sven@vyatta.com Thu Jun 27 16:40:17 2013 -0700
>> Date: Thu Jun 27 16:40:17 2013 -0700:
>>
>>
>> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>> index dfc39d4..695858b 100644
>> --- a/net/ipv4/devinet.c
>> +++ b/net/ipv4/devinet.c
>> @@ -1705,7 +1705,16 @@ static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>>   		return -EMSGSIZE;
>>
>>   	ncm = nlmsg_data(nlh);
>> -	ncm->ncm_family = AF_INET;
>> +
>> +	switch (type) {
>> +		case NETCONFA_MC_FORWARDING:
>> +			ncm->ncm_family = RTNL_FAMILY_IPMR;
>> +			break;
>> +
>> +		default:
>> +			ncm->ncm_family = AF_INET;
>> +			break;
>> +	}
>>
>>   	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
>>   		goto nla_put_failure;
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 4ab4c38..a177da4 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -492,7 +492,16 @@ static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>>   		return -EMSGSIZE;
>>
>>   	ncm = nlmsg_data(nlh);
>> -	ncm->ncm_family = AF_INET6;
>> +
>> +	switch (type) {
>> +		case NETCONFA_MC_FORWARDING:
>> +			ncm->ncm_family = RTNL_FAMILY_IP6MR;
>> +			break;
>> +
>> +		default:
>> +			ncm->ncm_family = AF_INET6;
>> +			break;
>> +	}
>>
>>   	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
>>   		goto nla_put_failure;
>>
>
> Found another issue that needs some thought.
> If type == ALL, in that case the family comes up as AF_INET.
> That means that if application is doing a request to get netconf
> it will receive a different answer than if it is montoring for netconf
> changes.
>
> One way to solve would be to split fill_devconf into two parts, one
> for unicast, and one for multicast.
If I understand well, to get all conf variables for IPv4, you will need to make 
two dump?

Note that the initial idea of netconf was to be able to dump via netlink the 
content of /proc/sys/net/ipv4/conf/ or /proc/sys/net/ipv6/conf/, hence 
AF_INET[6] was used to specify 'ipv4' or 'ipv6'.

[RFC net] netconf: set mulitcast family for multicast forwarding messages

[Stephen Hemminger]

Revised version of Sven's patch. The idea is that multicast forwarding
should be under the multicast address family like other multicast netlink
messages.

This version generates each family under separate headers when
doing dump all.

Compile tested only, this is to show some of the issues that need
to be covered.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>


--- a/net/ipv4/devinet.c	2013-06-11 09:50:21.550918636 -0700
+++ b/net/ipv4/devinet.c	2013-06-28 09:07:18.147829543 -0700
@@ -1685,6 +1685,11 @@ static int inet_netconf_msgsize_devconf(
 		size += nla_total_size(4);
 	if (type == -1 || type == NETCONFA_RP_FILTER)
 		size += nla_total_size(4);
+
+	/* additional header for MC family */
+	if (type == -1)
+		size += NLMSG_ALIGN(sizeof(struct netconfmsg))
+		   + nla_total_size(4);
 	if (type == -1 || type == NETCONFA_MC_FORWARDING)
 		size += nla_total_size(4);
 
@@ -1694,7 +1699,7 @@ static int inet_netconf_msgsize_devconf(
 static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
 				     struct ipv4_devconf *devconf, u32 portid,
 				     u32 seq, int event, unsigned int flags,
-				     int type)
+				     u8 family, int type)
 {
 	struct nlmsghdr  *nlh;
 	struct netconfmsg *ncm;
@@ -1705,21 +1710,24 @@ static int inet_netconf_fill_devconf(str
 		return -EMSGSIZE;
 
 	ncm = nlmsg_data(nlh);
-	ncm->ncm_family = AF_INET;
+	ncm->ncm_family = family;
 
 	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
 		goto nla_put_failure;
 
 	/* type -1 is used for ALL */
-	if ((type == -1 || type == NETCONFA_FORWARDING) &&
+	if (((type == -1 && family == AF_INET) ||
+	     type == NETCONFA_FORWARDING)  &&
 	    nla_put_s32(skb, NETCONFA_FORWARDING,
 			IPV4_DEVCONF(*devconf, FORWARDING)) < 0)
 		goto nla_put_failure;
-	if ((type == -1 || type == NETCONFA_RP_FILTER) &&
+	if (((type == -1 && family == AF_INET) ||
+	     type == NETCONFA_RP_FILTER) &&
 	    nla_put_s32(skb, NETCONFA_RP_FILTER,
 			IPV4_DEVCONF(*devconf, RP_FILTER)) < 0)
 		goto nla_put_failure;
-	if ((type == -1 || type == NETCONFA_MC_FORWARDING) &&
+	if (((type == -1 || family == RTNL_FAMILY_IPMR) ||
+	     type == NETCONFA_MC_FORWARDING) &&
 	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
 			IPV4_DEVCONF(*devconf, MC_FORWARDING)) < 0)
 		goto nla_put_failure;
@@ -1737,12 +1745,17 @@ void inet_netconf_notify_devconf(struct
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
 
+	BUG_ON(type == -1); /* ALL is not valid for notification */
+
 	skb = nlmsg_new(inet_netconf_msgsize_devconf(type), GFP_ATOMIC);
 	if (skb == NULL)
 		goto errout;
 
 	err = inet_netconf_fill_devconf(skb, ifindex, devconf, 0, 0,
-					RTM_NEWNETCONF, 0, type);
+					RTM_NEWNETCONF, 0,
+					(type == NETCONFA_MC_FORWARDING) ?
+					RTNL_FAMILY_IPMR : AF_INET,
+					type);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in inet_netconf_msgsize_devconf() */
 		WARN_ON(err == -EMSGSIZE);
@@ -1811,13 +1824,23 @@ static int inet_netconf_get_devconf(stru
 	err = inet_netconf_fill_devconf(skb, ifindex, devconf,
 					NETLINK_CB(in_skb).portid,
 					nlh->nlmsg_seq, RTM_NEWNETCONF, 0,
-					-1);
+					AF_INET, -1);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in inet_netconf_msgsize_devconf() */
 		WARN_ON(err == -EMSGSIZE);
 		kfree_skb(skb);
 		goto errout;
 	}
+
+	err = inet_netconf_fill_devconf(skb, ifindex, devconf,
+					NETLINK_CB(in_skb).portid,
+					nlh->nlmsg_seq, RTM_NEWNETCONF, 0,
+					RTNL_FAMILY_IPMR, -1);
+	if (err < 0) {
+		WARN_ON(err == -EMSGSIZE);
+		kfree_skb(skb);
+		goto errout;
+	}
 	err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
 errout:
 	return err;
@@ -1855,10 +1878,23 @@ static int inet_netconf_dump_devconf(str
 						      cb->nlh->nlmsg_seq,
 						      RTM_NEWNETCONF,
 						      NLM_F_MULTI,
+						      AF_INET,
+						      -1) <= 0) {
+				rcu_read_unlock();
+				goto done;
+			}
+			if (inet_netconf_fill_devconf(skb, dev->ifindex,
+						      &in_dev->cnf,
+						      NETLINK_CB(cb->skb).portid,
+						      cb->nlh->nlmsg_seq,
+						      RTM_NEWNETCONF,
+						      NLM_F_MULTI,
+						      RTNL_FAMILY_IPMR,
 						      -1) <= 0) {
 				rcu_read_unlock();
 				goto done;
 			}
+
 			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
 cont:
 			idx++;
@@ -1871,10 +1907,18 @@ cont:
 					      NETLINK_CB(cb->skb).portid,
 					      cb->nlh->nlmsg_seq,
 					      RTM_NEWNETCONF, NLM_F_MULTI,
-					      -1) <= 0)
+					      AF_INET, -1) <= 0)
+			goto done;
+
+		if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_ALL,
+					      net->ipv4.devconf_all,
+					      NETLINK_CB(cb->skb).portid,
+					      cb->nlh->nlmsg_seq,
+					      RTM_NEWNETCONF, NLM_F_MULTI,
+					      RTNL_FAMILY_IPMR, -1) <= 0)
 			goto done;
-		else
-			h++;
+
+		h++;
 	}
 	if (h == NETDEV_HASHENTRIES + 1) {
 		if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
@@ -1882,10 +1926,18 @@ cont:
 					      NETLINK_CB(cb->skb).portid,
 					      cb->nlh->nlmsg_seq,
 					      RTM_NEWNETCONF, NLM_F_MULTI,
-					      -1) <= 0)
+					      AF_INET, -1) <= 0)
 			goto done;
-		else
-			h++;
+
+		if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
+					      net->ipv4.devconf_dflt,
+					      NETLINK_CB(cb->skb).portid,
+					      cb->nlh->nlmsg_seq,
+					      RTM_NEWNETCONF, NLM_F_MULTI,
+					      RTNL_FAMILY_IPMR, -1) <= 0)
+			goto done;
+
+		h++;
 	}
 done:
 	cb->args[0] = h;
--- a/net/ipv6/addrconf.c	2013-06-28 08:17:16.424664740 -0700
+++ b/net/ipv6/addrconf.c	2013-06-28 09:10:32.336959806 -0700
@@ -471,6 +471,9 @@ static int inet6_netconf_msgsize_devconf
 	if (type == -1 || type == NETCONFA_FORWARDING)
 		size += nla_total_size(4);
 #ifdef CONFIG_IPV6_MROUTE
+	if (type == -1)
+		size += NLMSG_ALIGN(sizeof(struct netconfmsg))
+			+ nla_total_size(4);
 	if (type == -1 || type == NETCONFA_MC_FORWARDING)
 		size += nla_total_size(4);
 #endif
@@ -481,7 +484,7 @@ static int inet6_netconf_msgsize_devconf
 static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
 				      struct ipv6_devconf *devconf, u32 portid,
 				      u32 seq, int event, unsigned int flags,
-				      int type)
+				      u8 family, int type)
 {
 	struct nlmsghdr  *nlh;
 	struct netconfmsg *ncm;
@@ -492,17 +495,19 @@ static int inet6_netconf_fill_devconf(st
 		return -EMSGSIZE;
 
 	ncm = nlmsg_data(nlh);
-	ncm->ncm_family = AF_INET6;
+	ncm->ncm_family = family;
 
 	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
 		goto nla_put_failure;
 
 	/* type -1 is used for ALL */
-	if ((type == -1 || type == NETCONFA_FORWARDING) &&
+	if (((type == -1 && family == AF_INET6) ||
+	     type == NETCONFA_FORWARDING) &&
 	    nla_put_s32(skb, NETCONFA_FORWARDING, devconf->forwarding) < 0)
 		goto nla_put_failure;
 #ifdef CONFIG_IPV6_MROUTE
-	if ((type == -1 || type == NETCONFA_MC_FORWARDING) &&
+	if (((type == -1 && family == RTNL_FAMILY_IP6MR) ||
+	     type == NETCONFA_MC_FORWARDING) &&
 	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
 			devconf->mc_forwarding) < 0)
 		goto nla_put_failure;
@@ -520,12 +525,23 @@ void inet6_netconf_notify_devconf(struct
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
 
+	BUG_ON(type == -1);
+
 	skb = nlmsg_new(inet6_netconf_msgsize_devconf(type), GFP_ATOMIC);
 	if (skb == NULL)
 		goto errout;
 
+#ifdef CONFIG_IPV6_MROUTE
+	if (type == NETCONFA_MC_FORWARDING)
+		err = inet6_netconf_fill_devconf(skb, ifindex, devconf, 0, 0,
+						 RTM_NEWNETCONF, 0,
+						 RTNL_FAMILY_IP6MR, type);
+	else
+#endif
 	err = inet6_netconf_fill_devconf(skb, ifindex, devconf, 0, 0,
-					 RTM_NEWNETCONF, 0, type);
+					 RTM_NEWNETCONF, 0,
+					 AF_INET6, type);
+#endif
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in inet6_netconf_msgsize_devconf() */
 		WARN_ON(err == -EMSGSIZE);
@@ -592,13 +608,25 @@ static int inet6_netconf_get_devconf(str
 	err = inet6_netconf_fill_devconf(skb, ifindex, devconf,
 					 NETLINK_CB(in_skb).portid,
 					 nlh->nlmsg_seq, RTM_NEWNETCONF, 0,
-					 -1);
+					 AF_INET6, -1);
+	if (err < 0) {
+		/* -EMSGSIZE implies BUG in inet6_netconf_msgsize_devconf() */
+		WARN_ON(err == -EMSGSIZE);
+		kfree_skb(skb);
+		goto errout;
+	}
+#ifdef CONFIG_IPV6_MROUTE
+	err = inet6_netconf_fill_devconf(skb, ifindex, devconf,
+					 NETLINK_CB(in_skb).portid,
+					 nlh->nlmsg_seq, RTM_NEWNETCONF, 0,
+					 RTNL_FAMILY_IP6MR, -1);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in inet6_netconf_msgsize_devconf() */
 		WARN_ON(err == -EMSGSIZE);
 		kfree_skb(skb);
 		goto errout;
 	}
+#endif
 	err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
 errout:
 	return err;
@@ -636,10 +664,23 @@ static int inet6_netconf_dump_devconf(st
 						       cb->nlh->nlmsg_seq,
 						       RTM_NEWNETCONF,
 						       NLM_F_MULTI,
+						       AF_INET6, -1) <= 0) {
+				rcu_read_unlock();
+				goto done;
+			}
+#ifdef CONFIG_IPV6_MROUTE
+			if (inet6_netconf_fill_devconf(skb, dev->ifindex,
+						       &idev->cnf,
+						       NETLINK_CB(cb->skb).portid,
+						       cb->nlh->nlmsg_seq,
+						       RTM_NEWNETCONF,
+						       NLM_F_MULTI,
+						       RTNL_FAMILY_IP6MR,
 						       -1) <= 0) {
 				rcu_read_unlock();
 				goto done;
 			}
+#endif
 			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
 cont:
 			idx++;
@@ -652,10 +693,18 @@ cont:
 					       NETLINK_CB(cb->skb).portid,
 					       cb->nlh->nlmsg_seq,
 					       RTM_NEWNETCONF, NLM_F_MULTI,
-					       -1) <= 0)
+					       AF_INET6, -1) <= 0)
 			goto done;
-		else
-			h++;
+#ifdef CONFIG_IPV6_MROUTE
+		if (inet6_netconf_fill_devconf(skb, NETCONFA_IFINDEX_ALL,
+					       net->ipv6.devconf_all,
+					       NETLINK_CB(cb->skb).portid,
+					       cb->nlh->nlmsg_seq,
+					       RTM_NEWNETCONF, NLM_F_MULTI,
+					       RTNL_FAMILY_IP6MR, -1) <= 0)
+			goto done;
+#endif
+		h++;
 	}
 	if (h == NETDEV_HASHENTRIES + 1) {
 		if (inet6_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
@@ -663,9 +712,17 @@ cont:
 					       NETLINK_CB(cb->skb).portid,
 					       cb->nlh->nlmsg_seq,
 					       RTM_NEWNETCONF, NLM_F_MULTI,
-					       -1) <= 0)
+					       AF_INET6, -1) <= 0)
+			goto done;
+#ifdef CONFIG_IPV6_MROUTE
+		if (inet6_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
+					       net->ipv6.devconf_dflt,
+					       NETLINK_CB(cb->skb).portid,
+					       cb->nlh->nlmsg_seq,
+					       RTM_NEWNETCONF, NLM_F_MULTI,
+					       RTNL_FAMILY_IP6MR, -1) <= 0)
 			goto done;
-		else
+#endif
 			h++;
 	}
 done:

Re: [RFC net] netconf: set mulitcast family for multicast forwarding messages

[Nicolas Dichtel]

Le 28/06/2013 18:13, Stephen Hemminger a écrit :
> Revised version of Sven's patch. The idea is that multicast forwarding
> should be under the multicast address family like other multicast netlink
> messages.
>
> This version generates each family under separate headers when
> doing dump all.
>
> Compile tested only, this is to show some of the issues that need
> to be covered.
I'm ok with the principle, just some comment below.

>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>
>
> --- a/net/ipv4/devinet.c	2013-06-11 09:50:21.550918636 -0700
> +++ b/net/ipv4/devinet.c	2013-06-28 09:07:18.147829543 -0700
> @@ -1685,6 +1685,11 @@ static int inet_netconf_msgsize_devconf(
>   		size += nla_total_size(4);
>   	if (type == -1 || type == NETCONFA_RP_FILTER)
>   		size += nla_total_size(4);
> +
> +	/* additional header for MC family */
> +	if (type == -1)
> +		size += NLMSG_ALIGN(sizeof(struct netconfmsg))
> +		   + nla_total_size(4);
>   	if (type == -1 || type == NETCONFA_MC_FORWARDING)
>   		size += nla_total_size(4);
>
> @@ -1694,7 +1699,7 @@ static int inet_netconf_msgsize_devconf(
>   static int inet_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>   				     struct ipv4_devconf *devconf, u32 portid,
>   				     u32 seq, int event, unsigned int flags,
> -				     int type)
> +				     u8 family, int type)
>   {
>   	struct nlmsghdr  *nlh;
>   	struct netconfmsg *ncm;
> @@ -1705,21 +1710,24 @@ static int inet_netconf_fill_devconf(str
>   		return -EMSGSIZE;
>
>   	ncm = nlmsg_data(nlh);
> -	ncm->ncm_family = AF_INET;
> +	ncm->ncm_family = family;
>
>   	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
>   		goto nla_put_failure;
>
>   	/* type -1 is used for ALL */
> -	if ((type == -1 || type == NETCONFA_FORWARDING) &&
> +	if (((type == -1 && family == AF_INET) ||
> +	     type == NETCONFA_FORWARDING)  &&
>   	    nla_put_s32(skb, NETCONFA_FORWARDING,
>   			IPV4_DEVCONF(*devconf, FORWARDING)) < 0)
>   		goto nla_put_failure;
> -	if ((type == -1 || type == NETCONFA_RP_FILTER) &&
> +	if (((type == -1 && family == AF_INET) ||
> +	     type == NETCONFA_RP_FILTER) &&
>   	    nla_put_s32(skb, NETCONFA_RP_FILTER,
>   			IPV4_DEVCONF(*devconf, RP_FILTER)) < 0)
>   		goto nla_put_failure;
> -	if ((type == -1 || type == NETCONFA_MC_FORWARDING) &&
> +	if (((type == -1 || family == RTNL_FAMILY_IPMR) ||
should be '&&'

> +	     type == NETCONFA_MC_FORWARDING) &&
>   	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
>   			IPV4_DEVCONF(*devconf, MC_FORWARDING)) < 0)
>   		goto nla_put_failure;
> @@ -1737,12 +1745,17 @@ void inet_netconf_notify_devconf(struct
>   	struct sk_buff *skb;
>   	int err = -ENOBUFS;
>
> +	BUG_ON(type == -1); /* ALL is not valid for notification */
> +
>   	skb = nlmsg_new(inet_netconf_msgsize_devconf(type), GFP_ATOMIC);
>   	if (skb == NULL)
>   		goto errout;
>
>   	err = inet_netconf_fill_devconf(skb, ifindex, devconf, 0, 0,
> -					RTM_NEWNETCONF, 0, type);
> +					RTM_NEWNETCONF, 0,
> +					(type == NETCONFA_MC_FORWARDING) ?
> +					RTNL_FAMILY_IPMR : AF_INET,
> +					type);
>   	if (err < 0) {
>   		/* -EMSGSIZE implies BUG in inet_netconf_msgsize_devconf() */
>   		WARN_ON(err == -EMSGSIZE);
> @@ -1811,13 +1824,23 @@ static int inet_netconf_get_devconf(stru
>   	err = inet_netconf_fill_devconf(skb, ifindex, devconf,
>   					NETLINK_CB(in_skb).portid,
>   					nlh->nlmsg_seq, RTM_NEWNETCONF, 0,
> -					-1);
> +					AF_INET, -1);
>   	if (err < 0) {
>   		/* -EMSGSIZE implies BUG in inet_netconf_msgsize_devconf() */
>   		WARN_ON(err == -EMSGSIZE);
>   		kfree_skb(skb);
>   		goto errout;
>   	}
> +
> +	err = inet_netconf_fill_devconf(skb, ifindex, devconf,
> +					NETLINK_CB(in_skb).portid,
> +					nlh->nlmsg_seq, RTM_NEWNETCONF, 0,
> +					RTNL_FAMILY_IPMR, -1);
> +	if (err < 0) {
> +		WARN_ON(err == -EMSGSIZE);
> +		kfree_skb(skb);
> +		goto errout;
> +	}
inet_netconf_fill_devconf() is called twice, so to be homogeneous, we can call 
inet_netconf_msgsize_devconf() twice too and add a argument 'family' to this 
function. Same for IPv6.

>   	err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
>   errout:
>   	return err;
> @@ -1855,10 +1878,23 @@ static int inet_netconf_dump_devconf(str
>   						      cb->nlh->nlmsg_seq,
>   						      RTM_NEWNETCONF,
>   						      NLM_F_MULTI,
> +						      AF_INET,
> +						      -1) <= 0) {
> +				rcu_read_unlock();
> +				goto done;
> +			}
> +			if (inet_netconf_fill_devconf(skb, dev->ifindex,
> +						      &in_dev->cnf,
> +						      NETLINK_CB(cb->skb).portid,
> +						      cb->nlh->nlmsg_seq,
> +						      RTM_NEWNETCONF,
> +						      NLM_F_MULTI,
> +						      RTNL_FAMILY_IPMR,
>   						      -1) <= 0) {
>   				rcu_read_unlock();
>   				goto done;
>   			}
> +
>   			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
>   cont:
>   			idx++;
> @@ -1871,10 +1907,18 @@ cont:
>   					      NETLINK_CB(cb->skb).portid,
>   					      cb->nlh->nlmsg_seq,
>   					      RTM_NEWNETCONF, NLM_F_MULTI,
> -					      -1) <= 0)
> +					      AF_INET, -1) <= 0)
> +			goto done;
> +
> +		if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_ALL,
> +					      net->ipv4.devconf_all,
> +					      NETLINK_CB(cb->skb).portid,
> +					      cb->nlh->nlmsg_seq,
> +					      RTM_NEWNETCONF, NLM_F_MULTI,
> +					      RTNL_FAMILY_IPMR, -1) <= 0)
>   			goto done;
> -		else
> -			h++;
> +
> +		h++;
>   	}
>   	if (h == NETDEV_HASHENTRIES + 1) {
>   		if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
> @@ -1882,10 +1926,18 @@ cont:
>   					      NETLINK_CB(cb->skb).portid,
>   					      cb->nlh->nlmsg_seq,
>   					      RTM_NEWNETCONF, NLM_F_MULTI,
> -					      -1) <= 0)
> +					      AF_INET, -1) <= 0)
>   			goto done;
> -		else
> -			h++;
> +
> +		if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
> +					      net->ipv4.devconf_dflt,
> +					      NETLINK_CB(cb->skb).portid,
> +					      cb->nlh->nlmsg_seq,
> +					      RTM_NEWNETCONF, NLM_F_MULTI,
> +					      RTNL_FAMILY_IPMR, -1) <= 0)
> +			goto done;
> +
> +		h++;
>   	}
>   done:
>   	cb->args[0] = h;
> --- a/net/ipv6/addrconf.c	2013-06-28 08:17:16.424664740 -0700
> +++ b/net/ipv6/addrconf.c	2013-06-28 09:10:32.336959806 -0700
> @@ -471,6 +471,9 @@ static int inet6_netconf_msgsize_devconf
>   	if (type == -1 || type == NETCONFA_FORWARDING)
>   		size += nla_total_size(4);
>   #ifdef CONFIG_IPV6_MROUTE
> +	if (type == -1)
> +		size += NLMSG_ALIGN(sizeof(struct netconfmsg))
> +			+ nla_total_size(4);
>   	if (type == -1 || type == NETCONFA_MC_FORWARDING)
>   		size += nla_total_size(4);
>   #endif
> @@ -481,7 +484,7 @@ static int inet6_netconf_msgsize_devconf
>   static int inet6_netconf_fill_devconf(struct sk_buff *skb, int ifindex,
>   				      struct ipv6_devconf *devconf, u32 portid,
>   				      u32 seq, int event, unsigned int flags,
> -				      int type)
> +				      u8 family, int type)
>   {
>   	struct nlmsghdr  *nlh;
>   	struct netconfmsg *ncm;
> @@ -492,17 +495,19 @@ static int inet6_netconf_fill_devconf(st
>   		return -EMSGSIZE;
>
>   	ncm = nlmsg_data(nlh);
> -	ncm->ncm_family = AF_INET6;
> +	ncm->ncm_family = family;
>
>   	if (nla_put_s32(skb, NETCONFA_IFINDEX, ifindex) < 0)
>   		goto nla_put_failure;
>
>   	/* type -1 is used for ALL */
> -	if ((type == -1 || type == NETCONFA_FORWARDING) &&
> +	if (((type == -1 && family == AF_INET6) ||
> +	     type == NETCONFA_FORWARDING) &&
>   	    nla_put_s32(skb, NETCONFA_FORWARDING, devconf->forwarding) < 0)
>   		goto nla_put_failure;
>   #ifdef CONFIG_IPV6_MROUTE
> -	if ((type == -1 || type == NETCONFA_MC_FORWARDING) &&
> +	if (((type == -1 && family == RTNL_FAMILY_IP6MR) ||
> +	     type == NETCONFA_MC_FORWARDING) &&
>   	    nla_put_s32(skb, NETCONFA_MC_FORWARDING,
>   			devconf->mc_forwarding) < 0)
>   		goto nla_put_failure;
> @@ -520,12 +525,23 @@ void inet6_netconf_notify_devconf(struct
>   	struct sk_buff *skb;
>   	int err = -ENOBUFS;
>
> +	BUG_ON(type == -1);
> +
>   	skb = nlmsg_new(inet6_netconf_msgsize_devconf(type), GFP_ATOMIC);
>   	if (skb == NULL)
>   		goto errout;
>
> +#ifdef CONFIG_IPV6_MROUTE
> +	if (type == NETCONFA_MC_FORWARDING)
> +		err = inet6_netconf_fill_devconf(skb, ifindex, devconf, 0, 0,
> +						 RTM_NEWNETCONF, 0,
> +						 RTNL_FAMILY_IP6MR, type);
> +	else
> +#endif
>   	err = inet6_netconf_fill_devconf(skb, ifindex, devconf, 0, 0,
> -					 RTM_NEWNETCONF, 0, type);
> +					 RTM_NEWNETCONF, 0,
> +					 AF_INET6, type);
> +#endif
>   	if (err < 0) {
>   		/* -EMSGSIZE implies BUG in inet6_netconf_msgsize_devconf() */
>   		WARN_ON(err == -EMSGSIZE);
> @@ -592,13 +608,25 @@ static int inet6_netconf_get_devconf(str
>   	err = inet6_netconf_fill_devconf(skb, ifindex, devconf,
>   					 NETLINK_CB(in_skb).portid,
>   					 nlh->nlmsg_seq, RTM_NEWNETCONF, 0,
> -					 -1);
> +					 AF_INET6, -1);
> +	if (err < 0) {
> +		/* -EMSGSIZE implies BUG in inet6_netconf_msgsize_devconf() */
> +		WARN_ON(err == -EMSGSIZE);
> +		kfree_skb(skb);
> +		goto errout;
> +	}
> +#ifdef CONFIG_IPV6_MROUTE
> +	err = inet6_netconf_fill_devconf(skb, ifindex, devconf,
> +					 NETLINK_CB(in_skb).portid,
> +					 nlh->nlmsg_seq, RTM_NEWNETCONF, 0,
> +					 RTNL_FAMILY_IP6MR, -1);
>   	if (err < 0) {
>   		/* -EMSGSIZE implies BUG in inet6_netconf_msgsize_devconf() */
>   		WARN_ON(err == -EMSGSIZE);
>   		kfree_skb(skb);
>   		goto errout;
>   	}
> +#endif
>   	err = rtnl_unicast(skb, net, NETLINK_CB(in_skb).portid);
>   errout:
>   	return err;
> @@ -636,10 +664,23 @@ static int inet6_netconf_dump_devconf(st
>   						       cb->nlh->nlmsg_seq,
>   						       RTM_NEWNETCONF,
>   						       NLM_F_MULTI,
> +						       AF_INET6, -1) <= 0) {
> +				rcu_read_unlock();
> +				goto done;
> +			}
> +#ifdef CONFIG_IPV6_MROUTE
> +			if (inet6_netconf_fill_devconf(skb, dev->ifindex,
> +						       &idev->cnf,
> +						       NETLINK_CB(cb->skb).portid,
> +						       cb->nlh->nlmsg_seq,
> +						       RTM_NEWNETCONF,
> +						       NLM_F_MULTI,
> +						       RTNL_FAMILY_IP6MR,
>   						       -1) <= 0) {
>   				rcu_read_unlock();
>   				goto done;
>   			}
> +#endif
>   			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
>   cont:
>   			idx++;
> @@ -652,10 +693,18 @@ cont:
>   					       NETLINK_CB(cb->skb).portid,
>   					       cb->nlh->nlmsg_seq,
>   					       RTM_NEWNETCONF, NLM_F_MULTI,
> -					       -1) <= 0)
> +					       AF_INET6, -1) <= 0)
>   			goto done;
> -		else
> -			h++;
> +#ifdef CONFIG_IPV6_MROUTE
> +		if (inet6_netconf_fill_devconf(skb, NETCONFA_IFINDEX_ALL,
> +					       net->ipv6.devconf_all,
> +					       NETLINK_CB(cb->skb).portid,
> +					       cb->nlh->nlmsg_seq,
> +					       RTM_NEWNETCONF, NLM_F_MULTI,
> +					       RTNL_FAMILY_IP6MR, -1) <= 0)
> +			goto done;
> +#endif
> +		h++;
>   	}
>   	if (h == NETDEV_HASHENTRIES + 1) {
>   		if (inet6_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
> @@ -663,9 +712,17 @@ cont:
>   					       NETLINK_CB(cb->skb).portid,
>   					       cb->nlh->nlmsg_seq,
>   					       RTM_NEWNETCONF, NLM_F_MULTI,
> -					       -1) <= 0)
> +					       AF_INET6, -1) <= 0)
> +			goto done;
> +#ifdef CONFIG_IPV6_MROUTE
> +		if (inet6_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
> +					       net->ipv6.devconf_dflt,
> +					       NETLINK_CB(cb->skb).portid,
> +					       cb->nlh->nlmsg_seq,
> +					       RTM_NEWNETCONF, NLM_F_MULTI,
> +					       RTNL_FAMILY_IP6MR, -1) <= 0)
>   			goto done;
> -		else
> +#endif
>   			h++;
This line should be reindent too.

>   	}
>   done:
>