Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH nf] Revert "netfilter: flowtable: Remove redundant hw
 refresh bit"
To:     Pablo Neira Ayuso <pablo@netfilter.org>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc:     coreteam@netfilter.org, davem@davemloft.net, fw@strlen.de,
        kadlec@netfilter.org, kuba@kernel.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        netfilter-devel@vger.kernel.org, olek2@wp.pl, roid@nvidia.com
References: <20210614215351.GA734@salvia>
 <20210711010244.1709329-1-martin.blumenstingl@googlemail.com>
 <20210712094652.GA6320@salvia>
From:   Felix Fietkau <nbd@nbd.name>
Message-ID: <f9f01b3b-f6ab-ef82-6604-5dd8e925abea@nbd.name>
Date:   Mon, 12 Jul 2021 12:18:26 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210712094652.GA6320@salvia>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2021-07-12 11:46, Pablo Neira Ayuso wrote:
> Maybe the user reporting this issue is enabling the --hw option?
> As I said, the patch that is being proposed to be revert is just
> amplifying.
> 
> The only way to trigger this bug that I can find is:
> 
> - NF_FLOWTABLE_HW_OFFLOAD is enabled.
> - packets are following the software path.
> 
> I don't see yet how this can happen with upstream codebase, nftables
> enables NF_FLOWTABLE_HW_OFFLOAD at configuration time, if the driver
> does not support for hardware offload, then NF_FLOWTABLE_HW_OFFLOAD is
> not set.
> 
> Is xt_flowoffload rejecting the rule load if user specifies --hw and
> the hardware does not support for hardware offload?
> 
> By reading Felix's discussion on the IRC, it seems to me he does not
> like that the packet path retries to offload flows. If so, it should
> be possible to add a driver flag to disable this behaviour, so driver
> developers select what they prefer that flowtable core retries to
> offload entries. I can have a look into adding such flag and use it
> from the mtk driver.
I'd prefer making the retry behavior depend on the error code during
setup. For example, if we get -ENOMEM, -EAGAIN or something like that,
we should definitely retry.
If we get -EOPNOTSUPP or -EINVAL, I don't think a retry makes any sense
on any driver.

- Felix