| Message ID | 1370360273-20633-1-git-send-email-fw@strlen.de |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = {
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.