netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
@ 2024-05-13 22:00 Antonio Ojea
  2024-05-13 22:00 ` [PATCH v3 1/2] netfilter: nft_queue: compute SCTP checksum Antonio Ojea
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Antonio Ojea @ 2024-05-13 22:00 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo, fw, Antonio Ojea

Fixes the bug described in
https://bugzilla.netfilter.org/show_bug.cgi?id=1742
causing netfilter to drop SCTP packets when using
nfqueue and GSO due to incorrect checksum.

Patch 1 adds a new helper to process the sctp checksum
correctly.

Patch 2 adds a selftest regression test.

Antonio Ojea (2):
  netfilter: nft_queue: compute SCTP checksum
  selftests: net: netfilter: nft_queue.sh: sctp checksum

 net/netfilter/nfnetlink_queue.c               | 10 ++++-
 .../selftests/net/netfilter/nft_queue.sh      | 38 +++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.45.0.118.g7fe29c98d7-goog


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 1/2] netfilter: nft_queue: compute SCTP checksum
  2024-05-13 22:00 [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum Antonio Ojea
@ 2024-05-13 22:00 ` Antonio Ojea
  2024-05-13 22:18   ` Florian Westphal
  2024-05-13 22:00 ` [PATCH v3 2/2] selftests: net: netfilter: nft_queue.sh: sctp checksum Antonio Ojea
  2024-05-20 11:27 ` [PATCH v3 0/2] netfilter: nfqueue: incorrect " Pablo Neira Ayuso
  2 siblings, 1 reply; 17+ messages in thread
From: Antonio Ojea @ 2024-05-13 22:00 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo, fw, Antonio Ojea

when packet is enqueued with nfqueue and GSO is enabled, checksum
calculation has to take into account the protocol, as SCTP uses a
32 bits CRC checksum.

Signed-off-by: Antonio Ojea <aojea@google.com>
---
V1 -> V2: add a helper function to process the checksum
V2 -> V3: use tabs instead of whitespaces

 net/netfilter/nfnetlink_queue.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 00f4bd21c59b..13802907ddb8 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -538,6 +538,14 @@ static int nfqnl_put_bridge(struct nf_queue_entry *entry, struct sk_buff *skb)
 	return -1;
 }
 
+static int nf_queue_checksum_help(struct sk_buff *entskb)
+{
+	if (skb_csum_is_sctp(entskb))
+		return skb_crc32c_csum_help(entskb);
+
+	return skb_checksum_help(entskb);
+}
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			   struct nf_queue_entry *entry,
@@ -600,7 +608,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	case NFQNL_COPY_PACKET:
 		if (!(queue->flags & NFQA_CFG_F_GSO) &&
 		    entskb->ip_summed == CHECKSUM_PARTIAL &&
-		    skb_checksum_help(entskb))
+		    nf_queue_checksum_help(entskb))
 			return NULL;
 
 		data_len = READ_ONCE(queue->copy_range);
-- 
2.45.0.118.g7fe29c98d7-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 2/2] selftests: net: netfilter: nft_queue.sh: sctp checksum
  2024-05-13 22:00 [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum Antonio Ojea
  2024-05-13 22:00 ` [PATCH v3 1/2] netfilter: nft_queue: compute SCTP checksum Antonio Ojea
