mbox series

[nf-next,v4,00/10] netfilter: nft_bitwise: shift support

Message ID 20200115213216.77493-1-jeremy@azazel.net
Headers show
Series netfilter: nft_bitwise: shift support | expand

Message

Jeremy Sowden Jan. 15, 2020, 9:32 p.m. UTC
The connmark xtables extension supports bit-shifts.  Add support for
shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:

  nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
  nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8

Changes since v3:

  * the length of shift values sent by nft may be less than sizeof(u32).

Changes since v2:

  * convert NFTA_BITWISE_DATA from u32 to nft_data;
  * add check that shift value is not too large;
  * use BITS_PER_TYPE to get the size of u32, rather than hard-coding it
    when evaluating shifts.

Changes since v1:

  * more white-space fixes;
  * move bitwise op enum to UAPI;
  * add NFTA_BITWISE_OP and NFTA_BITWISE_DATA;
  * remove NFTA_BITWISE_LSHIFT and NFTA_BITWISE_RSHIFT;
  * add helpers for initializaing, evaluating and dumping different
    types of operation.

Jeremy Sowden (10):
  netfilter: nf_tables: white-space fixes.
  netfilter: bitwise: remove NULL comparisons from attribute checks.
  netfilter: bitwise: replace gotos with returns.
  netfilter: bitwise: add NFTA_BITWISE_OP attribute.
  netfilter: bitwise: add helper for initializing boolean operations.
  netfilter: bitwise: add helper for evaluating boolean operations.
  netfilter: bitwise: add helper for dumping boolean operations.
  netfilter: bitwise: only offload boolean operations.
  netfilter: bitwise: add NFTA_BITWISE_DATA attribute.
  netfilter: bitwise: add support for shifts.

 include/uapi/linux/netfilter/nf_tables.h |  24 ++-
 net/netfilter/nft_bitwise.c              | 217 ++++++++++++++++++-----
 net/netfilter/nft_set_bitmap.c           |   4 +-
 net/netfilter/nft_set_hash.c             |   2 +-
 4 files changed, 200 insertions(+), 47 deletions(-)

Comments

Jeremy Sowden Jan. 16, 2020, 8:51 a.m. UTC | #1
On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> The connmark xtables extension supports bit-shifts.  Add support for
> shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:
>
>   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
>   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
>
> Changes since v3:
>
>   * the length of shift values sent by nft may be less than sizeof(u32).

