[RFC PATCH -next] netfilter: nfqueue: add ability to dump list of supported attributes

[RFC PATCH -next] netfilter: nfqueue: add ability to dump list of supported attributes

[Florian Westphal]

Allows userspace to dump the list of known (extended) attributes.
For SKB_INFO, we set all the info flag bits supported by the running kernel.

The latter is required because absence of some bits in the info attribute will
either mean "skb did not have the property" OR "this kernel doesn't know about
this flag".

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Recent discussion of the
 "netfilter: nf_queue: add NFQA_SKB_CSUM_NOTVERIFIED info flag" patch
 (http://patchwork.ozlabs.org/patch/246460/) shows that we'll need
 to tell userspace the supported info bits in the NFAQ_SKB_INFO attribute.

 While we could add another NFAQ_SKB_INFO_FLAGS attribute, that seems
 insane since the value will never change (unless kernel update).

 Adding feature flags also seems to be wrong (API abuse, plus we'd
 waste a new feature flag for every new SKB_INFO bit).

 On startup, userspace would bind the queue, and then send
 NFQNL_MSG_ATTRIBUTES request to kernel.

 In the reply, the known extended attrs will be set, i.e:

if (attr[NFQA_SKB_INFO]) {
  uint32_t skbinfo = ntohl(mnl_attr_get_u32(attr[NFQA_SKB_INFO]));
  if (skbinfo & NFQA_SKB_CSUMNOTVERIFIED) /* kernel support checksum hints */

After this, userspace can process queued packets and interpret absence of flags
correctly.

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index a2308ae..efb02ae 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -9,6 +9,7 @@ enum nfqnl_msg_types {
 	NFQNL_MSG_VERDICT,		/* verdict from userspace to kernel */
 	NFQNL_MSG_CONFIG,		/* connect to a particular queue */
 	NFQNL_MSG_VERDICT_BATCH,	/* batchv from userspace to kernel */
+	NFQNL_MSG_ATTRIBUTES,		/* list of supported attributes */
 
 	NFQNL_MSG_MAX
 };
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 3880899..f16ef3a 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -997,6 +997,56 @@ nfqnl_recv_verdict(struct sock *ctnl, struct sk_buff *skb,
 }
 
 static int
+__nfqnl_dump_attrs(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct nlmsghdr *nlh;
+	int i;
+	uint32_t value;
+
+	if (cb->args[0] >= __NFQA_MAX)
+		return 0;
+
+	if (cb->args[0] == 0)
+		cb->args[0] = NFQA_CT;
+
+	nlh = nlmsg_put(skb, 0, 0,
+		NFNL_SUBSYS_QUEUE << 8 | NFQNL_MSG_ATTRIBUTES,
+		sizeof(struct nfgenmsg), 0);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	for (i = cb->args[0]; i < __NFQA_MAX; i++) {
+		switch (i) {
+		case NFQA_SKB_INFO:
+			value = NFQA_SKB_GSO | NFQA_SKB_CSUMNOTREADY;
+			break;
+		default:
+			value = 0;
+			break;
+		}
+		if (nla_put_u32(skb, i, htonl(value)))
+			break;
+	}
+
+	if (cb->args[0] <= i) {
+		nlmsg_end(skb, nlh);
+		return -EMSGSIZE;
+	}
+	cb->args[0] = i;
+	return nlmsg_end(skb, nlh);
+}
+
+static int nfqnl_dump_attrs(struct sock *ctnl, struct sk_buff *skb,
+			  const struct nlmsghdr *nlh,
+			  const struct nlattr * const attr[])
+{
+	struct netlink_dump_control c = {
+		.dump = __nfqnl_dump_attrs,
+	};
+	return netlink_dump_start(ctnl, skb, nlh, &c);
+}
+
+static int
 nfqnl_recv_unsupp(struct sock *ctnl, struct sk_buff *skb,
 		  const struct nlmsghdr *nlh,
 		  const struct nlattr * const nfqa[])
@@ -1145,6 +1195,7 @@ static const struct nfnl_callback nfqnl_cb[NFQNL_MSG_MAX] = {
 	[NFQNL_MSG_VERDICT_BATCH]={ .call_rcu = nfqnl_recv_verdict_batch,
 				    .attr_count = NFQA_MAX,
 				    .policy = nfqa_verdict_batch_policy },
+	[NFQNL_MSG_ATTRIBUTES]   = { .call = nfqnl_dump_attrs, },
 };
 
 static const struct nfnetlink_subsystem nfqnl_subsys = {
-- 
1.7.8.6

Re: [RFC PATCH -next] netfilter: nfqueue: add ability to dump list of supported attributes

[Pablo Neira Ayuso]

Hi Florian,

On Tue, Jun 04, 2013 at 05:37:53PM +0200, Florian Westphal wrote:
> Allows userspace to dump the list of known (extended) attributes.
> For SKB_INFO, we set all the info flag bits supported by the running kernel.
> 
> The latter is required because absence of some bits in the info attribute will
> either mean "skb did not have the property" OR "this kernel doesn't know about
> this flag".
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Recent discussion of the
>  "netfilter: nf_queue: add NFQA_SKB_CSUM_NOTVERIFIED info flag" patch
>  (http://patchwork.ozlabs.org/patch/246460/) shows that we'll need
>  to tell userspace the supported info bits in the NFAQ_SKB_INFO attribute.

After going back to this issue, I think I prefer to enqueue:

netfilter: nf_queue: add NFQA_SKB_CSUM_NOTVERIFIED info flag

to net-next, and ask for -stable submission once this hits net, as it
will be fixing an inconsistent behaviour. Another reason for that is
that such patch is fairly small (~16 LOC).

Both approaches we discussed that are:

a) adding a new config flags

and

b) adding this infrastructure

look a bit too much to me just to resolve this.

Once this patch hits -stable, we can consistently say that:

NFQA_SKB_CSUMNOTREADY
NFQA_SKB_GSO
NFQA_SKB_CSUM_NOTVERIFIED

are obtained via NFQA_CFG_F_GSO.

For user-space application running with different, they can still send
every configuration incrementally, so you spot a message telling what
is available and what is not. I planned to use this to add GSO support
for the user-space helper infrastructure.

Let me know.

Re: [RFC PATCH -next] netfilter: nfqueue: add ability to dump list of supported attributes

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jun 04, 2013 at 05:37:53PM +0200, Florian Westphal wrote:
> > Allows userspace to dump the list of known (extended) attributes.
> > For SKB_INFO, we set all the info flag bits supported by the running kernel.
> > 
> > The latter is required because absence of some bits in the info attribute will
> > either mean "skb did not have the property" OR "this kernel doesn't know about
> > this flag".
> >
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  Recent discussion of the
> >  "netfilter: nf_queue: add NFQA_SKB_CSUM_NOTVERIFIED info flag" patch
> >  (http://patchwork.ozlabs.org/patch/246460/) shows that we'll need
> >  to tell userspace the supported info bits in the NFAQ_SKB_INFO attribute.
> 
> After going back to this issue, I think I prefer to enqueue:
> 
> netfilter: nf_queue: add NFQA_SKB_CSUM_NOTVERIFIED info flag
> 
> to net-next, and ask for -stable submission once this hits net, as it
> will be fixing an inconsistent behaviour. Another reason for that is
> that such patch is fairly small (~16 LOC).
> 
> Both approaches we discussed that are:
> 
> a) adding a new config flags
> 
> and
> 
> b) adding this infrastructure
> 
> look a bit too much to me just to resolve this.
> 
> Once this patch hits -stable, we can consistently say that:
> 
> NFQA_SKB_CSUMNOTREADY
> NFQA_SKB_GSO
> NFQA_SKB_CSUM_NOTVERIFIED
> 
> are obtained via NFQA_CFG_F_GSO.
> 
> For user-space application running with different, they can still send
> every configuration incrementally, so you spot a message telling what
> is available and what is not. I planned to use this to add GSO support
> for the user-space helper infrastructure.
> 
> Let me know.

I have no objections.

However, we cannot escape this forever -- it will be insanity to just
ask for -stable submission whenever a new flag is added and expect noone
to yell at us :-)

But for the time being, sure, b) _is_ overkill.