Message-ID: <YqA7lOw8CKNMaQ28@salvia>
Date:   Wed, 8 Jun 2022 08:03:00 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org, pabeni@...hat.com, edumazet@...gle.com
Subject: Re: [PATCH net 7/7] netfilter: nf_tables: bail out early if hardware
 offload is not supported

Hi Jakub,

On Tue, Jun 07, 2022 at 06:00:25PM -0700, Jakub Kicinski wrote:
> On Mon,  6 Jun 2022 23:20:55 +0200 Pablo Neira Ayuso wrote:
> > If user requests for NFT_CHAIN_HW_OFFLOAD, then check if either device
> > provides the .ndo_setup_tc interface or there is an indirect flow block
> > that has been registered. Otherwise, bail out early from the preparation
> > phase. Moreover, validate that family == NFPROTO_NETDEV and hook is
> > NF_NETDEV_INGRESS.
> 
> The whole series is pretty light on the "why".

  - [net,1/7] netfilter: nat: really support inet nat without l3 address
    https://git.kernel.org/netdev/net/c/282e5f8fe907

  This is a fix, otherwise NAT with the inet family (which allows both
  IPv4 and IPv6 traffic) remains broken. It's a datapath fix, the
  control plane was accepting the rule, however NAT was not applied if
  user specified no layer 4 address, which might happen for, eg. redirect.

  - [net,2/7] netfilter: nf_tables: use kfree_rcu(ptr, rcu) to release hooks in clean_net path
    https://git.kernel.org/netdev/net/c/ab5e5c062f67

  This is an incremental fix for f9a43007d3f7 ("netfilter: nf_tables:
  double hook unregistration in netns path"), it is using kfree_rcu(ptr)
  variant which works but it has some limitations. Use of free_rcu(ptr)
  was not intentional, hence free_rcu(ptr, rcu)

  - [net,3/7] netfilter: nf_tables: delete flowtable hooks via transaction list
    https://git.kernel.org/netdev/net/c/b6d9014a3335

  Deleting twice the same device on the flowtable might lead to ENOENT
  since hook->inactive is not honored. Instead of honoring such flag,
  this patch is fixing up this by using a flowtable hook list in the
  transaction object to convey the hook that are going to be deleted
  which looks cleaner to me.

  - [net,4/7] netfilter: nf_tables: always initialize flowtable hook list in transaction
    https://git.kernel.org/netdev/net/c/2c9e4559773c

  This is a oneliner, not urgent but Florian already reported in the
  past that the flowtable hook list in the transaction object was not
  initialized (even if not used). This patch initializes it to
  increase robustness, this list is going to be empty/unused for the
  non-update path anyway. Arguably I could have postpone this
  oneliner.

  - [net,5/7] netfilter: nf_tables: release new hooks on unsupported flowtable flags
    https://git.kernel.org/netdev/net/c/c271cc9febaa

  This is a fix. nft_flowtable_parse_hook() populates the hook list,
  but the flowtable flags update logic was not releasing these objects
  from the error path, hence, leading to a memleak.

  - [net,6/7] netfilter: nf_tables: memleak flow rule from commit path
    https://git.kernel.org/netdev/net/c/9dd732e0bdf5

  kmemleak reported this memleak while running a series of test with
  nf_tables hardware offload support for these objects, this is a fix.

> This patch is particularly bad, no idea what the user visible bug
> was here.

  Are you refering to this?

  - [net,7/7] netfilter: nf_tables: bail out early if hardware offload is not supported
    https://git.kernel.org/netdev/net/c/3a41c64d9c11

  Arguably, I could have postponed this patch, but quite recently
  there was a silly bug in the hardware offload infrastructure, see
  b1a5983f56e3 ("netfilter: nf_tables_offload: incorrect flow offload
  action array size. The reporter triggered the bug with the _loopback
  interface_, he wondered why this infrastructure is exposed to all
  devices while only a dozen of NICs support hardware offload, hence
  this patch to disable hardware offload earlier in the control plane
  path.

Powered by blists - more mailing lists