Re: [PATCHv2 net-next 2/6] lwtunnel: add LWTUNNEL_IP_OPTS support for lwtunnel_ip

[Xin Long <lucien.xin@gmail.com>]

On Fri, Oct 11, 2019 at 1:31 PM Simon Horman <simon.horman@netronome.com> wrote:
>
> On Thu, Oct 10, 2019 at 05:45:14PM +0800, Xin Long wrote:
> > On Wed, Oct 9, 2019 at 3:55 PM Simon Horman <simon.horman@netronome.com> wrote:
> > >
> > > On Tue, Oct 08, 2019 at 11:16:12PM +0800, Xin Long wrote:
> > > > This patch is to add LWTUNNEL_IP_OPTS into lwtunnel_ip_t, by which
> > > > users will be able to set options for ip_tunnel_info by "ip route
> > > > encap" for erspan and vxlan's private metadata. Like one way to go
> > > > in iproute is:
> > > >
> > > >   # ip route add 1.1.1.0/24 encap ip id 1 erspan ver 1 idx 123 \
> > > >       dst 10.1.0.2 dev erspan1
> > > >   # ip route show
> > > >     1.1.1.0/24  encap ip id 1 src 0.0.0.0 dst 10.1.0.2 ttl 0 \
> > > >       tos 0 erspan ver 1 idx 123 dev erspan1 scope link
> > > >
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > >
> > > Hi Xin,
> > >
> > > thanks for your patch.
> > >
> > > While I have no objection to allowing options to be set, as per the sample
> > > ip commands above, I am concerned that the approach you have taken exposes
> > > to userspace the internal encoding used by the kernel for these options.
> > >
> > > This is the same concerned that was raised by others when I posed a patch
> > > to allow setting of Geneve options in a similar manner. I think what is
> > > called for here, as was the case in the Geneve work, is to expose netlink
> > > attributes for each option that may be set and have the kernel form
> > > these into the internal format (which appears to also be the wire format).
> >
> > Understand.
> >
> > Do you think if it's necessary to support for setting these options
> > by ip route command?
> >
> > or if yes, should we introduce a global lwtun_option_ops_list where
> > geneve/vxlan/erspan could register their own option parsing functions?
>
> Hi Xin,
>
> In the case of Geneve the options are (now) exposed via tc cls_flower and
> act_tunnel rather than ip route. The approach taken was to create an
> ENC_OPTS attribute with a ENC_OPTS_GENEVE sub-attribute which in turn has
> sub-attributes that allow options to be described.
>
> The idea behind this design, as I recall, was to allow other options, say
> for VXLAN, to be added via a new sub-attribute of ENC_OPTS.  Without
> examining the details beyond your patch I think a similar approach would
> work well for options supplied via ip route.
Thanks, I will give it a try.