@ 2024-05-13 22:00 ` Antonio Ojea
  2024-05-13 22:18   ` Florian Westphal
  2024-05-20 11:27 ` [PATCH v3 0/2] netfilter: nfqueue: incorrect " Pablo Neira Ayuso
  2 siblings, 1 reply; 17+ messages in thread
From: Antonio Ojea @ 2024-05-13 22:00 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo, fw, Antonio Ojea

Test that nfqueue, when using GSO, process SCTP packets
correctly.

Regression test for https://bugzilla.netfilter.org/show_bug.cgi?id=1742

Signed-off-by: Antonio Ojea <aojea@google.com>
---
 .../selftests/net/netfilter/nft_queue.sh      | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/testing/selftests/net/netfilter/nft_queue.sh b/tools/testing/selftests/net/netfilter/nft_queue.sh
index 8538f08c64c2..5e075c7e0350 100755
--- a/tools/testing/selftests/net/netfilter/nft_queue.sh
+++ b/tools/testing/selftests/net/netfilter/nft_queue.sh
@@ -25,6 +25,9 @@ cleanup()
 }
 
 checktool "nft --version" "test without nft tool"
+checktool "socat -h" "run test without socat"
+
+modprobe -q sctp
 
 trap cleanup EXIT
 
@@ -375,6 +378,40 @@ EOF
 	wait 2>/dev/null
 }
 
+test_sctp_forward()
+{
+        ip netns exec "$nsrouter" nft -f /dev/stdin <<EOF
+table inet sctpq {
+        chain forward {
+        type filter hook forward priority 0; policy accept;
+                sctp dport 12345 queue num 10
+        }
+}
+EOF
+        ip netns exec "$nsrouter" ./nf_queue -q 10 -G -t "$timeout" &
+        local nfqpid=$!
+
+        timeout 5 ip netns exec "$ns2" socat -u SCTP-LISTEN:12345 STDOUT > "$TMPFILE1" &
+        local rpid=$!
+
+        # ss does not show the sctp socket?
+        busywait "$BUSYWAIT_TIMEOUT" sh -c "ps axf | grep -q SCTP-LISTEN" "$ns2"
+
+        ip netns exec "$ns1" socat -u STDIN SCTP:10.0.2.99:12345 <"$TMPINPUT" >/dev/null
+
+        if ! ip netns exec "$nsrouter" nft delete table inet sctpq; then
+                echo "FAIL:  Could not delete sctpq table"
+                exit 1
+        fi
+
+        if ! diff -u "$TMPINPUT" "$TMPFILE1" ; then
+                echo "FAIL: lost packets?!" 1>&2
+                return
+        fi
+
+        wait "$rpid" && echo "PASS: sctp and nfqueue in forward chain with GSO"
+}
+
 ip netns exec "$nsrouter" sysctl net.ipv6.conf.all.forwarding=1 > /dev/null
 ip netns exec "$nsrouter" sysctl net.ipv4.conf.veth0.forwarding=1 > /dev/null
 ip netns exec "$nsrouter" sysctl net.ipv4.conf.veth1.forwarding=1 > /dev/null
@@ -413,5 +450,6 @@ test_tcp_localhost
 test_tcp_localhost_connectclose
 test_tcp_localhost_requeue
 test_icmp_vrf
+test_sctp_forward
 
 exit $ret
-- 
2.45.0.118.g7fe29c98d7-goog


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/2] netfilter: nft_queue: compute SCTP checksum
  2024-05-13 22:00 ` [PATCH v3 1/2] netfilter: nft_queue: compute SCTP checksum Antonio Ojea
@ 2024-05-13 22:18   ` Florian Westphal
  0 siblings, 0 replies; 17+ messages in thread
From: Florian Westphal @ 2024-05-13 22:18 UTC (permalink / raw)
  To: Antonio Ojea; +Cc: netfilter-devel, pablo, fw

Antonio Ojea <aojea@google.com> wrote:
> when packet is enqueued with nfqueue and GSO is enabled, checksum
> calculation has to take into account the protocol, as SCTP uses a
> 32 bits CRC checksum.

Reviewed-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/2] selftests: net: netfilter: nft_queue.sh: sctp checksum
  2024-05-13 22:00 ` [PATCH v3 2/2] selftests: net: netfilter: nft_queue.sh: sctp checksum Antonio Ojea