Actually, having thought about this some more, I believe I had it right
in v3.  The difference between v3 and v4 is this:

  @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
                              tb[NFTA_BITWISE_DATA]);
          if (err < 0)
                  return err;
  -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
  +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
              priv->data.data[0] >= BITS_PER_TYPE(u32)) {
                  nft_data_release(&priv->data, d.type);
                  return -EINVAL;

However, I now think the problem is in userspace and nft should always
send four bytes.  If it sends fewer, it makes it more complicated to get
the endianness right.

Unless you think there are other changes needed that will required a v5,
shall we just ignore v4 and stick with v3?

> Changes since v2:
>
>   * convert NFTA_BITWISE_DATA from u32 to nft_data;
>   * add check that shift value is not too large;
>   * use BITS_PER_TYPE to get the size of u32, rather than hard-coding it
>     when evaluating shifts.
>
> Changes since v1:
>
>   * more white-space fixes;
>   * move bitwise op enum to UAPI;
>   * add NFTA_BITWISE_OP and NFTA_BITWISE_DATA;
>   * remove NFTA_BITWISE_LSHIFT and NFTA_BITWISE_RSHIFT;
>   * add helpers for initializaing, evaluating and dumping different
>     types of operation.
>
> Jeremy Sowden (10):
>   netfilter: nf_tables: white-space fixes.
>   netfilter: bitwise: remove NULL comparisons from attribute checks.
>   netfilter: bitwise: replace gotos with returns.
>   netfilter: bitwise: add NFTA_BITWISE_OP attribute.
>   netfilter: bitwise: add helper for initializing boolean operations.
>   netfilter: bitwise: add helper for evaluating boolean operations.
>   netfilter: bitwise: add helper for dumping boolean operations.
>   netfilter: bitwise: only offload boolean operations.
>   netfilter: bitwise: add NFTA_BITWISE_DATA attribute.
>   netfilter: bitwise: add support for shifts.
>
>  include/uapi/linux/netfilter/nf_tables.h |  24 ++-
>  net/netfilter/nft_bitwise.c              | 217 ++++++++++++++++++-----
>  net/netfilter/nft_set_bitmap.c           |   4 +-
>  net/netfilter/nft_set_hash.c             |   2 +-
>  4 files changed, 200 insertions(+), 47 deletions(-)
>
> --
> 2.24.1

J.
Pablo Neira Ayuso Jan. 16, 2020, 11:22 a.m. UTC | #2
On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > The connmark xtables extension supports bit-shifts.  Add support for
> > shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:
> >
> >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> >
> > Changes since v3:
> >
> >   * the length of shift values sent by nft may be less than sizeof(u32).
> 
> Actually, having thought about this some more, I believe I had it right
> in v3.  The difference between v3 and v4 is this:
> 
>   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
>                               tb[NFTA_BITWISE_DATA]);
>           if (err < 0)
>                   return err;
>   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
>   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
>               priv->data.data[0] >= BITS_PER_TYPE(u32)) {

Why restrict this to 32-bits?

>                   nft_data_release(&priv->data, d.type);
>                   return -EINVAL;
> 
> However, I now think the problem is in userspace and nft should always
> send four bytes.  If it sends fewer, it makes it more complicated to get
> the endianness right.
> 
> Unless you think there are other changes needed that will required a v5,
> shall we just ignore v4 and stick with v3?
Pablo Neira Ayuso Jan. 16, 2020, 11:28 a.m. UTC | #3
On Thu, Jan 16, 2020 at 12:22:47PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> > On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > > The connmark xtables extension supports bit-shifts.  Add support for
> > > shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:
> > >
> > >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> > >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> > >
> > > Changes since v3:
> > >
> > >   * the length of shift values sent by nft may be less than sizeof(u32).
> > 
> > Actually, having thought about this some more, I believe I had it right
> > in v3.  The difference between v3 and v4 is this:
> > 
> >   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> >                               tb[NFTA_BITWISE_DATA]);
> >           if (err < 0)
> >                   return err;
> >   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
> >   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
> >               priv->data.data[0] >= BITS_PER_TYPE(u32)) {
> 
> Why restrict this to 32-bits?

Ah, I see, only for the shift case. Indeed, makes sense :-)

Let me have a look at v3.
Jeremy Sowden Jan. 16, 2020, 11:41 a.m. UTC | #4
On 2020-01-16, at 12:22:47 +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> > On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > > The connmark xtables extension supports bit-shifts.  Add support
> > > for shifts to nft_bitwise in order to allow nftables to do
> > > likewise, e.g.:
> > >
> > >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> > >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> > >
> > > Changes since v3:
> > >
> > >   * the length of shift values sent by nft may be less than
> > >   sizeof(u32).
> >
> > Actually, having thought about this some more, I believe I had it
> > right in v3.  The difference between v3 and v4 is this:
> >
> >   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> >                               tb[NFTA_BITWISE_DATA]);
> >           if (err < 0)
> >                   return err;
> >   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
> >   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
> >               priv->data.data[0] >= BITS_PER_TYPE(u32)) {
>
> Why restrict this to 32-bits?

Because of how I implemented the shifts.  Here's the current rshift:

  static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
                                      const struct nft_bitwise *priv)
  {
          u32 shift = priv->data.data[0];
          unsigned int i;
          u32 carry = 0;

          for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
                  dst[i] = carry | (src[i] >> shift);
                  carry = src[i] << (BITS_PER_TYPE(u32) - shift);
          }
  }

