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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.