Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191112102518.4406-1-mcroce@redhat.com> <20191112150046.2aehmeoq7ri6duwo@netronome.com>
In-Reply-To: <20191112150046.2aehmeoq7ri6duwo@netronome.com>
From:   Matteo Croce <mcroce@redhat.com>
Date:   Tue, 12 Nov 2019 16:46:12 +0100
Message-ID: <CAGnkfhyt7wV-qDODQL1DtDoW0anoehVX7zoVk8y_C4WB0tMuUw@mail.gmail.com>
Subject: Re: [PATCH net-next] openvswitch: add TTL decrement action
To:     Simon Horman <simon.horman@netronome.com>
Cc:     netdev <netdev@vger.kernel.org>, dev@openvswitch.org,
        LKML <linux-kernel@vger.kernel.org>,
        Pravin B Shelar <pshelar@ovn.org>,
        "David S. Miller" <davem@davemloft.net>,
        Bindiya Kurle <bindiyakurle@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 12, 2019 at 4:00 PM Simon Horman <simon.horman@netronome.com> w=
rote:
>
> On Tue, Nov 12, 2019 at 11:25:18AM +0100, Matteo Croce wrote:
> > New action to decrement TTL instead of setting it to a fixed value.
> > This action will decrement the TTL and, in case of expired TTL, send th=
e
> > packet to userspace via output_userspace() to take care of it.
> >
> > Supports both IPv4 and IPv6 via the ttl and hop_limit fields, respectiv=
ely.
> >
>
> Usually OVS achieves this behaviour by matching on the TTL and
> setting it to the desired value, pre-calculated as TTL -1.
> With that in mind could you explain the motivation for this
> change?
>

Hi,

the problem is that OVS creates a flow for each ttl it see. I can let
vswitchd create 255 flows with like this:

$ for i in {2..255}; do ping 192.168.0.2 -t $i -c1 -w1 &>/dev/null & done
$ ovs-dpctl dump-flows |fgrep -c 'set(ipv4(ttl'
255


> > @@ -1174,6 +1174,43 @@ static int execute_check_pkt_len(struct datapath=
 *dp, struct sk_buff *skb,
> >                            nla_len(actions), last, clone_flow_key);
> >  }
> >
> > +static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *ke=
y)
> > +{
> > +     int err;
> > +
> > +     if (skb->protocol =3D=3D htons(ETH_P_IPV6)) {
> > +             struct ipv6hdr *nh =3D ipv6_hdr(skb);
> > +
> > +             err =3D skb_ensure_writable(skb, skb_network_offset(skb) =
+
> > +                                       sizeof(*nh));
> > +             if (unlikely(err))
> > +                     return err;
> > +
> > +             if (nh->hop_limit <=3D 1)
> > +                     return -EHOSTUNREACH;
> > +
> > +             key->ip.ttl =3D --nh->hop_limit;
> > +     } else {
> > +             struct iphdr *nh =3D ip_hdr(skb);
> > +             u8 old_ttl;
> > +
> > +             err =3D skb_ensure_writable(skb, skb_network_offset(skb) =
+
> > +                                       sizeof(*nh));
> > +             if (unlikely(err))
> > +                     return err;
> > +
> > +             if (nh->ttl <=3D 1)
> > +                     return -EHOSTUNREACH;
> > +
> > +             old_ttl =3D nh->ttl--;
> > +             csum_replace2(&nh->check, htons(old_ttl << 8),
> > +                           htons(nh->ttl << 8));
> > +             key->ip.ttl =3D nh->ttl;
> > +     }
>
> The above may send packets with TTL =3D 0, is that desired?
>

If TTL is 1 or 0, execute_dec_ttl() returns -EHOSTUNREACH, and the
caller will just send the packet to userspace and then free it.
I think this is enough, am I missing something?

Bye,
--=20
Matteo Croce
per aspera ad upstream