[conntrack-tools PATCH] conntrack: Don't override mark in non-list mode

[conntrack-tools PATCH] conntrack: Don't override mark in non-list mode

[Jacek Tomasiak]

When creating new rules with (e.g. with `conntrack -I -m 123 -u UNSET ...`),
the mark from `-m` was overriden by value from `-u`. Additional
condition ensures that this happens only in list mode.

This behavior was introduced in 1a5828f491c6a1593f30cb5f1551fe9f9cf76a8d
("conntrack: enable kernel-based status filtering with -L -u STATUS") for
filtering the output of `-L` option but caused a regression in other cases.

Signed-off-by: Jacek Tomasiak <jtomasiak@arista.com>
Signed-off-by: Jacek Tomasiak <jacek.tomasiak@gmail.com>
---
 src/conntrack.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index bf72739..78d3a07 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -3007,7 +3007,9 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
 			if (tmpl->filter_status_kernel.mask == 0)
 				tmpl->filter_status_kernel.mask = status;
 
-			tmpl->mark.value = status;
+			// set mark only in list mode to not override value from -m
+			if (command & CT_LIST)
+				tmpl->mark.value = status;
 			tmpl->filter_status_kernel.val = tmpl->mark.value;
 			tmpl->filter_status_kernel_set = true;
 			break;
-- 
2.35.3

Re: [conntrack-tools PATCH] conntrack: Don't override mark in non-list mode

[Pablo Neira Ayuso]

Hi,

Cc'ing Florian.

On Wed, Jun 14, 2023 at 06:24:05PM +0200, Jacek Tomasiak wrote:
> When creating new rules with (e.g. with `conntrack -I -m 123 -u UNSET ...`),
> the mark from `-m` was overriden by value from `-u`. Additional
> condition ensures that this happens only in list mode.
> 
> This behavior was introduced in 1a5828f491c6a1593f30cb5f1551fe9f9cf76a8d
> ("conntrack: enable kernel-based status filtering with -L -u STATUS") for
> filtering the output of `-L` option but caused a regression in other cases.

In 1a5828f491c6a:

          tmpl->mark.value = status;
          tmpl->filter_status_kernel.val = tmpl->mark.value;

Not sure what the mark has to do this the -L -u STATUS filtering.

> Signed-off-by: Jacek Tomasiak <jtomasiak@arista.com>
> Signed-off-by: Jacek Tomasiak <jacek.tomasiak@gmail.com>
> ---
>  src/conntrack.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conntrack.c b/src/conntrack.c
> index bf72739..78d3a07 100644
> --- a/src/conntrack.c
> +++ b/src/conntrack.c
> @@ -3007,7 +3007,9 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  			if (tmpl->filter_status_kernel.mask == 0)
>  				tmpl->filter_status_kernel.mask = status;
>  
> -			tmpl->mark.value = status;
> +			// set mark only in list mode to not override value from -m
> +			if (command & CT_LIST)
> +				tmpl->mark.value = status;

The existing code also means that -L -u STATUS cannot be combined with
-L -m mark, right?

>  			tmpl->filter_status_kernel.val = tmpl->mark.value;
>  			tmpl->filter_status_kernel_set = true;
>  			break;
> -- 
> 2.35.3
> 

Re: [conntrack-tools PATCH] conntrack: Don't override mark in non-list mode

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi,
> 
> Cc'ing Florian.
> 
> On Wed, Jun 14, 2023 at 06:24:05PM +0200, Jacek Tomasiak wrote:
> > When creating new rules with (e.g. with `conntrack -I -m 123 -u UNSET ...`),
> > the mark from `-m` was overriden by value from `-u`. Additional
> > condition ensures that this happens only in list mode.
> > 
> > This behavior was introduced in 1a5828f491c6a1593f30cb5f1551fe9f9cf76a8d
> > ("conntrack: enable kernel-based status filtering with -L -u STATUS") for
> > filtering the output of `-L` option but caused a regression in other cases.
> 
> In 1a5828f491c6a:
> 
>           tmpl->mark.value = status;
>           tmpl->filter_status_kernel.val = tmpl->mark.value;
> 
> Not sure what the mark has to do this the -L -u STATUS filtering.
> 
> > Signed-off-by: Jacek Tomasiak <jtomasiak@arista.com>
> > Signed-off-by: Jacek Tomasiak <jacek.tomasiak@gmail.com>
> > ---
> >  src/conntrack.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/conntrack.c b/src/conntrack.c
> > index bf72739..78d3a07 100644
> > --- a/src/conntrack.c
> > +++ b/src/conntrack.c
> > @@ -3007,7 +3007,9 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
> >  			if (tmpl->filter_status_kernel.mask == 0)
> >  				tmpl->filter_status_kernel.mask = status;
> >  
> > -			tmpl->mark.value = status;
> > +			// set mark only in list mode to not override value from -m
> > +			if (command & CT_LIST)
> > +				tmpl->mark.value = status;

This should be

-                       tmpl->mark.value = status;
-                       tmpl->filter_status_kernel.val = tmpl->mark.value;
+                       tmpl->filter_status_kernel.val = status;
                        tmpl->filter_status_kernel_set = true;

> The existing code also means that -L -u STATUS cannot be combined with
> -L -m mark, right?

This is a bug.

conntrack -L -u status -m 1 will work, -m 1 -u status won't work.

The clobbering of mark utterly bogus.

Re: [conntrack-tools PATCH] conntrack: Don't override mark in non-list mode

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> This should be
> 
> -                       tmpl->mark.value = status;
> -                       tmpl->filter_status_kernel.val = tmpl->mark.value;
> +                       tmpl->filter_status_kernel.val = status;
>                         tmpl->filter_status_kernel_set = true;

I've pushed Jaceks patch with the content amended as above.

Jan, please report if that doesn't resolve the bug you reported.

Thanks everyone.