In order to support larger shifts, it would need to look something
like:

  static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
                                      const struct nft_bitwise *priv)
  {
          unsigned len = DIV_ROUND_UP(priv->len, sizeof(u32));
          unsigned int d = shift / BITS_PER_TYPE(u32), s = 0;
          u32 shift = priv->data.data[0];
          u32 carry = 0;

          if (d > 0) {
                  memset(dst, '\0', d * sizeof(*dst));
                  shift %= BITS_PER_TYPE(u32);
          }

          for (s = 0; d < len; d++, s++) {
                  dst[d] = carry | (src[s] >> shift);
                  carry = src[s] << (BITS_PER_TYPE(u32) - shift);
          }
  }

J.
Pablo Neira Ayuso Jan. 16, 2020, 12:09 p.m. UTC | #5
On Thu, Jan 16, 2020 at 11:41:53AM +0000, Jeremy Sowden wrote:
> On 2020-01-16, at 12:22:47 +0100, Pablo Neira Ayuso wrote:
> > On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> > > On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > > > The connmark xtables extension supports bit-shifts.  Add support
> > > > for shifts to nft_bitwise in order to allow nftables to do
> > > > likewise, e.g.:
> > > >
> > > >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> > > >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> > > >
> > > > Changes since v3:
> > > >
> > > >   * the length of shift values sent by nft may be less than
> > > >   sizeof(u32).
> > >
> > > Actually, having thought about this some more, I believe I had it
> > > right in v3.  The difference between v3 and v4 is this:
> > >
> > >   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> > >                               tb[NFTA_BITWISE_DATA]);
> > >           if (err < 0)
> > >                   return err;
> > >   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
> > >   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
> > >               priv->data.data[0] >= BITS_PER_TYPE(u32)) {
> >
> > Why restrict this to 32-bits?
> 
> Because of how I implemented the shifts.  Here's the current rshift:
> 
>   static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
>                                       const struct nft_bitwise *priv)
>   {
>           u32 shift = priv->data.data[0];
>           unsigned int i;
>           u32 carry = 0;
> 
>           for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
>                   dst[i] = carry | (src[i] >> shift);
>                   carry = src[i] << (BITS_PER_TYPE(u32) - shift);
>           }
>   }
> 
> In order to support larger shifts, it would need to look something
> like:

No need for larger shift indeed, no need for this.

I just wanted to make sure NFTA_BITWISE_DATA can be reused later on in
future new operation that might require larger data.

All good then, I'll review v3, OK?
Jeremy Sowden Jan. 16, 2020, 12:13 p.m. UTC | #6
On 2020-01-16, at 13:09:25 +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 16, 2020 at 11:41:53AM +0000, Jeremy Sowden wrote:
> > On 2020-01-16, at 12:22:47 +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Jan 16, 2020 at 08:51:33AM +0000, Jeremy Sowden wrote:
> > > > On 2020-01-15, at 21:32:06 +0000, Jeremy Sowden wrote:
> > > > > The connmark xtables extension supports bit-shifts.  Add support
> > > > > for shifts to nft_bitwise in order to allow nftables to do
> > > > > likewise, e.g.:
> > > > >
> > > > >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> > > > >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> > > > >
> > > > > Changes since v3:
> > > > >
> > > > >   * the length of shift values sent by nft may be less than
> > > > >   sizeof(u32).
> > > >
> > > > Actually, having thought about this some more, I believe I had it
> > > > right in v3.  The difference between v3 and v4 is this:
> > > >
> > > >   @@ -146,7 +146,7 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> > > >                               tb[NFTA_BITWISE_DATA]);
> > > >           if (err < 0)
> > > >                   return err;
> > > >   -       if (d.type != NFT_DATA_VALUE || d.len != sizeof(u32) ||
> > > >   +       if (d.type != NFT_DATA_VALUE || d.len > sizeof(u32) ||
> > > >               priv->data.data[0] >= BITS_PER_TYPE(u32)) {
> > >
> > > Why restrict this to 32-bits?
> >
> > Because of how I implemented the shifts.  Here's the current rshift:
> >
> >   static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
> >                                       const struct nft_bitwise *priv)
> >   {
> >           u32 shift = priv->data.data[0];
> >           unsigned int i;
> >           u32 carry = 0;
> >
> >           for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
> >                   dst[i] = carry | (src[i] >> shift);
> >                   carry = src[i] << (BITS_PER_TYPE(u32) - shift);
> >           }
> >   }
> >
> > In order to support larger shifts, it would need to look something
> > like:
>
> No need for larger shift indeed, no need for this.
>
> I just wanted to make sure NFTA_BITWISE_DATA can be reused later on in
> future new operation that might require larger data.
>
> All good then, I'll review v3, OK?

