[PATCH conntrack] conntrack: label update requires a previous label in place

[PATCH conntrack] conntrack: label update requires a previous label in place

[Pablo Neira Ayuso]

You have to set an initial label if you plan to update it later on.  If
conntrack comes with no initial label, then it is not possible to attach
it later because conntrack extensions are created by the time the new
entry is created.

Skip entries with no label to skip ENOSPC error for conntracks that have
no initial label (this is assuming a scenario with conntracks with and
_without_ labels is possible, and the conntrack command line tool is used
to update all entries regardless they have or not an initial label, e.g.
conntrack -U --label-add "testlabel".

 # conntrack -U --label-add testlabel --dst 9.9.9.9
 icmp     1 13 src=192.168.2.130 dst=9.9.9.9 type=8 code=0 id=50997 src=9.9.9.9 dst=192.168.2.130 type=0 code=0 id=50997 mark=0 use=2 labels=default,testlabel
conntrack v1.4.8 (conntrack-tools): 1 flow entries have been updated.
 # conntrack -C
 8

Note the remaining 7 conntracks have no label, hence, they could not be
updated.

Update manpage to document this behaviour.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1622
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 conntrack.8     | 2 ++
 src/conntrack.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/conntrack.8 b/conntrack.8
index 031eaa4e9fef..97c60079889f 100644
--- a/conntrack.8
+++ b/conntrack.8
@@ -193,6 +193,8 @@ Use multiple \-l options to specify multiple labels that need to be set.
 Specify the conntrack label to add to the selected conntracks.
 This option is only available in conjunction with "\-I, \-\-create",
 "\-A, \-\-add" or "\-U, \-\-update".
+You must set a default label for conntracks initially if you plan to update it
+later. "\-U, \-\-update" on conntracks with no initial entry will be ignored.
 .TP
 .BI "--label-del " "[LABEL]"
 Specify the conntrack label to delete from the selected conntracks.
diff --git a/src/conntrack.c b/src/conntrack.c
index f9758d78d39b..06c2fee7ac4b 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -2195,6 +2195,10 @@ static int mnl_nfct_update_cb(const struct nlmsghdr *nlh, void *data)
 		/* the entry has vanish in middle of the update */
 		if (errno == ENOENT)
 			goto destroy_ok;
+		else if (!(cmd->options & (CT_OPT_ADD_LABEL | CT_OPT_DEL_LABEL)) &&
+			 errno == ENOSPC)
+			goto destroy_ok;
+
 		exit_error(OTHER_PROBLEM,
 			   "Operation failed: %s",
 			   err2str(errno, CT_UPDATE));
-- 
2.30.2

Re: [PATCH conntrack] conntrack: label update requires a previous label in place

[Pablo Neira Ayuso]

On Wed, Oct 11, 2023 at 11:55:03AM +0200, Pablo Neira Ayuso wrote:
> You have to set an initial label if you plan to update it later on.  If
> conntrack comes with no initial label, then it is not possible to attach
> it later because conntrack extensions are created by the time the new
> entry is created.
> 
> Skip entries with no label to skip ENOSPC error for conntracks that have
> no initial label (this is assuming a scenario with conntracks with and
> _without_ labels is possible, and the conntrack command line tool is used
> to update all entries regardless they have or not an initial label, e.g.
> conntrack -U --label-add "testlabel".

Still not fully correct.

Current behaviour is:

If there is at least one rule in the ruleset that uses the connlabel,
then connlabel conntrack extension is always allocated.

I wonder if this needs a sysctl toggle just like
nf_conntrack_timestamp. Otherwise I am not sure how to document this.

>  # conntrack -U --label-add testlabel --dst 9.9.9.9
>  icmp     1 13 src=192.168.2.130 dst=9.9.9.9 type=8 code=0 id=50997 src=9.9.9.9 dst=192.168.2.130 type=0 code=0 id=50997 mark=0 use=2 labels=default,testlabel
> conntrack v1.4.8 (conntrack-tools): 1 flow entries have been updated.
>  # conntrack -C
>  8
> 
> Note the remaining 7 conntracks have no label, hence, they could not be
> updated.
> 
> Update manpage to document this behaviour.
> 
> Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1622
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  conntrack.8     | 2 ++
>  src/conntrack.c | 4 ++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/conntrack.8 b/conntrack.8
> index 031eaa4e9fef..97c60079889f 100644
> --- a/conntrack.8
> +++ b/conntrack.8
> @@ -193,6 +193,8 @@ Use multiple \-l options to specify multiple labels that need to be set.
>  Specify the conntrack label to add to the selected conntracks.
>  This option is only available in conjunction with "\-I, \-\-create",
>  "\-A, \-\-add" or "\-U, \-\-update".
> +You must set a default label for conntracks initially if you plan to update it
> +later. "\-U, \-\-update" on conntracks with no initial entry will be ignored.
>  .TP
>  .BI "--label-del " "[LABEL]"
>  Specify the conntrack label to delete from the selected conntracks.
> diff --git a/src/conntrack.c b/src/conntrack.c
> index f9758d78d39b..06c2fee7ac4b 100644
> --- a/src/conntrack.c
> +++ b/src/conntrack.c
> @@ -2195,6 +2195,10 @@ static int mnl_nfct_update_cb(const struct nlmsghdr *nlh, void *data)
>  		/* the entry has vanish in middle of the update */
>  		if (errno == ENOENT)
>  			goto destroy_ok;
> +		else if (!(cmd->options & (CT_OPT_ADD_LABEL | CT_OPT_DEL_LABEL)) &&
> +			 errno == ENOSPC)

This check is also not correct, this needs a v3. I have to check is
ATTRS_CONNLABEL is set and cmd->options & (CT_OPT_ADD_LABEL |
CT_OPT_DEL_LABEL) too, then check for ENOSPC, to avoid for bogus
error reports to userspace.

> +			goto destroy_ok;
> +
>  		exit_error(OTHER_PROBLEM,
>  			   "Operation failed: %s",
>  			   err2str(errno, CT_UPDATE));
> -- 
> 2.30.2
> 

Re: [PATCH conntrack] conntrack: label update requires a previous label in place

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Oct 11, 2023 at 11:55:03AM +0200, Pablo Neira Ayuso wrote:
> > You have to set an initial label if you plan to update it later on.  If
> > conntrack comes with no initial label, then it is not possible to attach
> > it later because conntrack extensions are created by the time the new
> > entry is created.
> > 
> > Skip entries with no label to skip ENOSPC error for conntracks that have
> > no initial label (this is assuming a scenario with conntracks with and
> > _without_ labels is possible, and the conntrack command line tool is used
> > to update all entries regardless they have or not an initial label, e.g.
> > conntrack -U --label-add "testlabel".
> 
> Still not fully correct.
> 
> Current behaviour is:
> 
> If there is at least one rule in the ruleset that uses the connlabel,
> then connlabel conntrack extension is always allocated.
> 
> I wonder if this needs a sysctl toggle just like
> nf_conntrack_timestamp. Otherwise I am not sure how to document this.

Rationale was that if you have no rules that check on labels then
there is never a need to allocate the space.

I'm working on a patchset that will also set/enable the label
extension if its enabled on the template. The idea is to convert
ovs and act_ct to it, currently they point-blank increment
net->ct.labels_used which means that all conntrack objects get the
label area allocated.

But thats not what the counter was (originally) meant to convey, it
was really 'number of connlabel rules'.

As soon as act_ct or ovs modules are loaded, then all the namespaces
see 'I need conntrack labels', which completely voids all attempts to
avoid ct->ext allocation.

Re: [PATCH conntrack] conntrack: label update requires a previous label in place

[Pablo Neira Ayuso]

On Wed, Oct 11, 2023 at 01:10:29PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Oct 11, 2023 at 11:55:03AM +0200, Pablo Neira Ayuso wrote:
> > > You have to set an initial label if you plan to update it later on.  If
> > > conntrack comes with no initial label, then it is not possible to attach
> > > it later because conntrack extensions are created by the time the new
> > > entry is created.
> > > 
> > > Skip entries with no label to skip ENOSPC error for conntracks that have
> > > no initial label (this is assuming a scenario with conntracks with and
> > > _without_ labels is possible, and the conntrack command line tool is used
> > > to update all entries regardless they have or not an initial label, e.g.
> > > conntrack -U --label-add "testlabel".
> > 
> > Still not fully correct.
> > 
> > Current behaviour is:
> > 
> > If there is at least one rule in the ruleset that uses the connlabel,
> > then connlabel conntrack extension is always allocated.
> > 
> > I wonder if this needs a sysctl toggle just like
> > nf_conntrack_timestamp. Otherwise I am not sure how to document this.
> 
> Rationale was that if you have no rules that check on labels then
> there is never a need to allocate the space.
> 
> I'm working on a patchset that will also set/enable the label
> extension if its enabled on the template. The idea is to convert
> ovs and act_ct to it, currently they point-blank increment
> net->ct.labels_used which means that all conntrack objects get the
> label area allocated.
> 
> But thats not what the counter was (originally) meant to convey, it
> was really 'number of connlabel rules'.

> As soon as act_ct or ovs modules are loaded, then all the namespaces
> see 'I need conntrack labels', which completely voids all attempts to
> avoid ct->ext allocation.

OK, so instead a of per-netns sysctl toggle, you propose to use the
conntrack template to selectively enable this.

Re: [PATCH conntrack] conntrack: label update requires a previous label in place

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Rationale was that if you have no rules that check on labels then
> > there is never a need to allocate the space.
> > 
> > I'm working on a patchset that will also set/enable the label
> > extension if its enabled on the template. The idea is to convert
> > ovs and act_ct to it, currently they point-blank increment
> > net->ct.labels_used which means that all conntrack objects get the
> > label area allocated.
> > 
> > But thats not what the counter was (originally) meant to convey, it
> > was really 'number of connlabel rules'.
> 
> > As soon as act_ct or ovs modules are loaded, then all the namespaces
> > see 'I need conntrack labels', which completely voids all attempts to
> > avoid ct->ext allocation.
> 
> OK, so instead a of per-netns sysctl toggle, you propose to use the
> conntrack template to selectively enable this.

I think for iptables/nftables current approach is fine.

Otherwise someone has to explain to me what the use case is for
setting connlabels from netlink but no rules in place that make
any decision based on that.

Re: [PATCH conntrack] conntrack: label update requires a previous label in place

[Pablo Neira Ayuso]

On Wed, Oct 11, 2023 at 04:00:07PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Rationale was that if you have no rules that check on labels then
> > > there is never a need to allocate the space.
> > > 
> > > I'm working on a patchset that will also set/enable the label
> > > extension if its enabled on the template. The idea is to convert
> > > ovs and act_ct to it, currently they point-blank increment
> > > net->ct.labels_used which means that all conntrack objects get the
> > > label area allocated.
> > > 
> > > But thats not what the counter was (originally) meant to convey, it
> > > was really 'number of connlabel rules'.
> > 
> > > As soon as act_ct or ovs modules are loaded, then all the namespaces
> > > see 'I need conntrack labels', which completely voids all attempts to
> > > avoid ct->ext allocation.
> > 
> > OK, so instead a of per-netns sysctl toggle, you propose to use the
> > conntrack template to selectively enable this.
> 
> I think for iptables/nftables current approach is fine.
> 
> Otherwise someone has to explain to me what the use case is for
> setting connlabels from netlink but no rules in place that make
> any decision based on that.

Agreed.

I don't need this myself. I just found this bugzilla ticket while
reviewing recent reports. I think my patch for conntrack command line
is fine by now (documentation plus deal with corner case that triggers
ENOSPC from update path).

So yes, agreed, if anyone has a use case for no rules while having
connlabel, they should come here and explain.

[PATCH conntrack] conntrack: label update requires a previous label in place

[Pablo Neira Ayuso]

You have to set an initial label if you plan to update it later on.  If
conntrack comes with no initial label, then it is not possible to attach
it later because conntrack extensions are created by the time the new
entry is created.

Update manpage to document this behaviour.

Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1622
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 conntrack.8 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/conntrack.8 b/conntrack.8
index 031eaa4e9fef..f3610b15d4a6 100644
--- a/conntrack.8
+++ b/conntrack.8
@@ -193,6 +193,9 @@ Use multiple \-l options to specify multiple labels that need to be set.
 Specify the conntrack label to add to the selected conntracks.
 This option is only available in conjunction with "\-I, \-\-create",
 "\-A, \-\-add" or "\-U, \-\-update".
+You must set a default label for conntracks initially if you plan to update it
+later, that is, "\-U, \-\-update" requires an initial label already. If you
+update a conntrack entry without an initial label, an error will be reported.
 .TP
 .BI "--label-del " "[LABEL]"
 Specify the conntrack label to delete from the selected conntracks.
-- 
2.30.2