[PATCH nf 0/4] netfilter: conntrack: remove 64kb max size assumptions

[PATCH nf 0/4] netfilter: conntrack: remove 64kb max size assumptions

[Florian Westphal]

Some of our dated conntrack helpers assume skbs can't contain
tcp packets larger than 64kb.

Update those.  For SANE, I don't see a reason for the
'full-packet-copy', just extract the sane header.

For h323, packet gets capped at 64k; larger one will be
seen as truncated.

For irc, cap at 4k: a line length should not exceed 512 bytes.
For ftp, use skb_linearize(), its the most simple way to resolve this.

Florian Westphal (4):
  netfilter: conntrack: sane: remove pseudo skb linearization
  netfilter: conntrack: h323: cap packet size at 64k
  netfilter: conntrack_ftp: prefer skb_linearize
  netfilter: conntrack_irc: cap packet search space to 4k

 net/netfilter/nf_conntrack_ftp.c       | 24 +++------
 net/netfilter/nf_conntrack_h323_main.c | 10 +++-
 net/netfilter/nf_conntrack_irc.c       | 12 +++--
 net/netfilter/nf_conntrack_sane.c      | 68 ++++++++++++--------------
 4 files changed, 54 insertions(+), 60 deletions(-)

-- 
2.35.1

[PATCH nf 1/4] netfilter: conntrack: sane: remove pseudo skb linearization

[Florian Westphal]

For historical reason this code performs pseudo linearization of skbs
via skb_header_pointer and a global 64k buffer.

With arrival of BIG TCP, packets generated by TCP stack can exceed 64kb.

Rewrite this to only extract the needed header data.  This also allows
to get rid of the locking.

Fixes: 7c4e983c4f3c ("net: allow gso_max_size to exceed 65536")
Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_sane.c | 68 ++++++++++++++-----------------
 1 file changed, 30 insertions(+), 38 deletions(-)

diff --git a/net/netfilter/nf_conntrack_sane.c b/net/netfilter/nf_conntrack_sane.c
index fcb33b1d5456..13dc421fc4f5 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -34,10 +34,6 @@ MODULE_AUTHOR("Michal Schmidt <mschmidt@redhat.com>");
 MODULE_DESCRIPTION("SANE connection tracking helper");
 MODULE_ALIAS_NFCT_HELPER(HELPER_NAME);
 
-static char *sane_buffer;
-
-static DEFINE_SPINLOCK(nf_sane_lock);
-
 #define MAX_PORTS 8
 static u_int16_t ports[MAX_PORTS];
 static unsigned int ports_c;
