linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Reji Thomas <rejithomas@juniper.net>
Cc: davem@davemloft.net, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, rejithomas.d@gmail.com,
	kernel test robot <lkp@intel.com>,
	Mathieu Xhonneux <m.xhonneux@gmail.com>,
	David Lebrun <david.lebrun@uclouvain.be>
Subject: Re: [PATCH v2] IPv6: sr: Fix End.X nexthop to use oif.
Date: Sun, 18 Oct 2020 16:01:47 -0700	[thread overview]
Message-ID: <20201018160147.6b3c940a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20201015082119.68287-1-rejithomas@juniper.net>

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?

  reply	other threads:[~2020-10-18 23:07 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-15  8:21 [PATCH v2] IPv6: sr: Fix End.X nexthop to use oif Reji Thomas
2020-10-18 23:01 ` Jakub Kicinski [this message]
2020-10-19  3:55   ` Reji Thomas
2020-10-19 16:17     ` Jakub Kicinski
2020-10-20 12:25       ` Reji Thomas
2020-10-20  9:28     ` Ahmed Abdelsalam
2020-10-20  9:34       ` Ahmed Abdelsalam
2020-10-20 12:35         ` Reji Thomas
2020-10-21  8:12           ` Ahmed Abdelsalam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201018160147.6b3c940a@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=davem@davemloft.net \
    --cc=david.lebrun@uclouvain.be \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lkp@intel.com \
    --cc=m.xhonneux@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=rejithomas.d@gmail.com \
    --cc=rejithomas@juniper.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).