nfqueue: detect when packet has already been checksummed?

nfqueue: detect when packet has already been checksummed?

[Florian Westphal]

Hi.

When using nfqueue, userspace currently has no way
to tell wheter queued packets have a bad checksum, i.e.
applications that need data integrity must do full checksum
validation in userspace (except maybe when only queueing in OUTPUT).

However, there are several places where incoming packets are already
checksummed in kernel, before packet hits nfqueue, e.g. via nic rx
csum offload, or in conntrack.

So I think it would be nice to provide a hint that kernel already did
checksumming.

The SKB_INFO attribute added in -net for GRO support seems like a
candidate.  However, since 'already checksummed' is the common case this
would mean adding that attribute most of the time.

Unless we would do the opposite hint, i.e. tell userspace when
checksumming has NOT been performed yet.

Such change would however need to go into -net, else userspace can't tell
'checksum ok' from 'kernel too old to provide flag in SKB_INFO attribute'.

Following patch illustrates what I had in mind, adding hint for incoming
packets and packets that are most likely not locally generated (forwarded
packet in POSTROUTING).

Comments appreciated.

[PATCH] netfilter: nf_queue: add NFQA_SKB_CSUM_NOTVERIFIED info flag

[Florian Westphal]

The common case is that TCP/IP checksums have already been
verified, e.g. by hardware (rx checksum offload), or conntrack.

Userspace can use this flag to determine when the checksum
has not been validated yet.

If the flag is set, this doesn't necessarily mean that the packet has
an invalid checksum, e.g. if NIC doesn't support rx checksum.

Userspace that sucessfully enabled NFQA_CFG_F_GSO queue feature flag can
infer that IP/TCP checksum has already been validated if either the
SKB_INFO attribute is not present or the NFQA_SKB_CSUM_NOTVERIFIED
flag is unset.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  2 ++
 net/netfilter/nfnetlink_queue_core.c           | 16 ++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index a2308ae..3a9b921 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -105,5 +105,7 @@ enum nfqnl_attr_config {
 #define NFQA_SKB_CSUMNOTREADY (1 << 0)
 /* packet is GSO (i.e., exceeds device mtu) */
 #define NFQA_SKB_GSO (1 << 1)
+/* csum not validated (incoming device doesn't support hw checksum, etc.) */
+#define NFQA_SKB_CSUM_NOTVERIFIED (1 << 2)
 
 #endif /* _NFNETLINK_QUEUE_H */
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 2e0e835..d9bddbf 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -272,12 +272,17 @@ nfqnl_zcopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
 	skb_shinfo(to)->nr_frags = j;
 }
 