@ 2024-05-13 22:18   ` Florian Westphal
  0 siblings, 0 replies; 17+ messages in thread
From: Florian Westphal @ 2024-05-13 22:18 UTC (permalink / raw)
  To: Antonio Ojea; +Cc: netfilter-devel, pablo, fw

Antonio Ojea <aojea@google.com> wrote:
> Test that nfqueue, when using GSO, process SCTP packets
> correctly.
> 
> Regression test for https://bugzilla.netfilter.org/show_bug.cgi?id=1742
> 

Acked-by: Florian Westphal <fw@strlen.de>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-13 22:00 [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum Antonio Ojea
  2024-05-13 22:00 ` [PATCH v3 1/2] netfilter: nft_queue: compute SCTP checksum Antonio Ojea
  2024-05-13 22:00 ` [PATCH v3 2/2] selftests: net: netfilter: nft_queue.sh: sctp checksum Antonio Ojea
@ 2024-05-20 11:27 ` Pablo Neira Ayuso
  2024-05-20 15:44   ` Pablo Neira Ayuso
  2 siblings, 1 reply; 17+ messages in thread
From: Pablo Neira Ayuso @ 2024-05-20 11:27 UTC (permalink / raw)
  To: Antonio Ojea; +Cc: netfilter-devel, fw

On Mon, May 13, 2024 at 10:00:31PM +0000, Antonio Ojea wrote:
> Fixes the bug described in
> https://bugzilla.netfilter.org/show_bug.cgi?id=1742
> causing netfilter to drop SCTP packets when using
> nfqueue and GSO due to incorrect checksum.
> 
> Patch 1 adds a new helper to process the sctp checksum
> correctly.
> 
> Patch 2 adds a selftest regression test.

I am inclined to integrated this into nf.git, I will pick a Fixes: tag
sufficiently old so -stable picks up.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-20 11:27 ` [PATCH v3 0/2] netfilter: nfqueue: incorrect " Pablo Neira Ayuso
@ 2024-05-20 15:44   ` Pablo Neira Ayuso
  2024-05-20 18:33     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 17+ messages in thread
From: Pablo Neira Ayuso @ 2024-05-20 15:44 UTC (permalink / raw)
  To: Antonio Ojea; +Cc: netfilter-devel, fw

On Mon, May 20, 2024 at 01:27:22PM +0200, Pablo Neira Ayuso wrote:
> On Mon, May 13, 2024 at 10:00:31PM +0000, Antonio Ojea wrote:
> > Fixes the bug described in
> > https://bugzilla.netfilter.org/show_bug.cgi?id=1742
> > causing netfilter to drop SCTP packets when using
> > nfqueue and GSO due to incorrect checksum.
> > 
> > Patch 1 adds a new helper to process the sctp checksum
> > correctly.
> > 
> > Patch 2 adds a selftest regression test.
> 
> I am inclined to integrated this into nf.git, I will pick a Fixes: tag
> sufficiently old so -stable picks up.

I have to collapse this chunk, otherwise I hit one issue with missing
exported symbol. No need to resend, I will amend here. Just for the
record.

diff --git a/net/core/dev.c b/net/core/dev.c
index e1bb6d7856d9..e7d83b95e6cc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3384,6 +3384,7 @@ int skb_crc32c_csum_help(struct sk_buff *skb)
 out:
        return ret;
 }
+EXPORT_SYMBOL(skb_crc32c_csum_help);
 
 __be16 skb_network_protocol(struct sk_buff *skb, int *depth)
 {

ERROR: modpost: "skb_crc32c_csum_help" [net/netfilter/nfnetlink_queue.ko] undefined!
make[2]: *** [scripts/Makefile.modpost:145: Module.symvers] Error

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-20 15:44   ` Pablo Neira Ayuso
@ 2024-05-20 18:33     ` Pablo Neira Ayuso
  2024-05-20 18:47       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 17+ messages in thread
From: Pablo Neira Ayuso @ 2024-05-20 18:33 UTC (permalink / raw)
  To: Antonio Ojea; +Cc: netfilter-devel, fw

On Mon, May 20, 2024 at 05:44:35PM +0200, Pablo Neira Ayuso wrote:
> On Mon, May 20, 2024 at 01:27:22PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, May 13, 2024 at 10:00:31PM +0000, Antonio Ojea wrote:
> > > Fixes the bug described in
> > > https://bugzilla.netfilter.org/show_bug.cgi?id=1742
> > > causing netfilter to drop SCTP packets when using
> > > nfqueue and GSO due to incorrect checksum.
> > > 
> > > Patch 1 adds a new helper to process the sctp checksum
> > > correctly.
> > > 
> > > Patch 2 adds a selftest regression test.
> > 
> > I am inclined to integrated this into nf.git, I will pick a Fixes: tag
> > sufficiently old so -stable picks up.
> 
> I have to collapse this chunk, otherwise I hit one issue with missing
> exported symbol. No need to resend, I will amend here. Just for the
> record.

Hm. SCTP GSO support is different too, because it keeps a list of segments.

static int
nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
{
[...]
        if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb))
                return __nfqnl_enqueue_packet(net, queue, entry);

I think this needs to be:

        if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb) || !skb_is_gso_sctp(skb))
                return __nfqnl_enqueue_packet(net, queue, entry);

so SCTP GSO packets enters this path below:

        nf_bridge_adjust_skb_data(skb);
        segs = skb_gso_segment(skb, 0);

to deliver separated segments to userspace.

Otherwise, I don't see yet how userspace can deal with several SCTP
segments, from nf_reinject() there is a list of segments no more.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-20 18:33     ` Pablo Neira Ayuso
@ 2024-05-20 18:47       ` Pablo Neira Ayuso
  2024-05-20 18:59         ` Antonio Ojea
  0 siblings, 1 reply; 17+ messages in thread
