| Message ID | 20201015082119.68287-1-rejithomas@juniper.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] IPv6: sr: Fix End.X nexthop to use oif. | expand |
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. > > 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.. > + > + 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? Also doesn't SEG6_LOCAL_ACTION_END_DX6 need a similar treatment?
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.
On Mon, 19 Oct 2020 09:25:12 +0530 Reji Thomas wrote: > > > @@ -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. If we can save the device lookup and still be semantically correct, that's probably better.
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.
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>
On Mon, Oct 19, 2020 at 9:47 PM Jakub Kicinski <kuba@kernel.org> wrote: > > If we can save the device lookup and still be semantically correct, > that's probably better. I am afraid we need to do a device lookup if I am looking up the neighbor. neigh_lookup apis expect dev to be passed. If I go through the current lookup ie ip6_pol_route() I can drop the dev lookup at risk of returning a non neighbor route as it exists today.
Hi Ahmed, On Tue, Oct 20, 2020 at 3:04 PM Ahmed Abdelsalam <ahabdels.dev@gmail.com> wrote: > > 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. > The only catch is OIF cannot be an optional parameter for linklocal address. For global address, it can be made to depend on user specifying the oif in which case code can enforce the lookup with oif and in other cases do an "any" interface lookup. That would also solve Jakub's concern in breaking any existing implementations(using global address) thereby passing the onus to the control plane to enforce the interface behavior as needed for global address. I will wait for the patch. Hope there will be a way to enforce the oif for linklocal address.. > > 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>
Hi Reji, We are going to send the patch to the mailer in the next days. As for the link local addresses, we can add a check for that and kernel returns error in case of link local and OIF not provided. We will add you and Jakub to the recipients when we send the patch. Best, Ahmed On Tue, 20 Oct 2020 18:05:47 +0530 Reji Thomas <rejithomas.d@gmail.com> wrote: > Hi Ahmed, > > > On Tue, Oct 20, 2020 at 3:04 PM Ahmed Abdelsalam <ahabdels.dev@gmail.com> wrote: > > > > 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. > > > The only catch is OIF cannot be an optional parameter for linklocal address. > For global address, it can be made to depend on user specifying the > oif in which case > code can enforce the lookup with oif and in other cases do an "any" > interface lookup. > That would also solve Jakub's concern in breaking any existing > implementations(using global address) > thereby passing the onus to the control plane to enforce the interface > behavior as > needed for global address. > > I will wait for the patch. Hope there will be a way to enforce the oif > for linklocal address.. > > > > > > > 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>
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c index eba23279912d..4603daed9de6 100644 --- a/net/ipv6/seg6_local.c +++ b/net/ipv6/seg6_local.c @@ -153,7 +153,7 @@ static void advance_nextseg(struct ipv6_sr_hdr *srh, struct in6_addr *daddr) static int seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr, - u32 tbl_id, bool local_delivery) + int oif, u32 tbl_id, bool local_delivery) { struct net *net = dev_net(skb->dev); struct ipv6hdr *hdr = ipv6_hdr(skb); @@ -164,6 +164,7 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr, int dev_flags = 0; fl6.flowi6_iif = skb->dev->ifindex; + fl6.flowi6_oif = oif; fl6.daddr = nhaddr ? *nhaddr : hdr->daddr; fl6.saddr = hdr->saddr; fl6.flowlabel = ip6_flowinfo(hdr); @@ -173,7 +174,10 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr, if (nhaddr) fl6.flowi6_flags = FLOWI_FLAG_KNOWN_NH; - if (!tbl_id) { + if (oif) + flags |= RT6_LOOKUP_F_IFACE; + + if (!tbl_id && !oif) { dst = ip6_route_input_lookup(net, skb->dev, &fl6, skb, flags); } else { struct fib6_table *table; @@ -182,7 +186,7 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr, if (!table) goto out; - rt = ip6_pol_route(net, table, 0, &fl6, skb, flags); + rt = ip6_pol_route(net, table, oif, &fl6, skb, flags); dst = &rt->dst; } @@ -212,7 +216,14 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr, int seg6_lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr, u32 tbl_id) { - return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false); + return seg6_lookup_any_nexthop(skb, nhaddr, 0, tbl_id, false); +} + +static int +seg6_strict_lookup_nexthop(struct sk_buff *skb, + struct in6_addr *nhaddr, int oif, u32 tbl_id) +{ + return seg6_lookup_any_nexthop(skb, nhaddr, oif, tbl_id, false); } /* regular endpoint function */ @@ -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); 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; + + seg6_strict_lookup_nexthop(skb, &slwt->nh6, odev->ifindex, 0); return dst_input(skb); @@ -412,7 +429,7 @@ static int input_action_end_dt6(struct sk_buff *skb, skb_set_transport_header(skb, sizeof(struct ipv6hdr)); - seg6_lookup_any_nexthop(skb, NULL, slwt->table, true); + seg6_lookup_any_nexthop(skb, NULL, 0, slwt->table, true); 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, }, {
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> --- Changes in v2: - Fixed seg6_strict_lookup_nexthop() to be a static function Reported-by: kernel test robot <lkp@intel.com> - Fixed comments from Jakub Kicinski <kuba@kernel.org> -Sorted the variable declarations from longest to shortest. -fixes:tag and blank line fixed. --- net/ipv6/seg6_local.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)