[RFC][PATCH] ipmr: Fix struct mfcctl to be independent of MAXVIFS.

[RFC][PATCH] ipmr: Fix struct mfcctl to be independent of MAXVIFS.

[Eric W. Biederman]

Right now if you recompile the kernel to support more VIFS users of
the MRT_ADD_VIF and MRT_DEL_VIF will break because the ABI changes.

Correct this by forcing the number of VIFS handled in mfcctl to 32.
Allow for larger MAXVIFS by placing a second array of ttls at the end
of struct mfcctl.

struct mfcctl is insane.  The last 4 fields are dead, and the mfc_ttls
array is only 2 byte aligned, with a 2 byte hole right after it.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/mroute.h |    4 +++-
 net/ipv4/ipmr.c        |   29 +++++++++++++++++++++++------
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/linux/mroute.h b/include/linux/mroute.h
index c5f3d53..c5e066c 100644
--- a/include/linux/mroute.h
+++ b/include/linux/mroute.h
@@ -76,15 +76,17 @@ struct vifctl {
  *	Cache manipulation structures for mrouted and PIMd
  */
  
+#define MFCCTL_VIFS 32
 struct mfcctl {
 	struct in_addr mfcc_origin;		/* Origin of mcast	*/
 	struct in_addr mfcc_mcastgrp;		/* Group in question	*/
 	vifi_t	mfcc_parent;			/* Where it arrived	*/
-	unsigned char mfcc_ttls[MAXVIFS];	/* Where it is going	*/
+	unsigned char mfcc_ttls[MFCCTL_VIFS];	/* Where it is going	*/
 	unsigned int mfcc_pkt_cnt;		/* pkt count for src-grp */
 	unsigned int mfcc_byte_cnt;
 	unsigned int mfcc_wrong_if;
 	int	     mfcc_expire;
+	unsigned char mfcc_ttls_extra[];	/* The rest of where it is going */
 };
 
 /* 
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 0b9d03c..2120668 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -797,7 +797,8 @@ static int ipmr_mfc_delete(struct net *net, struct mfcctl *mfc)
 	return -ENOENT;
 }
 
-static int ipmr_mfc_add(struct net *net, struct mfcctl *mfc, int mrtsock)
+static int ipmr_mfc_add(struct net *net, struct mfcctl *mfc,
+			unsigned char *ttls, int mrtsock)
 {
 	int line;
 	struct mfc_cache *uc, *c, **cp;
@@ -817,7 +818,7 @@ static int ipmr_mfc_add(struct net *net, struct mfcctl *mfc, int mrtsock)
 	if (c != NULL) {
 		write_lock_bh(&mrt_lock);
 		c->mfc_parent = mfc->mfcc_parent;
-		ipmr_update_thresholds(c, mfc->mfcc_ttls);
+		ipmr_update_thresholds(c, ttls);
 		if (!mrtsock)
 			c->mfc_flags |= MFC_STATIC;
 		write_unlock_bh(&mrt_lock);
@@ -834,7 +835,7 @@ static int ipmr_mfc_add(struct net *net, struct mfcctl *mfc, int mrtsock)
 	c->mfc_origin = mfc->mfcc_origin.s_addr;
 	c->mfc_mcastgrp = mfc->mfcc_mcastgrp.s_addr;
 	c->mfc_parent = mfc->mfcc_parent;
-	ipmr_update_thresholds(c, mfc->mfcc_ttls);
+	ipmr_update_thresholds(c, ttls);
 	if (!mrtsock)
 		c->mfc_flags |= MFC_STATIC;
 
@@ -954,6 +955,8 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 	int ret;
 	struct vifctl vif;
 	struct mfcctl mfc;
+	unsigned char ttls[MAXVIFS];
+	unsigned extra_oifs;
 	struct net *net = sock_net(sk);
 
 	if (optname != MRT_INIT) {
@@ -961,7 +964,7 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 			return -EACCES;
 	}
 
-	switch (optname) {
+	switch (optname){
 	case MRT_INIT:
 		if (sk->sk_type != SOCK_RAW ||
 		    inet_sk(sk)->inet_num != IPPROTO_IGMP)
@@ -1012,15 +1015,29 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 		 */
 	case MRT_ADD_MFC:
 	case MRT_DEL_MFC:
-		if (optlen != sizeof(mfc))
+		/* How many extra interfaces do we have information for? */
+		extra_oifs = optlen - sizeof(mfc);
+		if (extra_oifs > (MAXVIFS - MFCCTL_VIFS))
+			extra_oifs = MAXVIFS - MFCCTL_VIFS;
+
+		if (optlen < sizeof(mfc))
 			return -EINVAL;
 		if (copy_from_user(&mfc, optval, sizeof(mfc)))
 			return -EFAULT;
+
+		memcpy(ttls, mfc.mfcc_ttls, sizeof(mfc.mfcc_ttls));
+		memset(ttls + MFCCTL_VIFS, 255, MAXVIFS - MFCCTL_VIFS);
+		if (copy_from_user(ttls + MFCCTL_VIFS,optval + sizeof(mfc),
+				   extra_oifs))
+			return -EFAULT;
+
+
 		rtnl_lock();
 		if (optname == MRT_DEL_MFC)
 			ret = ipmr_mfc_delete(net, &mfc);
 		else
-			ret = ipmr_mfc_add(net, &mfc, sk == net->ipv4.mroute_sk);
+			ret = ipmr_mfc_add(net, &mfc, ttls,
+					   sk == net->ipv4.mroute_sk);
 		rtnl_unlock();
 		return ret;
 		/*
-- 
1.6.5.2.143.g8cc62

[RFC][PATCH] ipmr: Fix struct mfcctl to be independent of MAXVIFS v2

[Eric W. Biederman]

Right now if you recompile the kernel increasing MAXVIFS
to support more VIFS users of the MRT_ADD_VIF and MRT_DEL_VIF
will break because the ABI changed.

My goal is an API that works with just a recompile of existing
applications, and an ABI that continues to work for old
applications.

The unused/dead fields at the end of struct mfcctl make this
exercise more difficult than it should be.

- Rename the existing struct mfcctl mfcctl_old.
- Define a new and larger struct mfcctl that we can detect
  by size.

  The new and larger struct mfcctl won't have trailing garbage
  fields so we can accept anything of that size or larger,
  and simply ignore the entries that are above MAXVIFS.

My new struct mfcctl is now 128 bytes which is noticeable on
the stack but should still be small enough not to cause problems.

v2:  Rework the support larger arrays so that most/all? existing
   applications can simply be recompiled and work with a larger
   maximum number of VIFS.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/mroute.h |   20 +++++++++++++++-----
 net/ipv4/ipmr.c        |   17 ++++++++++++++---
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/include/linux/mroute.h b/include/linux/mroute.h
index c5f3d53..6dbdebf 100644
--- a/include/linux/mroute.h
+++ b/include/linux/mroute.h
@@ -76,15 +76,25 @@ struct vifctl {
  *	Cache manipulation structures for mrouted and PIMd
  */
  
+struct mfcctl_old {
+#define MFCCTL_OLD_VIFS 32
+	struct in_addr mfcc_origin;		/* Origin of mcast	*/
+	struct in_addr mfcc_mcastgrp;		/* Group in question	*/
+	vifi_t	mfcc_parent;			/* Where it arrived	*/
+	unsigned char mfcc_ttls[MFCCTL_OLD_VIFS];		/* Where it is going	*/
+	unsigned int mfcc_pkt_cnt;		/* dead */
+	unsigned int mfcc_byte_cnt;		/* dead */
+	unsigned int mfcc_wrong_if;		/* dead */
+	int	     mfcc_expire;		/* dead */
+};
+
 struct mfcctl {
+#define MFCCTL_VIFS 118
 	struct in_addr mfcc_origin;		/* Origin of mcast	*/
 	struct in_addr mfcc_mcastgrp;		/* Group in question	*/
 	vifi_t	mfcc_parent;			/* Where it arrived	*/
-	unsigned char mfcc_ttls[MAXVIFS];	/* Where it is going	*/
-	unsigned int mfcc_pkt_cnt;		/* pkt count for src-grp */
-	unsigned int mfcc_byte_cnt;
-	unsigned int mfcc_wrong_if;
-	int	     mfcc_expire;
+	unsigned char mfcc_ttls[MFCCTL_VIFS];	/* Where it is going 	*/
+	/* Don't put anything here as mfcc_ttls should grow into here */
 };
 
 /* 
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 0b9d03c..516289b 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1012,10 +1012,18 @@ int ip_mroute_setsockopt(struct sock *sk, int optname, char __user *optval, unsi
 		 */
 	case MRT_ADD_MFC:
 	case MRT_DEL_MFC:
-		if (optlen != sizeof(mfc))
+
+		if (optlen == sizeof(struct mfcctl_old)) {
+			if (copy_from_user(&mfc, optval, sizeof(struct mfcctl_old)))
+				return -EFAULT;
+			memset(mfc.mfcc_ttls + MFCCTL_OLD_VIFS, 255,
+			       MFCCTL_VIFS - MFCCTL_OLD_VIFS);
+		} else if (optlen >= (sizeof(struct mfcctl))) {
+			if (copy_from_user(&mfc, optval, sizeof(mfc)))
+				return -EFAULT;
+		} else
 			return -EINVAL;
-		if (copy_from_user(&mfc, optval, sizeof(mfc)))
-			return -EFAULT;
+
 		rtnl_lock();
 		if (optname == MRT_DEL_MFC)
 			ret = ipmr_mfc_delete(net, &mfc);
@@ -2032,6 +2040,9 @@ int __init ip_mr_init(void)
 {
 	int err;
 
+	BUILD_BUG_ON(MFCCTL_VIFS < MAXVIFS);
+	BUILD_BUG_ON(sizeof(struct mfcctl) <= sizeof(struct mfcctl_old));
+
 	mrt_cachep = kmem_cache_create("ip_mrt_cache",
 				       sizeof(struct mfc_cache),
 				       0, SLAB_HWCACHE_ALIGN|SLAB_PANIC,
-- 
1.6.5.2.143.g8cc62

Re: [RFC][PATCH] ipmr: Fix struct mfcctl to be independent of MAXVIFS v2

[Ben Greear]

On 04/06/2010 08:38 AM, Eric W. Biederman wrote:
>
> Right now if you recompile the kernel increasing MAXVIFS
> to support more VIFS users of the MRT_ADD_VIF and MRT_DEL_VIF
> will break because the ABI changed.
>
> My goal is an API that works with just a recompile of existing
> applications, and an ABI that continues to work for old
> applications.
>
> The unused/dead fields at the end of struct mfcctl make this
> exercise more difficult than it should be.
>
> - Rename the existing struct mfcctl mfcctl_old.
> - Define a new and larger struct mfcctl that we can detect
>    by size.
>
>    The new and larger struct mfcctl won't have trailing garbage
>    fields so we can accept anything of that size or larger,
>    and simply ignore the entries that are above MAXVIFS.
>
> My new struct mfcctl is now 128 bytes which is noticeable on
> the stack but should still be small enough not to cause problems.
>
> v2:  Rework the support larger arrays so that most/all? existing
>     applications can simply be recompiled and work with a larger
>     maximum number of VIFS.

If we're going to change the ABI, can we not support an arbitrary
number of VIFS instead of just a larger fixed maximum?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC][PATCH] ipmr: Fix struct mfcctl to be independent of MAXVIFS v2

[Eric W. Biederman]

Ben Greear <greearb@candelatech.com> writes:

> On 04/06/2010 08:38 AM, Eric W. Biederman wrote:
>>
>> Right now if you recompile the kernel increasing MAXVIFS
>> to support more VIFS users of the MRT_ADD_VIF and MRT_DEL_VIF
>> will break because the ABI changed.
>>
>> My goal is an API that works with just a recompile of existing
>> applications, and an ABI that continues to work for old
>> applications.
>>
>> The unused/dead fields at the end of struct mfcctl make this
>> exercise more difficult than it should be.
>>
>> - Rename the existing struct mfcctl mfcctl_old.
>> - Define a new and larger struct mfcctl that we can detect
>>    by size.
>>
>>    The new and larger struct mfcctl won't have trailing garbage
>>    fields so we can accept anything of that size or larger,
>>    and simply ignore the entries that are above MAXVIFS.
>>
>> My new struct mfcctl is now 128 bytes which is noticeable on
>> the stack but should still be small enough not to cause problems.
>>
>> v2:  Rework the support larger arrays so that most/all? existing
>>     applications can simply be recompiled and work with a larger
>>     maximum number of VIFS.
>
> If we're going to change the ABI, can we not support an arbitrary
> number of VIFS instead of just a larger fixed maximum?

The ABI as I have specified should work for any larger structure than
I have specified.  But like select many applications will limit themselves
to use the definition of struct mfcctl that is passed to them.

Eric