[ovs-dev] conntrack: Allow inner NAT of related fragments.

Currently conntrack will refuse to extract metadata from fragmented
IPv4 packets. Usually the fragments would be processed by the ipf
module, but this isn't the case for ICMP related packets. The current
handling will result in these being incorrectly processed.

This patch checks for a frag offset instead of just frag flags, which is
similar to how conntrack handles fragments in the kernel.

Reported-at: https://issues.redhat.com/browse/FDP-136
Reported-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
 lib/conntrack.c       |  2 +-
 tests/ofproto-dpif.at | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

Mike Pattrick via dev <ovs-dev@openvswitch.org> writes:

> Currently conntrack will refuse to extract metadata from fragmented
> IPv4 packets. Usually the fragments would be processed by the ipf
> module, but this isn't the case for ICMP related packets. The current
> handling will result in these being incorrectly processed.
>
> This patch checks for a frag offset instead of just frag flags, which is
> similar to how conntrack handles fragments in the kernel.
>
> Reported-at: https://issues.redhat.com/browse/FDP-136
> Reported-by: Ales Musil <amusil@redhat.com>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---
>  lib/conntrack.c       |  2 +-
>  tests/ofproto-dpif.at | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index fe1ecc159..b02174ec4 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1705,7 +1705,7 @@ extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
>          return false;
>      }
>  
> -    if (IP_IS_FRAGMENT(ip->ip_frag_off)) {
> +    if (ip->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {

Stylistic, but we may want to have something like the following
(untested code) in lib/packets.h::

#define IP_IS_FIRST_FRAG(ip_frag_off) \
   (((ip_frag_off) & htons(IP_MORE_FRAGMENTS | IP_FRAG_OFF_MASK)) == \
    htons(IP_MORE_FRAGMENTS))

And the update this block to say:

    if (!IP_IS_FIRST_FRAG(ip->ip_frag_off))

Because the first frag should be allowed to be processed to extract
these details.

It may also be a good idea to update extract_l4() to check that we are
not the first frag and exit there as well (since the l4 data won't be
available - it can be considered an early shunt).

>          return false;
>      }
>  
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 3d84618b3..7930b3f29 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -11597,6 +11597,38 @@ udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=4,dport=3),reply=(src=10.1.1.1,dst=10.
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +AT_SETUP([ofproto-dpif - conntrack - related nat])
> +OVS_VSWITCHD_START
> +
> +add_of_ports --pcap br0 1 2
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,priority=100,arp,action=normal
> +table=0,priority=10,ip,in_port=1,udp,action=ct(commit,table=1,nat(dst=1.2.3.4:10000-10000))
> +table=0,priority=10,ip,in_port=2,action=ct(nat,table=1)
> +table=0,priority=1,action=drop
> +table=1,priority=10,in_port=1,ct_state=+trk,action=2
> +table=1,priority=10,in_port=2,ct_state=+trk,action=1
> +table=1,priority=1,action=drop
> +])
> +
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +dnl 1. Send and UDP packet to port 5555.
> +packet1=c6f94ecb72dbe64c473528c9080045000021317040004011b138ac100001ac100002a28e15b3000d20966369616f0a
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=$packet1 actions=resubmit(,0)"])
> +
> +dnl 2. Send an fragmented ICMP related reply from 1.2.3.4:10000.
> +packet2=e64c473528c9c6f94ecb72db080045c000382e87000040019b6701020304ac1000010303ad2d000000004500001c317020004011794aac10000101020304a28e2710000d8623
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=$packet2 actions=resubmit(,0)"])
> +
> +dnl 3. Check that the inner packet is translated.
> +packet3=e64c473528c9c6f94ecb72db080045c000382e8700004001f35aac100002ac1000010303553a000000004500001c317020004011d13dac100001ac100002a28e15b3000def73
> +OVS_WAIT_UNTIL([ovs-pcap p1-tx.pcap | grep -q "$packet3"])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([ofproto-dpif - conntrack - ipv6])
>  OVS_VSWITCHD_START

Aaron Conole <aconole@redhat.com> writes:

