Re: [RFC 1/9] ipv6: implement dataplane support for rthdr type 4 (Segment Routing Header)

[David Lebrun <david.lebrun@uclouvain.be>]

[-- Attachment #1: Type: text/plain, Size: 2193 bytes --]

On 08/31/2016 04:51 PM, Nicolas Dichtel wrote:
> Thanks for proposing this feature. It would be great to have it upstream.
> 

Thanks for the feedback :)

> [snip]
>> +config IPV6_SEG6
>> +	bool "IPv6: Segment Routing support"
>> +	depends on IPV6
>> +	---help---
>> +	  Experimental support for IPv6 Segment Routing dataplane as defined
>> +	  in IETF draft-ietf-6man-segment-routing-header-01. This option
>> +	  enables the processing of SR-enabled packets allowing the kernel
>> +	  to act as a segment endpoint (intermediate or egress).
>> +
>> +	  If unsure, say N.
>> +
> I don't think that the option is needed. At the end, every distributions will
> turn it on.
> 

Are you sure ? This is a rather specific feature, used in specific
environments. Not that I would mind removing the option if it makes sense.

> [snip]
>> +#ifdef CONFIG_IPV6_SEG6
>> +	{
>> +		.procname	= "seg6_enabled",
>> +		.data		= &ipv6_devconf.seg6_enabled,
>> +		.maxlen		= sizeof(int),
>> +		.mode		= 0644,
>> +		.proc_handler	= proc_dointvec,
>> +	},
>> +#endif
> Don't forget to document this option in Documentation/networking/ip-sysctl.txt.
> Don't forget to explain how 'all' works ;-)
> It would be nice to also add it in netconf subsystem (see 'git grep netconf
> net/ipv6').
> 

Right ! I didn't think of that doc file. Noted for netconf.

> [snip]
>> +#ifdef CONFIG_IPV6_SEG6
>> +static int ipv6_srh_rcv(struct sk_buff *skb)
>> +{
>> +	struct inet6_skb_parm *opt = IP6CB(skb);
>> +	struct in6_addr *addr = NULL, *last_addr = NULL, *active_addr = NULL;
>> +	struct ipv6_sr_hdr *hdr;
>> +	struct net *net = dev_net(skb->dev);
>> +	int cleanup = 0;
>> +	struct inet6_dev *idev;
>> +	int accept_seg6;
> nit: better to follow the 'reverse christmas tree' scheme when declaring variables.
> 

Noted


>> +
>> +	ip6_route_input(skb);
> The destination address has now changed and the packet is routed again.
> skb->nfct is not updated, it is intentional? I'm asking me if it's conceptually
> right.
> 

I fail to see any usecase where conntrack would run on SR-enabled
packets. Things such as NAT would just defeat the purpose.

David


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]