From: Pablo Neira Ayuso @ 2024-05-20 18:47 UTC (permalink / raw)
  To: Antonio Ojea; +Cc: netfilter-devel, fw

On Mon, May 20, 2024 at 08:33:39PM +0200, Pablo Neira Ayuso wrote:
> On Mon, May 20, 2024 at 05:44:35PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, May 20, 2024 at 01:27:22PM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, May 13, 2024 at 10:00:31PM +0000, Antonio Ojea wrote:
> > > > Fixes the bug described in
> > > > https://bugzilla.netfilter.org/show_bug.cgi?id=1742
> > > > causing netfilter to drop SCTP packets when using
> > > > nfqueue and GSO due to incorrect checksum.
> > > > 
> > > > Patch 1 adds a new helper to process the sctp checksum
> > > > correctly.
> > > > 
> > > > Patch 2 adds a selftest regression test.
> > > 
> > > I am inclined to integrated this into nf.git, I will pick a Fixes: tag
> > > sufficiently old so -stable picks up.
> > 
> > I have to collapse this chunk, otherwise I hit one issue with missing
> > exported symbol. No need to resend, I will amend here. Just for the
> > record.
> 
> Hm. SCTP GSO support is different too, because it keeps a list of segments.
> 
> static int
> nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
> {
> [...]
>         if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb))
>                 return __nfqnl_enqueue_packet(net, queue, entry);
> 
> I think this needs to be:
> 
>         if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb) || !skb_is_gso_sctp(skb))

This is not correct either:

        if (queue->flags & NFQA_CFG_F_GSO) is true, this also needs !skb_is_gso_sctp(skb)

I can see the current selftest disables the NFQA_CFG_F_GSO flag (-G
option in nf_queue test program), I suspect that's why this is working.

>                 return __nfqnl_enqueue_packet(net, queue, entry);
> 
> so SCTP GSO packets enters this path below:
> 
>         nf_bridge_adjust_skb_data(skb);
>         segs = skb_gso_segment(skb, 0);
> 
> to deliver separated segments to userspace.
> 
> Otherwise, I don't see yet how userspace can deal with several SCTP
> segments, from nf_reinject() there is a list of segments no more.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-20 18:47       ` Pablo Neira Ayuso
@ 2024-05-20 18:59         ` Antonio Ojea
  2024-05-21 10:48           ` Pablo Neira Ayuso
  0 siblings, 1 reply; 17+ messages in thread
From: Antonio Ojea @ 2024-05-20 18:59 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, fw

On Mon, May 20, 2024 at 7:47 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Mon, May 20, 2024 at 08:33:39PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, May 20, 2024 at 05:44:35PM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, May 20, 2024 at 01:27:22PM +0200, Pablo Neira Ayuso wrote:
> > > > On Mon, May 13, 2024 at 10:00:31PM +0000, Antonio Ojea wrote:
> > > > > Fixes the bug described in
> > > > > https://bugzilla.netfilter.org/show_bug.cgi?id=1742
> > > > > causing netfilter to drop SCTP packets when using
> > > > > nfqueue and GSO due to incorrect checksum.
> > > > >
> > > > > Patch 1 adds a new helper to process the sctp checksum
> > > > > correctly.
> > > > >
> > > > > Patch 2 adds a selftest regression test.
> > > >
> > > > I am inclined to integrated this into nf.git, I will pick a Fixes: tag
> > > > sufficiently old so -stable picks up.
> > >
> > > I have to collapse this chunk, otherwise I hit one issue with missing
> > > exported symbol. No need to resend, I will amend here. Just for the
> > > record.
> >
> > Hm. SCTP GSO support is different too, because it keeps a list of segments.
> >
> > static int
> > nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
> > {
> > [...]
> >         if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb))
> >                 return __nfqnl_enqueue_packet(net, queue, entry);
> >
> > I think this needs to be:
> >
> >         if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb) || !skb_is_gso_sctp(skb))
>
> This is not correct either:
>
>         if (queue->flags & NFQA_CFG_F_GSO) is true, this also needs !skb_is_gso_sctp(skb)
>
> I can see the current selftest disables the NFQA_CFG_F_GSO flag (-G
> option in nf_queue test program), I suspect that's why this is working.
>

I see, so I fixed the bug in one direction and regressed in the other
one, let me retest both things locallly

> >                 return __nfqnl_enqueue_packet(net, queue, entry);
> >
> > so SCTP GSO packets enters this path below:
> >
> >         nf_bridge_adjust_skb_data(skb);
> >         segs = skb_gso_segment(skb, 0);
> >
> > to deliver separated segments to userspace.
> >
> > Otherwise, I don't see yet how userspace can deal with several SCTP
> > segments, from nf_reinject() there is a list of segments no more.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-20 18:59         ` Antonio Ojea
@ 2024-05-21 10:48           ` Pablo Neira Ayuso
  2024-05-21 10:51             ` Florian Westphal
  0 siblings, 1 reply; 17+ messages in thread
