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

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