[PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[Matti Vaittinen]

Hi dee Ho again.

Here's the support for NLM_F_* flags at IPv6 routing requests once again.

This time if no NLM_F_CREATE flag is not defined for RTM_NEWROUTE request,
warning is printed, but no error is returned. Instead new route is added.

Exception is when NLM_F_REPLACE flag is given without NLM_F_CREATE, and
no matching route is found. In this case it should be safe to assume
that the request issuer is familiar with NLM_F_* flags, and does really
not want route to be created.

Specifying NLM_F_REPLACE flag will now make the kernel to search for
matching route, and replace it with new one. If no route is found and
NLM_F_CREATE is specified as well, then new route is created.

Also, specifying NLM_F_EXCL will yield returning of error if matching route 
is found.

Patch is created against linux-3.1-rc4


Best Regards 
Matti Vaittinen





Signed-off-by: Matti Vaittinen <Mazziesaccount@gmail.com>
---
diff -uNr linux-3.1-rc4.orig/net/ipv6/ip6_fib.c linux-3.1-rc4.new/net/ipv6/ip6_fib.c
--- linux-3.1-rc4.orig/net/ipv6/ip6_fib.c	2011-11-01 14:01:55.000000000 +0200
+++ linux-3.1-rc4.new/net/ipv6/ip6_fib.c	2011-11-01 14:12:28.000000000 +0200
@@ -39,6 +39,7 @@
 #include <net/ip6_fib.h>
 #include <net/ip6_route.h>
 
+#define RT6_CANT_CREATE ((int)-1)
 #define RT6_DEBUG 2
 
 #if RT6_DEBUG >= 3
@@ -429,17 +430,34 @@
 
 static struct fib6_node * fib6_add_1(struct fib6_node *root, void *addr,
 				     int addrlen, int plen,
-				     int offset)
+				     int offset, struct nl_info *info)
 {
 	struct fib6_node *fn, *in, *ln;
 	struct fib6_node *pn = NULL;
 	struct rt6key *key;
 	int	bit;
+
+
+	int allow_create = 1;
+	int replace_required = 0;
+
+
 	__be32	dir = 0;
 	__u32	sernum = fib6_new_sernum();
 
 	RT6_TRACE("fib6_add_1\n");
 
+	if (NULL != info &&
+	    NULL != info->nlh &&
+	    (info->nlh->nlmsg_flags&NLM_F_REPLACE)) {
+		replace_required = 1;
+	}
+	if (NULL != info &&
+	    NULL != info->nlh &&
+	    !(info->nlh->nlmsg_flags&NLM_F_CREATE)) {
+			allow_create = 0;
+	}
+
 	/* insert node in tree */
 
 	fn = root;
@@ -451,8 +469,12 @@
 		 *	Prefix match
 		 */
 		if (plen < fn->fn_bit ||
-		    !ipv6_prefix_equal(&key->addr, addr, fn->fn_bit))
+		    !ipv6_prefix_equal(&key->addr, addr, fn->fn_bit)) {
+			if (!allow_create)
+				printk(KERN_WARNING
+				    "NLM_F_CREATE should be specified when creating new rt\n");
 			goto insert_above;
+		}
 
 		/*
 		 *	Exact match ?
@@ -481,10 +503,27 @@
 		fn = dir ? fn->right: fn->left;
 	} while (fn);
 
+
+	if (replace_required && !allow_create) {
+		/* We should not create new node because
+		 * NLM_F_REPLACE was specified without NLM_F_CREATE
+		 * I assume it is safe to require NLM_F_CREATE when
+		 * REPLACE flag is used! Later we may want to remove the
+		 * check for replace_required, because according
+		 * to netlink specification, NLM_F_CREATE
+		 * MUST be specified if new route is created.
+		 * That would keep IPv6 consistent with IPv4
+		 */
+		printk(KERN_WARNING
+		    "NLM_F_CREATE should be specified when creating new rt - ignoring request\n");
+		return (struct fib6_node *)RT6_CANT_CREATE;
+	}
 	/*
 	 *	We walked to the bottom of tree.
 	 *	Create new leaf node without children.
 	 */
+	if (!allow_create)
+		printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
 
 	ln = node_alloc();
 
@@ -567,7 +606,6 @@
 		fn->parent = in;
 
 		ln->fn_sernum = sernum;