> Mike Pattrick via dev <ovs-dev@openvswitch.org> writes:
>
>> Currently conntrack will refuse to extract metadata from fragmented
>> IPv4 packets. Usually the fragments would be processed by the ipf
>> module, but this isn't the case for ICMP related packets. The current
>> handling will result in these being incorrectly processed.
>>
>> This patch checks for a frag offset instead of just frag flags, which is
>> similar to how conntrack handles fragments in the kernel.
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-136
>> Reported-by: Ales Musil <amusil@redhat.com>
>> Signed-off-by: Mike Pattrick <mkp@redhat.com>

Missed that this should also have:

Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.")

>> ---
>>  lib/conntrack.c       |  2 +-
>>  tests/ofproto-dpif.at | 32 ++++++++++++++++++++++++++++++++
>>  2 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index fe1ecc159..b02174ec4 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -1705,7 +1705,7 @@ extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
>>          return false;
>>      }
>>  
>> -    if (IP_IS_FRAGMENT(ip->ip_frag_off)) {
>> +    if (ip->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
>
> Stylistic, but we may want to have something like the following
> (untested code) in lib/packets.h::
>
> #define IP_IS_FIRST_FRAG(ip_frag_off) \
>    (((ip_frag_off) & htons(IP_MORE_FRAGMENTS | IP_FRAG_OFF_MASK)) == \
>     htons(IP_MORE_FRAGMENTS))
>
> And the update this block to say:
>
>     if (!IP_IS_FIRST_FRAG(ip->ip_frag_off))
>
> Because the first frag should be allowed to be processed to extract
> these details.
>
> It may also be a good idea to update extract_l4() to check that we are
> not the first frag and exit there as well (since the l4 data won't be
> available - it can be considered an early shunt).
>
>>          return false;
>>      }
>>  
>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> index 3d84618b3..7930b3f29 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -11597,6 +11597,38 @@ udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=4,dport=3),reply=(src=10.1.1.1,dst=10.
>>  OVS_VSWITCHD_STOP
>>  AT_CLEANUP
>>  
>> +AT_SETUP([ofproto-dpif - conntrack - related nat])
>> +OVS_VSWITCHD_START
>> +
>> +add_of_ports --pcap br0 1 2
>> +
>> +AT_DATA([flows.txt], [dnl
>> +table=0,priority=100,arp,action=normal
>> +table=0,priority=10,ip,in_port=1,udp,action=ct(commit,table=1,nat(dst=1.2.3.4:10000-10000))
>> +table=0,priority=10,ip,in_port=2,action=ct(nat,table=1)
>> +table=0,priority=1,action=drop
>> +table=1,priority=10,in_port=1,ct_state=+trk,action=2
>> +table=1,priority=10,in_port=2,ct_state=+trk,action=1
>> +table=1,priority=1,action=drop
>> +])
>> +
>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>> +
>> +dnl 1. Send and UDP packet to port 5555.
>> +packet1=c6f94ecb72dbe64c473528c9080045000021317040004011b138ac100001ac100002a28e15b3000d20966369616f0a
>> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=$packet1 actions=resubmit(,0)"])
>> +
>> +dnl 2. Send an fragmented ICMP related reply from 1.2.3.4:10000.
>> +packet2=e64c473528c9c6f94ecb72db080045c000382e87000040019b6701020304ac1000010303ad2d000000004500001c317020004011794aac10000101020304a28e2710000d8623
>> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=$packet2 actions=resubmit(,0)"])
>> +
>> +dnl 3. Check that the inner packet is translated.
>> +packet3=e64c473528c9c6f94ecb72db080045c000382e8700004001f35aac100002ac1000010303553a000000004500001c317020004011d13dac100001ac100002a28e15b3000def73
>> +OVS_WAIT_UNTIL([ovs-pcap p1-tx.pcap | grep -q "$packet3"])
>> +
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>>  AT_SETUP([ofproto-dpif - conntrack - ipv6])
>>  OVS_VSWITCHD_START

On Thu, Jun 5, 2025 at 10:57 AM Aaron Conole <aconole@redhat.com> wrote:
>
> Mike Pattrick via dev <ovs-dev@openvswitch.org> writes:
>
> > Currently conntrack will refuse to extract metadata from fragmented
> > IPv4 packets. Usually the fragments would be processed by the ipf
> > module, but this isn't the case for ICMP related packets. The current
> > handling will result in these being incorrectly processed.
> >
> > This patch checks for a frag offset instead of just frag flags, which is
> > similar to how conntrack handles fragments in the kernel.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-136
> > Reported-by: Ales Musil <amusil@redhat.com>
> > Signed-off-by: Mike Pattrick <mkp@redhat.com>
> > ---
> >  lib/conntrack.c       |  2 +-
> >  tests/ofproto-dpif.at | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index fe1ecc159..b02174ec4 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -1705,7 +1705,7 @@ extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
> >          return false;
> >      }
> >
> > -    if (IP_IS_FRAGMENT(ip->ip_frag_off)) {
> > +    if (ip->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
>
> Stylistic, but we may want to have something like the following
> (untested code) in lib/packets.h::
>
> #define IP_IS_FIRST_FRAG(ip_frag_off) \
>    (((ip_frag_off) & htons(IP_MORE_FRAGMENTS | IP_FRAG_OFF_MASK)) == \
>     htons(IP_MORE_FRAGMENTS))
>
> And the update this block to say:
>
>     if (!IP_IS_FIRST_FRAG(ip->ip_frag_off))

I agree, looks better.

>
> Because the first frag should be allowed to be processed to extract
> these details.
>
> It may also be a good idea to update extract_l4() to check that we are
> not the first frag and exit there as well (since the l4 data won't be
> available - it can be considered an early shunt).

Is it possible for a frag to get to extract_l4 without hitting the
check in extract_l3 first?

-M

>
> >          return false;
> >      }
> >
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 3d84618b3..7930b3f29 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -11597,6 +11597,38 @@ udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=4,dport=3),reply=(src=10.1.1.1,dst=10.
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> >
> > +AT_SETUP([ofproto-dpif - conntrack - related nat])
> > +OVS_VSWITCHD_START
> > +
> > +add_of_ports --pcap br0 1 2
> > +
> > +AT_DATA([flows.txt], [dnl
> > +table=0,priority=100,arp,action=normal
> > +table=0,priority=10,ip,in_port=1,udp,action=ct(commit,table=1,nat(dst=1.2.3.4:10000-10000))
> > +table=0,priority=10,ip,in_port=2,action=ct(nat,table=1)
> > +table=0,priority=1,action=drop
> > +table=1,priority=10,in_port=1,ct_state=+trk,action=2
> > +table=1,priority=10,in_port=2,ct_state=+trk,action=1
> > +table=1,priority=1,action=drop
> > +])
> > +
> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> > +
> > +dnl 1. Send and UDP packet to port 5555.
> > +packet1=c6f94ecb72dbe64c473528c9080045000021317040004011b138ac100001ac100002a28e15b3000d20966369616f0a
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=$packet1 actions=resubmit(,0)"])
> > +
> > +dnl 2. Send an fragmented ICMP related reply from 1.2.3.4:10000.
> > +packet2=e64c473528c9c6f94ecb72db080045c000382e87000040019b6701020304ac1000010303ad2d000000004500001c317020004011794aac10000101020304a28e2710000d8623
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=$packet2 actions=resubmit(,0)"])
> > +
> > +dnl 3. Check that the inner packet is translated.
> > +packet3=e64c473528c9c6f94ecb72db080045c000382e8700004001f35aac100002ac1000010303553a000000004500001c317020004011d13dac100001ac100002a28e15b3000def73
> > +OVS_WAIT_UNTIL([ovs-pcap p1-tx.pcap | grep -q "$packet3"])
> > +
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> >  AT_SETUP([ofproto-dpif - conntrack - ipv6])
> >  OVS_VSWITCHD_START
>

Mike Pattrick <mkp@redhat.com> writes:

> On Thu, Jun 5, 2025 at 10:57 AM Aaron Conole <aconole@redhat.com> wrote:
>>
>> Mike Pattrick via dev <ovs-dev@openvswitch.org> writes:
>>
>> > Currently conntrack will refuse to extract metadata from fragmented
>> > IPv4 packets. Usually the fragments would be processed by the ipf
>> > module, but this isn't the case for ICMP related packets. The current
>> > handling will result in these being incorrectly processed.
>> >
>> > This patch checks for a frag offset instead of just frag flags, which is
>> > similar to how conntrack handles fragments in the kernel.
>> >
>> > Reported-at: https://issues.redhat.com/browse/FDP-136
>> > Reported-by: Ales Musil <amusil@redhat.com>
>> > Signed-off-by: Mike Pattrick <mkp@redhat.com>
>> > ---
>> >  lib/conntrack.c       |  2 +-
>> >  tests/ofproto-dpif.at | 32 ++++++++++++++++++++++++++++++++
>> >  2 files changed, 33 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/lib/conntrack.c b/lib/conntrack.c
>> > index fe1ecc159..b02174ec4 100644
>> > --- a/lib/conntrack.c
>> > +++ b/lib/conntrack.c
>> > @@ -1705,7 +1705,7 @@ extract_l3_ipv4(struct conn_key *key, const void *data, size_t size,
>> >          return false;
>> >      }
>> >
>> > -    if (IP_IS_FRAGMENT(ip->ip_frag_off)) {
>> > +    if (ip->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
>>
>> Stylistic, but we may want to have something like the following
>> (untested code) in lib/packets.h::
>>
>> #define IP_IS_FIRST_FRAG(ip_frag_off) \
>>    (((ip_frag_off) & htons(IP_MORE_FRAGMENTS | IP_FRAG_OFF_MASK)) == \
>>     htons(IP_MORE_FRAGMENTS))
>>
>> And the update this block to say:
>>
>>     if (!IP_IS_FIRST_FRAG(ip->ip_frag_off))
>
> I agree, looks better.
>
>>
>> Because the first frag should be allowed to be processed to extract
>> these details.
>>
>> It may also be a good idea to update extract_l4() to check that we are
>> not the first frag and exit there as well (since the l4 data won't be
>> available - it can be considered an early shunt).
>
> Is it possible for a frag to get to extract_l4 without hitting the
> check in extract_l3 first?

At present, not that I'm aware.  It's fine to also not do it :)

> -M
>
>>
>> >          return false;
>> >      }
>> >
>> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> > index 3d84618b3..7930b3f29 100644
>> > --- a/tests/ofproto-dpif.at
>> > +++ b/tests/ofproto-dpif.at
>> > @@ -11597,6 +11597,38 @@ udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=4,dport=3),reply=(src=10.1.1.1,dst=10.
>> >  OVS_VSWITCHD_STOP
>> >  AT_CLEANUP
>> >
>> > +AT_SETUP([ofproto-dpif - conntrack - related nat])
>> > +OVS_VSWITCHD_START
>> > +
>> > +add_of_ports --pcap br0 1 2
>> > +
>> > +AT_DATA([flows.txt], [dnl
>> > +table=0,priority=100,arp,action=normal
>> > +table=0,priority=10,ip,in_port=1,udp,action=ct(commit,table=1,nat(dst=1.2.3.4:10000-10000))
>> > +table=0,priority=10,ip,in_port=2,action=ct(nat,table=1)
>> > +table=0,priority=1,action=drop
>> > +table=1,priority=10,in_port=1,ct_state=+trk,action=2
>> > +table=1,priority=10,in_port=2,ct_state=+trk,action=1
>> > +table=1,priority=1,action=drop
>> > +])
>> > +
>> > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>> > +
>> > +dnl 1. Send and UDP packet to port 5555.
>> > +packet1=c6f94ecb72dbe64c473528c9080045000021317040004011b138ac100001ac100002a28e15b3000d20966369616f0a
>> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=$packet1 actions=resubmit(,0)"])
>> > +
>> > +dnl 2. Send an fragmented ICMP related reply from 1.2.3.4:10000.
>> > +packet2=e64c473528c9c6f94ecb72db080045c000382e87000040019b6701020304ac1000010303ad2d000000004500001c317020004011794aac10000101020304a28e2710000d8623
>> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=$packet2 actions=resubmit(,0)"])
>> > +
>> > +dnl 3. Check that the inner packet is translated.
>> > +packet3=e64c473528c9c6f94ecb72db080045c000382e8700004001f35aac100002ac1000010303553a000000004500001c317020004011d13dac100001ac100002a28e15b3000def73
>> > +OVS_WAIT_UNTIL([ovs-pcap p1-tx.pcap | grep -q "$packet3"])
>> > +
>> > +OVS_VSWITCHD_STOP
>> > +AT_CLEANUP
>> > +
>> >  AT_SETUP([ofproto-dpif - conntrack - ipv6])
>> >  OVS_VSWITCHD_START
>>