| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 2 Nov 2011 09:21:28 -0700
From: Stephen Hemminger <shemminger@...tta.com>
To: Matti Vaittinen <matti.vaittinen@....com>
Cc: davem@...emloft.net, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] IPv6 - support for NLM_F_* flags at IPv6 routing
requests
On Wed, 02 Nov 2011 09:09:51 +0200
Matti Vaittinen <matti.vaittinen@....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@...il.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists