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

[Florian Westphal <fw@strlen.de>]

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.