Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Jeremy Sowden <jeremy@azazel.net>
Cc: Netfilter Devel <netfilter-devel@vger.kernel.org>
Subject: Re: [PATCH nf-next 3/3] netfilter: bitwise: add support for shifts.
Date: Mon, 13 Jan 2020 22:59:25 +0100
Message-ID: <20200113215925.uh5ir5ntcrtuh7qq@salvia> (raw)
In-Reply-To: <20200110123312.106438-4-jeremy@azazel.net>

Hi Jeremy,

On Fri, Jan 10, 2020 at 12:33:12PM +0000, Jeremy Sowden wrote:
> Currently nft_bitwise only supports boolean operations: NOT, AND, OR and
> XOR.  Extend it to do shifts as well.
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  include/uapi/linux/netfilter/nf_tables.h |  9 ++-
>  net/netfilter/nft_bitwise.c              | 84 ++++++++++++++++++++++--
>  2 files changed, 86 insertions(+), 7 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index dd4611767933..8f244ada0ad3 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -492,12 +492,15 @@ enum nft_immediate_attributes {
>   * @NFTA_BITWISE_LEN: length of operands (NLA_U32)
>   * @NFTA_BITWISE_MASK: mask value (NLA_NESTED: nft_data_attributes)
>   * @NFTA_BITWISE_XOR: xor value (NLA_NESTED: nft_data_attributes)
> + * @NFTA_BITWISE_LSHIFT: left shift value (NLA_U32)
> + * @NFTA_BITWISE_RSHIFT: right shift value (NLA_U32)

I'd suggest you add:

NFTA_BITWISE_OP

possible values are:

NFT_BITWISE_XOR (or _BOOL, you pick the more appropriate name for me)
NFT_BITWISE_RSHIFT
NFT_BITWISE_LSHIFT

You can introduce the NFTA_BITWISE_OP attribute in a initial
preparation patch.

If NFTA_BITWISE_OP is not specified, then default to NFT_BITWISE_XOR
to ensure backward compatibility with old userspace tooling.

>   *
> - * The bitwise expression performs the following operation:
> + * The bitwise expression supports boolean and shift operations.  It implements
> + * the boolean operations by performing the following operation:
>   *
>   * dreg = (sreg & mask) ^ xor
>   *
> - * which allow to express all bitwise operations:
> + * with these mask and xor values:
>   *
>   * 		mask	xor
>   * NOT:		1	1
> @@ -512,6 +515,8 @@ enum nft_bitwise_attributes {
>  	NFTA_BITWISE_LEN,
>  	NFTA_BITWISE_MASK,
>  	NFTA_BITWISE_XOR,
> +	NFTA_BITWISE_LSHIFT,
> +	NFTA_BITWISE_RSHIFT,

On top of NFTA_BITWISE_OP.

I'd suggest you add NFTA_BITWISE_DATA instead of NFTA_BITWISE_{R,L}SHIFT.

>  	__NFTA_BITWISE_MAX
>  };
>  #define NFTA_BITWISE_MAX	(__NFTA_BITWISE_MAX - 1)
> diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> index d7724250be1f..e4db77057b0e 100644
> --- a/net/netfilter/nft_bitwise.c
> +++ b/net/netfilter/nft_bitwise.c
> @@ -15,12 +15,20 @@
>  #include <net/netfilter/nf_tables.h>
>  #include <net/netfilter/nf_tables_offload.h>
>  
> +enum nft_bitwise_ops {
> +	OP_BOOL,
> +	OP_LSHIFT,
> +	OP_RSHIFT,
> +};
> +
>  struct nft_bitwise {
>  	enum nft_registers	sreg:8;
>  	enum nft_registers	dreg:8;
> +	enum nft_bitwise_ops	op:8;
>  	u8			len;
>  	struct nft_data		mask;
>  	struct nft_data		xor;
> +	u8			shift;
>  };
>  
>  void nft_bitwise_eval(const struct nft_expr *expr,
> @@ -31,6 +39,26 @@ void nft_bitwise_eval(const struct nft_expr *expr,
>  	u32 *dst = &regs->data[priv->dreg];
>  	unsigned int i;
>  
> +	if (priv->op == OP_LSHIFT) {

I'd suggest you turn this into something like:

        switch (priv->op) {
        case NFT_BITWISE_RSHIFT:
                nft_bitwise_rshift(...);
                break;
        case NFT_BITWISE_LSHIFT:
                nft_bitwise_lshift(...);
                break;
        case ...
        }

so these code below is store in a function.

> +		u32 carry = 0;
> +
> +		for (i = DIV_ROUND_UP(priv->len, sizeof(u32)); i > 0; i--) {
> +			dst[i - 1] = (src[i - 1] << priv->shift) | carry;
> +			carry = src[i - 1] >> (32 - priv->shift);
> +		}
> +		return;
> +	}
> +
> +	if (priv->op == OP_RSHIFT) {
> +		u32 carry = 0;
> +
> +		for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
> +			dst[i] = carry | (src[i] >> priv->shift);
> +			carry = src[i] << (32 - priv->shift);
> +		}
> +		return;
> +	}
> +
>  	for (i = 0; i < DIV_ROUND_UP(priv->len, 4); i++)
>  		dst[i] = (src[i] & priv->mask.data[i]) ^ priv->xor.data[i];

Probably an initial preparation patch to move this code to a function
would be fine.

>  }
> @@ -41,6 +69,8 @@ static const struct nla_policy nft_bitwise_policy[NFTA_BITWISE_MAX + 1] = {
>  	[NFTA_BITWISE_LEN]	= { .type = NLA_U32 },
>  	[NFTA_BITWISE_MASK]	= { .type = NLA_NESTED },
>  	[NFTA_BITWISE_XOR]	= { .type = NLA_NESTED },
> +	[NFTA_BITWISE_LSHIFT]	= { .type = NLA_U32 },
> +	[NFTA_BITWISE_RSHIFT]	= { .type = NLA_U32 },
>  };
>  
>  static int nft_bitwise_init(const struct nft_ctx *ctx,
> @@ -52,11 +82,9 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
>  	u32 len;
>  	int err;
>  
> -	if (tb[NFTA_BITWISE_SREG] == NULL ||
> -	    tb[NFTA_BITWISE_DREG] == NULL ||
> -	    tb[NFTA_BITWISE_LEN] == NULL ||
> -	    tb[NFTA_BITWISE_MASK] == NULL ||
> -	    tb[NFTA_BITWISE_XOR] == NULL)
> +	if (!tb[NFTA_BITWISE_SREG] ||
> +	    !tb[NFTA_BITWISE_DREG] ||
> +	    !tb[NFTA_BITWISE_LEN])
>  		return -EINVAL;
>  
>  	err = nft_parse_u32_check(tb[NFTA_BITWISE_LEN], U8_MAX, &len);
> @@ -76,6 +104,36 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
>  	if (err < 0)
>  		return err;
>  
> +	if (tb[NFTA_BITWISE_LSHIFT]) {
> +		u32 shift;
> +
> +		err = nft_parse_u32_check(tb[NFTA_BITWISE_LSHIFT], U8_MAX,
> +					  &shift);
> +		if (err < 0)
> +			return err;
> +
> +		priv->shift = shift;
> +		priv->op = OP_LSHIFT;
> +		return 0;
> +	}
> +
> +	if (tb[NFTA_BITWISE_RSHIFT]) {
> +		u32 shift;
> +
> +		err = nft_parse_u32_check(tb[NFTA_BITWISE_RSHIFT], U8_MAX,
> +					  &shift);
> +		if (err < 0)
> +			return err;
> +
> +		priv->shift = shift;
> +		priv->op = OP_RSHIFT;
> +		return 0;
> +	}

I would like to see that the netlink interface really bails out for
combinations this does not support. That will make it easier later on
to extend it from userspace.

Therefore, something like:

        switch (priv->op) {
        case NFT_BITWISE_RSHIFT:
        case NFT_BITWISE_LSHIFT:
                if (!tb[NFTA_BITWISE_SHIFT])
                        return -EINVAL;

                if (tb[NFTA_BITWISE_MASK] ||
                    tb[NFTA_BITWISE_XOR])
                        return -EINVAL;

                break;
        case NFT_BITWISE_BOOL:
                if (!tb[NFTA_BITWISE_MASK] ||
                    !tb[NFTA_BITWISE_XOR])
                        return -EINVAL;

                if (tb[NFTA_BITWISE_SHIFT)
                        return -EINVAL;
                break;

> +	if (!tb[NFTA_BITWISE_MASK] ||
> +	    !tb[NFTA_BITWISE_XOR])
> +		return -EINVAL;
> +
>  	err = nft_data_init(NULL, &priv->mask, sizeof(priv->mask), &d1,
>  			    tb[NFTA_BITWISE_MASK]);
>  	if (err < 0)
> @@ -94,6 +152,7 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
>  		goto err2;
>  	}
>  
> +	priv->op = OP_BOOL;
>  	return 0;
>  err2:
>  	nft_data_release(&priv->xor, d2.type);
> @@ -113,6 +172,18 @@ static int nft_bitwise_dump(struct sk_buff *skb, const struct nft_expr *expr)
>  	if (nla_put_be32(skb, NFTA_BITWISE_LEN, htonl(priv->len)))
>  		return -1;
>  
> +	if (priv->op == OP_LSHIFT) {
> +		if (nla_put_be32(skb, NFTA_BITWISE_LSHIFT, htonl(priv->shift)))
> +			return -1;
> +		return 0;
> +	}
> +
> +	if (priv->op == OP_RSHIFT) {
> +		if (nla_put_be32(skb, NFTA_BITWISE_RSHIFT, htonl(priv->shift)))
> +			return -1;
> +		return 0;
> +	}

With one single NFTA_BITWISE_SHIFT, this will be consolidated.

Thanks for working on this.

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 12:33 [PATCH nf-next 0/3] netfilter: nft_bitwise: shift support Jeremy Sowden
2020-01-10 12:33 ` [PATCH nf-next 1/3] netfilter: nf_tables: white-space fixes Jeremy Sowden
2020-01-10 12:33 ` [PATCH nf-next 2/3] netfilter: bitwise: replace gotos with returns Jeremy Sowden
2020-01-10 12:33 ` [PATCH nf-next 3/3] netfilter: bitwise: add support for shifts Jeremy Sowden
2020-01-13 21:59   ` Pablo Neira Ayuso [this message]
2020-01-14 21:28 [PATCH nf-next 0/3] netfilter: nft_bitwise: shift support Jeremy Sowden
2020-01-14 21:29 ` [PATCH nf-next 3/3] netfilter: bitwise: add support for shifts Jeremy Sowden

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=20200113215925.uh5ir5ntcrtuh7qq@salvia \
    --to=pablo@netfilter.org \
    --cc=jeremy@azazel.net \
    --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

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git