From: Pablo Neira Ayuso @ 2024-05-21 10:48 UTC (permalink / raw)
  To: Antonio Ojea; +Cc: netfilter-devel, fw

On Mon, May 20, 2024 at 07:59:06PM +0100, Antonio Ojea wrote:
> On Mon, May 20, 2024 at 7:47 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > On Mon, May 20, 2024 at 08:33:39PM +0200, Pablo Neira Ayuso wrote:
> > > On Mon, May 20, 2024 at 05:44:35PM +0200, Pablo Neira Ayuso wrote:
> > > > On Mon, May 20, 2024 at 01:27:22PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Mon, May 13, 2024 at 10:00:31PM +0000, Antonio Ojea wrote:
> > > > > > Fixes the bug described in
> > > > > > https://bugzilla.netfilter.org/show_bug.cgi?id=1742
> > > > > > causing netfilter to drop SCTP packets when using
> > > > > > nfqueue and GSO due to incorrect checksum.
> > > > > >
> > > > > > Patch 1 adds a new helper to process the sctp checksum
> > > > > > correctly.
> > > > > >
> > > > > > Patch 2 adds a selftest regression test.
> > > > >
> > > > > I am inclined to integrated this into nf.git, I will pick a Fixes: tag
> > > > > sufficiently old so -stable picks up.
> > > >
> > > > I have to collapse this chunk, otherwise I hit one issue with missing
> > > > exported symbol. No need to resend, I will amend here. Just for the
> > > > record.
> > >
> > > Hm. SCTP GSO support is different too, because it keeps a list of segments.
> > >
> > > static int
> > > nfqnl_enqueue_packet(struct nf_queue_entry *entry, unsigned int queuenum)
> > > {
> > > [...]
> > >         if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb))
> > >                 return __nfqnl_enqueue_packet(net, queue, entry);
> > >
> > > I think this needs to be:
> > >
> > >         if ((queue->flags & NFQA_CFG_F_GSO) || !skb_is_gso(skb) || !skb_is_gso_sctp(skb))
> >
> > This is not correct either:
> >
> >         if (queue->flags & NFQA_CFG_F_GSO) is true, this also needs !skb_is_gso_sctp(skb)
> >
> > I can see the current selftest disables the NFQA_CFG_F_GSO flag (-G
> > option in nf_queue test program), I suspect that's why this is working.
> >
> 
> I see, so I fixed the bug in one direction and regressed in the other
> one, let me retest both things locallly

The check to force GSO SCTP to be segmented before being sent to
userspace, my proposal:

              if (!skb_is_gso(skb) || ((queue->flags & NFQA_CFG_F_GSO) && !skb_is_gso_sctp(skb)))
                        return __nfqnl_enqueue_packet(net, queue, entry);

              nf_bridge_adjust_skb_data(skb);
              segs = skb_gso_segment(skb, 0);
              ...

and extend selftest to cover for the case where -G is not enabled.