Yup. :)
Pablo Neira Ayuso Jan. 16, 2020, 2:48 p.m. UTC | #7
On Wed, Jan 15, 2020 at 09:32:06PM +0000, Jeremy Sowden wrote:
> The connmark xtables extension supports bit-shifts.  Add support for
> shifts to nft_bitwise in order to allow nftables to do likewise, e.g.:
> 
>   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
>   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> 
> Changes since v3:
> 
>   * the length of shift values sent by nft may be less than sizeof(u32).
> 
> Changes since v2:
> 
>   * convert NFTA_BITWISE_DATA from u32 to nft_data;
>   * add check that shift value is not too large;
>   * use BITS_PER_TYPE to get the size of u32, rather than hard-coding it
>     when evaluating shifts.

Series applied, thanks.

I made a few updates:

* Replaced -EINVAL by -EOPNOTSUPP in case NFTA_BITWISE_OP is not
  supported. -EINVAL is usually reserved to missing netlink attribute /
  malformed netlink message (actually, you can find many spots where
  this is a bit overloaded with different "meanings", but just trying
  to stick to those semantics here).

* Replaced:

        return nft_bitwise_init_bool(priv, tb);

  by:

        err = nft_bitwise_init_bool(priv, tb);
        break;
  }

  return err;

  in a few spots, I hope I did not break anything.

I tend to find that easier to read today, minor things like this are
very much debatable.

Thanks.
Jeremy Sowden Jan. 16, 2020, 2:59 p.m. UTC | #8
On 2020-01-16, at 15:48:33 +0100, Pablo Neira Ayuso wrote:
> On Wed, Jan 15, 2020 at 09:32:06PM +0000, Jeremy Sowden wrote:
> > The connmark xtables extension supports bit-shifts.  Add support for
> > shifts to nft_bitwise in order to allow nftables to do likewise,
> > e.g.:
> >
> >   nft add rule t c oif lo ct mark set meta mark << 8 | 0xab
> >   nft add rule t c iif lo meta mark & 0xff 0xab ct mark set meta mark >> 8
> >
> > Changes since v3:
> >
> >   * the length of shift values sent by nft may be less than
> >     sizeof(u32).
> >
> > Changes since v2:
> >
> >   * convert NFTA_BITWISE_DATA from u32 to nft_data;
> >   * add check that shift value is not too large;
> >   * use BITS_PER_TYPE to get the size of u32, rather than
> >     hard-coding it when evaluating shifts.
>
> Series applied, thanks.

Cheers. :) I'll update the userspace changes and send them out.

J.
Pablo Neira Ayuso Jan. 26, 2020, 11:12 a.m. UTC | #9
Hi Jeremy,

I've been looking into (ab)using bitwise to implement add/sub. I would
like to not add nft_arith for only this, and it seems to me much of
your code can be reused.

Do you think something like this would work?

Thanks.
Jeremy Sowden Jan. 27, 2020, 11:13 a.m. UTC | #10
On 2020-01-26, at 12:12:51 +0100, Pablo Neira Ayuso wrote:
> I've been looking into (ab)using bitwise to implement add/sub. I would
> like to not add nft_arith for only this, and it seems to me much of
> your code can be reused.
>
> Do you think something like this would work?

