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

[Matteo Croce]

On Tue, Nov 12, 2019 at 4:00 PM Simon Horman <simon.horman@xxxxxxxxxxxxx> 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


> > @@ -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?

Bye,
-- 
Matteo Croce
per aspera ad upstream