Re: AUDIT_NETFILTER_PKT message format

[Paul Moore]

On Tue, Feb 7, 2017 at 3:52 PM, Richard Guy Briggs  wrote:
> So while I'm not advocating this is what should be done and I'm trying
> to establish bounds to the scope of this feature, but would it be
> reasonable to simply not log packets that were transiting this machine
> without a local endpoint?

I'm still waiting on more detailed requirements information from
Steve, but based on what we've heard so far, it seems that ignoring
forwarded traffic is a reasonable thing to do.

-- 
paul moore
security @ redhat
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: fix description of skb_find_text() according to removed functionality

[David Miller]

How about you make edits to this interface when you add an in-tree
user as we mentioned in our responses to your previous patch?

Thank you.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] net: fix description of skb_find_text() according to removed functionality

[Igor Pylypiv]

Textsearch state parameter was moved to local scope of the function.
This eliminates usage of textsearch_next() to find subsequent occurrences.

Fixes: 59a2440fd3cf ("net: Remove state argument from skb_find_text()")
Signed-off-by: Igor Pylypiv 
---
 net/core/skbuff.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9ccba86..90366c5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2919,9 +2919,8 @@ static void skb_ts_finish(struct ts_config *conf, struct 
ts_state *state)
  * @config: textsearch configuration
  *
  * Finds a pattern in the skb data according to the specified
- * textsearch configuration. Use textsearch_next() to retrieve
- * subsequent occurrences of the pattern. Returns the offset
- * to the first occurrence or UINT_MAX if no match was found.
+ * textsearch configuration. Returns the offset to the first
+ * occurrence or UINT_MAX if no match was found.
  */
 unsigned int skb_find_text(struct sk_buff *skb, unsigned int from,
   unsigned int to, struct ts_config *config)
--
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf-next v2,1/2] netfilter: nft_exthdr: Add support for existence check

[Phil Sutter]

Hi,

On Tue, Feb 07, 2017 at 09:20:27PM +0100, Pablo Neira Ayuso wrote:
> From: Phil Sutter 
> 
> If NFT_EXTHDR_F_PRESENT is set, exthdr will not copy any header field
> data into *dest, but instead set it to 1 if the header is found and 0
> otherwise.
> 
> Signed-off-by: Phil Sutter 
> Signed-off-by: Pablo Neira Ayuso 
> ---
> @Phil: I have enhanced your patch shrink flags to u8, also check for
>unexisting flags. As well as the sparse warning. Please review
>if this looks good to you. Thanks.

Yes, looks good to me!

Thanks, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf-next v2,2/2] netfilter: nft_exthdr: add TCP option matching

[Phil Sutter]

On Tue, Feb 07, 2017 at 09:20:28PM +0100, Pablo Neira Ayuso wrote:
> From: Manuel Messner 
> 
> This patch implements the kernel side of the TCP option patch.
> 
> Signed-off-by: Manuel Messner 
> Reviewed-by: Florian Westphal 
> Signed-off-by: Pablo Neira Ayuso 

Acked-by: Phil Sutter 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] audit: normalize NETFILTER_PKT

[Richard Guy Briggs]

On 2017-02-06 14:41, Paul Moore wrote:
> On Sat, Feb 4, 2017 at 8:25 AM, Steve Grubb  wrote:
> > On Friday, February 3, 2017 6:44:16 PM EST Paul Moore wrote:
> >> I'm still trying to understand what purpose this record actually
> >> serves, and what requirements may exist.  In an earlier thread
> >> somewhere Steve mentioned some broad requirements around data
> >> import/export, and I really wonder if the NETFILTER_PKT record
> >> provides anything useful here when it really isn't connecting the
> >> traffic to the sender/receiver without a lot of additional logging and
> >> post-processing smarts.  If you were interested in data import/export
> >> I think auditing the socket syscalls would provide a much more useful
> >> set of records in the audit log.
> >
> > The problem here is we cannot be selective enough through the syscall
> > interface to get exactly what we want. For example, any auditing of connect
> > and accept will also get af_unix traffic which is likely to be uid/gid 
> > lookups
> > through sssd or glibc. Typically we want the IPv4/6 traffic. The netfilter 
> > rules
> > are better suited to describing which packets are of interest.
> 
> Okay, but how useful are these NETFILTER_PKT records, really?  The
> only linkage you have back to the process on the local machine is via
> the addr/proto/port tuple and that seems far from ideal.

