Re: [PATCH net-next] openvswitch: add TTL decrement action

[Simon Horman <simon.horman@netronome.com>]

On Tue, Nov 12, 2019 at 04:46:12PM +0100, Matteo Croce wrote:
> On Tue, Nov 12, 2019 at 4:00 PM Simon Horman <simon.horman@netronome.com> wrote:
> >
> > 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 the
> > > packet to userspace via output_userspace() to take care of it.
> > >
> > > Supports both IPv4 and IPv6 via the ttl and hop_limit fields, respectively.
> > >
> >
> > 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

Hi,

so the motivation is to reduce the number of megaflows in the case
where flows otherwise match but the TTL differs. I think this makes sense
and the absence of this feature may date back to designs made before
megaflow support was added - just guessing.

I think this is a reasonable feature but I think it would be good
to explain the motivation in the changelog.

> > > @@ -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 *key)
> > > +{
> > > +     int err;
> > > +
> > > +     if (skb->protocol == htons(ETH_P_IPV6)) {
> > > +             struct ipv6hdr *nh = ipv6_hdr(skb);
> > > +
> > > +             err = skb_ensure_writable(skb, skb_network_offset(skb) +
> > > +                                       sizeof(*nh));
> > > +             if (unlikely(err))
> > > +                     return err;
> > > +
> > > +             if (nh->hop_limit <= 1)
> > > +                     return -EHOSTUNREACH;
> > > +
> > > +             key->ip.ttl = --nh->hop_limit;
> > > +     } else {
> > > +             struct iphdr *nh = ip_hdr(skb);
> > > +             u8 old_ttl;
> > > +
> > > +             err = skb_ensure_writable(skb, skb_network_offset(skb) +
> > > +                                       sizeof(*nh));
> > > +             if (unlikely(err))
> > > +                     return err;
> > > +
> > > +             if (nh->ttl <= 1)
> > > +                     return -EHOSTUNREACH;
> > > +
> > > +             old_ttl = nh->ttl--;
> > > +             csum_replace2(&nh->check, htons(old_ttl << 8),
> > > +                           htons(nh->ttl << 8));
> > > +             key->ip.ttl = nh->ttl;
> > > +     }
> >
> > The above may send packets with TTL = 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?

No, you are not. I was missing something.
I now think this logic is fine.