-static int nfqnl_put_packet_info(struct sk_buff *nlskb, struct sk_buff *packet)
+static int
+nfqnl_put_packet_info(struct sk_buff *nlskb, struct sk_buff *packet,
+		      bool csum_verify)
 {
 	__u32 flags = 0;
 
 	if (packet->ip_summed == CHECKSUM_PARTIAL)
 		flags = NFQA_SKB_CSUMNOTREADY;
+	else if (csum_verify)
+		flags = NFQA_SKB_CSUM_NOTVERIFIED;
+
 	if (skb_is_gso(packet))
 		flags |= NFQA_SKB_GSO;
 
@@ -302,6 +307,7 @@ nfqnl_build_packet_message(struct nfqnl_instance *queue,
 	struct net_device *outdev;
 	struct nf_conn *ct = NULL;
 	enum ip_conntrack_info uninitialized_var(ctinfo);
+	bool csum_verify;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -319,6 +325,12 @@ nfqnl_build_packet_message(struct nfqnl_instance *queue,
 	if (entskb->tstamp.tv64)
 		size += nla_total_size(sizeof(struct nfqnl_msg_packet_timestamp));
 
+	if (entry->hook <= NF_INET_FORWARD ||
+	   (entry->hook == NF_INET_POST_ROUTING && entskb->sk == NULL))
+		csum_verify = !skb_csum_unnecessary(entskb);
+	else
+		csum_verify = false;
+
 	outdev = entry->outdev;
 
 	switch ((enum nfqnl_config_mode)ACCESS_ONCE(queue->copy_mode)) {
@@ -468,7 +480,7 @@ nfqnl_build_packet_message(struct nfqnl_instance *queue,
 	if (cap_len > 0 && nla_put_be32(skb, NFQA_CAP_LEN, htonl(cap_len)))
 		goto nla_put_failure;
 
-	if (nfqnl_put_packet_info(skb, entskb))
+	if (nfqnl_put_packet_info(skb, entskb, csum_verify))
 		goto nla_put_failure;
 
 	if (data_len) {
-- 
1.8.1.5

Re: [PATCH] netfilter: nf_queue: add NFQA_SKB_CSUM_NOTVERIFIED info flag

[Pablo Neira Ayuso]

Hi Florian,

On Sun, May 26, 2013 at 10:48:09PM +0200, Florian Westphal wrote:
> The common case is that TCP/IP checksums have already been
> verified, e.g. by hardware (rx checksum offload), or conntrack.
> 
> Userspace can use this flag to determine when the checksum
> has not been validated yet.
> 
> If the flag is set, this doesn't necessarily mean that the packet has
> an invalid checksum, e.g. if NIC doesn't support rx checksum.
> 
> Userspace that sucessfully enabled NFQA_CFG_F_GSO queue feature flag can
> infer that IP/TCP checksum has already been validated if either the
> SKB_INFO attribute is not present or the NFQA_SKB_CSUM_NOTVERIFIED
> flag is unset.

I have applied this patch to nf-next, thanks.

Re: nfqueue: detect when packet has already been checksummed?

[Pablo Neira Ayuso]

Hi Florian,

On Sun, May 26, 2013 at 10:48:26PM +0200, Florian Westphal wrote:
> Hi.
> 
> When using nfqueue, userspace currently has no way
> to tell wheter queued packets have a bad checksum, i.e.
> applications that need data integrity must do full checksum
> validation in userspace (except maybe when only queueing in OUTPUT).
> 
> However, there are several places where incoming packets are already
> checksummed in kernel, before packet hits nfqueue, e.g. via nic rx
> csum offload, or in conntrack.
> 
> So I think it would be nice to provide a hint that kernel already did
> checksumming.
> 
> The SKB_INFO attribute added in -net for GRO support seems like a
> candidate.  However, since 'already checksummed' is the common case this
> would mean adding that attribute most of the time.
> 
> Unless we would do the opposite hint, i.e. tell userspace when
> checksumming has NOT been performed yet.
> 
> Such change would however need to go into -net, else userspace can't tell
> 'checksum ok' from 'kernel too old to provide flag in SKB_INFO attribute'.

User-space has no way to know what info flags are available. If we add
more info flags in the future, we'll have the same problem that we have
now. I mean, currently an unset info flag from userspace may mean:

* It's actually unset.
* It is not available in this kernel.

So I think we need to pass a mask of available info flags, so
user-space knows how to interpret what 'unset' means. The mask would
also help to tell user-space that some info flag is not available.

Regards.

Re: nfqueue: detect when packet has already been checksummed?

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, May 26, 2013 at 10:48:26PM +0200, Florian Westphal wrote:
> > Hi.
> > 
> > When using nfqueue, userspace currently has no way
> > to tell wheter queued packets have a bad checksum, i.e.
> > applications that need data integrity must do full checksum
> > validation in userspace (except maybe when only queueing in OUTPUT).
> > 
> > However, there are several places where incoming packets are already
> > checksummed in kernel, before packet hits nfqueue, e.g. via nic rx
> > csum offload, or in conntrack.
> > 
> > So I think it would be nice to provide a hint that kernel already did
> > checksumming.
> > 
> > The SKB_INFO attribute added in -net for GRO support seems like a
> > candidate.  However, since 'already checksummed' is the common case this
> > would mean adding that attribute most of the time.
> > 
> > Unless we would do the opposite hint, i.e. tell userspace when
> > checksumming has NOT been performed yet.
> > 
> > Such change would however need to go into -net, else userspace can't tell
> > 'checksum ok' from 'kernel too old to provide flag in SKB_INFO attribute'.
> 
> User-space has no way to know what info flags are available. If we add
> more info flags in the future, we'll have the same problem that we have
> now. I mean, currently an unset info flag from userspace may mean:
>
> * It's actually unset.
> * It is not available in this kernel.

Yes, I understand this.

> So I think we need to pass a mask of available info flags, so
> user-space knows how to interpret what 'unset' means. The mask would
> also help to tell user-space that some info flag is not available.

I plan to add a query feature that userspace can use to determine
supported attributes/flags.

Userspace can then bind the queue and then figure out what
attributes are support.

A more simple solution is to simply add a 32bit revision number
that is dumped to userspace.

But regardless of the solution, its definitely -next material.

I simply tried do get one-more-thing into -net.  If you say no,
thats ok.

It merely means I need to add the detection-thing before resubmitting
this patch.

Re: nfqueue: detect when packet has already been checksummed?

[Pablo Neira Ayuso]

On Wed, May 29, 2013 at 01:25:42PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, May 26, 2013 at 10:48:26PM +0200, Florian Westphal wrote:
> > > Hi.
> > > 
> > > When using nfqueue, userspace currently has no way
> > > to tell wheter queued packets have a bad checksum, i.e.
> > > applications that need data integrity must do full checksum
> > > validation in userspace (except maybe when only queueing in OUTPUT).
> > > 
> > > However, there are several places where incoming packets are already
> > > checksummed in kernel, before packet hits nfqueue, e.g. via nic rx
> > > csum offload, or in conntrack.
> > > 
> > > So I think it would be nice to provide a hint that kernel already did
> > > checksumming.
> > > 
> > > The SKB_INFO attribute added in -net for GRO support seems like a
> > > candidate.  However, since 'already checksummed' is the common case this
> > > would mean adding that attribute most of the time.
> > > 
> > > Unless we would do the opposite hint, i.e. tell userspace when
> > > checksumming has NOT been performed yet.
> > > 
> > > Such change would however need to go into -net, else userspace can't tell
> > > 'checksum ok' from 'kernel too old to provide flag in SKB_INFO attribute'.
> > 
> > User-space has no way to know what info flags are available. If we add
> > more info flags in the future, we'll have the same problem that we have
> > now. I mean, currently an unset info flag from userspace may mean:
> >
> > * It's actually unset.
> > * It is not available in this kernel.
> 
> Yes, I understand this.
> 
> > So I think we need to pass a mask of available info flags, so
> > user-space knows how to interpret what 'unset' means. The mask would
> > also help to tell user-space that some info flag is not available.
> 
> I plan to add a query feature that userspace can use to determine
> supported attributes/flags.
>
> Userspace can then bind the queue and then figure out what
> attributes are support.
> 
> A more simple solution is to simply add a 32bit revision number
> that is dumped to userspace.
>
> But regardless of the solution, its definitely -next material.
>
> I simply tried do get one-more-thing into -net.  If you say no,
> thats ok.

I agree that the current situation is inconsistent. We have no way to
know if the kernel validated the checksum or not from user-space, and
I think this needs a fix.

We can add a new NFQA_CFG_F_CSUM flag so user-space explicitly ask for
assistance regarding checksumming from the kernel. If user-space tries
to set that flag and the kernel does not support it, it will hit
-EOPNOTSUPP. Thus, we can skip the feature retrieval thing.

We would still need your patch as well.

Re: nfqueue: detect when packet has already been checksummed?

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I agree that the current situation is inconsistent. We have no way to
> know if the kernel validated the checksum or not from user-space, and
> I think this needs a fix.

Good :-)

> We can add a new NFQA_CFG_F_CSUM flag so user-space explicitly ask for
> assistance regarding checksumming from the kernel. If user-space tries
> to set that flag and the kernel does not support it, it will hit
> -EOPNOTSUPP. Thus, we can skip the feature retrieval thing.

Yes, but this looks like abuse of the flag semantics to me.
Unless you mean that setting this feat flag should prompt the kernel
to explicitly call a valiation function in case skb_csum_unnecessary()
returns false?

I think that this is overkill, and, it might not work
in all cases (e.g. if the layer4 protocol is unknown to us).

Re: nfqueue: detect when packet has already been checksummed?

[Pablo Neira Ayuso]

On Wed, May 29, 2013 at 02:03:29PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I agree that the current situation is inconsistent. We have no way to
> > know if the kernel validated the checksum or not from user-space, and
> > I think this needs a fix.
> 
> Good :-)
> 
> > We can add a new NFQA_CFG_F_CSUM flag so user-space explicitly ask for
> > assistance regarding checksumming from the kernel. If user-space tries
> > to set that flag and the kernel does not support it, it will hit
> > -EOPNOTSUPP. Thus, we can skip the feature retrieval thing.
> 
> Yes, but this looks like abuse of the flag semantics to me.
> Unless you mean that setting this feat flag should prompt the kernel
> to explicitly call a valiation function in case skb_csum_unnecessary()
> returns false?

I mean exactly the current behaviour but forcing user-space to ask for
it. So skb info flag regarding checksumming comes from kernel if
explicitly requested from user-space.

Re: nfqueue: detect when packet has already been checksummed?

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Yes, but this looks like abuse of the flag semantics to me.
> > Unless you mean that setting this feat flag should prompt the kernel
> > to explicitly call a valiation function in case skb_csum_unnecessary()
> > returns false?
> 
> I mean exactly the current behaviour but forcing user-space to ask for
> it. So skb info flag regarding checksumming comes from kernel if
> explicitly requested from user-space.

Fair enough, I can resubmit this for -next with this behaviour,
I had hoped to avoid a new flag, since there is no reason for it
OTHER than to figure out if the kernel knows about that checksumming
feature.