netdev - Re: [PATCH v2] IPv6: sr: Fix End.X nexthop to use oif.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAA8Zg7Gcua1=6CgSkJ-z8uKJneDjedB4z6zm2a+DcYt-_YcmSQ@mail.gmail.com>
Date:   Mon, 19 Oct 2020 09:25:12 +0530
From:   Reji Thomas <rejithomas.d@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Reji Thomas <rejithomas@...iper.net>, davem@...emloft.net,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel test robot <lkp@...el.com>,
        Mathieu Xhonneux <m.xhonneux@...il.com>,
        David Lebrun <david.lebrun@...ouvain.be>
Subject: Re: [PATCH v2] IPv6: sr: Fix End.X nexthop to use oif.

Hi,

Please find my replies inline below.

Regards
Reji

On Mon, Oct 19, 2020 at 4:31 AM Jakub Kicinski <kuba@...nel.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@...iper.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.