> > >                 return __nfqnl_enqueue_packet(net, queue, entry);
> > >
> > > so SCTP GSO packets enters this path below:
> > >
> > >         nf_bridge_adjust_skb_data(skb);
> > >         segs = skb_gso_segment(skb, 0);
> > >
> > > to deliver separated segments to userspace.
> > >
> > > Otherwise, I don't see yet how userspace can deal with several SCTP
> > > segments, from nf_reinject() there is a list of segments no more.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-21 10:48           ` Pablo Neira Ayuso
@ 2024-05-21 10:51             ` Florian Westphal
  2024-05-21 12:07               ` Pablo Neira Ayuso
  0 siblings, 1 reply; 17+ messages in thread
From: Florian Westphal @ 2024-05-21 10:51 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Antonio Ojea, netfilter-devel, fw

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I see, so I fixed the bug in one direction and regressed in the other
> > one, let me retest both things locallly
> 
> The check to force GSO SCTP to be segmented before being sent to
> userspace, my proposal:
> 
>               if (!skb_is_gso(skb) || ((queue->flags & NFQA_CFG_F_GSO) && !skb_is_gso_sctp(skb)))
>                         return __nfqnl_enqueue_packet(net, queue, entry);

This disables F_GSO with sctp packets, is sctp incompatible with nfqueue?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-21 10:51             ` Florian Westphal
@ 2024-05-21 12:07               ` Pablo Neira Ayuso
  2024-05-21 12:48                 ` Florian Westphal
  0 siblings, 1 reply; 17+ messages in thread
From: Pablo Neira Ayuso @ 2024-05-21 12:07 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Antonio Ojea, netfilter-devel

On Tue, May 21, 2024 at 12:51:24PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > I see, so I fixed the bug in one direction and regressed in the other
> > > one, let me retest both things locallly
> > 
> > The check to force GSO SCTP to be segmented before being sent to
> > userspace, my proposal:
> > 
> >               if (!skb_is_gso(skb) || ((queue->flags & NFQA_CFG_F_GSO) && !skb_is_gso_sctp(skb)))
> >                         return __nfqnl_enqueue_packet(net, queue, entry);
> 
> This disables F_GSO with sctp packets, is sctp incompatible with nfqueue?

This will send a big SCTP payload to userspace (larger than mtu),
then, userspace will send the such big SCTP payload to kernelspace via
nf_reinject().

Can kernel deal with SCTP packets larger than MTU?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-21 12:07               ` Pablo Neira Ayuso
@ 2024-05-21 12:48                 ` Florian Westphal
  2024-05-21 12:58                   ` Pablo Neira Ayuso
  0 siblings, 1 reply; 17+ messages in thread
From: Florian Westphal @ 2024-05-21 12:48 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, Antonio Ojea, netfilter-devel

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > userspace, my proposal:
> > > 
> > >               if (!skb_is_gso(skb) || ((queue->flags & NFQA_CFG_F_GSO) && !skb_is_gso_sctp(skb)))
> > >                         return __nfqnl_enqueue_packet(net, queue, entry);
> > 
> > This disables F_GSO with sctp packets, is sctp incompatible with nfqueue?
> 
> This will send a big SCTP payload to userspace (larger than mtu),
> then, userspace will send the such big SCTP payload to kernelspace via
> nf_reinject().

I'm not following.

  if (!skb_is_gso(skb) || ((queue->flags & NFQA_CFG_F_GSO) && !skb_is_gso_sctp(skb)))
     return __nfqnl_enqueue_packet(net, queue, entry);

Means:
1. skb isn't gso -> queue, OR
2. Userspace can cope with large packets AND packet is not sctp-gso
-> queue

-> sctp gso packets are fully software-segmented in kernel, and
we queue n packets instead of 1 superpacket.

Apparently GSO SCTP is incompatible resp. skb_zerocopy() lacks
GSO_BY_FRAGS support.

Too bad.

> Can kernel deal with SCTP packets larger than MTU?

As far as I can see the gso helpers used in fwd and output
path handle this transparently, i.e., yes.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-21 12:48                 ` Florian Westphal
@ 2024-05-21 12:58                   ` Pablo Neira Ayuso
  2024-06-02 15:30                     ` Florian Westphal
  0 siblings, 1 reply; 17+ messages in thread
From: Pablo Neira Ayuso @ 2024-05-21 12:58 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Antonio Ojea, netfilter-devel

On Tue, May 21, 2024 at 02:48:50PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > userspace, my proposal:
> > > > 
> > > >               if (!skb_is_gso(skb) || ((queue->flags & NFQA_CFG_F_GSO) && !skb_is_gso_sctp(skb)))
> > > >                         return __nfqnl_enqueue_packet(net, queue, entry);
> > > 
> > > This disables F_GSO with sctp packets, is sctp incompatible with nfqueue?
> > 
> > This will send a big SCTP payload to userspace (larger than mtu),
> > then, userspace will send the such big SCTP payload to kernelspace via
> > nf_reinject().
> 
> I'm not following.
> 
>   if (!skb_is_gso(skb) || ((queue->flags & NFQA_CFG_F_GSO) && !skb_is_gso_sctp(skb)))
>      return __nfqnl_enqueue_packet(net, queue, entry);
> 
> Means:
> 1. skb isn't gso -> queue, OR
> 2. Userspace can cope with large packets AND packet is not sctp-gso