@@ -67,14 +63,16 @@ static int help(struct sk_buff *skb,
 	unsigned int dataoff, datalen;
 	const struct tcphdr *th;
 	struct tcphdr _tcph;
-	void *sb_ptr;
 	int ret = NF_ACCEPT;
 	int dir = CTINFO2DIR(ctinfo);
 	struct nf_ct_sane_master *ct_sane_info = nfct_help_data(ct);
 	struct nf_conntrack_expect *exp;
 	struct nf_conntrack_tuple *tuple;
-	struct sane_request *req;
 	struct sane_reply_net_start *reply;
+	union {
+		struct sane_request req;
+		struct sane_reply_net_start repl;
+	} buf;
 
 	/* Until there's been traffic both ways, don't look in packets. */
 	if (ctinfo != IP_CT_ESTABLISHED &&
@@ -92,59 +90,62 @@ static int help(struct sk_buff *skb,
 		return NF_ACCEPT;
 
 	datalen = skb->len - dataoff;
-
-	spin_lock_bh(&nf_sane_lock);
-	sb_ptr = skb_header_pointer(skb, dataoff, datalen, sane_buffer);
-	if (!sb_ptr) {
-		spin_unlock_bh(&nf_sane_lock);
-		return NF_ACCEPT;
-	}
-
 	if (dir == IP_CT_DIR_ORIGINAL) {
+		const struct sane_request *req;
+
 		if (datalen != sizeof(struct sane_request))
-			goto out;
+			return NF_ACCEPT;
+
+		req = skb_header_pointer(skb, dataoff, datalen, &buf.req);
+		if (!req)
+			return NF_ACCEPT;
 
-		req = sb_ptr;
 		if (req->RPC_code != htonl(SANE_NET_START)) {
 			/* Not an interesting command */
-			ct_sane_info->state = SANE_STATE_NORMAL;
-			goto out;
+			WRITE_ONCE(ct_sane_info->state, SANE_STATE_NORMAL);
+			return NF_ACCEPT;
 		}
 
 		/* We're interested in the next reply */
-		ct_sane_info->state = SANE_STATE_START_REQUESTED;
-		goto out;
+		WRITE_ONCE(ct_sane_info->state, SANE_STATE_START_REQUESTED);
+		return NF_ACCEPT;
 	}
 
+	/* IP_CT_DIR_REPLY */
+
 	/* Is it a reply to an uninteresting command? */
-	if (ct_sane_info->state != SANE_STATE_START_REQUESTED)
-		goto out;
+	if (READ_ONCE(ct_sane_info->state) != SANE_STATE_START_REQUESTED)
+		return NF_ACCEPT;
 
 	/* It's a reply to SANE_NET_START. */
-	ct_sane_info->state = SANE_STATE_NORMAL;
+	WRITE_ONCE(ct_sane_info->state, SANE_STATE_NORMAL);
 
 	if (datalen < sizeof(struct sane_reply_net_start)) {
 		pr_debug("NET_START reply too short\n");
-		goto out;
+		return NF_ACCEPT;
 	}
 
-	reply = sb_ptr;
+	datalen = sizeof(struct sane_reply_net_start);
+
+	reply = skb_header_pointer(skb, dataoff, datalen, &buf.repl);
+	if (!reply)
+		return NF_ACCEPT;
+
 	if (reply->status != htonl(SANE_STATUS_SUCCESS)) {
 		/* saned refused the command */
 		pr_debug("unsuccessful SANE_STATUS = %u\n",
 			 ntohl(reply->status));
-		goto out;
+		return NF_ACCEPT;
 	}
 
 	/* Invalid saned reply? Ignore it. */
 	if (reply->zero != 0)
-		goto out;
+		return NF_ACCEPT;
 
 	exp = nf_ct_expect_alloc(ct);
 	if (exp == NULL) {
 		nf_ct_helper_log(skb, ct, "cannot alloc expectation");
-		ret = NF_DROP;
-		goto out;
+		return NF_DROP;
 	}
 
 	tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
@@ -162,9 +163,6 @@ static int help(struct sk_buff *skb,
 	}
 
 	nf_ct_expect_put(exp);
-
-out:
-	spin_unlock_bh(&nf_sane_lock);
 	return ret;
 }
 
@@ -178,7 +176,6 @@ static const struct nf_conntrack_expect_policy sane_exp_policy = {
 static void __exit nf_conntrack_sane_fini(void)
 {
 	nf_conntrack_helpers_unregister(sane, ports_c * 2);
-	kfree(sane_buffer);
 }
 
 static int __init nf_conntrack_sane_init(void)
@@ -187,10 +184,6 @@ static int __init nf_conntrack_sane_init(void)
 
 	NF_CT_HELPER_BUILD_BUG_ON(sizeof(struct nf_ct_sane_master));
 
-	sane_buffer = kmalloc(65536, GFP_KERNEL);
-	if (!sane_buffer)
-		return -ENOMEM;
-
 	if (ports_c == 0)
 		ports[ports_c++] = SANE_PORT;
 
@@ -210,7 +203,6 @@ static int __init nf_conntrack_sane_init(void)
 	ret = nf_conntrack_helpers_register(sane, ports_c * 2);
 	if (ret < 0) {
 		pr_err("failed to register helpers\n");
-		kfree(sane_buffer);
 		return ret;
 	}
 
-- 
2.35.1

[PATCH nf 2/4] netfilter: conntrack: h323: cap packet size at 64k

[Florian Westphal]

With BIG TCP, packets generated by tcp stack may exceed 64kb.
Cap datalen at 64kb.  The internal message format uses 16bit fields,
so no embedded message can exceed 64k size.

Multiple h323 messages in a single superpacket may now result
in a message to get treated as incomplete/truncated, but thats
better than scribbling past h323_buffer.

Another alternative suitable for net tree would be a switch to
skb_linearize().

Fixes: 7c4e983c4f3c ("net: allow gso_max_size to exceed 65536")
Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_h323_main.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index bb76305bb7ff..5a9bce24f3c3 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -34,6 +34,8 @@
 #include <net/netfilter/nf_conntrack_zones.h>
 #include <linux/netfilter/nf_conntrack_h323.h>
 
+#define H323_MAX_SIZE 65535
+
 /* Parameters */
 static unsigned int default_rrq_ttl __read_mostly = 300;
 module_param(default_rrq_ttl, uint, 0600);
@@ -86,6 +88,9 @@ static int get_tpkt_data(struct sk_buff *skb, unsigned int protoff,
 	if (tcpdatalen <= 0)	/* No TCP data */
 		goto clear_out;
 
+	if (tcpdatalen > H323_MAX_SIZE)
+		tcpdatalen = H323_MAX_SIZE;
+
 	if (*data == NULL) {	/* first TPKT */
 		/* Get first TPKT pointer */
 		tpkt = skb_header_pointer(skb, tcpdataoff, tcpdatalen,
@@ -1169,6 +1174,9 @@ static unsigned char *get_udp_data(struct sk_buff *skb, unsigned int protoff,
 	if (dataoff >= skb->len)
 		return NULL;
 	*datalen = skb->len - dataoff;
+	if (*datalen > H323_MAX_SIZE)
+		*datalen = H323_MAX_SIZE;
+
 	return skb_header_pointer(skb, dataoff, *datalen, h323_buffer);
 }
 
@@ -1770,7 +1778,7 @@ static int __init nf_conntrack_h323_init(void)
 
 	NF_CT_HELPER_BUILD_BUG_ON(sizeof(struct nf_ct_h323_master));
 
-	h323_buffer = kmalloc(65536, GFP_KERNEL);
+	h323_buffer = kmalloc(H323_MAX_SIZE + 1, GFP_KERNEL);
 	if (!h323_buffer)
 		return -ENOMEM;
 	ret = h323_helper_init();
-- 
2.35.1

[PATCH nf 3/4] netfilter: conntrack_ftp: prefer skb_linearize

[Florian Westphal]

This uses a pseudo-linearization scheme with a 64k global buffer,
but BIG TCP arrival means IPv6 TCP stack can generate skbs
that exceed this size.

Use skb_linearize.  It should be possible to rewrite this to properly
deal with segmented skbs (i.e., only do small chunk-wise accesses),
but this is going to be a lot more intrusive than this because every
helper function needs to get the sk_buff instead of a pointer to a raw
data buffer.

In practice, provided we're really looking at FTP control channel packets,
there should never be a case where we deal with huge packets.

Fixes: 7c4e983c4f3c ("net: allow gso_max_size to exceed 65536")
Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_ftp.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index a414274338cf..0d9332e9cf71 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -34,11 +34,6 @@ MODULE_DESCRIPTION("ftp connection tracking helper");
 MODULE_ALIAS("ip_conntrack_ftp");
 MODULE_ALIAS_NFCT_HELPER(HELPER_NAME);
 
-/* This is slow, but it's simple. --RR */
-static char *ftp_buffer;
-
-static DEFINE_SPINLOCK(nf_ftp_lock);
-
 #define MAX_PORTS 8
 static u_int16_t ports[MAX_PORTS];
 static unsigned int ports_c;
@@ -398,6 +393,9 @@ static int help(struct sk_buff *skb,
 		return NF_ACCEPT;
 	}
 
+	if (unlikely(skb_linearize(skb)))
+		return NF_DROP;
+
 	th = skb_header_pointer(skb, protoff, sizeof(_tcph), &_tcph);
 	if (th == NULL)
 		return NF_ACCEPT;
@@ -411,12 +409,8 @@ static int help(struct sk_buff *skb,
 	}
 	datalen = skb->len - dataoff;
 
-	spin_lock_bh(&nf_ftp_lock);
-	fb_ptr = skb_header_pointer(skb, dataoff, datalen, ftp_buffer);
-	if (!fb_ptr) {
-		spin_unlock_bh(&nf_ftp_lock);
-		return NF_ACCEPT;
-	}
+	spin_lock_bh(&ct->lock);
+	fb_ptr = skb->data + dataoff;
 
 	ends_in_nl = (fb_ptr[datalen - 1] == '\n');
 	seq = ntohl(th->seq) + datalen;
@@ -544,7 +538,7 @@ static int help(struct sk_buff *skb,
 	if (ends_in_nl)
 		update_nl_seq(ct, seq, ct_ftp_info, dir, skb);
  out:
-	spin_unlock_bh(&nf_ftp_lock);
+	spin_unlock_bh(&ct->lock);
 	return ret;
 }
 
@@ -571,7 +565,6 @@ static const struct nf_conntrack_expect_policy ftp_exp_policy = {
 static void __exit nf_conntrack_ftp_fini(void)
 {
 	nf_conntrack_helpers_unregister(ftp, ports_c * 2);
-	kfree(ftp_buffer);
 }
 
 static int __init nf_conntrack_ftp_init(void)
@@ -580,10 +573,6 @@ static int __init nf_conntrack_ftp_init(void)
 
 	NF_CT_HELPER_BUILD_BUG_ON(sizeof(struct nf_ct_ftp_master));
 
-	ftp_buffer = kmalloc(65536, GFP_KERNEL);
-	if (!ftp_buffer)
-		return -ENOMEM;
-
 	if (ports_c == 0)
 		ports[ports_c++] = FTP_PORT;
 
@@ -603,7 +592,6 @@ static int __init nf_conntrack_ftp_init(void)
 	ret = nf_conntrack_helpers_register(ftp, ports_c * 2);
 	if (ret < 0) {
 		pr_err("failed to register helpers\n");
-		kfree(ftp_buffer);
 		return ret;
 	}
 
-- 
2.35.1

[PATCH nf 4/4] netfilter: conntrack_irc: cap packet search space to 4k

[Florian Westphal]

This uses a pseudo-linearization scheme with a 64k global buffer,
but BIG TCP arrival means IPv6 TCP stack can generate skbs
that exceed this size.

In practice, IRC commands are not expected to exceed 512 bytes, plus
this is interactive protocol, so we should not see large packets
in practice.

Given most IRC connections nowadays use TLS so this helper could also be
removed in the near future.

Fixes: 7c4e983c4f3c ("net: allow gso_max_size to exceed 65536")
Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_irc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 08ee4e760a3d..1796c456ac98 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -39,6 +39,7 @@ unsigned int (*nf_nat_irc_hook)(struct sk_buff *skb,
 EXPORT_SYMBOL_GPL(nf_nat_irc_hook);
 
 #define HELPER_NAME "irc"
+#define MAX_SEARCH_SIZE	4095
 
 MODULE_AUTHOR("Harald Welte <laforge@netfilter.org>");
 MODULE_DESCRIPTION("IRC (DCC) connection tracking helper");
@@ -121,6 +122,7 @@ static int help(struct sk_buff *skb, unsigned int protoff,
 	int i, ret = NF_ACCEPT;
 	char *addr_beg_p, *addr_end_p;
 	typeof(nf_nat_irc_hook) nf_nat_irc;
+	unsigned int datalen;
 
 	/* If packet is coming from IRC server */
 	if (dir == IP_CT_DIR_REPLY)
@@ -140,8 +142,12 @@ static int help(struct sk_buff *skb, unsigned int protoff,
 	if (dataoff >= skb->len)
 		return NF_ACCEPT;
 
+	datalen = skb->len - dataoff;
+	if (datalen > MAX_SEARCH_SIZE)
+		datalen = MAX_SEARCH_SIZE;
+
 	spin_lock_bh(&irc_buffer_lock);
-	ib_ptr = skb_header_pointer(skb, dataoff, skb->len - dataoff,
+	ib_ptr = skb_header_pointer(skb, dataoff, datalen,
 				    irc_buffer);
 	if (!ib_ptr) {
 		spin_unlock_bh(&irc_buffer_lock);
@@ -149,7 +155,7 @@ static int help(struct sk_buff *skb, unsigned int protoff,
 	}
 
 	data = ib_ptr;
-	data_limit = ib_ptr + skb->len - dataoff;
+	data_limit = ib_ptr + datalen;
 
 	/* strlen("\1DCC SENT t AAAAAAAA P\1\n")=24
 	 * 5+MINMATCHLEN+strlen("t AAAAAAAA P\1\n")=14 */
@@ -251,7 +257,7 @@ static int __init nf_conntrack_irc_init(void)
 	irc_exp_policy.max_expected = max_dcc_channels;
 	irc_exp_policy.timeout = dcc_timeout;
 
-	irc_buffer = kmalloc(65536, GFP_KERNEL);
+	irc_buffer = kmalloc(MAX_SEARCH_SIZE + 1, GFP_KERNEL);
 	if (!irc_buffer)
 		return -ENOMEM;
 
-- 
2.35.1

RE: [PATCH nf 3/4] netfilter: conntrack_ftp: prefer skb_linearize

[Alexander Duyck]

> -----Original Message-----
> From: Florian Westphal <fw@strlen.de>
> Sent: Tuesday, August 9, 2022 6:17 AM
> To: netfilter-devel@vger.kernel.org
> Cc: Alexander Duyck <alexanderduyck@fb.com>; edumazet@google.com;
> Florian Westphal <fw@strlen.de>
> Subject: [PATCH nf 3/4] netfilter: conntrack_ftp: prefer skb_linearize
> 
> > 
> This uses a pseudo-linearization scheme with a 64k global buffer, but BIG TCP
> arrival means IPv6 TCP stack can generate skbs that exceed this size.
> 
> Use skb_linearize.  It should be possible to rewrite this to properly deal with
> segmented skbs (i.e., only do small chunk-wise accesses), but this is going to
> be a lot more intrusive than this because every helper function needs to get
> the sk_buff instead of a pointer to a raw data buffer.
> 
> In practice, provided we're really looking at FTP control channel packets,
> there should never be a case where we deal with huge packets.
> 
> Fixes: 7c4e983c4f3c ("net: allow gso_max_size to exceed 65536")
> Fixes: 0fe79f28bfaf ("net: allow gro_max_size to exceed 65536")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/nf_conntrack_ftp.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_ftp.c
> b/net/netfilter/nf_conntrack_ftp.c
> index a414274338cf..0d9332e9cf71 100644
> --- a/net/netfilter/nf_conntrack_ftp.c
> +++ b/net/netfilter/nf_conntrack_ftp.c
> @@ -34,11 +34,6 @@ MODULE_DESCRIPTION("ftp connection tracking
> helper");  MODULE_ALIAS("ip_conntrack_ftp");
> MODULE_ALIAS_NFCT_HELPER(HELPER_NAME);
> 
> -/* This is slow, but it's simple. --RR */ -static char *ftp_buffer;
> -
> -static DEFINE_SPINLOCK(nf_ftp_lock);
> -
>  #define MAX_PORTS 8
>  static u_int16_t ports[MAX_PORTS];
>  static unsigned int ports_c;
> @@ -398,6 +393,9 @@ static int help(struct sk_buff *skb,
>  		return NF_ACCEPT;
>  	}
> 
> +	if (unlikely(skb_linearize(skb)))
> +		return NF_DROP;
> +
>  	th = skb_header_pointer(skb, protoff, sizeof(_tcph), &_tcph);
>  	if (th == NULL)
>  		return NF_ACCEPT;

Doing a full linearize seems like it would be much more expensive than it needs to be for a full frame.

> @@ -411,12 +409,8 @@ static int help(struct sk_buff *skb,
>  	}
>  	datalen = skb->len - dataoff;
> 
> -	spin_lock_bh(&nf_ftp_lock);
> -	fb_ptr = skb_header_pointer(skb, dataoff, datalen, ftp_buffer);
> -	if (!fb_ptr) {
> -		spin_unlock_bh(&nf_ftp_lock);
> -		return NF_ACCEPT;
> -	}
> +	spin_lock_bh(&ct->lock);
> +	fb_ptr = skb->data + dataoff;
> 
>  	ends_in_nl = (fb_ptr[datalen - 1] == '\n');
>  	seq = ntohl(th->seq) + datalen;

Rather than using skb_header_pointer/skb_linearize is there any reason why you couldn't use pskb_may_pull? It seems like that would be much closer to what you are looking for here rather than linearizing the entire buffer. With that you would have access to all the same headers you did with the skb_header_pointer approach and in most cases the copy should just be skipped since the headlen is already in the skb->data buffer.

Re: [PATCH nf 3/4] netfilter: conntrack_ftp: prefer skb_linearize

[Florian Westphal]

Alexander Duyck <alexanderduyck@fb.com> wrote:
> > -	spin_lock_bh(&nf_ftp_lock);
> > -	fb_ptr = skb_header_pointer(skb, dataoff, datalen, ftp_buffer);
> > -	if (!fb_ptr) {
> > -		spin_unlock_bh(&nf_ftp_lock);
> > -		return NF_ACCEPT;
> > -	}
> > +	spin_lock_bh(&ct->lock);
> > +	fb_ptr = skb->data + dataoff;
> > 
> >  	ends_in_nl = (fb_ptr[datalen - 1] == '\n');
> >  	seq = ntohl(th->seq) + datalen;
> 
> Rather than using skb_header_pointer/skb_linearize is there any reason why you couldn't use pskb_may_pull? It seems like that would be much closer to what you are looking for here rather than linearizing the entire buffer. With that you would have access to all the same headers you did with the skb_header_pointer approach and in most cases the copy should just be skipped since the headlen is already in the skb->data buffer.

This helper is written with the assumption that everything is searchable
via 'const char *'.

I'm not going to submit a patch to -net that rewrites this,
and I'm not sure its worth it to spend time on it for -next either.

RE: [PATCH nf 3/4] netfilter: conntrack_ftp: prefer skb_linearize

[Alexander Duyck]

> -----Original Message-----
> From: Florian Westphal <fw@strlen.de>
> Sent: Tuesday, August 9, 2022 9:07 AM
> To: Alexander Duyck <alexanderduyck@fb.com>
> Cc: Florian Westphal <fw@strlen.de>; netfilter-devel@vger.kernel.org;
> edumazet@google.com
> Subject: Re: [PATCH nf 3/4] netfilter: conntrack_ftp: prefer skb_linearize
> Alexander Duyck <alexanderduyck@fb.com> wrote:
> > > -	spin_lock_bh(&nf_ftp_lock);
> > > -	fb_ptr = skb_header_pointer(skb, dataoff, datalen, ftp_buffer);
> > > -	if (!fb_ptr) {
> > > -		spin_unlock_bh(&nf_ftp_lock);
> > > -		return NF_ACCEPT;
> > > -	}
> > > +	spin_lock_bh(&ct->lock);
> > > +	fb_ptr = skb->data + dataoff;
> > >
> > >  	ends_in_nl = (fb_ptr[datalen - 1] == '\n');
> > >  	seq = ntohl(th->seq) + datalen;
> >
> > Rather than using skb_header_pointer/skb_linearize is there any reason
> why you couldn't use pskb_may_pull? It seems like that would be much
> closer to what you are looking for here rather than linearizing the entire
> buffer. With that you would have access to all the same headers you did with
> the skb_header_pointer approach and in most cases the copy should just be
> skipped since the headlen is already in the skb->data buffer.
> 
> This helper is written with the assumption that everything is searchable via
> 'const char *'.
> 
> I'm not going to submit a patch to -net that rewrites this, and I'm not sure its
> worth it to spend time on it for -next either.

My bad, I misread it. I thought it was looking at the headers, instead this is looking at everything after the headers. I am honestly surprised it is using this approach since copying the entire buffer over to a linear buffer would be really expensive. I am assuming this code isn't exercised often? If so I guess skb_linearize works here, but it will be much more prone to failure for larger buffers as the higher order memory allocations will be likely to fail as memory gets more fragmented over time.

Re: [PATCH nf 3/4] netfilter: conntrack_ftp: prefer skb_linearize

[Florian Westphal]

Alexander Duyck <alexanderduyck@fb.com> wrote:
> > why you couldn't use pskb_may_pull? It seems like that would be much
> > closer to what you are looking for here rather than linearizing the entire
> > buffer. With that you would have access to all the same headers you did with
> > the skb_header_pointer approach and in most cases the copy should just be
> > skipped since the headlen is already in the skb->data buffer.
> > 
> > This helper is written with the assumption that everything is searchable via
> > 'const char *'.
> > 
> > I'm not going to submit a patch to -net that rewrites this, and I'm not sure its
> > worth it to spend time on it for -next either.
> 
> My bad, I misread it. I thought it was looking at the headers, instead this is looking at everything after the headers. I am honestly surprised it is using this approach since copying the entire buffer over to a linear buffer would be really expensive. I am assuming this code isn't exercised often? If so I guess skb_linearize works here, but it will be much more prone to failure for larger buffers as the higher order memory allocations will be likely to fail as memory gets more fragmented over time.

It snoops the ftp control channel, so yes, this should be low overhead.

This code is ~20 years old, the original implementation is from a time when skbs were always linear.

Re: [PATCH nf 0/4] netfilter: conntrack: remove 64kb max size assumptions

[Pablo Neira Ayuso]

On Tue, Aug 09, 2022 at 03:16:31PM +0200, Florian Westphal wrote:
> Some of our dated conntrack helpers assume skbs can't contain
> tcp packets larger than 64kb.
> 
> Update those.  For SANE, I don't see a reason for the
> 'full-packet-copy', just extract the sane header.
> 
> For h323, packet gets capped at 64k; larger one will be
> seen as truncated.
> 
> For irc, cap at 4k: a line length should not exceed 512 bytes.
> For ftp, use skb_linearize(), its the most simple way to resolve this.

Applied, thanks.