From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [libnftables PATCH v2] ct: fix key and dir requirements
Date: Thu, 16 Jan 2014 19:05:38 +0100 [thread overview]
Message-ID: <20140116180538.GA18501@localhost> (raw)
In-Reply-To: <20140115181939.25239.19905.stgit@nfdev.cica.es>
On Wed, Jan 15, 2014 at 07:20:22PM +0100, Arturo Borrero Gonzalez wrote:
> Follow linux/net/netfilter/nft_ct.c to adjust key and dir attributes.
>
> The dir attribute is needed only when using certaing keys, and prohibited with
> others.
>
> Key is always mandatory.
>
> Previous to this patch, using XML/JSON to manage this expr led to some
> undefined and erroneous behaviours.
>
> While at it, update tests files in order to pass nft-parsing-test.
>
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
> v2: fixed wrong logic in the XML parser. Added support for NFT_CT_L3PROTOCOL
>
> src/expr/ct.c | 97 +++++++++++++++++++++++++++------------
> tests/xmlfiles/24-rule-ct.xml | 2 -
> tests/xmlfiles/37-rule-real.xml | 2 -
> tests/xmlfiles/39-rule-real.xml | 2 -
> tests/xmlfiles/50-rule-real.xml | 2 -
> tests/xmlfiles/51-rule-real.xml | 2 -
> tests/xmlfiles/52-rule-real.xml | 2 -
> tests/xmlfiles/53-rule-real.xml | 2 -
> tests/xmlfiles/54-rule-real.xml | 2 -
> tests/xmlfiles/55-rule-real.xml | 2 -
> tests/xmlfiles/56-rule-real.xml | 2 -
> tests/xmlfiles/57-rule-real.xml | 2 -
> 12 files changed, 78 insertions(+), 41 deletions(-)
>
> diff --git a/src/expr/ct.c b/src/expr/ct.c
> index e960134..e74c36c 100644
> --- a/src/expr/ct.c
> +++ b/src/expr/ct.c
> @@ -179,6 +179,28 @@ static inline int str2ctkey(const char *ctkey)
> return -1;
> }
>
> +static bool ctkey_req_dir(int ctkey)
> +{
> + switch (ctkey) {
> + case NFT_CT_STATE:
> + case NFT_CT_DIRECTION:
> + case NFT_CT_STATUS:
> + case NFT_CT_MARK:
> + case NFT_CT_SECMARK:
> + case NFT_CT_EXPIRATION:
> + case NFT_CT_HELPER:
> + return false;
> + case NFT_CT_L3PROTOCOL:
> + case NFT_CT_SRC:
> + case NFT_CT_DST:
> + case NFT_CT_PROTOCOL:
> + case NFT_CT_PROTO_SRC:
> + case NFT_CT_PROTO_DST:
> + default:
> + return true;
> + }
The kernel will complain if we pass invalid combinations, I don't want
to have this early validation code in the library.
> +}
> +
> static int nft_rule_expr_ct_json_parse(struct nft_rule_expr *e, json_t *root,
> struct nft_parse_err *err)
> {
> @@ -193,22 +215,19 @@ static int nft_rule_expr_ct_json_parse(struct nft_rule_expr *e, json_t *root,
>
> nft_rule_expr_set_u32(e, NFT_EXPR_CT_DREG, reg);
>
> - if (nft_jansson_node_exist(root, "key")) {
> - key_str = nft_jansson_parse_str(root, "key", err);
> - if (key_str == NULL)
> - return -1;
> -
> - key = str2ctkey(key_str);
> - if (key < 0)
> - goto err;
> + key_str = nft_jansson_parse_str(root, "key", err);
> + if (key_str == NULL)
> + return -1;
>
> - nft_rule_expr_set_u32(e, NFT_EXPR_CT_KEY, key);
> + key = str2ctkey(key_str);
> + if (key < 0)
> + goto err;
>
> - }
> + nft_rule_expr_set_u32(e, NFT_EXPR_CT_KEY, key);
>
> - if (nft_jansson_node_exist(root, "dir")) {
> - if (nft_jansson_parse_val(root, "dir", NFT_TYPE_U8, &dir,
> - err) < 0)
> + if (ctkey_req_dir(key)) {
> + if (nft_jansson_parse_val(root, "dir", NFT_TYPE_U8,
> + &dir, err) < 0)
> return -1;
>
> if (dir != IP_CT_DIR_ORIGINAL && dir != IP_CT_DIR_REPLY)
> @@ -257,15 +276,18 @@ static int nft_rule_expr_ct_xml_parse(struct nft_rule_expr *e, mxml_node_t *tree
> ct->key = key;
> e->flags |= (1 << NFT_EXPR_CT_KEY);
>
> - if (nft_mxml_num_parse(tree, "dir", MXML_DESCEND_FIRST, BASE_DEC,
> - &dir, NFT_TYPE_U8, NFT_XML_MAND, err) != 0)
> - return -1;
> + if (ctkey_req_dir(key)) {
so this should be: if "dir" is present, parse it. Otherwise, just
skip it.
> + if (nft_mxml_num_parse(tree, "dir", MXML_DESCEND_FIRST,
> + BASE_DEC, &dir, NFT_TYPE_U8,
> + NFT_XML_MAND, err) != 0)
> + return -1;
>
> - if (dir != IP_CT_DIR_ORIGINAL && dir != IP_CT_DIR_REPLY)
> - goto err;
> + if (dir != IP_CT_DIR_ORIGINAL && dir != IP_CT_DIR_REPLY)
> + goto err;
>
> - ct->dir = dir;
> - e->flags |= (1 << NFT_EXPR_CT_DIR);
> + ct->dir = dir;
> + e->flags |= (1 << NFT_EXPR_CT_DIR);
Not related to this patch, but better I prefer if you use:
nft_rule_expr_set_u8(...) instead of these two lines above.
> + }
>
> return 0;
> err:
> @@ -286,19 +308,37 @@ nft_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
> ret = snprintf(buf, len, "\"dreg\":%u", ct->dreg);
> SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>
> - if (e->flags & (1 << NFT_EXPR_CT_KEY)) {
> - ret = snprintf(buf+offset, len, ",\"key\":\"%s\"",
> - ctkey2str(ct->key));
> - SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> - }
> + ret = snprintf(buf+offset, len, ",\"key\":\"%s\"",
> + ctkey2str(ct->key));
> + SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>
> - if (e->flags & (1 << NFT_EXPR_CT_DIR)) {
> + if (ctkey_req_dir(ct->key) && (e->flags & (1 << NFT_EXPR_CT_DIR))) {
Same thing here, you should print this if the direction is set,
otherwise, skip it.
I prefer if you use nft_rule_expr_is_set(...) instead.
> ret = snprintf(buf+offset, len, ",\"dir\":%u", ct->dir);
> SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> }
>
> return offset;
> +}
> +
> +static int
> +nft_expr_ct_snprintf_xml(char *buf, size_t size, struct nft_rule_expr *e)
> +{
> + int ret, len = size, offset = 0;
> + struct nft_expr_ct *ct = nft_expr_data(e);
> +
> + ret = snprintf(buf, len, "<dreg>%u</dreg>", ct->dreg);
> + SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +
> + ret = snprintf(buf+offset, len, "<key>%s</key>",
> + ctkey2str(ct->key));
> + SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>
> + if (ctkey_req_dir(ct->key) && (e->flags & (1 << NFT_EXPR_CT_DIR))) {
> + ret = snprintf(buf+offset, len, "<dir>%u</dir>", ct->dir);
> + SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> + }
> +
> + return offset;
> }
>
> static int
> @@ -312,10 +352,7 @@ nft_rule_expr_ct_snprintf(char *buf, size_t len, uint32_t type,
> return snprintf(buf, len, "load %s => reg %u dir %u ",
> ctkey2str(ct->key), ct->dreg, ct->dir);
> case NFT_OUTPUT_XML:
> - return snprintf(buf, len, "<dreg>%u</dreg>"
> - "<key>%s</key>"
> - "<dir>%u</dir>",
> - ct->dreg, ctkey2str(ct->key), ct->dir);
> + return nft_expr_ct_snprintf_xml(buf, len, e);
> case NFT_OUTPUT_JSON:
> return nft_expr_ct_snprintf_json(buf, len, e);
> default:
next prev parent reply other threads:[~2014-01-16 18:05 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-15 18:20 [libnftables PATCH v2] ct: fix key and dir requirements Arturo Borrero Gonzalez
2014-01-16 18:05 ` Pablo Neira Ayuso [this message]
2014-01-16 20:46 ` Arturo Borrero Gonzalez
2014-01-16 21:22 ` Pablo Neira Ayuso
2014-01-16 22:46 ` Arturo Borrero Gonzalez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140116180538.GA18501@localhost \
--to=pablo@netfilter.org \
--cc=arturo.borrero.glez@gmail.com \
--cc=netfilter-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.