Absolutely.

A couple of questions.  What's the use-case?  I find the combination of
applying the delta to every u32 and having a carry curious.  Do you want
to support bigendian arithmetic (i.e., carrying to the left) as well?

I've suggested a couple of changes below.

J.

> Thanks.
>
> diff --git a/include/uapi/linux/netfilter/nf_tables.h
> b/include/uapi/linux/netfilter/nf_tables.h
> index 065218a20bb7..c4078359b6e4 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -508,11 +508,15 @@ enum nft_immediate_attributes {
>   *                    XOR boolean operations
>   * @NFT_BITWISE_LSHIFT: left-shift operation
>   * @NFT_BITWISE_RSHIFT: right-shift operation
> + * @NFT_BITWISE_ADD: add operation
> + * @NFT_BITWISE_SUB: subtract operation
>   */
>  enum nft_bitwise_ops {
>  	NFT_BITWISE_BOOL,
>  	NFT_BITWISE_LSHIFT,
>  	NFT_BITWISE_RSHIFT,
> +	NFT_BITWISE_ADD,
> +	NFT_BITWISE_SUB,
>  };
>
>  /**
> diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> index 0ed2281f03be..fd0cd2b4722a 100644
> --- a/net/netfilter/nft_bitwise.c
> +++ b/net/netfilter/nft_bitwise.c
> @@ -60,6 +60,38 @@ static void nft_bitwise_eval_rshift(u32 *dst, const
> u32 *src,
>  	}
>  }
>
> +static void nft_bitwise_eval_add(u32 *dst, const u32 *src,
> +				 const struct nft_bitwise *priv)
> +{
> +	u32 delta = priv->data.data[0];
> +	unsigned int i, words;
> +	u32 tmp = 0;
> +
> +	words = DIV_ROUND_UP(priv->len, sizeof(u32));
> +	for (i = 0; i < words; i++) {
> +		tmp = src[i];
> +		dst[i] = src[i] + delta;
> +		if (dst[i] < tmp && i + 1 < words)
> +			dst[i + 1]++;
> +	}
> +}

for (i = 0; i < words; i++) {
	dst[i] = src[i] + delta + tmp;
	tmp = dst[i] < src[i] ? 1 : 0;
}

> +static void nft_bitwise_eval_sub(u32 *dst, const u32 *src,
> +				 const struct nft_bitwise *priv)
> +{
> +	u32 delta = priv->data.data[0];
> +	unsigned int i, words;
> +	u32 tmp = 0;
> +
> +	words = DIV_ROUND_UP(priv->len, sizeof(u32));
> +	for (i = 0; i < words; i++) {
> +		tmp = src[i];
> +		dst[i] = src[i] - delta;
> +		if (dst[i] > tmp && i + 1 < words)
> +			dst[i + 1]--;
> +	}
> +}

for (i = 0; i < words; i++) {
	dst[i] = src[i] - delta - tmp;
	tmp = dst[i] > src[i] ? 1 : 0;
}

>  void nft_bitwise_eval(const struct nft_expr *expr,
>  		      struct nft_regs *regs, const struct nft_pktinfo *pkt)
>  {
> @@ -77,6 +109,12 @@ void nft_bitwise_eval(const struct nft_expr *expr,
>  	case NFT_BITWISE_RSHIFT:
>  		nft_bitwise_eval_rshift(dst, src, priv);
>  		break;
> +	case NFT_BITWISE_ADD:
> +		nft_bitwise_eval_add(dst, src, priv);
> +		break;
> +	case NFT_BITWISE_SUB:
> +		nft_bitwise_eval_sub(dst, src, priv);
> +		break;
>  	}
>  }
>
> @@ -129,8 +167,8 @@ static int nft_bitwise_init_bool(struct
> nft_bitwise *priv,
>  	return err;
>  }
>
> -static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> -				  const struct nlattr *const tb[])
> +static int nft_bitwise_init_data(struct nft_bitwise *priv,
> +				 const struct nlattr *const tb[])
>  {
>  	struct nft_data_desc d;
>  	int err;
> @@ -191,6 +229,8 @@ static int nft_bitwise_init(const struct nft_ctx
> *ctx,
>  		case NFT_BITWISE_BOOL:
>  		case NFT_BITWISE_LSHIFT:
>  		case NFT_BITWISE_RSHIFT:
> +		case NFT_BITWISE_ADD:
> +		case NFT_BITWISE_SUB:
>  			break;
>  		default:
>  			return -EOPNOTSUPP;
> @@ -205,7 +245,9 @@ static int nft_bitwise_init(const struct nft_ctx
> *ctx,
>  		break;
>  	case NFT_BITWISE_LSHIFT:
>  	case NFT_BITWISE_RSHIFT:
> -		err = nft_bitwise_init_shift(priv, tb);
> +	case NFT_BITWISE_ADD:
> +	case NFT_BITWISE_SUB:
> +		err = nft_bitwise_init_data(priv, tb);
>  		break;
>  	}
>
> @@ -226,8 +268,8 @@ static int nft_bitwise_dump_bool(struct sk_buff
> *skb,
>  	return 0;
>  }
>
> -static int nft_bitwise_dump_shift(struct sk_buff *skb,
> -				  const struct nft_bitwise *priv)
> +static int nft_bitwise_dump_data(struct sk_buff *skb,
> +				 const struct nft_bitwise *priv)
>  {
>  	if (nft_data_dump(skb, NFTA_BITWISE_DATA, &priv->data,
>  			  NFT_DATA_VALUE, sizeof(u32)) < 0)
> @@ -255,7 +297,9 @@ static int nft_bitwise_dump(struct sk_buff *skb,
> const struct nft_expr *expr)
>  		break;
>  	case NFT_BITWISE_LSHIFT:
>  	case NFT_BITWISE_RSHIFT:
> -		err = nft_bitwise_dump_shift(skb, priv);
> +	case NFT_BITWISE_ADD:
> +	case NFT_BITWISE_SUB:
> +		err = nft_bitwise_dump_data(skb, priv);
>  		break;
>  	}
>
Pablo Neira Ayuso Jan. 28, 2020, 10 a.m. UTC | #11
On Mon, Jan 27, 2020 at 11:13:14AM +0000, Jeremy Sowden wrote:
> On 2020-01-26, at 12:12:51 +0100, Pablo Neira Ayuso wrote:
> > I've been looking into (ab)using bitwise to implement add/sub. I would
> > like to not add nft_arith for only this, and it seems to me much of
> > your code can be reused.
> >
> > Do you think something like this would work?
> 
> Absolutely.
> 
> A couple of questions.  What's the use-case?

inc/dec ip ttl field.

> I find the combination of applying the delta to every u32 and having
> a carry curious.  Do you want to support bigendian arithmetic (i.e.,
> carrying to the left) as well?

Userspace should convert to host endianess before doing arithmetics.

> I've suggested a couple of changes below.
[...]
> > diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> > index 0ed2281f03be..fd0cd2b4722a 100644
> > --- a/net/netfilter/nft_bitwise.c
> > +++ b/net/netfilter/nft_bitwise.c
> > @@ -60,6 +60,38 @@ static void nft_bitwise_eval_rshift(u32 *dst, const
> > u32 *src,
> >  	}
> >  }
> >
> > +static void nft_bitwise_eval_add(u32 *dst, const u32 *src,
> > +				 const struct nft_bitwise *priv)
> > +{
> > +	u32 delta = priv->data.data[0];
> > +	unsigned int i, words;
> > +	u32 tmp = 0;
> > +
> > +	words = DIV_ROUND_UP(priv->len, sizeof(u32));
> > +	for (i = 0; i < words; i++) {
> > +		tmp = src[i];
> > +		dst[i] = src[i] + delta;
> > +		if (dst[i] < tmp && i + 1 < words)
> > +			dst[i + 1]++;
> > +	}
> > +}
> 
> for (i = 0; i < words; i++) {
> 	dst[i] = src[i] + delta + tmp;
> 	tmp = dst[i] < src[i] ? 1 : 0;
> }

Much simpler indeed, thanks.
Jeremy Sowden Jan. 28, 2020, 11:31 a.m. UTC | #12
On 2020-01-28, at 11:00:35 +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 27, 2020 at 11:13:14AM +0000, Jeremy Sowden wrote:
> > On 2020-01-26, at 12:12:51 +0100, Pablo Neira Ayuso wrote:
> > > I've been looking into (ab)using bitwise to implement add/sub. I
> > > would like to not add nft_arith for only this, and it seems to me
> > > much of your code can be reused.
> > >
> > > Do you think something like this would work?
> >
> > Absolutely.
> >
> > A couple of questions.  What's the use-case?
>
> inc/dec ip ttl field.

If it's just a simple addition or subtraction on one value, would
this make more sense?

        for (i = 0; i < words; i++) {
	        dst[i] = src[i] + delta;
	        delta = dst[i] < src[i] ? 1 : 0;
        }

> > I find the combination of applying the delta to every u32 and having
> > a carry curious.  Do you want to support bigendian arithmetic (i.e.,
> > carrying to the left) as well?
>
> Userspace should convert to host endianess before doing arithmetics.

Yes, but if the host is bigendian, the least significant bytes will be
on the right, and we need to carry to the left, don't we?

        for (i = words; i > 0; i--) {
	        dst[i - 1] = src[i - 1] + delta;
	        delta = dst[i - 1] < src[i - 1] ? 1 : 0;
        }

J.
Pablo Neira Ayuso Jan. 28, 2020, 1:18 p.m. UTC | #13
On Tue, Jan 28, 2020 at 11:31:39AM +0000, Jeremy Sowden wrote:
> On 2020-01-28, at 11:00:35 +0100, Pablo Neira Ayuso wrote:
> > On Mon, Jan 27, 2020 at 11:13:14AM +0000, Jeremy Sowden wrote:
> > > On 2020-01-26, at 12:12:51 +0100, Pablo Neira Ayuso wrote:
> > > > I've been looking into (ab)using bitwise to implement add/sub. I
> > > > would like to not add nft_arith for only this, and it seems to me
> > > > much of your code can be reused.
> > > >
> > > > Do you think something like this would work?
> > >
> > > Absolutely.
> > >
> > > A couple of questions.  What's the use-case?
> >
> > inc/dec ip ttl field.
> 
> If it's just a simple addition or subtraction on one value, would
> this make more sense?
> 
>         for (i = 0; i < words; i++) {
> 	        dst[i] = src[i] + delta;
> 	        delta = dst[i] < src[i] ? 1 : 0;
>         }

This can be done through _INC / _DEC instead, however...

> > > I find the combination of applying the delta to every u32 and having
> > > a carry curious.  Do you want to support bigendian arithmetic (i.e.,
> > > carrying to the left) as well?
> >
> > Userspace should convert to host endianess before doing arithmetics.
> 
> Yes, but if the host is bigendian, the least significant bytes will be
> on the right, and we need to carry to the left, don't we?
> 
>         for (i = words; i > 0; i--) {
> 	        dst[i - 1] = src[i - 1] + delta;
> 	        delta = dst[i - 1] < src[i - 1] ? 1 : 0;
>         }

I think some simplified version of bignum add/subtract is needed,
something like:

        for (i = len - 1; i >= 0; i--) {
                res[i] = a[i] + b[i] + carry;
                carry = res[i] < a[i] + b[i];
        }

where 'len' is in bytes. Values in a[] and b[] are u8 and data is
represented in big endian.