-
 		if (addr_bit_set(addr, bit)) {
 			in->right = ln;
 			in->left  = fn;
@@ -585,6 +623,7 @@
 
 		ln = node_alloc();
 
+
 		if (ln == NULL)
 			return NULL;
 
@@ -618,6 +657,12 @@
 {
 	struct rt6_info *iter = NULL;
 	struct rt6_info **ins;
+	int replace = (NULL != info &&
+	    NULL != info->nlh &&
+	    (info->nlh->nlmsg_flags&NLM_F_REPLACE));
+	int add = ((NULL == info || NULL == info->nlh) ||
+	    (info->nlh->nlmsg_flags&NLM_F_CREATE));
+	int found = 0;
 
 	ins = &fn->leaf;
 
@@ -630,6 +675,13 @@
 			/*
 			 *	Same priority level
 			 */
+			if (NULL != info->nlh &&
+			    (info->nlh->nlmsg_flags&NLM_F_EXCL))
+				return -EEXIST;
+			if (replace) {
+				found++;
+				break;
+			}
 
 			if (iter->rt6i_dev == rt->rt6i_dev &&
 			    iter->rt6i_idev == rt->rt6i_idev &&
@@ -659,19 +711,41 @@
 	/*
 	 *	insert node
 	 */
+	if (!replace) {
+		if (!add)
+			printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
+
+add:
+		rt->dst.rt6_next = iter;
+		*ins = rt;
+		rt->rt6i_node = fn;
+		atomic_inc(&rt->rt6i_ref);
+		inet6_rt_notify(RTM_NEWROUTE, rt, info);
+		info->nl_net->ipv6.rt6_stats->fib_rt_entries++;
+
+		if ((fn->fn_flags & RTN_RTINFO) == 0) {
+			info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
+			fn->fn_flags |= RTN_RTINFO;
+		}
 
-	rt->dst.rt6_next = iter;
-	*ins = rt;
-	rt->rt6i_node = fn;
-	atomic_inc(&rt->rt6i_ref);
-	inet6_rt_notify(RTM_NEWROUTE, rt, info);
-	info->nl_net->ipv6.rt6_stats->fib_rt_entries++;
-
-	if ((fn->fn_flags & RTN_RTINFO) == 0) {
-		info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
-		fn->fn_flags |= RTN_RTINFO;
+	} else {
+		if (!found) {
+			if (add)
+				goto add;
+			printk(KERN_WARNING "add rtinfo to node - NLM_F_REPLACE specified, but no existing node found! bailing out\n");
+			return -ENOENT;
+		}
+		*ins = rt;
+		rt->rt6i_node = fn;
+		rt->dst.rt6_next = iter->dst.rt6_next;
+		atomic_inc(&rt->rt6i_ref);
+		inet6_rt_notify(RTM_NEWROUTE, rt, info);
+		rt6_release(iter);
+		if ((fn->fn_flags & RTN_RTINFO) == 0) {
+			info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
+			fn->fn_flags |= RTN_RTINFO;
+		}
 	}
-
 	return 0;
 }
 
@@ -700,10 +774,29 @@
 {
 	struct fib6_node *fn, *pn = NULL;
 	int err = -ENOMEM;
+	int allow_create = 1;
+	int allow_replace = 1;
+	if (NULL != info &&
+	    NULL != info->nlh &&
+	    !(info->nlh->nlmsg_flags&NLM_F_REPLACE)) {
+		allow_replace = 0;
+	}
+	if (NULL != info &&
+	    NULL != info->nlh &&
+	    !(info->nlh->nlmsg_flags&NLM_F_CREATE)) {
+			allow_create = 0;
+	}
+	if (!allow_create && !allow_replace)
+		printk(KERN_WARNING "RTM_NEWROUTE with no NLM_F_CREATE or NLM_F_REPLACE\n");
 
 	fn = fib6_add_1(root, &rt->rt6i_dst.addr, sizeof(struct in6_addr),
-			rt->rt6i_dst.plen, offsetof(struct rt6_info, rt6i_dst));
+		    rt->rt6i_dst.plen, offsetof(struct rt6_info, rt6i_dst),
+		    info);
 
+	if (RT6_CANT_CREATE == (int)fn) {
+		err = -EINVAL;
+		fn = NULL;
+	}
 	if (fn == NULL)
 		goto out;
 
@@ -716,6 +809,8 @@
 		if (fn->subtree == NULL) {
 			struct fib6_node *sfn;
 
+			if (!allow_create)
+				printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
 			/*
 			 * Create subtree.
 			 *
@@ -740,7 +835,8 @@
 
 			sn = fib6_add_1(sfn, &rt->rt6i_src.addr,
 					sizeof(struct in6_addr), rt->rt6i_src.plen,
-					offsetof(struct rt6_info, rt6i_src));
+					offsetof(struct rt6_info, rt6i_src),
+				    info);
 
 			if (sn == NULL) {
 				/* If it is failed, discard just allocated
@@ -757,8 +853,13 @@
 		} else {
 			sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
 					sizeof(struct in6_addr), rt->rt6i_src.plen,
-					offsetof(struct rt6_info, rt6i_src));
+					offsetof(struct rt6_info, rt6i_src),
+					info);
 
+			if (RT6_CANT_CREATE == (int)sn) {
+				err = -EINVAL;
+				sn = NULL;
+			}
 			if (sn == NULL)
 				goto st_failure;
 		}
diff -uNr linux-3.1-rc4.orig/net/ipv6/route.c linux-3.1-rc4.new/net/ipv6/route.c
--- linux-3.1-rc4.orig/net/ipv6/route.c	2011-11-01 14:01:55.000000000 +0200
+++ linux-3.1-rc4.new/net/ipv6/route.c	2011-10-27 10:45:05.000000000 +0300
@@ -1223,9 +1223,18 @@
 	if (cfg->fc_metric == 0)
 		cfg->fc_metric = IP6_RT_PRIO_USER;
 
-	table = fib6_new_table(net, cfg->fc_table);
+	err = -ENOBUFS;
+	if (NULL != cfg->fc_nlinfo.nlh &&
+	    !(cfg->fc_nlinfo.nlh->nlmsg_flags&NLM_F_CREATE)) {
+		table = fib6_get_table(net, cfg->fc_table);
+		if (table == NULL) {
+			printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
+			table = fib6_new_table(net, cfg->fc_table);
+		}
+	} else {
+		table = fib6_new_table(net, cfg->fc_table);
+	}
 	if (table == NULL) {
-		err = -ENOBUFS;
 		goto out;
 	}
 

Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[Stephen Hemminger]

On Tue, 01 Nov 2011 16:22:27 +0200
Matti Vaittinen <matti.vaittinen@nsn.com> wrote:

>  
> +#define RT6_CANT_CREATE ((int)-1)
>  #define RT6_DEBUG 2

Rather than introduce a new error flag, why not convert the code
to use the kernel standard PTR_ERR() macros?

Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[Maz The Northener]

On Tue, Nov 1, 2011 at 11:43 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> On Tue, 01 Nov 2011 16:22:27 +0200
> Matti Vaittinen <matti.vaittinen@nsn.com> wrote:
>
>>
>> +#define RT6_CANT_CREATE ((int)-1)
>>  #define RT6_DEBUG 2
>
> Rather than introduce a new error flag, why not convert the code
> to use the kernel standard PTR_ERR() macros?
> --

Thanks for comment Stephen, I admitt I felt there was something rotten
in adding a new define. And as to why not use PTR_ERR() - I did not
know about such a facility. I'll fix this to be

return ERR_PTR(-ENOENT);

and at calling function check for
if (PTR_ERR(fn) == -ENOENT)

I guess that is correct way?

And finally couple of questions about sending patches (I'm a new guy
in this "house" of yours):
Is it Ok to add patch as attachment? I'm having difficulties in using
a client which allows sending plain text.
And is it Ok to create patch against linux 3.1-rc4? I've never used
git, and thus obtaining 3.1-rc4 from kernel.org is the easiest way for
me.

--Matti Vaittinen

Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[Matti Vaittinen]

On Tue, 2011-11-01 at 16:27 +0200, Matti Vaittinen wrote:
> Hi dee Ho again.
> 
> Here's the support for NLM_F_* flags at IPv6 routing requests once again.
> 
> This time if no NLM_F_CREATE flag is not defined for RTM_NEWROUTE request,
> warning is printed, but no error is returned. Instead new route is added.
> 
> Exception is when NLM_F_REPLACE flag is given without NLM_F_CREATE, and
> no matching route is found. In this case it should be safe to assume
> that the request issuer is familiar with NLM_F_* flags, and does really
> not want route to be created.
> 
> Specifying NLM_F_REPLACE flag will now make the kernel to search for
> matching route, and replace it with new one. If no route is found and
> NLM_F_CREATE is specified as well, then new route is created.
> 
> Also, specifying NLM_F_EXCL will yield returning of error if matching route 
> is found.
> 
> Patch is created against linux-3.1-rc4
> 

New patch where the definition of new error is removed as Stephen suggested.

Signed-off-by: Matti Vaittinen <Mazziesaccount@gmail.com>
---
diff -uNr linux-3.1-rc4.orig/net/ipv6/ip6_fib.c linux-3.1-rc4.new/net/ipv6/ip6_fib.c
--- linux-3.1-rc4.orig/net/ipv6/ip6_fib.c	2011-11-01 14:01:55.000000000 +0200
+++ linux-3.1-rc4.new/net/ipv6/ip6_fib.c	2011-11-02 08:37:21.000000000 +0200
@@ -429,17 +429,34 @@
 
 static struct fib6_node * fib6_add_1(struct fib6_node *root, void *addr,
 				     int addrlen, int plen,
-				     int offset)
+				     int offset, struct nl_info *info)
 {
 	struct fib6_node *fn, *in, *ln;
 	struct fib6_node *pn = NULL;
 	struct rt6key *key;
 	int	bit;
+
+
+	int allow_create = 1;
+	int replace_required = 0;
+
+
 	__be32	dir = 0;
 	__u32	sernum = fib6_new_sernum();
 
 	RT6_TRACE("fib6_add_1\n");
 
+	if (NULL != info &&
+	    NULL != info->nlh &&
+	    (info->nlh->nlmsg_flags&NLM_F_REPLACE)) {
+		replace_required = 1;
+	}
+	if (NULL != info &&
+	    NULL != info->nlh &&
+	    !(info->nlh->nlmsg_flags&NLM_F_CREATE)) {
+			allow_create = 0;
+	}
+
 	/* insert node in tree */
 
 	fn = root;
@@ -451,8 +468,12 @@
 		 *	Prefix match
 		 */
 		if (plen < fn->fn_bit ||
-		    !ipv6_prefix_equal(&key->addr, addr, fn->fn_bit))
+		    !ipv6_prefix_equal(&key->addr, addr, fn->fn_bit)) {
+			if (!allow_create)
+				printk(KERN_WARNING
+				    "NLM_F_CREATE should be specified when creating new rt\n");
 			goto insert_above;
+		}
 
 		/*
 		 *	Exact match ?
@@ -481,10 +502,27 @@
 		fn = dir ? fn->right: fn->left;
 	} while (fn);
 
+
+	if (replace_required && !allow_create) {
+		/* We should not create new node because
+		 * NLM_F_REPLACE was specified without NLM_F_CREATE
+		 * I assume it is safe to require NLM_F_CREATE when
+		 * REPLACE flag is used! Later we may want to remove the
+		 * check for replace_required, because according
+		 * to netlink specification, NLM_F_CREATE
+		 * MUST be specified if new route is created.
+		 * That would keep IPv6 consistent with IPv4
+		 */
+		printk(KERN_WARNING
+		    "NLM_F_CREATE should be specified when creating new rt - ignoring request\n");
+		return ERR_PTR(-ENOENT);
+	}
 	/*
 	 *	We walked to the bottom of tree.
 	 *	Create new leaf node without children.
 	 */
+	if (!allow_create)
+		printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
 
 	ln = node_alloc();
 
@@ -567,7 +605,6 @@
 		fn->parent = in;
 
 		ln->fn_sernum = sernum;
-
 		if (addr_bit_set(addr, bit)) {
 			in->right = ln;
 			in->left  = fn;
@@ -585,6 +622,7 @@
 
 		ln = node_alloc();
 
+
 		if (ln == NULL)
 			return NULL;
 
@@ -618,6 +656,12 @@
 {
 	struct rt6_info *iter = NULL;
 	struct rt6_info **ins;
+	int replace = (NULL != info &&
+	    NULL != info->nlh &&
+	    (info->nlh->nlmsg_flags&NLM_F_REPLACE));
+	int add = ((NULL == info || NULL == info->nlh) ||
+	    (info->nlh->nlmsg_flags&NLM_F_CREATE));
+	int found = 0;
 
 	ins = &fn->leaf;
 
@@ -630,6 +674,13 @@
 			/*
 			 *	Same priority level
 			 */
+			if (NULL != info->nlh &&
+			    (info->nlh->nlmsg_flags&NLM_F_EXCL))
+				return -EEXIST;
+			if (replace) {
+				found++;
+				break;
+			}
 
 			if (iter->rt6i_dev == rt->rt6i_dev &&
 			    iter->rt6i_idev == rt->rt6i_idev &&
@@ -659,19 +710,41 @@
 	/*
 	 *	insert node
 	 */
+	if (!replace) {
+		if (!add)
+			printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
+
+add:
+		rt->dst.rt6_next = iter;
+		*ins = rt;
+		rt->rt6i_node = fn;
+		atomic_inc(&rt->rt6i_ref);
+		inet6_rt_notify(RTM_NEWROUTE, rt, info);
+		info->nl_net->ipv6.rt6_stats->fib_rt_entries++;
+
+		if ((fn->fn_flags & RTN_RTINFO) == 0) {
+			info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
+			fn->fn_flags |= RTN_RTINFO;
+		}
 
-	rt->dst.rt6_next = iter;
-	*ins = rt;
-	rt->rt6i_node = fn;
-	atomic_inc(&rt->rt6i_ref);
-	inet6_rt_notify(RTM_NEWROUTE, rt, info);
-	info->nl_net->ipv6.rt6_stats->fib_rt_entries++;
-
-	if ((fn->fn_flags & RTN_RTINFO) == 0) {
-		info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
-		fn->fn_flags |= RTN_RTINFO;
+	} else {
+		if (!found) {
+			if (add)
+				goto add;
+			printk(KERN_WARNING "add rtinfo to node - NLM_F_REPLACE specified, but no existing node found! bailing out\n");
+			return -ENOENT;
+		}
+		*ins = rt;
+		rt->rt6i_node = fn;
+		rt->dst.rt6_next = iter->dst.rt6_next;
+		atomic_inc(&rt->rt6i_ref);
+		inet6_rt_notify(RTM_NEWROUTE, rt, info);
+		rt6_release(iter);
+		if ((fn->fn_flags & RTN_RTINFO) == 0) {
+			info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
+			fn->fn_flags |= RTN_RTINFO;
+		}
 	}
-
 	return 0;
 }
 
@@ -700,10 +773,29 @@
 {
 	struct fib6_node *fn, *pn = NULL;
 	int err = -ENOMEM;
+	int allow_create = 1;
+	int allow_replace = 1;
+	if (NULL != info &&
+	    NULL != info->nlh &&
+	    !(info->nlh->nlmsg_flags&NLM_F_REPLACE)) {
+		allow_replace = 0;
+	}
+	if (NULL != info &&
+	    NULL != info->nlh &&
+	    !(info->nlh->nlmsg_flags&NLM_F_CREATE)) {
+			allow_create = 0;
+	}
+	if (!allow_create && !allow_replace)
+		printk(KERN_WARNING "RTM_NEWROUTE with no NLM_F_CREATE or NLM_F_REPLACE\n");
 
 	fn = fib6_add_1(root, &rt->rt6i_dst.addr, sizeof(struct in6_addr),
-			rt->rt6i_dst.plen, offsetof(struct rt6_info, rt6i_dst));
+		    rt->rt6i_dst.plen, offsetof(struct rt6_info, rt6i_dst),
+		    info);
 
+	if (-ENOENT == PTR_ERR(fn)) {
+		err = -EINVAL;
+		fn = NULL;
+	}
 	if (fn == NULL)
 		goto out;
 
@@ -716,6 +808,8 @@
 		if (fn->subtree == NULL) {
 			struct fib6_node *sfn;
 
+			if (!allow_create)
+				printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
 			/*
 			 * Create subtree.
 			 *
@@ -740,7 +834,8 @@
 
 			sn = fib6_add_1(sfn, &rt->rt6i_src.addr,
 					sizeof(struct in6_addr), rt->rt6i_src.plen,
-					offsetof(struct rt6_info, rt6i_src));
+					offsetof(struct rt6_info, rt6i_src),
+				    info);
 
 			if (sn == NULL) {
 				/* If it is failed, discard just allocated
@@ -757,8 +852,13 @@
 		} else {
 			sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
 					sizeof(struct in6_addr), rt->rt6i_src.plen,
-					offsetof(struct rt6_info, rt6i_src));
+					offsetof(struct rt6_info, rt6i_src),
+					info);
 
+			if (-ENOENT == PTR_ERR(sn)) {
+				err = -EINVAL;
+				sn = NULL;
+			}
 			if (sn == NULL)
 				goto st_failure;
 		}
diff -uNr linux-3.1-rc4.orig/net/ipv6/route.c linux-3.1-rc4.new/net/ipv6/route.c
--- linux-3.1-rc4.orig/net/ipv6/route.c	2011-11-01 14:01:55.000000000 +0200
+++ linux-3.1-rc4.new/net/ipv6/route.c	2011-10-27 10:45:05.000000000 +0300
@@ -1223,9 +1223,18 @@
 	if (cfg->fc_metric == 0)
 		cfg->fc_metric = IP6_RT_PRIO_USER;
 
-	table = fib6_new_table(net, cfg->fc_table);
+	err = -ENOBUFS;
+	if (NULL != cfg->fc_nlinfo.nlh &&
+	    !(cfg->fc_nlinfo.nlh->nlmsg_flags&NLM_F_CREATE)) {
+		table = fib6_get_table(net, cfg->fc_table);
+		if (table == NULL) {
+			printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
+			table = fib6_new_table(net, cfg->fc_table);
+		}
+	} else {
+		table = fib6_new_table(net, cfg->fc_table);
+	}
 	if (table == NULL) {
-		err = -ENOBUFS;
 		goto out;
 	}
 

Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[David Miller]

From: Matti Vaittinen <matti.vaittinen@nsn.com>
Date: Wed, 02 Nov 2011 09:09:51 +0200

> On Tue, 2011-11-01 at 16:27 +0200, Matti Vaittinen wrote:
>> Hi dee Ho again.
>> 
>> Here's the support for NLM_F_* flags at IPv6 routing requests once again.
>> 
>> This time if no NLM_F_CREATE flag is not defined for RTM_NEWROUTE request,
>> warning is printed, but no error is returned. Instead new route is added.
>> 
>> Exception is when NLM_F_REPLACE flag is given without NLM_F_CREATE, and
>> no matching route is found. In this case it should be safe to assume
>> that the request issuer is familiar with NLM_F_* flags, and does really
>> not want route to be created.
>> 
>> Specifying NLM_F_REPLACE flag will now make the kernel to search for
>> matching route, and replace it with new one. If no route is found and
>> NLM_F_CREATE is specified as well, then new route is created.
>> 
>> Also, specifying NLM_F_EXCL will yield returning of error if matching route 
>> is found.
>> 
>> Patch is created against linux-3.1-rc4
>> 
> 
> New patch where the definition of new error is removed as Stephen suggested.
> 
> Signed-off-by: Matti Vaittinen <Mazziesaccount@gmail.com>

Please do not submit new versions of patches in this way by replying
and quoting your original commit log message.  That makes for lots
of work for me.

Instead, submit a fresh new full patch posting and prefix your subject
with something like "[PATCH v2]" to indicate it's a new version.

Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[Stephen Hemminger]

On Wed, 02 Nov 2011 09:09:51 +0200
Matti Vaittinen <matti.vaittinen@nsn.com> wrote:

> On Tue, 2011-11-01 at 16:27 +0200, Matti Vaittinen wrote:
> > Hi dee Ho again.
> > 
> > Here's the support for NLM_F_* flags at IPv6 routing requests once again.
> > 
> > This time if no NLM_F_CREATE flag is not defined for RTM_NEWROUTE request,
> > warning is printed, but no error is returned. Instead new route is added.
> > 
> > Exception is when NLM_F_REPLACE flag is given without NLM_F_CREATE, and
> > no matching route is found. In this case it should be safe to assume
> > that the request issuer is familiar with NLM_F_* flags, and does really
> > not want route to be created.
> > 
> > Specifying NLM_F_REPLACE flag will now make the kernel to search for
> > matching route, and replace it with new one. If no route is found and
> > NLM_F_CREATE is specified as well, then new route is created.
> > 
> > Also, specifying NLM_F_EXCL will yield returning of error if matching route 
> > is found.
> > 
> > Patch is created against linux-3.1-rc4
> > 
> 
> New patch where the definition of new error is removed as Stephen suggested.
> 
> Signed-off-by: Matti Vaittinen <Mazziesaccount@gmail.com>
> ---
> diff -uNr linux-3.1-rc4.orig/net/ipv6/ip6_fib.c linux-3.1-rc4.new/net/ipv6/ip6_fib.c
> --- linux-3.1-rc4.orig/net/ipv6/ip6_fib.c	2011-11-01 14:01:55.000000000 +0200
> +++ linux-3.1-rc4.new/net/ipv6/ip6_fib.c	2011-11-02 08:37:21.000000000 +0200
> @@ -429,17 +429,34 @@
>  
>  static struct fib6_node * fib6_add_1(struct fib6_node *root, void *addr,
>  				     int addrlen, int plen,
> -				     int offset)
> +				     int offset, struct nl_info *info)
>  {
>  	struct fib6_node *fn, *in, *ln;
>  	struct fib6_node *pn = NULL;
>  	struct rt6key *key;
>  	int	bit;
> +
> +

Gratuitous unnecessary whitespace added.

> +	int allow_create = 1;
> +	int replace_required = 0;
> +
> +

Personally, I dislike boolean flag variables, it is often a sign
of poorly executed logic flow


>  	__be32	dir = 0;
>  	__u32	sernum = fib6_new_sernum();
>  
>  	RT6_TRACE("fib6_add_1\n");
>  
> +	if (NULL != info &&
> +	    NULL != info->nlh &&
> +	    (info->nlh->nlmsg_flags&NLM_F_REPLACE)) {
> +		replace_required = 1;
> +	}
> +	if (NULL != info &&
> +	    NULL != info->nlh &&
> +	    !(info->nlh->nlmsg_flags&NLM_F_CREATE)) {
> +			allow_create = 0;
> +	}

I would move the flag calculation out to the caller and keep fib6_add_1
clean.

>  	/* insert node in tree */
>  
>  	fn = root;
> @@ -451,8 +468,12 @@
>  		 *	Prefix match
>  		 */
>  		if (plen < fn->fn_bit ||
> -		    !ipv6_prefix_equal(&key->addr, addr, fn->fn_bit))
> +		    !ipv6_prefix_equal(&key->addr, addr, fn->fn_bit)) {
> +			if (!allow_create)
> +				printk(KERN_WARNING
> +				    "NLM_F_CREATE should be specified when creating new rt\n");
>  			goto insert_above;
> +		}
>  
>  		/*
>  		 *	Exact match ?
> @@ -481,10 +502,27 @@
>  		fn = dir ? fn->right: fn->left;
>  	} while (fn);
>  
> +
> +	if (replace_required && !allow_create) {
> +		/* We should not create new node because
> +		 * NLM_F_REPLACE was specified without NLM_F_CREATE
> +		 * I assume it is safe to require NLM_F_CREATE when
> +		 * REPLACE flag is used! Later we may want to remove the
> +		 * check for replace_required, because according
> +		 * to netlink specification, NLM_F_CREATE
> +		 * MUST be specified if new route is created.
> +		 * That would keep IPv6 consistent with IPv4
> +		 */
> +		printk(KERN_WARNING
> +		    "NLM_F_CREATE should be specified when creating new rt - ignoring request\n");
> +		return ERR_PTR(-ENOENT);
> +	}
>  	/*
>  	 *	We walked to the bottom of tree.
>  	 *	Create new leaf node without children.
>  	 */
> +	if (!allow_create)
> +		printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
>  
>  	ln = node_alloc();
>  
> @@ -567,7 +605,6 @@
>  		fn->parent = in;
>  
>  		ln->fn_sernum = sernum;
> -
>  		if (addr_bit_set(addr, bit)) {
>  			in->right = ln;
>  			in->left  = fn;

Useless whitespace changes should not be part of the patch.

> @@ -585,6 +622,7 @@
>  
>  		ln = node_alloc();
>  
> +
>  		if (ln == NULL)
>  			return NULL;

More useless changes

> @@ -618,6 +656,12 @@
>  {
>  	struct rt6_info *iter = NULL;
>  	struct rt6_info **ins;
> +	int replace = (NULL != info &&
> +	    NULL != info->nlh &&
> +	    (info->nlh->nlmsg_flags&NLM_F_REPLACE));
> +	int add = ((NULL == info || NULL == info->nlh) ||
> +	    (info->nlh->nlmsg_flags&NLM_F_CREATE));
> +	int found = 0;
>  
>  	ins = &fn->leaf;
>  
> @@ -630,6 +674,13 @@
>  			/*
>  			 *	Same priority level
>  			 */
> +			if (NULL != info->nlh &&
> +			    (info->nlh->nlmsg_flags&NLM_F_EXCL))
> +				return -EEXIST;
> +			if (replace) {
> +				found++;
> +				break;
> +			}
>  
>  			if (iter->rt6i_dev == rt->rt6i_dev &&
>  			    iter->rt6i_idev == rt->rt6i_idev &&
> @@ -659,19 +710,41 @@
>  	/*
>  	 *	insert node
>  	 */
> +	if (!replace) {
> +		if (!add)
> +			printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
> +
> +add:
> +		rt->dst.rt6_next = iter;
> +		*ins = rt;
> +		rt->rt6i_node = fn;
> +		atomic_inc(&rt->rt6i_ref);
> +		inet6_rt_notify(RTM_NEWROUTE, rt, info);
> +		info->nl_net->ipv6.rt6_stats->fib_rt_entries++;
> +
> +		if ((fn->fn_flags & RTN_RTINFO) == 0) {
> +			info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
> +			fn->fn_flags |= RTN_RTINFO;
> +		}
>  
> -	rt->dst.rt6_next = iter;
> -	*ins = rt;
> -	rt->rt6i_node = fn;
> -	atomic_inc(&rt->rt6i_ref);
> -	inet6_rt_notify(RTM_NEWROUTE, rt, info);
> -	info->nl_net->ipv6.rt6_stats->fib_rt_entries++;
> -
> -	if ((fn->fn_flags & RTN_RTINFO) == 0) {
> -		info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
> -		fn->fn_flags |= RTN_RTINFO;
> +	} else {
> +		if (!found) {
> +			if (add)
> +				goto add;
> +			printk(KERN_WARNING "add rtinfo to node - NLM_F_REPLACE specified, but no existing node found! bailing out\n");
> +			return -ENOENT;
> +		}
> +		*ins = rt;
> +		rt->rt6i_node = fn;
> +		rt->dst.rt6_next = iter->dst.rt6_next;
> +		atomic_inc(&rt->rt6i_ref);
> +		inet6_rt_notify(RTM_NEWROUTE, rt, info);
> +		rt6_release(iter);
> +		if ((fn->fn_flags & RTN_RTINFO) == 0) {
> +			info->nl_net->ipv6.rt6_stats->fib_route_nodes++;
> +			fn->fn_flags |= RTN_RTINFO;
> +		}
>  	}
> -
>  	return 0;
>  }
>  
> @@ -700,10 +773,29 @@
>  {
>  	struct fib6_node *fn, *pn = NULL;
>  	int err = -ENOMEM;
> +	int allow_create = 1;
> +	int allow_replace = 1;
> +	if (NULL != info &&
> +	    NULL != info->nlh &&
> +	    !(info->nlh->nlmsg_flags&NLM_F_REPLACE)) {
> +		allow_replace = 0;
> +	}
> +	if (NULL != info &&
> +	    NULL != info->nlh &&
> +	    !(info->nlh->nlmsg_flags&NLM_F_CREATE)) {
> +			allow_create = 0;
> +	}
> +	if (!allow_create && !allow_replace)
> +		printk(KERN_WARNING "RTM_NEWROUTE with no NLM_F_CREATE or NLM_F_REPLACE\n");
>  
>  	fn = fib6_add_1(root, &rt->rt6i_dst.addr, sizeof(struct in6_addr),
> -			rt->rt6i_dst.plen, offsetof(struct rt6_info, rt6i_dst));
> +		    rt->rt6i_dst.plen, offsetof(struct rt6_info, rt6i_dst),
> +		    info);
>  
> +	if (-ENOENT == PTR_ERR(fn)) {
> +		err = -EINVAL;
> +		fn = NULL;
> +	}
>  	if (fn == NULL)
>  		goto out;
>  
> @@ -716,6 +808,8 @@
>  		if (fn->subtree == NULL) {
>  			struct fib6_node *sfn;
>  
> +			if (!allow_create)
> +				printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
>  			/*
>  			 * Create subtree.
>  			 *
> @@ -740,7 +834,8 @@
>  
>  			sn = fib6_add_1(sfn, &rt->rt6i_src.addr,
>  					sizeof(struct in6_addr), rt->rt6i_src.plen,
> -					offsetof(struct rt6_info, rt6i_src));
> +					offsetof(struct rt6_info, rt6i_src),
> +				    info);
>  
>  			if (sn == NULL) {
>  				/* If it is failed, discard just allocated
> @@ -757,8 +852,13 @@
>  		} else {
>  			sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
>  					sizeof(struct in6_addr), rt->rt6i_src.plen,
> -					offsetof(struct rt6_info, rt6i_src));
> +					offsetof(struct rt6_info, rt6i_src),
> +					info);
>  
> +			if (-ENOENT == PTR_ERR(sn)) {
> +				err = -EINVAL;

This is not how to use PTR_ERR; the more common convention is:

		if (IS_ERR(sn)) {
			err = PTR_ERR(sn);
...


> +				sn = NULL;
> +			}
>  			if (sn == NULL)
>  				goto st_failure;
>  		}
> diff -uNr linux-3.1-rc4.orig/net/ipv6/route.c linux-3.1-rc4.new/net/ipv6/route.c
> --- linux-3.1-rc4.orig/net/ipv6/route.c	2011-11-01 14:01:55.000000000 +0200
> +++ linux-3.1-rc4.new/net/ipv6/route.c	2011-10-27 10:45:05.000000000 +0300
> @@ -1223,9 +1223,18 @@
>  	if (cfg->fc_metric == 0)
>  		cfg->fc_metric = IP6_RT_PRIO_USER;
>  
> -	table = fib6_new_table(net, cfg->fc_table);
> +	err = -ENOBUFS;
> +	if (NULL != cfg->fc_nlinfo.nlh &&
> +	    !(cfg->fc_nlinfo.nlh->nlmsg_flags&NLM_F_CREATE)) {
> +		table = fib6_get_table(net, cfg->fc_table);
> +		if (table == NULL) {
> +			printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
> +			table = fib6_new_table(net, cfg->fc_table);
> +		}
> +	} else {
> +		table = fib6_new_table(net, cfg->fc_table);
> +	}
>  	if (table == NULL) {
> -		err = -ENOBUFS;
>  		goto out;
>  	}

This could be a separate patch

Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing requests

[Maz The Northener]

On Wed, Nov 2, 2011 at 6:21 PM, Stephen Hemminger <shemminger@vyatta.com> wrote:
> On Wed, 02 Nov 2011 09:09:51 +0200
> Matti Vaittinen <matti.vaittinen@nsn.com> wrote:
>

>> +
>> +
>
> Gratuitous unnecessary whitespace added.
>

I will fix the whitespace errors.

>> +     int allow_create = 1;
>> +     int replace_required = 0;
>> +
>> +
>
> Personally, I dislike boolean flag variables, it is often a sign
> of poorly executed logic flow
>
>

I tend to agree to some level. However sometimes well named variables
make following code easier. And I do not claim the logic flow couldn't
be improved, but I'm not the one going to make big changes to FIB
handling. I would probably end up breaking something.

>>       __be32  dir = 0;
>>       __u32   sernum = fib6_new_sernum();
>>
>>       RT6_TRACE("fib6_add_1\n");
>>
>> +     if (NULL != info &&
>> +         NULL != info->nlh &&
>> +         (info->nlh->nlmsg_flags&NLM_F_REPLACE)) {
>> +             replace_required = 1;
>> +     }
>> +     if (NULL != info &&
>> +         NULL != info->nlh &&
>> +         !(info->nlh->nlmsg_flags&NLM_F_CREATE)) {
>> +                     allow_create = 0;
>> +     }
>
> I would move the flag calculation out to the caller and keep fib6_add_1
> clean.

Can be done, I just didn't want to introduce two more parameters in
function call. But I'll do that.


>>                       sn = fib6_add_1(fn->subtree, &rt->rt6i_src.addr,
>>                                       sizeof(struct in6_addr), rt->rt6i_src.plen,
>> -                                     offsetof(struct rt6_info, rt6i_src));
>> +                                     offsetof(struct rt6_info, rt6i_src),
>> +                                     info);
>>
>> +                     if (-ENOENT == PTR_ERR(sn)) {
>> +                             err = -EINVAL;
>
> This is not how to use PTR_ERR; the more common convention is:
>
>                if (IS_ERR(sn)) {
>                        err = PTR_ERR(sn);
> ...

Makes sense.

>
>
>> +                             sn = NULL;
>> +                     }
>>                       if (sn == NULL)
>>                               goto st_failure;
>>               }
>> diff -uNr linux-3.1-rc4.orig/net/ipv6/route.c linux-3.1-rc4.new/net/ipv6/route.c
>> --- linux-3.1-rc4.orig/net/ipv6/route.c       2011-11-01 14:01:55.000000000 +0200
>> +++ linux-3.1-rc4.new/net/ipv6/route.c        2011-10-27 10:45:05.000000000 +0300
>> @@ -1223,9 +1223,18 @@
>>       if (cfg->fc_metric == 0)
>>               cfg->fc_metric = IP6_RT_PRIO_USER;
>>
>> -     table = fib6_new_table(net, cfg->fc_table);
>> +     err = -ENOBUFS;
>> +     if (NULL != cfg->fc_nlinfo.nlh &&
>> +         !(cfg->fc_nlinfo.nlh->nlmsg_flags&NLM_F_CREATE)) {
>> +             table = fib6_get_table(net, cfg->fc_table);
>> +             if (table == NULL) {
>> +                     printk(KERN_WARNING "NLM_F_CREATE should be specified when creating new rt\n");
>> +                     table = fib6_new_table(net, cfg->fc_table);
>> +             }
>> +     } else {
>> +             table = fib6_new_table(net, cfg->fc_table);
>> +     }
>>       if (table == NULL) {
>> -             err = -ENOBUFS;
>>               goto out;
>>       }
>
> This could be a separate patch

Allright. I'll break up the patch. Thanks for taking the time to check
this. I'll send new patches soonish.

--Matti Vaittinen.