diff mbox series

[RFC] netfilter: nf_tables: ignore -EOPNOTSUPP on flowtable device offload setup

Message ID 20230831201420.63178-1-nbd@nbd.name (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC] netfilter: nf_tables: ignore -EOPNOTSUPP on flowtable device offload setup | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1346 this patch: 1346
netdev/cc_maintainers warning 7 maintainers not CCed: kuba@kernel.org fw@strlen.de coreteam@netfilter.org davem@davemloft.net kadlec@netfilter.org pabeni@redhat.com edumazet@google.com
netdev/build_clang success Errors and warnings before: 1353 this patch: 1353
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1369 this patch: 1369
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Felix Fietkau Aug. 31, 2023, 8:14 p.m. UTC
On many embedded devices, it is common to configure flowtable offloading for
a mix of different devices, some of which have hardware offload support and
some of which don't.
The current code limits the ability of user space to properly set up such a
configuration by only allowing adding devices with hardware offload support to
a offload-enabled flowtable.
Given that offload-enabled flowtables also imply fallback to pure software
offloading, this limitation makes little sense.
Fix it by not bailing out when the offload setup returns -EOPNOTSUPP

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Pablo Neira Ayuso Sept. 1, 2023, 8:39 a.m. UTC | #1
Hi Felix,

On Thu, Aug 31, 2023 at 10:14:20PM +0200, Felix Fietkau wrote:
> On many embedded devices, it is common to configure flowtable offloading for
> a mix of different devices, some of which have hardware offload support and
> some of which don't.
> The current code limits the ability of user space to properly set up such a
> configuration by only allowing adding devices with hardware offload support to
> a offload-enabled flowtable.
> Given that offload-enabled flowtables also imply fallback to pure software
> offloading, this limitation makes little sense.
> Fix it by not bailing out when the offload setup returns -EOPNOTSUPP

Would you send a v2 to untoggle the offload flag when listing the
ruleset if EOPNOTSUPP is reported? Thus, the user knows that no
hardware offload is being used.

> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  net/netfilter/nf_tables_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 41b826dff6f5..dfa2ea98088b 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -8103,7 +8103,7 @@ static int nft_register_flowtable_net_hooks(struct net *net,
>  		err = flowtable->data.type->setup(&flowtable->data,
>  						  hook->ops.dev,
>  						  FLOW_BLOCK_BIND);
> -		if (err < 0)
> +		if (err < 0 && err != -EOPNOTSUPP)
>  			goto err_unregister_net_hooks;
>  
>  		err = nf_register_net_hook(net, &hook->ops);
> -- 
> 2.41.0
>
Felix Fietkau Sept. 1, 2023, 10:30 a.m. UTC | #2
On 01.09.23 10:39, Pablo Neira Ayuso wrote:
> Hi Felix,
> 
> On Thu, Aug 31, 2023 at 10:14:20PM +0200, Felix Fietkau wrote:
>> On many embedded devices, it is common to configure flowtable offloading for
>> a mix of different devices, some of which have hardware offload support and
>> some of which don't.
>> The current code limits the ability of user space to properly set up such a
>> configuration by only allowing adding devices with hardware offload support to
>> a offload-enabled flowtable.
>> Given that offload-enabled flowtables also imply fallback to pure software
>> offloading, this limitation makes little sense.
>> Fix it by not bailing out when the offload setup returns -EOPNOTSUPP
> 
> Would you send a v2 to untoggle the offload flag when listing the
> ruleset if EOPNOTSUPP is reported? Thus, the user knows that no
> hardware offload is being used.

Wouldn't that mess up further updates to the flowtable? From what I can 
tell, when updating a flow table, changing its offload flag is not 
supported.

- Felix
Pablo Neira Ayuso Sept. 1, 2023, 12:29 p.m. UTC | #3
On Fri, Sep 01, 2023 at 12:30:37PM +0200, Felix Fietkau wrote:
> On 01.09.23 10:39, Pablo Neira Ayuso wrote:
> > Hi Felix,
> > 
> > On Thu, Aug 31, 2023 at 10:14:20PM +0200, Felix Fietkau wrote:
> > > On many embedded devices, it is common to configure flowtable offloading for
> > > a mix of different devices, some of which have hardware offload support and
> > > some of which don't.
> > > The current code limits the ability of user space to properly set up such a
> > > configuration by only allowing adding devices with hardware offload support to
> > > a offload-enabled flowtable.
> > > Given that offload-enabled flowtables also imply fallback to pure software
> > > offloading, this limitation makes little sense.
> > > Fix it by not bailing out when the offload setup returns -EOPNOTSUPP
> > 
> > Would you send a v2 to untoggle the offload flag when listing the
> > ruleset if EOPNOTSUPP is reported? Thus, the user knows that no
> > hardware offload is being used.
> 
> Wouldn't that mess up further updates to the flowtable? From what I can
> tell, when updating a flow table, changing its offload flag is not
> supported.

The flag would be untoggled if hardware offload is not supported. What
problematic scenario are you having in mind that might break?

In any case, there is a need to provide a way to tell the user if the
hardware offload is actually happening or not, if not what I suggest,
then propose a different way. But user really needs to know if it runs
software or hardware plane to debug issues.
Felix Fietkau Sept. 1, 2023, 12:47 p.m. UTC | #4
On 01.09.23 14:29, Pablo Neira Ayuso wrote:
> On Fri, Sep 01, 2023 at 12:30:37PM +0200, Felix Fietkau wrote:
>> On 01.09.23 10:39, Pablo Neira Ayuso wrote:
>> > Hi Felix,
>> > 
>> > On Thu, Aug 31, 2023 at 10:14:20PM +0200, Felix Fietkau wrote:
>> > > On many embedded devices, it is common to configure flowtable offloading for
>> > > a mix of different devices, some of which have hardware offload support and
>> > > some of which don't.
>> > > The current code limits the ability of user space to properly set up such a
>> > > configuration by only allowing adding devices with hardware offload support to
>> > > a offload-enabled flowtable.
>> > > Given that offload-enabled flowtables also imply fallback to pure software
>> > > offloading, this limitation makes little sense.
>> > > Fix it by not bailing out when the offload setup returns -EOPNOTSUPP
>> > 
>> > Would you send a v2 to untoggle the offload flag when listing the
>> > ruleset if EOPNOTSUPP is reported? Thus, the user knows that no
>> > hardware offload is being used.
>> 
>> Wouldn't that mess up further updates to the flowtable? From what I can
>> tell, when updating a flow table, changing its offload flag is not
>> supported.
> 
> The flag would be untoggled if hardware offload is not supported. What
> problematic scenario are you having in mind that might break?

The scenario I'm thinking about is this:
Initially, the flowtable is created with a set of devices which don't 
support offload.
Afterwards, the flowtable gets updated with the intention of adding an 
extra device which *does* support hw offload to the existing flowtable.
If the flag was cleared after initially creating the table, I think the 
update would fail. Or did I misread the code?

> In any case, there is a need to provide a way to tell the user if the
> hardware offload is actually happening or not, if not what I suggest,
> then propose a different way. But user really needs to know if it runs
> software or hardware plane to debug issues.

In my opinion, a single flag indication for the flow table is mostly 
useless. Much more useful would be if you could query which of the 
devices that were added to the flowtable support hw offload and which 
ones don't. That requires some API changes though, and I don't think 
that should be done in this patch.

- Felix
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 41b826dff6f5..dfa2ea98088b 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8103,7 +8103,7 @@  static int nft_register_flowtable_net_hooks(struct net *net,
 		err = flowtable->data.type->setup(&flowtable->data,
 						  hook->ops.dev,
 						  FLOW_BLOCK_BIND);
-		if (err < 0)
+		if (err < 0 && err != -EOPNOTSUPP)
 			goto err_unregister_net_hooks;
 
 		err = nf_register_net_hook(net, &hook->ops);