Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [net-next v5 1/2] seg6: inherit DSCP of inner IPv4 packets
To:     David Ahern <dsahern@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     andrea.mayer@uniroma2.it
References: <20200825160236.1123-1-ahabdels@gmail.com>
 <efaf3273-e147-c27e-d5b8-241930335b82@gmail.com>
 <75f7be67-2362-e931-6793-1ce12c69b4ea@gmail.com>
 <71351d27-0719-6ed9-f5c6-4aee20547c58@gmail.com>
From:   Ahmed Abdelsalam <ahabdels@gmail.com>
Message-ID: <ab0869f7-9e69-b6fd-af5c-8e3ce432452b@gmail.com>
Date:   Wed, 26 Aug 2020 14:12:59 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <71351d27-0719-6ed9-f5c6-4aee20547c58@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 26/08/2020 02:45, David Ahern wrote:
> On 8/25/20 5:45 PM, Ahmed Abdelsalam wrote:
>>
>> Hi David
>>
>> The seg6 encap is implemented through the seg6_lwt rather than
>> seg6_local_lwt.
> 
> ok. I don't know the seg6 code; just taking a guess from a quick look.
> 
>> We can add a flag(SEG6_IPTUNNEL_DSCP) in seg6_iptunnel.h if we do not
>> want to go the sysctl direction.
> 
> sysctl is just a big hammer with side effects.
> 
> It struck me that the DSCP propagation is very similar to the TTL
> propagation with MPLS which is per route entry (MPLS_IPTUNNEL_TTL and
> stored as ttl_propagate in mpls_iptunnel_encap). Hence the question of
> whether SR could make this a per route attribute. Consistency across
> implementations is best.
>SRv6 does not have an issue of having this per route.
Actually, as SRv6 leverage IPv6 encapsulation, I would say it should 
consistent with ip6_tunnel not MPLS.

In ip6_tunnel, both ttl and flowinfo (tclass and flowlabel) are provided.

Ideally, SRv6 code should have done the same with:
TTL       := VLAUE | DEFAULT | inherit.
TCLASS    := 0x00 .. 0xFF | inherit
FLOWLABEL := { 0x00000 .. 0xfffff | inherit | compute.

>> Perhaps this would require various changes to seg6 infrastructure
>> including seg6_iptunnel_policy, seg6_build_state, fill_encap,
>> get_encap_size, etc.
>>
>> We have proposed a patch before to support optional parameters for SRv6
>> behaviors [1].
>> Unfortunately, this patch was rejected.
>>
> 
> not sure I follow why the patch was rejected. Does it change behavior of
> existing code?
>

The comment from David miller was "People taking advantage of this new 
flexibility will write applications that DO NOT WORK on older kernels."

Perhaps, here we can a bit of discussion. Because also applications that 
leverage SRv6 encapsulation will not work on kernels before 4.10. 
Applications that leverage SRv6 VPN behvaiors will not work on kernels 
before 4.14. Applications that leverages SRv6 capabilites in iptables 
will not work on kernels before 4.16.

So when people write an application they have minimum requirement (e.g., 
kernel 5.x)

I would like to get David miller feedback as well as yours on how we 
should proceed and I can work on these features.

> I would expect that new attributes can be added without affecting
> handling of current ones. Looking at seg6_iptunnel.c the new attribute
> would be ignored on older kernels but should be fine on new ones and
> forward.
> 
> ###
> 
> Since seg6 does not have strict attribute checking the only way to find
> out if it is supported is to send down the config and then read it back.
> If the attribute is missing, the kernel does not support. Ugly, but one
> way to determine support. The next time an attribute is added to seg6
> code, strict checking should be enabled so that going forward as new
> attributes are added older kernels with strict checking would reject it.
>