[PATCH] netfilter: ctnetlink: revert to dumping mark regardless of event type

[PATCH] netfilter: ctnetlink: revert to dumping mark regardless of event type

[Ivan Delalande]

I assume that change was unintentional, we have userspace code that
needs the mark while listening for events like REPLY, DESTROY, etc.

Cc: <stable@vger.kernel.org>
Fixes: 1feeae071507 ("netfilter: ctnetlink: fix compilation warning after data race fixes in ct mark")
Signed-off-by: Ivan Delalande <colona@arista.com>
---
 net/netfilter/nf_conntrack_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index c11dff91d52d..194822f8f1ee 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -831,7 +831,7 @@ ctnetlink_conntrack_event(unsigned int events, const struct nf_ct_event *item)
 	}
 
 #ifdef CONFIG_NF_CONNTRACK_MARK
-	if (events & (1 << IPCT_MARK) &&
+	if ((events & (1 << IPCT_MARK) || READ_ONCE(ct->mark)) &&
 	    ctnetlink_dump_mark(skb, ct) < 0)
 		goto nla_put_failure;
 #endif
-- 
Arista Networks

Re: [PATCH] netfilter: ctnetlink: revert to dumping mark regardless of event type

[Florian Westphal]

Ivan Delalande <colona@arista.com> wrote:
> I assume that change was unintentional, we have userspace code that
> needs the mark while listening for events like REPLY, DESTROY, etc.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 1feeae071507 ("netfilter: ctnetlink: fix compilation warning after data race fixes in ct mark")
> Signed-off-by: Ivan Delalande <colona@arista.com>
> ---
>  net/netfilter/nf_conntrack_netlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index c11dff91d52d..194822f8f1ee 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -831,7 +831,7 @@ ctnetlink_conntrack_event(unsigned int events, const struct nf_ct_event *item)
>  	}
>  
>  #ifdef CONFIG_NF_CONNTRACK_MARK
> -	if (events & (1 << IPCT_MARK) &&
> +	if ((events & (1 << IPCT_MARK) || READ_ONCE(ct->mark)) &&
>  	    ctnetlink_dump_mark(skb, ct) < 0)
>  		goto nla_put_failure;

Probably better to just drop the event bit test?

if (ctnetlink_dump_mark(skb, ct) < 0)
	goto nla_put_failure;

Re: [PATCH] netfilter: ctnetlink: revert to dumping mark regardless of event type

[Pablo Neira Ayuso]

On Thu, Mar 02, 2023 at 11:51:15AM +0100, Florian Westphal wrote:
> Ivan Delalande <colona@arista.com> wrote:
> > I assume that change was unintentional, we have userspace code that
> > needs the mark while listening for events like REPLY, DESTROY, etc.
> > 
> > Cc: <stable@vger.kernel.org>
> > Fixes: 1feeae071507 ("netfilter: ctnetlink: fix compilation warning after data race fixes in ct mark")
> > Signed-off-by: Ivan Delalande <colona@arista.com>
> > ---
> >  net/netfilter/nf_conntrack_netlink.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > index c11dff91d52d..194822f8f1ee 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -831,7 +831,7 @@ ctnetlink_conntrack_event(unsigned int events, const struct nf_ct_event *item)
> >  	}
> >  
> >  #ifdef CONFIG_NF_CONNTRACK_MARK
> > -	if (events & (1 << IPCT_MARK) &&
> > +	if ((events & (1 << IPCT_MARK) || READ_ONCE(ct->mark)) &&
> >  	    ctnetlink_dump_mark(skb, ct) < 0)
> >  		goto nla_put_failure;
> 
> Probably better to just drop the event bit test?
> 
> if (ctnetlink_dump_mark(skb, ct) < 0)
> 	goto nla_put_failure;

Looks good to me, but 1feeae071507 also changes behaviour in another
way: if ct->mark is reset to zero (being non-zero before), the event
will not show mark=0 which is probably relevant to userspace. It is a
strange corner case though.

Maybe this?

        if (ctnetlink_dump_mark(skb, ct, events & (1 << IPCT_MARK)))
                goto nla_put_failure;

then:

static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct, bool dump)
{
        u32 mark = READ_ONCE(ct->mark);

        if (!mark && !dump)
                return 0;

then in __ctnetlink_glue_build() and ctnetlink_dump_info():

        ctnetlink_dump_mark(..., true)

It seems 1feeae071507 also changed conntrack -L, before my update,
mark=0 was included in the listing.

Probably a good chance to fix these subtle changes.

Re: [PATCH] netfilter: ctnetlink: revert to dumping mark regardless of event type

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Mar 02, 2023 at 11:51:15AM +0100, Florian Westphal wrote:
> > Ivan Delalande <colona@arista.com> wrote:
> > > I assume that change was unintentional, we have userspace code that
> > > needs the mark while listening for events like REPLY, DESTROY, etc.
> > > 
> > > Cc: <stable@vger.kernel.org>
> > > Fixes: 1feeae071507 ("netfilter: ctnetlink: fix compilation warning after data race fixes in ct mark")
> > > Signed-off-by: Ivan Delalande <colona@arista.com>
> > > ---
> > >  net/netfilter/nf_conntrack_netlink.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > > index c11dff91d52d..194822f8f1ee 100644
> > > --- a/net/netfilter/nf_conntrack_netlink.c
> > > +++ b/net/netfilter/nf_conntrack_netlink.c
> > > @@ -831,7 +831,7 @@ ctnetlink_conntrack_event(unsigned int events, const struct nf_ct_event *item)
> > >  	}
> > >  
> > >  #ifdef CONFIG_NF_CONNTRACK_MARK
> > > -	if (events & (1 << IPCT_MARK) &&
> > > +	if ((events & (1 << IPCT_MARK) || READ_ONCE(ct->mark)) &&
> > >  	    ctnetlink_dump_mark(skb, ct) < 0)
> > >  		goto nla_put_failure;
> > 
> > Probably better to just drop the event bit test?
> > 
> > if (ctnetlink_dump_mark(skb, ct) < 0)
> > 	goto nla_put_failure;
> 
> Looks good to me, but 1feeae071507 also changes behaviour in another
> way: if ct->mark is reset to zero (being non-zero before), the event
> will not show mark=0 which is probably relevant to userspace. It is a
> strange corner case though.

Oh, right.

> Maybe this?
> 
>         if (ctnetlink_dump_mark(skb, ct, events & (1 << IPCT_MARK)))
>                 goto nla_put_failure;
> 
> then:
> 
> static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct, bool dump)
> {
>         u32 mark = READ_ONCE(ct->mark);
> 
>         if (!mark && !dump)
>                 return 0;

LGTM, Ivan, can you submit a v2 with Pablos suggested change?

Thanks!