And even that could be spoofed easily and gathering more corroborating
information would seem useful.

Would the presence of the SOCKADDR record in any SYSCALL record be
useful for somehow tagging a class of fd as being of interest?

> >> Considering that one of the primary motivations for the audit
> >> subsystem is to enable compliance with various security
> >> specifications, let's get the ones we know about listed in this thread
> >> and then figure out how best to meet those requirements.
> >
> > Common Criteria calls out for the ability to detect any attempt at 
> > information
> > flow. Everything else leverages the CC requirements.
> 
> Yes, you've mentioned this previously.  This is good, but we need to
> make these requirements a bit more concrete; we need something we can
> use to arrive at a working implementation that satisfies these
> requirements.
> 
> If this is purely about information flowing from A to B, would the
> source and destination addr/proto/port for TCP and UDP suffice?  Do we
> need anything else?
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: AUDIT_NETFILTER_PKT message format

[Richard Guy Briggs]

On 2017-01-20 09:49, Steve Grubb wrote:
> On Wednesday, January 18, 2017 6:35:29 PM EST Paul Moore wrote:
> > On Wed, Jan 18, 2017 at 10:15 AM, Richard Guy Briggs  
> > wrote:
> > > On 2017-01-18 07:32, Paul Moore wrote:
> > >> On Wed, Jan 18, 2017 at 12:39 AM, Richard Guy Briggs  
> wrote:
> > >> > On 2017-01-17 21:34, Richard Guy Briggs wrote:
> > >> >> On 2017-01-17 15:17, Paul Moore wrote:
> > >> >> > On Tue, Jan 17, 2017 at 11:12 AM, Richard Guy Briggs 
>  wrote:
> > >> >> > > On 2017-01-17 08:55, Steve Grubb wrote:
> > >> >> > >> On Tuesday, January 17, 2017 12:25:51 AM EST Richard Guy Briggs 
> wrote:
> > >> >> > ...
> > >> >> > 
> > >> >> > >> > Ones that are not so straightforward:
> > >> >> > >> > - "secmark" depends on a kernel config setting, so should it
> > >> >> > >> > always be
> > >> >> > >> > 
> > >> >> > >> >   present but "(none)" if that kernel feature is compiled out?
> > >> >> > >> 
> > >> >> > >> If this is selinux related, I'd treat it the same way that we do
> > >> >> > >> subj
> > >> >> > >> everywhere else.
> > >> >> > > 
> > >> >> > > Ok.
> > >> >> > 
> > >> >> > To be clear, a packet's secmark should be recorded via a dedicated
> > >> >> > field, e.g. "secmark", and not use the "subj" field (it isn't a
> > >> >> > subject label in the traditional sense).
> > >> >> 
> > >> >> I think Steve was talking about if, when or where to include that
> > >> >> field,
> > >> >> not what its label is.
> > >> > 
> > >> > In this case it is an "obj=" field, but since it is part of the LSM,
> > >> > each one has its own fields.
> > >> 
> > >> As I said above, use a "secmark" field and not the subject or object
> > >> fields; packet labeling is rather complex and there is value in
> > >> differentiating between secmark labels and network peer labels.
> > > 
> > > Ok, I'll change it from the existing "obj=" to "secmark=".  Since it is
> > > an LSM-dependent field, it will go away when that LSM module does.  It
> > > is the very last item in the list of fields, so I don't see this as a
> > > problem.
> > > 
> > > 
> > > I have more questions and observations:
> > > 
> > > Do we care if the rest of the record's fields are there if the packet is
> > > truncated?  In other words, can I omit all the following fields (that
> > > will end up being set to (none) or -1 since there is no data for them)?
> > > I'd prefer to complete the record, but Steve may not care and might
> > > prefer to save the bandwidth.
> > > 
> > > Can I truncate the field name "truncated" to "trunc" (since I don't see
> > > it yet in the audit field dictionary) if we do include all the fields?
> > > 
> > > I observe that support for IPPROTO_DCCP and IPPROTO_SCTP can be added
> > > virtually for free since the source and desination ports in their packet
> > > formats is identical to TCP and UDP (and UDPLITE).
> > > 
> > > 
> > > At this point, it looks like having one record for IP/IPv6 with
> > > TCP/UDP/DCCP/SCTP makes sense.  Whether or not to add ethernet bridge
> > > headers and ICMP* is a more difficult question.  Ethernet bridge adds 40
> > > chars if it isn't used, up to 62 if it is.  ICMP* adds 26 max.
> > > 
> > > It is an independent record, but it would be nice to be able to reuse
> > > the message ID with a new record type to list sub-parts of the packet,
> > > for example, reuse the existing record type (AUDIT_NETFILTER_PKT) for
> > > the first 5 fields, mark and secmark, then another record type
> > > (AUDIT_NETFILTER_PKT_ETH) for ethernet header, a record
> > > (AUDIT_NETFILTER_PKT_IP) for IP/IPv6 header, then another
> > > (AUDIT_NETFILTER_PKT_PROTO) for transport layer protocol.  This way, the
> > > absence of an ethernet bridge header won't swing out three fields, or
> > > waste 40 chars.  IPv4 adds about 20 chars not used by IPv6. 
> > > TCP/UDP/DCCP/SCTP vs ICMP* is about 25 chars each.  The max message is
> > > 322 chars (eth bridge, IPv6).  A non-ethernet-bridge non-IP* message
> > > would be as little as 76 without the extra fields, but as much as 219
> > > with the extra fields filled with unset values.
> > > 
> > > A full message could look like (I've left off secmark, which would go at
> > > the end): action=9 hook=99 len=99 inif= outif=
> > > mark=0x smac=FF:FF:FF:FF:FF:FF dmac=FF:FF:FF:FF:FF:FF
> > > macproto=0x trunc=9 saddr=:::::::
> > > daddr=::::::: ipid=-1 proto=255 frag=-1
> > > trunc=9 sport=9 dport=9 icmptype=999 icmpcode=999
> >
> > At this point I think it would be good to hear what requirements exist
> > for per-packet auditing.  Steve, are there any current Common Criteria
> > (or other) requirements that impact per-packet auditing?
> 
> I don't think you want to flood your logs. That is not helpful. It asks for 
> the 
> ability to detect information flow. Typically you want to know source and 
> destination, protocol, where on 

