diff mbox series

[PATCHv2,net-next,3/5] net: sched: return NF_ACCEPT when fails to add nat ext in tcf_ct_act_nat

Message ID 439676c5242282638057f92dc51314df7bcd0a73.1669138256.git.lucien.xin@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: eliminate the duplicate code in the ct nat functions of ovs and tc | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
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: 0 this patch: 0
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

Xin Long Nov. 22, 2022, 5:32 p.m. UTC 
This patch changes to return NF_ACCEPT when fails to add nat
ext before doing NAT in tcf_ct_act_nat(), to keep consistent
with OVS' processing in ovs_ct_nat().

Reviewed-by: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sched/act_ct.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marcelo Ricardo Leitner Nov. 23, 2022, 2:23 p.m. UTC | #1 
On Tue, Nov 22, 2022 at 12:32:19PM -0500, Xin Long wrote:
> This patch changes to return NF_ACCEPT when fails to add nat
> ext before doing NAT in tcf_ct_act_nat(), to keep consistent
> with OVS' processing in ovs_ct_nat().
> 
> Reviewed-by: Saeed Mahameed <saeed@kernel.org>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sched/act_ct.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index da0b7f665277..8869b3ef6642 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -994,7 +994,7 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
>  
>  	/* Add NAT extension if not confirmed yet. */
>  	if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
> -		return NF_DROP;   /* Can't NAT. */
> +		return NF_ACCEPT;   /* Can't NAT. */

I'm wondering if the fix should actually be in OVS, to make it drop
the packet? Aaron, Eelco?

If the user asked for NAT, and it can't NAT, it doesn't seem right to
forward the packet while not performing the asked action.

If we follow the code here, it may even commit the entry without the
NAT extension, rendering the connection useless/broken per the first
if condition above. It just won't try again.

>  
>  	if (ctinfo != IP_CT_NEW && (ct->status & IPS_NAT_MASK) &&
>  	    (ctinfo != IP_CT_RELATED || commit)) {
> -- 
> 2.31.1
>
Xin Long Dec. 1, 2022, 4:53 p.m. UTC | #2 
On Wed, Nov 23, 2022 at 9:24 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Tue, Nov 22, 2022 at 12:32:19PM -0500, Xin Long wrote:
> > This patch changes to return NF_ACCEPT when fails to add nat
> > ext before doing NAT in tcf_ct_act_nat(), to keep consistent
> > with OVS' processing in ovs_ct_nat().
> >
> > Reviewed-by: Saeed Mahameed <saeed@kernel.org>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sched/act_ct.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > index da0b7f665277..8869b3ef6642 100644
> > --- a/net/sched/act_ct.c
> > +++ b/net/sched/act_ct.c
> > @@ -994,7 +994,7 @@ static int tcf_ct_act_nat(struct sk_buff *skb,
> >
> >       /* Add NAT extension if not confirmed yet. */
> >       if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
> > -             return NF_DROP;   /* Can't NAT. */
> > +             return NF_ACCEPT;   /* Can't NAT. */
>
> I'm wondering if the fix should actually be in OVS, to make it drop
> the packet? Aaron, Eelco?
>
> If the user asked for NAT, and it can't NAT, it doesn't seem right to
> forward the packet while not performing the asked action.
>
> If we follow the code here, it may even commit the entry without the
> NAT extension, rendering the connection useless/broken per the first
> if condition above. It just won't try again.
nf_ct_nat_ext_add() returning NULL is caused by krealloc() failure
like an -ENOMEM error, and a similar thing could happen in
nfct_seqadj_ext_add() called by ovs_ct_nat() -> nf_nat_setup_info()
when doing NAT where it returns DROP. So I think it's right that
we should fix this in openvswitch instead of TC.

Anyway, in ovs_ct_nat():

        if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
                return NF_ACCEPT;   /* Can't NAT. */

git blame shows this was added at the beginning by:

    05752523e565 ("openvswitch: Interface with NAT.")

So add Jarno Rajahalme to Cc: list.

Thanks.

>
> >
> >       if (ctinfo != IP_CT_NEW && (ct->status & IPS_NAT_MASK) &&
> >           (ctinfo != IP_CT_RELATED || commit)) {
> > --
> > 2.31.1
> >
diff mbox series

Patch

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index da0b7f665277..8869b3ef6642 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -994,7 +994,7 @@  static int tcf_ct_act_nat(struct sk_buff *skb,
 
 	/* Add NAT extension if not confirmed yet. */
 	if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
-		return NF_DROP;   /* Can't NAT. */
+		return NF_ACCEPT;   /* Can't NAT. */
 
 	if (ctinfo != IP_CT_NEW && (ct->status & IPS_NAT_MASK) &&
 	    (ctinfo != IP_CT_RELATED || commit)) {