Yes, this is implicit case: skb is gso. This is adding an exception
for sctp-gso.

> -> queue
> 
> -> sctp gso packets are fully software-segmented in kernel, and
> we queue n packets instead of 1 superpacket.
> 
> Apparently GSO SCTP is incompatible resp. skb_zerocopy() lacks
> GSO_BY_FRAGS support.
> 
> Too bad.

Then, either extend skb_zerocopy() to deal with GSO_BY_FRAGS or poor
man approach as above: enqueue n packets until that is supported.

By reading this, my understanding is that you prefer to extend
skb_zerocopy(), I pick up from Antonio's work and send a patch.

> > Can kernel deal with SCTP packets larger than MTU?
> 
> As far as I can see the gso helpers used in fwd and output
> path handle this transparently, i.e., yes.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-05-21 12:58                   ` Pablo Neira Ayuso
@ 2024-06-02 15:30                     ` Florian Westphal
  2024-06-11  8:46                       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 17+ messages in thread
From: Florian Westphal @ 2024-06-02 15:30 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, Antonio Ojea, netfilter-devel

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Yes, this is implicit case: skb is gso. This is adding an exception
> for sctp-gso.

Is that because sctp gso carries a new / distinct sctp header
per frag?

If so, we can't extend skb_zerocopy, as it would create
a bogus packet.

The only "speedup" we could do in that case is that we
do not need to fix the header csum if userspace enabled
GSO support as long as we indicate csum correctness via
the NFQA_SKB_CSUMNOTREADY flag.

But that should probably be done at a later time.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum
  2024-06-02 15:30                     ` Florian Westphal
@ 2024-06-11  8:46                       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 17+ messages in thread
From: Pablo Neira Ayuso @ 2024-06-11  8:46 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Antonio Ojea, netfilter-devel

On Sun, Jun 02, 2024 at 05:30:33PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Yes, this is implicit case: skb is gso. This is adding an exception
> > for sctp-gso.
> 
> Is that because sctp gso carries a new / distinct sctp header
> per frag?
> 
> If so, we can't extend skb_zerocopy, as it would create
> a bogus packet.
> 
> The only "speedup" we could do in that case is that we
> do not need to fix the header csum if userspace enabled
> GSO support as long as we indicate csum correctness via
> the NFQA_SKB_CSUMNOTREADY flag.
> 
> But that should probably be done at a later time.

I have posted a new series:

https://patchwork.ozlabs.org/project/netfilter-devel/list/?series=410326

SCTP packets enter skb_gso_segment() before being sent to userspace
not to break semantics.

I have revisited and extended the tests to exercise GSO path from the
output path.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2024-06-11  8:46 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-13 22:00 [PATCH v3 0/2] netfilter: nfqueue: incorrect sctp checksum Antonio Ojea
2024-05-13 22:00 ` [PATCH v3 1/2] netfilter: nft_queue: compute SCTP checksum Antonio Ojea
2024-05-13 22:18   ` Florian Westphal
2024-05-13 22:00 ` [PATCH v3 2/2] selftests: net: netfilter: nft_queue.sh: sctp checksum Antonio Ojea
2024-05-13 22:18   ` Florian Westphal
2024-05-20 11:27 ` [PATCH v3 0/2] netfilter: nfqueue: incorrect " Pablo Neira Ayuso
2024-05-20 15:44   ` Pablo Neira Ayuso
2024-05-20 18:33     ` Pablo Neira Ayuso
2024-05-20 18:47       ` Pablo Neira Ayuso
2024-05-20 18:59         ` Antonio Ojea
2024-05-21 10:48           ` Pablo Neira Ayuso
2024-05-21 10:51             ` Florian Westphal
2024-05-21 12:07               ` Pablo Neira Ayuso
2024-05-21 12:48                 ` Florian Westphal
2024-05-21 12:58                   ` Pablo Neira Ayuso
2024-06-02 15:30                     ` Florian Westphal
2024-06-11  8:46                       ` Pablo Neira Ayuso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).