netdev - Re: [PATCH net-next 4/7] mpls: Basic support for adding and removing routes

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20150304215006.019f1eaffaf3e0bfa53aadcf@mindspring.com>
Date:	Wed, 4 Mar 2015 21:50:06 -0500
From:	Bill Fink <billfink@...dspring.com>
To:	ebiederm@...ssion.com (Eric W. Biederman)
Cc:	roopa <roopa@...ulusnetworks.com>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	Stephen Hemminger <stephen@...workplumber.org>,
	santiago@...reenet.org, Simon Horman <horms@...ge.net.au>
Subject: Re: [PATCH net-next 4/7] mpls: Basic support for adding and
 removing routes

On Wed, 04 Mar 2015, Eric W. Biederman wrote:

> roopa <roopa@...ulusnetworks.com> writes:
> 
> > On 3/3/15, 5:12 PM, Eric W. Biederman wrote:
> >> mpls_route_add and mpls_route_del implement the basic logic for adding
> >> and removing Next Hop Label Forwarding Entries from the MPLS input
> >> label map.  The addition and subtraction is done in a way that is
> >> consistent with how the existing routing table in Linux are
> >> maintained.  Thus all of the work to deal with NLM_F_APPEND,
> >> NLM_F_EXCL, NLM_F_REPLACE, and NLM_F_CREATE.
> >>
> >> Cases that are not clearly defined such as changing the interpretation
> >> of the mpls reserved labels is not allowed.
> >>
> >> Because it seems like the right thing to do adding an MPLS route without
> >> specifying an input label and allowing the kernel to pick a free label
> >> table entry is supported.   The implementation is currently less than optimal
> >> but that can be changed.
> >>
> >> As I don't have anything else to test with only ethernet and the loopback
> >> device are the only two device types currently supported for forwarding
> >> MPLS over.
> >>
> >> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> >> ---
> >>   net/mpls/af_mpls.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 133 insertions(+)
> >>
> >> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> >> index b097125dfa33..e432f092f2fb 100644
> >> --- a/net/mpls/af_mpls.c
> >> +++ b/net/mpls/af_mpls.c
> >> @@ -16,6 +16,7 @@
> >>   #include <net/netns/generic.h>
> >>   #include "internal.h"
> >>   +#define LABEL_NOT_SPECIFIED (1<<20)
> >>   #define MAX_NEW_LABELS 2
> >>     /* This maximum ha length copied from the definition of struct
> >> neighbour */
> >> @@ -211,6 +212,19 @@ static struct packet_type mpls_packet_type __read_mostly = {
> >>   	.func = mpls_forward,
> >>   };
> >>   +struct mpls_route_config {
> >> +	u32		rc_protocol;
> >> +	u32		rc_ifindex;
> >> +	u16		rc_via_family;
> >> +	u16		rc_via_alen;
> >> +	u8		rc_via[MAX_VIA_ALEN];
> >> +	u32		rc_label;
> >> +	u32		rc_output_labels;
> >> +	u32		rc_output_label[MAX_NEW_LABELS];
> >> +	u32		rc_nlflags;
> >> +	struct nl_info	rc_nlinfo;
> >> +};
> >> +
> >>   static struct mpls_route *mpls_rt_alloc(size_t alen)
> >>   {
> >>   	struct mpls_route *rt;
> >> @@ -245,6 +259,125 @@ static void mpls_route_update(struct net *net, unsigned index,
> >>   	mpls_rt_free(old);
> >>   }
> >>   +static unsigned find_free_label(struct net *net)
> >> +{
> >> +	unsigned index;
> >> +	for (index = 16; index < net->mpls.platform_labels; index++) {
> >> +		if (!net->mpls.platform_label[index])
> >> +			return index;
> >> +	}
> >> +	return LABEL_NOT_SPECIFIED;
> >> +}
> >> +
> >> +static int mpls_route_add(struct mpls_route_config *cfg)
> >> +{
> >> +	struct net *net = cfg->rc_nlinfo.nl_net;
> >> +	struct net_device *dev = NULL;
> >> +	struct mpls_route *rt, *old;
> >> +	unsigned index;
> >> +	int i;
> >> +	int err = -EINVAL;
> >> +
> >> +	index = cfg->rc_label;
> >> +
> >> +	/* If a label was not specified during insert pick one */
> >> +	if ((index == LABEL_NOT_SPECIFIED) &&
> >> +	    (cfg->rc_nlflags & NLM_F_CREATE)) {
> >> +		index = find_free_label(net);
> >> +	}
> >> +
> >> +	/* The first 16 labels are reserved, and may not be set */
> >> +	if (index < 16)
> >> +		goto errout;
> >> +
> >> +	/* The full 20 bit range may not be supported. */
> >> +	if (index >= net->mpls.platform_labels)
> >> +		goto errout;
> >> +
> >> +	/* Ensure only a supported number of labels are present */
> >> +	if (cfg->rc_output_labels > MAX_NEW_LABELS)
> >> +		goto errout;
> >> +
> >> +	err = -ENODEV;
> >> +	dev = dev_get_by_index(net, cfg->rc_ifindex);
> >> +	if (!dev)
> >> +		goto errout;
> >> +
> >> +	/* For now just support ethernet devices */
> >> +	err = -EINVAL;
> >> +	if ((dev->type != ARPHRD_ETHER) && (dev->type != ARPHRD_LOOPBACK))
> >> +		goto errout;
> >> +
> >> +	err = -EINVAL;
> >> +	if ((cfg->rc_via_family == AF_PACKET) &&
> >> +	    (dev->addr_len != cfg->rc_via_alen))
> >> +		goto errout;
> >> +
> >> +	/* Append makes no sense with mpls */
> >> +	err = -EINVAL;
> >
> > minor nit: should this be -ENOTSUPP in that case ? (NLM_F_REPLACE and NLM_F_APPEND are
> > really operations. But, one can argue that they are an attribute of the msg and hence -EINVAL might be ok).
> > I did not find any other such case for consistency check.
> 
> Yes.  IPv4 implements NLM_F_APPEND and IPv6 ignores it.
> 
> I will add a patch to change the error code.

I believe the error -ENOTSUPP is deprecated and -EOPNOTSUPP should
be used instead.

						-Bill
--
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