[PATCH nf-next v2,1/2] netfilter: nft_exthdr: Add support for existence check

[Pablo Neira Ayuso]

From: Phil Sutter 

If NFT_EXTHDR_F_PRESENT is set, exthdr will not copy any header field
data into *dest, but instead set it to 1 if the header is found and 0
otherwise.

Signed-off-by: Phil Sutter 
Signed-off-by: Pablo Neira Ayuso 
---
@Phil: I have enhanced your patch shrink flags to u8, also check for
   unexisting flags. As well as the sparse warning. Please review
   if this looks good to you. Thanks.

 include/uapi/linux/netfilter/nf_tables.h |  6 ++
 net/netfilter/nft_exthdr.c   | 20 ++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h 
b/include/uapi/linux/netfilter/nf_tables.h
index 7b730cab99bd..53aac8b8ed6b 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -704,6 +704,10 @@ enum nft_payload_attributes {
 };
 #define NFTA_PAYLOAD_MAX   (__NFTA_PAYLOAD_MAX - 1)
 
+enum nft_exthdr_flags {
+   NFT_EXTHDR_F_PRESENT = (1 << 0),
+};
+
 /**
  * enum nft_exthdr_attributes - nf_tables IPv6 extension header expression 
netlink attributes
  *
@@ -711,6 +715,7 @@ enum nft_payload_attributes {
  * @NFTA_EXTHDR_TYPE: extension header type (NLA_U8)
  * @NFTA_EXTHDR_OFFSET: extension header offset (NLA_U32)
  * @NFTA_EXTHDR_LEN: extension header length (NLA_U32)
+ * @NFTA_EXTHDR_FLAGS: extension header flags (NLA_U32)
  */
 enum nft_exthdr_attributes {
NFTA_EXTHDR_UNSPEC,
@@ -718,6 +723,7 @@ enum nft_exthdr_attributes {
NFTA_EXTHDR_TYPE,
NFTA_EXTHDR_OFFSET,
NFTA_EXTHDR_LEN,
+   NFTA_EXTHDR_FLAGS,
__NFTA_EXTHDR_MAX
 };
 #define NFTA_EXTHDR_MAX(__NFTA_EXTHDR_MAX - 1)
diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index 47beb3abcc9d..d43f750ab61c 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -23,6 +23,7 @@ struct nft_exthdr {
u8  offset;
u8  len;
enum nft_registers  dreg:8;
+   u8  flags;
 };
 
 static void nft_exthdr_eval(const struct nft_expr *expr,
@@ -35,8 +36,12 @@ static void nft_exthdr_eval(const struct nft_expr *expr,
int err;
 
err = ipv6_find_hdr(pkt->skb, , priv->type, NULL, NULL);
-   if (err < 0)
+   if (priv->flags & NFT_EXTHDR_F_PRESENT) {
+   *dest = (err >= 0);
+   return;
+   } else if (err < 0) {
goto err;
+   }
offset += priv->offset;
 
dest[priv->len / NFT_REG32_SIZE] = 0;
@@ -52,6 +57,7 @@ static const struct nla_policy 
nft_exthdr_policy[NFTA_EXTHDR_MAX + 1] = {
[NFTA_EXTHDR_TYPE]  = { .type = NLA_U8 },
[NFTA_EXTHDR_OFFSET]= { .type = NLA_U32 },
[NFTA_EXTHDR_LEN]   = { .type = NLA_U32 },
+   [NFTA_EXTHDR_FLAGS] = { .type = NLA_U32 },
 };
 
 static int nft_exthdr_init(const struct nft_ctx *ctx,
@@ -59,7 +65,7 @@ static int nft_exthdr_init(const struct nft_ctx *ctx,
   const struct nlattr * const tb[])
 {
struct nft_exthdr *priv = nft_expr_priv(expr);
-   u32 offset, len;
+   u32 offset, len, flags;
int err;
 
if (tb[NFTA_EXTHDR_DREG] == NULL ||
@@ -76,10 +82,18 @@ static int nft_exthdr_init(const struct nft_ctx *ctx,
if (err < 0)
return err;
 
+   err = nft_parse_u32_check(tb[NFTA_EXTHDR_FLAGS], U8_MAX, );
+   if (err < 0)
+   return err;
+
+   if (flags & ~NFT_EXTHDR_F_PRESENT)
+   return -EINVAL;
+
priv->type   = nla_get_u8(tb[NFTA_EXTHDR_TYPE]);
priv->offset = offset;
priv->len= len;
priv->dreg   = nft_parse_register(tb[NFTA_EXTHDR_DREG]);
+   priv->flags  = flags;
 
return nft_validate_register_store(ctx, priv->dreg, NULL,
   NFT_DATA_VALUE, priv->len);
@@ -97,6 +111,8 @@ static int nft_exthdr_dump(struct sk_buff *skb, const struct 
nft_expr *expr)
goto nla_put_failure;
if (nla_put_be32(skb, NFTA_EXTHDR_LEN, htonl(priv->len)))
goto nla_put_failure;
+   if (nla_put_be32(skb, NFTA_EXTHDR_FLAGS, htonl(priv->flags)))
+   goto nla_put_failure;
return 0;
 
 nla_put_failure:
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH nf-next v2,2/2] netfilter: nft_exthdr: add TCP option matching

[Pablo Neira Ayuso]

From: Manuel Messner 

This patch implements the kernel side of the TCP option patch.

Signed-off-by: Manuel Messner 
Reviewed-by: Florian Westphal 
Signed-off-by: Pablo Neira Ayuso 
---
v2: Rebase on top of Phil's update. I decided to handle this myself to speed
up things. Please review this rebase looks good to you. Thanks.

 include/uapi/linux/netfilter/nf_tables.h |  17 -
 net/netfilter/Kconfig|   4 +-
 net/netfilter/nft_exthdr.c   | 119 +++
 3 files changed, 124 insertions(+), 16 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h 
b/include/uapi/linux/netfilter/nf_tables.h
index 3e60ed78c538..207951516ede 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -709,13 +709,27 @@ enum nft_exthdr_flags {
 };
 
 /**
- * enum nft_exthdr_attributes - nf_tables IPv6 extension header expression 
netlink attributes
+ * enum nft_exthdr_op - nf_tables match options
+ *
+ * @NFT_EXTHDR_OP_IPV6: match against ipv6 extension headers
+ * @NFT_EXTHDR_OP_TCP: match against tcp options
+ */
+enum nft_exthdr_op {
+   NFT_EXTHDR_OP_IPV6,
+   NFT_EXTHDR_OP_TCPOPT,
+   __NFT_EXTHDR_OP_MAX
+};
+#define NFT_EXTHDR_OP_MAX  (__NFT_EXTHDR_OP_MAX - 1)
+
+/**
+ * enum nft_exthdr_attributes - nf_tables extension header expression netlink 
attributes
  *
  * @NFTA_EXTHDR_DREG: destination register (NLA_U32: nft_registers)
  * @NFTA_EXTHDR_TYPE: extension header type (NLA_U8)
  * @NFTA_EXTHDR_OFFSET: extension header offset (NLA_U32)
  * @NFTA_EXTHDR_LEN: extension header length (NLA_U32)
  * @NFTA_EXTHDR_FLAGS: extension header flags (NLA_U32)
+ * @NFTA_EXTHDR_OP: option match type (NLA_U8)
  */
 enum nft_exthdr_attributes {
NFTA_EXTHDR_UNSPEC,
@@ -724,6 +738,7 @@ enum nft_exthdr_attributes {
NFTA_EXTHDR_OFFSET,
NFTA_EXTHDR_LEN,
NFTA_EXTHDR_FLAGS,
+   NFTA_EXTHDR_OP,
__NFTA_EXTHDR_MAX
 };
 #define NFTA_EXTHDR_MAX(__NFTA_EXTHDR_MAX - 1)
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index ea479ed43373..9b28864cc36a 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -467,10 +467,10 @@ config NF_TABLES_NETDEV
  This option enables support for the "netdev" table.
 
 config NFT_EXTHDR
-   tristate "Netfilter nf_tables IPv6 exthdr module"
+   tristate "Netfilter nf_tables exthdr module"
help
  This option adds the "exthdr" expression that you can use to match
- IPv6 extension headers.
+ IPv6 extension headers and tcp options.
 
 config NFT_META
tristate "Netfilter nf_tables meta module"
diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index d43f750ab61c..8e94b6d55d97 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -15,20 +15,29 @@
 #include 
 #include 
 #include 
-// FIXME:
-#include 
+#include 
 
 struct nft_exthdr {
u8  type;
u8  offset;
u8  len;
+   u8  op;
enum nft_registers  dreg:8;
u8  flags;
 };
 
-static void nft_exthdr_eval(const struct nft_expr *expr,
-   struct nft_regs *regs,
-   const struct nft_pktinfo *pkt)
+static unsigned int optlen(const u8 *opt, unsigned int offset)
+{
+   /* Beware zero-length options: make finite progress */
+   if (opt[offset] <= TCPOPT_NOP || opt[offset + 1] == 0)
+   return 1;
+   else
+   return opt[offset + 1];
+}
+
+static void nft_exthdr_ipv6_eval(const struct nft_expr *expr,
+struct nft_regs *regs,
+const struct nft_pktinfo *pkt)
 {
struct nft_exthdr *priv = nft_expr_priv(expr);
u32 *dest = >data[priv->dreg];
@@ -52,6 +61,53 @@ static void nft_exthdr_eval(const struct nft_expr *expr,
regs->verdict.code = NFT_BREAK;
 }
 
+static void nft_exthdr_tcp_eval(const struct nft_expr *expr,
+   struct nft_regs *regs,
+   const struct nft_pktinfo *pkt)
+{
+   u8 buff[sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE];
+   struct nft_exthdr *priv = nft_expr_priv(expr);
+   unsigned int i, optl, tcphdr_len, offset;
+   u32 *dest = >data[priv->dreg];
+   struct tcphdr *tcph;
+   u8 *opt;
+
+   if (!pkt->tprot_set || pkt->tprot != IPPROTO_TCP)
+   goto err;
+
+   tcph = skb_header_pointer(pkt->skb, pkt->xt.thoff, sizeof(*tcph), buff);
+   if (!tcph)
+   goto err;
+
+   tcphdr_len = __tcp_hdrlen(tcph);
+   if (tcphdr_len < sizeof(*tcph))
+   goto err;
+
+   tcph = skb_header_pointer(pkt->skb, pkt->xt.thoff, tcphdr_len, buff);
+   if (!tcph)
+

Re: [PATCH] Revert "net: Remove state argument from skb_find_text()"

[Pablo Neira Ayuso]

On Sat, Feb 04, 2017 at 09:48:30PM -0800, Igor Pylypiv wrote:
> This reverts commit 059a2440fd3cf4ec57735db2c0a90401cde84fca.
> 
> Textsearch state parameter should be passed by pointer because
> its resulting value is needed for call to textsearch_next().

You're right this renders textsearch_next() useless.

But I remember David took this one time ago because there are no
clients for textsearch_next() in the tree, apart from
textsearch_find().

Am I missing anything?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nftables 5/9] src: add host byte order integer type

[Pablo Neira Ayuso]

On Tue, Feb 07, 2017 at 12:58:56PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Feb 06, 2017 at 11:33:01PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso  wrote:
> > > On Fri, Feb 03, 2017 at 01:35:52PM +0100, Florian Westphal wrote:
> > > > diff --git a/include/datatype.h b/include/datatype.h
> > > > index 9f127f2954e3..8c1c827253be 100644
> > > > --- a/include/datatype.h
> > > > +++ b/include/datatype.h
> > > > @@ -82,6 +82,7 @@ enum datatypes {
> > > > TYPE_DSCP,
> > > > TYPE_ECN,
> > > > TYPE_FIB_ADDR,
> > > > +   TYPE_U32,
> > > > __TYPE_MAX
> > > >  };
> > > >  #define TYPE_MAX   (__TYPE_MAX - 1)
> > > 
> > > Right, this is a real problem with host byteorder integer, the
> > > bytecode that we generate is not correct.
> > > 
> > > I have a patch to avoid this, it's still incomplete. I'm attaching it.
> > > 
> > > Note this is still incomplete, since this doesn't solve the netlink
> > > delinearize path. We can use the NFT_SET_USERDATA area and the tlv
> > > infrastructure that Carlos made in summer to store this
> > > metainformation that is only useful to
> > > 
> > > This shouldn't be a showstopper to get kernel patches in, we have a
> > > bit of time ahead to solve this userspace issue.
> > 
> > I don't understand why all this fuss is required.
> > 
> > The type always enocodes/decides the endianess, so I fail to see why we
> > need to store endianess also in the templates (f.e. meta_templates[],
> > it just seems 100% redundant ...)
> > 
> > Thats why it I added this host endian thing here, we could then replace
> > 
> > [NFT_META_CPU]  = META_TEMPLATE("cpu", _type, 4 * BITS_PER_BYTE, 
> > BYTEORDER_HOST_ENDIAN),
> > with
> > [NFT_META_CPU]  = META_TEMPLATE("cpu", _u32, 4 * BITS_PER_BYTE),
> >
> > and don't need this 'endianess override' in the templates anymore.
> > We might even be able to get rid of the endianess storage in the eval
> > context this way.
> > 
> > What am I missing?
> 
> This approach will not work for more stuff coming ahead, eg.
> concatenation support for integer datatypes. This currently doesn't
> work since nft complains on concatenation with datatypes with not
> fixed datatypes.
> 
> In that case, we will end up having integers of different sizes and
> byteorder. We would need one hardcoded type for each variant.
> 
> Note that these TYPE_XYZ are ABI too, so we should avoid changes once
> we push them into nftables.git. We also have a limited number of them,
> 6 bits since concatenations places up to 5 datatypes there using bit
> shifts.
> 
> I understand the override is not nice, that's one of the reasons why I
> did not push this yet. Probably we can allocate datatype instances
> dynamically, so we don't need this new field.

Oh, to add more info: There is one more corner case we have to
support:

add rule x y ip saddr . meta cpu eq 1.1.1.1 . 10

When using sets, we can stash the datatype, byteorder and size in
NFTA_SET_USERDATA. In this case above, we have to infer it from the
LHS of the relational that defines the concatenation. I guess this
will require a bit of code from rule_postprocess().
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nftables 5/9] src: add host byte order integer type

[Pablo Neira Ayuso]

On Mon, Feb 06, 2017 at 11:33:01PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso  wrote:
> > On Fri, Feb 03, 2017 at 01:35:52PM +0100, Florian Westphal wrote:
> > > diff --git a/include/datatype.h b/include/datatype.h
> > > index 9f127f2954e3..8c1c827253be 100644
> > > --- a/include/datatype.h
> > > +++ b/include/datatype.h
> > > @@ -82,6 +82,7 @@ enum datatypes {
> > >   TYPE_DSCP,
> > >   TYPE_ECN,
> > >   TYPE_FIB_ADDR,
> > > + TYPE_U32,
> > >   __TYPE_MAX
> > >  };
> > >  #define TYPE_MAX (__TYPE_MAX - 1)
> > 
> > Right, this is a real problem with host byteorder integer, the
> > bytecode that we generate is not correct.
> > 
> > I have a patch to avoid this, it's still incomplete. I'm attaching it.
> > 
> > Note this is still incomplete, since this doesn't solve the netlink
> > delinearize path. We can use the NFT_SET_USERDATA area and the tlv
> > infrastructure that Carlos made in summer to store this
> > metainformation that is only useful to
> > 
> > This shouldn't be a showstopper to get kernel patches in, we have a
> > bit of time ahead to solve this userspace issue.
> 
> I don't understand why all this fuss is required.
> 
> The type always enocodes/decides the endianess, so I fail to see why we
> need to store endianess also in the templates (f.e. meta_templates[],
> it just seems 100% redundant ...)
> 
> Thats why it I added this host endian thing here, we could then replace
> 
> [NFT_META_CPU]  = META_TEMPLATE("cpu", _type, 4 * BITS_PER_BYTE, 
> BYTEORDER_HOST_ENDIAN),
> with
> [NFT_META_CPU]  = META_TEMPLATE("cpu", _u32, 4 * BITS_PER_BYTE),
>
> and don't need this 'endianess override' in the templates anymore.
> We might even be able to get rid of the endianess storage in the eval
> context this way.
> 
> What am I missing?

This approach will not work for more stuff coming ahead, eg.
concatenation support for integer datatypes. This currently doesn't
work since nft complains on concatenation with datatypes with not
fixed datatypes.

In that case, we will end up having integers of different sizes and
byteorder. We would need one hardcoded type for each variant.

Note that these TYPE_XYZ are ABI too, so we should avoid changes once
we push them into nftables.git. We also have a limited number of them,
6 bits since concatenations places up to 5 datatypes there using bit
shifts.

I understand the override is not nice, that's one of the reasons why I
did not push this yet. Probably we can allocate datatype instances
dynamically, so we don't need this new field.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html