Skip to contentSkip to conversation

ipmr: Fix struct mfcctl to be independent of MAXVIFS.

RFC PATCH New Eric W. Biederman 2 participants 4 messages Apr 6, 2010

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

Eric W. Biederman

author
#0 lore raw

RFC PATCH 1/1 ipmr: Fix struct mfcctl to be independent of MAXVIFS.

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 <[email protected]>
---
 include/linux/mroute.h |    4 +++-
 net/ipv4/ipmr.c        |   29 +++++++++++++++++++++++------
 2 files changed, 26 insertions(+), 7 deletions(-)
include/linux/mroute.h +3 -1
index c5f3d53..c5e066c 100644
@@ -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 */
 };
 
 /*
net/ipv4/ipmr.c +23 -6
index 0b9d03c..2120668 100644
@@ -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;
 		/*
Signature · 1 line
1.6.5.2.143.g8cc62
author
#1 lore raw

RFC PATCH 1/1 ipmr: Fix struct mfcctl to be independent of MAXVIFS v2

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 <[email protected]>
---
 include/linux/mroute.h |   20 +++++++++++++++-----
 net/ipv4/ipmr.c        |   17 ++++++++++++++---
 2 files changed, 29 insertions(+), 8 deletions(-)
include/linux/mroute.h +15 -5
index c5f3d53..6dbdebf 100644
@@ -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 */
 };
 
 /*
net/ipv4/ipmr.c +14 -3
index 0b9d03c..516289b 100644
@@ -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,
Signature · 1 line
1.6.5.2.143.g8cc62
#2 lore raw

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

Quoted 20 lines · 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.

Quoted 6 lines

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

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
author
#3 lore raw

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

Quoted 24 lines · Ben Greear
Quoted 23 lines · 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.

Quoted 6 lines
Quoted 3 lines

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