lkml.org 
[lkml]   [2020]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] IPv6: sr: Fix End.X nexthop to use oif.
We are submitting the patch for End.DT4. End.DX4 is already there. 

So the optional parameter and OIF applies directly to End.X/End.DX6/End.DX4.


On Tue, 20 Oct 2020 11:28:29 +0200
Ahmed Abdelsalam <ahabdels.dev@gmail.com> wrote:

> Jakub, Reji,
>
> Andrea (CC'ed) and I have been working on a patch that could solve this issue.
> The patch allows to provide optional parameters to when SRv6 behavior.
> The OIF can be provided as an optional parameter when configuring SRv6 End.X,
> End.DX6 or End.DX4 (we are submiting in the next couple of days to support End.DX4).
>
> We can submit the optional parameter again. Then Reji can leverage this to provide OIF
> as an optional parameters.
>
> Would you agree ?
>
> Thanks
> Ahmed
>
>
>
> On Mon, 19 Oct 2020 09:25:12 +0530
> Reji Thomas <rejithomas.d@gmail.com> wrote:
>
> > Hi,
> >
> > Please find my replies inline below.
> >
> > Regards
> > Reji
> >
> > On Mon, Oct 19, 2020 at 4:31 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Thu, 15 Oct 2020 13:51:19 +0530 Reji Thomas wrote:
> > > > Currently End.X action doesn't consider the outgoing interface
> > > > while looking up the nexthop.This breaks packet path functionality
> > > > specifically while using link local address as the End.X nexthop.
> > > > The patch fixes this by enforcing End.X action to have both nh6 and
> > > > oif and using oif in lookup.It seems this is a day one issue.
> > > >
> > > > Fixes: 140f04c33bbc ("ipv6: sr: implement several seg6local actions")
> > > > Signed-off-by: Reji Thomas <rejithomas@juniper.net>
> > >
> > > David, Mathiey - any comments?
> > >
> > > > @@ -239,6 +250,8 @@ static int input_action_end(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> > > > static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> > > > {
> > > > struct ipv6_sr_hdr *srh;
> > > > + struct net_device *odev;
> > > > + struct net *net = dev_net(skb->dev);
> > >
> > > Order longest to shortest.
> > Sorry. Will fix it.
> >
> > >
> > >
> > > >
> > > > srh = get_and_validate_srh(skb);
> > > > if (!srh)
> > > > @@ -246,7 +259,11 @@ static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> > > >
> > > > advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
> > > >
> > > > - seg6_lookup_nexthop(skb, &slwt->nh6, 0);
> > > > + odev = dev_get_by_index_rcu(net, slwt->oif);
> > > > + if (!odev)
> > > > + goto drop;
> > >
> > > Are you doing this lookup just to make sure that oif exists?
> > > Looks a little wasteful for fast path, but more importantly
> > > it won't be backward compatible, right? See below..
> > >
> > Please see reply below.
> >
> > > > +
> > > > + seg6_strict_lookup_nexthop(skb, &slwt->nh6, odev->ifindex, 0);
> > > >
> > > > return dst_input(skb);
> > > >
> > >
> > > > @@ -566,7 +583,8 @@ static struct seg6_action_desc seg6_action_table[] = {
> > > > },
> > > > {
> > > > .action = SEG6_LOCAL_ACTION_END_X,
> > > > - .attrs = (1 << SEG6_LOCAL_NH6),
> > > > + .attrs = ((1 << SEG6_LOCAL_NH6) |
> > > > + (1 << SEG6_LOCAL_OIF)),
> > > > .input = input_action_end_x,
> > > > },
> > > > {
> > >
> > > If you set this parse_nla_action() will reject all
> > > SEG6_LOCAL_ACTION_END_X without OIF.
> > >
> > > As you say the OIF is only required for using link local addresses,
> > > so this change breaks perfectly legitimate configurations.
> > >
> > > Can we instead only warn about the missing OIF, and only do that when
> > > nh is link local?
> > >
> > End.X is defined as an adjacency-sid and is used to select a specific link to a
> > neighbor for both global and link-local addresses. The intention was
> > to drop the
> > packet even for global addresses if the route via the specific
> > interface is not found.
> > Alternatively(believe semantically correct for End.X definition) I
> > could do a neighbor lookup
> > for nexthop address over specific interface and send the packet out.
> >
> > > Also doesn't SEG6_LOCAL_ACTION_END_DX6 need a similar treatment?
> >
> > Yes. I will update the patch for End.DX6 based on the patch finalized for End.X.
>
>
> --
> Ahmed Abdelsalam <ahabdels.dev@gmail.com>


--
Ahmed Abdelsalam <ahabdels.dev@gmail.com>

\
 
 \ /
  Last update: 2020-10-20 11:35    [W:0.092 / U:0.588 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site